Hi Wolfgang,
Thanks for the comments.
On Sun, 2008-12-14 at 12:38 +0100, Wolfgang Denk wrote:
> Dear Peter Tyser,
>
> In message
> <300544b7901dacbe6b9e3edd052629f612b92735.1228160312.git.pty...@xes-inc.com>
> you wrote:
> > Initial support for the DS4510, a CPU supervisor with
> > integrated EEPROM, SRAM, and 4 programmable non-volatile
> > GPIO pins. The CONFIG_DS4510 define enables support
> > for the device while the CONFIG_CMD_DS4510 define
> > enables the ds4510 command. The additional
> > CONFIG_DS4510_INFO, CONFIG_DS4510_MEM, and
> > CONFIG_DS4510_RST defines add additional sub-commands
> > to the ds4510 command when defined.
>
> General note: the code shows a serious lack of error detection and
> error handling.
OK, I'll fix it.
> > Signed-off-by: Peter Tyser <pty...@xes-inc.com>
> > ---
> > README | 4 +
> > drivers/misc/Makefile | 1 +
> > drivers/misc/ds4510.c | 400
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > include/gpio/ds4510.h | 75 +++++++++
> > 4 files changed, 480 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/misc/ds4510.c
> > create mode 100644 include/gpio/ds4510.h
>
> The driver would probably be better placed in the drivers/hwmon/
> directory.
You commented in the original version of the patch:
"I think drivers/misc/ is probably the best place we have at the
moment, indeed."
I think drivers/misc makes the most sense, but hwmon is fine too. Let
me know what you prefer.
> Also, I don't see a reason to create a new include/gpio/ directory
> here, especially since this is not primarily a GPIO device.
The original patch series had the pca953x.h (a purely GPIO device) and
ds4510.h in include/gpio/. You're right that the ds4510.h doesn't
belong in include/gpio, I'll move it to include/. I'm assuming you'd
also like the pca953x.h in this patch series to be moved from
include/gpio to include?
> > diff --git a/README b/README
> > index bca8061..cea057d 100644
> > --- a/README
> > +++ b/README
> > @@ -574,6 +574,10 @@ The following options need to be configured:
> > CONFIG_CMD_DHCP * DHCP support
> > CONFIG_CMD_DIAG * Diagnostics
> > CONFIG_CMD_DOC * Disk-On-Chip Support
> > + CONFIG_CMD_DS4510 * ds4510 I2C gpio commands
> > + CONFIG_CMD_DS4510_INFO * ds4510 I2C info command
> > + CONFIG_CMD_DS4510_MEM * ds4510 I2C eeprom/sram commansd
> > + CONFIG_CMD_DS4510_RST * ds4510 I2C rst command
>
> Do we really need such granularity? If you have the device on your
> board and suppoort it in U-Boot, you probably always want to have
> CONFIG_CMD_DS4510_INFO and CONFIG_CMD_DS4510_RST. And
> CONFIG_CMD_DS4510_MEM seems reduindand - that should already be
> covered by the CONFIG_CMD_EEPROM setting.
I added finer granularity based on a comment you made about the pca953x
"info" command:
"Don't remove it, but maybe make it a selectable option so those who
like it can include it while others that are tryting to minimize the
memory footprint don't suffer from it."
If the "info" command could be enabled/disabled I reasoned the other
functions of the chip should also be.
The original patch didn't have any command-enabling granularity. For
our hardware setups, we only use the DS4510 as a non-volatile GPIO
device, the EEPROM, SRAM, and reset monitor functionality are not used
(I know, the part seems a bit overkill for what we use it for, but I
didn't design the hardware:).
The standard EEPROM commands could be used on the device, but I think it
would get confusing as part of the device's memory space is EEPROM only,
part SRAM only, and the other part EEPROM or SRAM depending on register
setting in the device. Thus the eeprom command's functionality would
depend on the settings of the DS4510 device (or be limited only to the
EEPROM portion of the device), and the ds4510 command would support the
SRAM, but not EEPROM which is confusing in my opinion.
So in any case, all I really care about is the GPIO functionality of the
chip as is - I added EEPROM, reset monitor, and SRAM support to cover
all uses of the chip. I'm fine with removing the finer granularity
command enabling/disabling, or even removing non-GPIO functionality if
you'd prefer:) I'd vote for leaving the command enabling/disabling
implementation as is, let me know if you'd like it changed.
> > +/* Use 'maxargs' field as minimum number of args to ease error checking */
> > +cmd_tbl_t cmd_ds4510[] = {
>
> Please do not mis-use variables with a clearly defined meaning in such
> a way. This is not acceptable.
Alright, I'll rework this and also the pca953x.c implementation in this
patchset.
> > +#ifdef CONFIG_CMD_DS4510_MEM
> > + "ds4510 eeprom read addr off cnt\n"
> > + "ds4510 eeprom write addr off cnt\n"
> > + " - read/write 'cnt' bytes at EEPROM offset 'off'\n"
> > + "ds4510 seeprom read addr off cnt\n"
> > + "ds4510 seeprom write addr off cnt\n"
> > + " - read/write 'cnt' bytes at SRAM-shadowed EEPROM offset 'off'\n"
> > + "ds4510 sram read addr off cnt\n"
> > + "ds4510 sram write addr off cnt\n"
> > + " - read/write 'cnt' bytes at SRAM offset 'off'\n"
> > +#endif
>
> We should not need a separate EEPROM command here, I think.
See comments above.
> And cannot this SRAM be written using plain "cp" commands?
The device is only accessible via I2C, so the "cp" commands would not
work.
Thanks for the comments,
Peter
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
