[U-Boot] [PATCH v4 2/3] Add support for Maxim's DS4510 I2C device

[Wolfgang Denk]

Dear Peter Tyser,

In message <300544b7901dacbe6b9e3edd052629f612b92735.1228160312.git.ptyser at 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 at 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 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: wd at denx.de
For every problem there is one solution which is  simple,  neat,  and
wrong.                                                - H. L. Mencken