This is the mail archive of the
ecos-patches@sourceware.org
mailing list for the eCos project.
AW: [PATCH 1] utility functions for using the extended VV interface to FIS
- From: "Neundorf, Alexander" <Alexander dot Neundorf at jenoptik dot com>
- To: "Andrew Lunn" <andrew at lunn dot ch>
- Cc: <ecos-patches at ecos dot sourceware dot org>, "Gary Thomas" <gary at mlbassoc dot com>
- Date: Mon, 8 Jan 2007 08:48:21 +0100
- Subject: AW: [PATCH 1] utility functions for using the extended VV interface to FIS
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