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

["Neundorf, Alexander" <Alexander dot Neundorf at jenoptik dot com>]

Hi,


> Von: Andrew Lunn [mailto:andrew@lunn.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.

Ok. 
What's actually the exact difference between libextras.a and the normal lib ? I just copied this from somewhere.

> +    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.

Ok.
 
> +// 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.

Ok.
 
> +/* 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().

It is used also in the filesystem implementation, that's why I removed the static.
 
> +   // write them for now into the flash
> +   
> CYGACC_CALL_IF_FLASH_FIS_OP2(CYGNUM_CALL_IF_FLASH_FIS_START_UP
> DATE, 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_U
> PDATE, 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 

Ok.
 
> Also, you don't seem to be consistent. fis_program_data() does not
> unlock/lock.
 
The unlock/lock calls are around the call to fis_program_data().

> I would like to see all these global scope functions have cyg_ prefix
> otherwise they cause name space pollution.


So cyg_fis_get_entry() etc. ?

Bye
Alex