Re: [PATCH 1] utility functions for using the extended VV interface to FIS

[Andrew Lunn <andrew at lunn dot ch>]

+cdl_package CYGPKG_FS_FIS {
+    display         "FIS update and filesystem"
+    include_dir     cyg/fs
+
+    requires        CYGPKG_ISOINFRA
+    requires        CYGINT_ISO_ERRNO
+    requires        CYGINT_ISO_ERRNO_CODES
+    requires        CYGPKG_LIBC_STRING
+
+    compile         -library=libextras.a fisupdate.c

Why should this go into libextras.a? It is not a device driver. We
want the linker to discard this code if its not used.

+
+    cdl_option CYGDBG_FS_FIS_DEBUG_OUTPUT {
+       display "Enable debug output"
+       default_value 1
+    }

This kind of suggests the code still needs debugging. I would prefer to see the default as 0.

+// how many entries can the FIS contain at most ? initialized in fis_init()
+int fis_max_entries=-1;

This should be a static variable to avoid pollution.

+/* If an image with the given name exists its index is returned and
+ entry is filled appropriatly. Using a NULL-pointer for entry is also
+ ok, then only the index will be returned. If no such image exists, it returns -1.*/
+int fis_find_entry(const char* name, struct fis_table_entry* entry)
+{

Should this be static? It looks like it is just a helper function for fis_get_entry().

+   // write them for now into the flash
+   CYGACC_CALL_IF_FLASH_FIS_OP2(CYGNUM_CALL_IF_FLASH_FIS_START_UPDATE, 0, NULL);
+   // and make the new written directory now valid, because once the erasing has started the data is corrupt
+   CYGACC_CALL_IF_FLASH_FIS_OP2(CYGNUM_CALL_IF_FLASH_FIS_FINISH_UPDATE, 0, NULL);
+
+   // erase it
+   cyg_interrupt_disable();
+   flash_unlock((void *)oldEntry.flash_base, oldEntry.size, (void **)&err_addr);

flash_unlock is not guaranteed to exist. You need to use CYGHWR_IO_FLASH_BLOCK_LOCKING 

Also, you don't seem to be consistent. fis_program_data() does not
unlock/lock.

        Andrew