Dear Peter Tyser,
In message 300544b7901dacbe6b9e3edd052629f612b92735.1228160312.git.ptyser@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 ptyser@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.
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 commandsCONFIG_CMD_DS4510_INFO * ds4510 I2C info commandCONFIG_CMD_DS4510_MEM * ds4510 I2C eeprom/sram commansdCONFIG_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;elsedata &= ~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