On Tue, 2008-11-18 at 22:37 +0100, Wolfgang Denk wrote:
> Dear Peter Tyser,
>
> In message <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> > ---
> > 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
