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

[Wolfgang Denk <wd@denx.de>]

Dear Peter Tyser,

In message <1224800639-31350-3-git-send-email-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.
> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
>  README                |    2 +
>  drivers/gpio/Makefile |    1 +
>  drivers/gpio/ds4510.c |  344 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/gpio/ds4510.h |   75 +++++++++++
>  4 files changed, 422 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/ds4510.c
>  create mode 100644 include/gpio/ds4510.h

This should go to devices/i2c/

> +		/* This delay isn't needed for SRAM writes but shouldn't delay
> +		 * things too much, so do it unconditionally for simplicity */

Please fix multiline comment style.

> +int do_ds4510(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
...
> +	case 4:
> +		val = simple_strtoul(argv[3], NULL, 16);
> +
> +		if (strcmp(argv[2], "nv") == 0)
> +			return ds4510_see_write(chip, val);
> +		else if (strcmp(argv[2], "rstdelay") == 0)
> +			return ds4510_rstdelay_write(chip, val);
> +		else if (strcmp(argv[2], "input") == 0)
> +			return (ds4510_gpio_read_val(chip) & (1 << val)) != 0;
> +		break;

We have generic code for processing multiple command selections.
Please use that.

> +	case 5:
...
> +		if (strcmp(argv[2], "output") == 0) {
> +			tmp = ds4510_gpio_read(chip);
> +			if (val)
> +				tmp |= (1 << pin);
> +			else
> +				tmp &= ~(1 << pin);
> +			return ds4510_gpio_write(chip, tmp);
> +		} else if (strcmp(argv[2], "pullup") == 0) {

Ditto.

> +	case 7:
...
> +		if (strcmp(argv[3], "read") == 0)
> +			rw_func = ds4510_mem_read;
> +		else if (strcmp(argv[3], "write") == 0)
> +			rw_func = ds4510_mem_write;
> +		else
> +			break;
> +
> +		if (strcmp(argv[2], "eeprom") == 0) {
> +			end = DS4510_EEPROM + DS4510_EEPROM_SIZE;
> +			off += DS4510_EEPROM;
> +		} else if (strcmp(argv[2], "seeprom") == 0) {
> +			end = DS4510_SEEPROM + DS4510_SEEPROM_SIZE;
> +			off += DS4510_SEEPROM;
> +		} else if (strcmp(argv[2], "sram") == 0) {
> +			end = DS4510_SRAM + DS4510_SRAM_SIZE;
> +			off += DS4510_SRAM;
> +		} else {
> +			break;
> +		}

Ditto.

> +U_BOOT_CMD(
> +	ds4510,	7,	2,	do_ds4510,
> +	"ds4510	- ds4510 eeprom/seeprom/sram/gpio access\n",
> +	"chip info\n"
> +	"	- display ds4510 info\n"
> +	"ds4510 chip nv 0|1\n"
> +	"	- make gpio and seeprom writes volatile/non-volatile\n"
> +	"ds4510 chip rstdelay 0-3\n"
> +	"	- set reset output delay\n"
> +	"ds4510 chip output pin 0|1\n"
> +	"	- set pin low or high-Z\n"
> +	"ds4510 chip input pin\n"
> +	"	- read value of pin\n"
> +	"ds4510 chip pullup pin 0|1\n"
> +	"	- disable/enable pullup on specified pin\n"
> +	"ds4510 chip eeprom read addr off cnt\n"
> +	"ds4510 chip eeprom write addr off cnt\n"
> +	"	- read/write 'cnt' bytes at EEPROM offset 'off'\n"
> +	"ds4510 chip seeprom read addr off cnt\n"
> +	"ds4510 chip seeprom write addr off cnt\n"
> +	"	- read/write 'cnt' bytes at SRAM-shadowed EEPROM offset 'off'\n"
> +	"ds4510 chip sram read addr off cnt\n"
> +	"ds4510 chip sram write addr off cnt\n"
> +	"	- read/write 'cnt' bytes at SRAM offset 'off'\n"

Why do we need the "chip" argument? It just adds typing...


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
You speak of courage. Obviously you do not know the  difference  bet-
ween  courage and foolhardiness. Always it is the brave ones who die,
the soldiers.
	-- Kor, the Klingon Commander, "Errand of Mercy",
	   stardate 3201.7