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

18 Nov 2008

      On Tue, 2008-11-18 at 22:37 +0100, Wolfgang Denk wrote:
...
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/
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