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. > Signed-off-by: Peter Tyser <[email protected]> > --- > 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. Also, I don't see a reason to create a new include/gpio/ directory here, especially since this is not primarily a GPIO device. > 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. > diff --git a/drivers/misc/ds4510.c b/drivers/misc/ds4510.c > new file mode 100644 > index 0000000..572dcb7 > --- /dev/null > +++ b/drivers/misc/ds4510.c ... > +/* > + * Write to DS4510, taking page boundaries into account > + */ > +int ds4510_mem_write(uint8_t chip, int offset, uint8_t *buf, int count) > +{ > + int wrlen; > + int i = 0; > + > + do { > + wrlen = DS4510_EEPROM_PAGE_SIZE - > + DS4510_EEPROM_PAGE_OFFSET(offset); > + if (count < wrlen) > + wrlen = count; > + i2c_write(chip, offset, 1, &buf[i], wrlen); i2c_write() can fail - it returns error codes. I'm missing error handling here and elsewhere. I'll flag a few locations, but please check all the code. > +int ds4510_gpio_write(uint8_t chip, uint8_t val) > +{ > + uint8_t data; > + int i; > + > + for (i = 0; i < DS4510_NUM_IO; i++) { > + i2c_read(chip, DS4510_IO0 - i, 1, &data, 1); Error handling? > + if (val & (0x1 << i)) > + data |= 0x1; > + else > + data &= ~0x1; > + > + ds4510_mem_write(chip, DS4510_IO0 - i, &data, 1); Error handling? > +int ds4510_gpio_read(uint8_t chip) > +{ > + uint8_t data; > + int val = 0; > + int i; > + > + for (i = 0; i < DS4510_NUM_IO; i++) { > + i2c_read(chip, DS4510_IO0 - i, 1, &data, 1); Error handling? > +static int ds4510_info(uint8_t chip) > +{ > + int i; > + int tmp; > + uint8_t data; > + > + printf("DS4510 @ 0x%x:\n\n", chip); > + > + i2c_read(chip, DS4510_RSTDELAY, 1, &data, 1); error handling? > + printf("rstdelay = 0x%x\n\n", data & DS4510_RSTDELAY_MASK); > + > + i2c_read(chip, DS4510_CFG, 1, &data, 1); error handling? > +/* 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. > +#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. And cannot this SRAM be written using plain "cp" commands? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected] For every problem there is one solution which is simple, neat, and wrong. - H. L. Mencken _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
