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

[Peter Tyser]

On Tue, 2008-11-18 at 22:37 +0100, Wolfgang Denk wrote:
> Dear Peter Tyser,
> 
> In message <1224800639-31350-3-git-send-email-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.
> > 
> > Signed-off-by: Peter Tyser <ptyser at 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/

I'm assuming you're referring to drivers/i2c.  I was under the
impression the driver/i2c directory was only for drivers which
controller I2C buses (similar to drivers/i2c/busses in Linux), not
actual I2C chip device drivers.  I don't currently see any other I2C
chip drivers in there and didn't want to be the first to add one:)

I agree that the ds4510 doesn't fit in the GPIO category well.  How
about drivers/misc?  If I'm mistaken about what is supposed to go in
drivers/i2c let me know and I'll move it in there.


> > +		/* 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.

Will do.

> 
> > +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.

Will do.

> > +	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...

In theory more than 1 chip could be on a board.  The chip uses an
address pin pull up/down to determine the device's i2c address.

Thanks,
Peter