Support for XPedite5370 and misc GPIO

Initial support for Extreme Engineering Solutions XPedite5370 - a MPC8572-based 3U VPX single board computer with a PMC/XMC site.

Signed-off-by Peter Tyser <[email protected]>

---
 MAINTAINERS                         |    3 +
 MAKEALL                             |    1 +
 Makefile                            |    3 +
 board/xes/common/Makefile           |   56 ++++
 board/xes/common/fsl_8572_clk.c     |   51 +++
 board/xes/common/fsl_85xx_ddr.c     |   93 ++++++
 board/xes/common/fsl_85xx_pci.c     |  287 +++++++++++++++++

board/xes/xpedite5370/xpedite5370.c new file +6 -0

+int board_early_init_r(void)
+{
+       const unsigned int flashbase = CONFIG_SYS_FLASH_BASE2;
+       int i;
+
+       /* Initialize PCA9557 devices */

Quoted 4 lines

 board/xes/common/fsl_8572_clk.c     |   51 +++
 board/xes/common/fsl_85xx_ddr.c     |   93 ++++++
 board/xes/common/fsl_85xx_pci.c     |  287 +++++++++++++++++

Could you take a look at the new PCI code Kumar submitted (which I just applied), and rework the board code to imitate that code (it should be a trivial change, but one that makes things cleaner)

board/xes/xpedite5370/xpedite5370.c line 95

Quoted 5 lines

board/xes/xpedite5370/xpedite5370.c new file +5 -0

+       /* Invalidate flash region */
+       for (i = 0; i < 256 * 1024 * 1024; i += 32) {
+               asm volatile ("dcbi %0,%1": : "b" (flashbase), "r" (i));
+               asm volatile ("icbi %0,%1": : "b" (flashbase), "r" (i));
+       }

Take a look at the current 8572DS code for doing this. By mapping things correctly, you can use dcbf, and not have to go through the entire memory space of your flash.

Initial support for Extreme Engineering Solutions XPedite5370 - a MPC8572-based 3U VPX single board computer with a PMC/XMC site.

board/xes/common/fsl_85xx_ddr.c new file +0 -0

new file mode 100644
index 0000000..30b4767

board/xes/common/fsl_85xx_ddr.c new file +28 -0

+	puts(" (");
+
+#if (CONFIG_NUM_DDR_CONTROLLERS > 1)
+	/* Print interleaving information */
+	if (ddr1->cs0_config & 0x20000000) {
+		switch ((ddr1->cs0_config >> 24) & 0xf) {
+		case 0:
+			puts("cache line");
+			break;
+		case 1:
+			puts("page");
+			break;
+		case 2:
+			puts("bank");
+			break;
+		case 3:
+			puts("super-bank");
+			break;
+		default:
+			puts("invalid");
+			break;
+		}
+	} else {
+		puts("no");
+	}
+
+	puts(" interleaving");
+#endif

board/xes/common/fsl_85xx_pci.c new file +0 -0

new file mode 100644
index 0000000..12437be

board/xes/xpedite5370/xpedite5370.c new file +0 -0

new file mode 100644
index 0000000..3526172

board/xes/xpedite5370/xpedite5370.c new file +15 -0

+#define	CONFIG_EXTRA_ENV_SETTINGS				\
+ "autoload=yes\0"						\
+ "download_cmd=tftp\0"						\
+ "console_args=console=ttyS0,115200\0"				\
+ "root_args=root=/dev/nfs rw\0"					\
+ "misc_args=ip=on\0"						\
+ "set_bootargs=setenv bootargs ${console_args} ${root_args} ${misc_args}\0" \
+ "bootfile=/home/user/file\0"					\
+ "osfile=/home/user/uImage-XPedite5370\0"			\
+ "fdtfile=/home/user/xpedite5370.dtb\0"				\
+ "ubootfile=/home/user/u-boot.bin\0"				\
+ "fdtaddr=c00000\0"						\
+ "osaddr=0x1000000\0"						\
+ "loadaddr=0x1000000\0"						\
+ "prog_uboot1="CONFIG_PROG_UBOOT1"\0"				\

Dear Peter Tyser,

Quoted 4 lines · In message <[email protected]> you wrote:

Initial support for Extreme Engineering Solutions XPedite5370 - a MPC8572-based 3U VPX single board computer with a PMC/XMC site.

...

Quoted 5 lines

board/xes/common/fsl_85xx_ddr.c new file +0 -0

new file mode 100644
index 0000000..30b4767

...

Quoted 32 lines

board/xes/common/fsl_85xx_ddr.c new file +32 -0

+	puts(" (");
+
+#if (CONFIG_NUM_DDR_CONTROLLERS > 1)
+	/* Print interleaving information */
+	if (ddr1->cs0_config & 0x20000000) {
+		switch ((ddr1->cs0_config >> 24) & 0xf) {
+		case 0:
+			puts("cache line");
+			break;
+		case 1:
+			puts("page");
+			break;
+		case 2:
+			puts("bank");
+			break;
+		case 3:
+			puts("super-bank");
+			break;
+		default:
+			puts("invalid");
+			break;
+		}
+	} else {
+		puts("no");
+	}
+
+	puts(" interleaving");
+#endif
+
+#if (CONFIG_NUM_DDR_CONTROLLERS > 1) && defined(CONFIG_DDR_ECC)
+	puts(", ");
+#endif

board/xes/common/fsl_85xx_ddr.c line 91

Quoted 6 lines

board/xes/common/fsl_85xx_ddr.c new file +6 -0

+
+#if defined(CONFIG_DDR_ECC)
+	puts("ECC enabled");
+#endif
+
+	puts(")");

Please don't be too noisy. Turn this into debug() calls.

Quoted 5 lines

board/xes/common/fsl_85xx_pci.c new file +0 -0

new file mode 100644
index 0000000..12437be

...

board/xes/common/fsl_85xx_pci.c line 105

Quoted 3 lines

board/xes/common/fsl_85xx_pci.c new file +3 -0

+	if (width && !(devdisr & MPC85xx_DEVDISR_PCIE)) {
+		printf("\n    PCIE1 connected as %s (x%d)",
+		       host ? "Root Complex" : "End Point", width);

Alignment not by TAB.

board/xes/common/fsl_85xx_pci.c line 109

Quoted 4 lines

board/xes/common/fsl_85xx_pci.c new file +4 -0

+		if (pci->pme_msg_det) {
+			pci->pme_msg_det = 0xffffffff;
+			debug(" with errors.  Clearing.  Now 0x%08x",
+			      pci->pme_msg_det);

Alignment not by TAB.

board/xes/common/fsl_85xx_pci.c line 118

Quoted 9 lines

board/xes/common/fsl_85xx_pci.c new file +9 -0

+		}
+		printf("\n");
+
+		/* inbound */
+		pci_set_region(hose->regions + 0,
+			       CONFIG_SYS_PCI_MEMORY_BUS,
+			       CONFIG_SYS_PCI_MEMORY_PHYS,
+			       CONFIG_SYS_PCI_MEMORY_SIZE,
+			       PCI_REGION_MEM | PCI_REGION_MEMORY);

Alignment not by TAB.

And so on.

Quoted 5 lines

board/xes/xpedite5370/xpedite5370.c new file +0 -0

new file mode 100644
index 0000000..3526172

...

board/xes/xpedite5370/xpedite5370.c line 66

Quoted 3 lines

board/xes/xpedite5370/xpedite5370.c new file +3 -0

+	/* Print boot dev and swap flash flash chip selects if booted from 2nd
+	 * flash.  Swapping chip selects presents user with a common memory
+	 * map regardless of which flash was booted from. */

Multiline comment style.

board/xes/xpedite5370/xpedite5370.c line 69

Quoted 3 lines

board/xes/xpedite5370/xpedite5370.c new file +3 -0

+	flash_sel = !((pca953x_get_val(CONFIG_SYS_I2C_PCA953X_ADDR0) &
+			  CONFIG_SYS_PCA953X_C0_FLASH_PASS_CS));
+	printf("FLASH: Executed from FLASH%d\n", flash_sel ? 2 : 1);

Please be less noisy! s/printf/debug/

Quoted 19 lines

+#define CONFIG_EXTRA_ENV_SETTINGS \
+ "autoload=yes\0" \
+ "download_cmd=tftp\0" \
+ "console_args=console=ttyS0,115200\0" \
+ "root_args=root=/dev/nfs rw\0" \
+ "misc_args=ip=on\0" \
+ "set_bootargs=setenv bootargs ${console_args} ${root_args} ${misc_args}\0" \
+ "bootfile=/home/user/file\0" \
+ "osfile=/home/user/uImage-XPedite5370\0" \
+ "fdtfile=/home/user/xpedite5370.dtb\0" \
+ "ubootfile=/home/user/u-boot.bin\0" \
+ "fdtaddr=c00000\0" \
+ "osaddr=0x1000000\0" \
+ "loadaddr=0x1000000\0" \
+ "prog_uboot1="CONFIG_PROG_UBOOT1"\0" \
+ "prog_uboot2="CONFIG_PROG_UBOOT2"\0" \
+ "prog_os1="CONFIG_PROG_OS1"\0" \
+ "prog_os2="CONFIG_PROG_OS2"\0" \
+ "prog_fdt1="CONFIG_PROG_FDT1"\0" \

Quoted 6 lines

+ "prog_fdt2="CONFIG_PROG_FDT2"\0" \
+ "bootcmd_net=run set_bootargs; "CONFIG_BOOT_OS_NET"\0" \
+ "bootcmd_flash1=run set_bootargs; bootm "CONFIG_OS1_FLASH_ADDR_STR" - "CONFIG_FDT1_FLASH_ADDR_STR"\0" \
+ "bootcmd_flash2=run set_bootargs; bootm "CONFIG_OS2_FLASH_ADDR_STR" - "CONFIG_FDT2_FLASH_ADDR_STR"\0" \
+ "bootcmd=run bootcmd_flash1\0"
+#endif /* __CONFIG_H */

Alignment not by TAB, lines way too long.

This information is very useful to a customer and doesn't add much as far as output. No newlines at least. The output also prints information which is configurable. I think giving the user feedback about how they have the board configured is useful.

We have the standard Freescale DDR printf's turned into debug as that is much, much more verbose than the output above.

Quoted 5 lines

board/xes/xpedite5370/xpedite5370.c line 69

Quoted 3 lines

board/xes/xpedite5370/xpedite5370.c new file +3 -0

+	flash_sel = !((pca953x_get_val(CONFIG_SYS_I2C_PCA953X_ADDR0) &
+			  CONFIG_SYS_PCA953X_C0_FLASH_PASS_CS));
+	printf("FLASH: Executed from FLASH%d\n", flash_sel ? 2 : 1);

Please be less noisy! s/printf/debug/

This information is also very useful to a customer.

Quoted 2 lines

Quoted 2 lines

+#define CONFIG_EXTRA_ENV_SETTINGS \
+ "autoload=yes\0" \

Quoted 8 lines

Quoted 6 lines

+ "prog_fdt2="CONFIG_PROG_FDT2"\0" \
+ "bootcmd_net=run set_bootargs; "CONFIG_BOOT_OS_NET"\0" \
+ "bootcmd_flash1=run set_bootargs; bootm "CONFIG_OS1_FLASH_ADDR_STR" - "CONFIG_FDT1_FLASH_ADDR_STR"\0" \
+ "bootcmd_flash2=run set_bootargs; bootm "CONFIG_OS2_FLASH_ADDR_STR" - "CONFIG_FDT2_FLASH_ADDR_STR"\0" \
+ "bootcmd=run bootcmd_flash1\0"
+#endif /* __CONFIG_H */

Alignment not by TAB, lines way too long.

What are you referring to as far as "Alignment not by TAB"?

Everyone's config file breaks the 80 column rule, is this really necessary to change? I think splitting it up makes an already confusing define even more confusing.

Dear Peter Tyser,

Quoted 5 lines · In message <[email protected]> you wrote:

This information is very useful to a customer and doesn't add much as far as output. No newlines at least. The output also prints information which is configurable. I think giving the user feedback about how they have the board configured is useful.

You really need this level of detail only for board bring up and debugging. After that, this is a constand and never changes, right? Si it is normally enough to print the RAM size (and eventually some speed characteristics).

Quoted 2 lines

We have the standard Freescale DDR printf's turned into debug as that is much, much more verbose than the output above.

Indeed :-(

Quoted 7 lines

Quoted 5 lines

board/xes/xpedite5370/xpedite5370.c line 69

Quoted 3 lines

board/xes/xpedite5370/xpedite5370.c new file +3 -0

+	flash_sel = !((pca953x_get_val(CONFIG_SYS_I2C_PCA953X_ADDR0) &
+			  CONFIG_SYS_PCA953X_C0_FLASH_PASS_CS));
+	printf("FLASH: Executed from FLASH%d\n", flash_sel ? 2 : 1);

Please be less noisy! s/printf/debug/

This information is also very useful to a customer.

Is it, really? well...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+#define CONFIG_EXTRA_ENV_SETTINGS \
+ "autoload=yes\0" \

...

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

+ "prog_fdt2="CONFIG_PROG_FDT2"\0" \
+ "bootcmd_net=run set_bootargs; "CONFIG_BOOT_OS_NET"\0" \
+ "bootcmd_flash1=run set_bootargs; bootm "CONFIG_OS1_FLASH_ADDR_STR" - "CONFIG_FDT1_FLASH_ADDR_STR"\0" \
+ "bootcmd_flash2=run set_bootargs; bootm "CONFIG_OS2_FLASH_ADDR_STR" - "CONFIG_FDT2_FLASH_ADDR_STR"\0" \
+ "bootcmd=run bootcmd_flash1\0"
+#endif /* __CONFIG_H */

Quoted 3 lines

Quoted 1 line

Alignment not by TAB, lines way too long.

What are you referring to as far as "Alignment not by TAB"?

You indent the lines by a single space, but they should be indented by a TAB.

Quoted 3 lines

Everyone's config file breaks the 80 column rule, is this really necessary to change? I think splitting it up makes an already confusing define even more confusing.

At least the ';' is a good place to split. And you might check if it really makes sense to use variable names with 25 characters like "CONFIG_OS2_FLASH_ADDR_STR" (no, it doesn't).

Quoted 3 lines

You indent the lines by a single space, but they should be indented by a TAB.

Many boards I look at don't use TABS. In particular Freescale reference platforms. Many boards even have different amounts of spaces:) I don't

That define is no longer than the standard CONFIG_EXTRA_ENV_SETTINGS define that its used in, or any number of other standard defines. And they aren't used anywhere other than this 40 line section of code in 1 file. I thought the increase in clarity of long defines made up for their ugliness. I'll go ahead and split the lines on the ';' as you suggested.

You have the final say, so if any of the above are sticking points to getting the code accepted let me know and I'll change them as requested.

Quoted 3 lines

Quoted 2 lines

We have the standard Freescale DDR printf's turned into debug as that is much, much more verbose than the output above.

Indeed :-(

Many boards I look at don't use TABS. In particular Freescale reference platforms. Many boards even have different amounts of spaces:) I don't see the value of indentation in this case as it doesn't increase readability and just makes already long lines even longer.

Quoted 3 lines · Peter Tyser

Quoted 3 lines

Quoted 2 lines

We have the standard Freescale DDR printf's turned into debug as that is much, much more verbose than the output above.

Indeed :-(

Hrm. Out of curiosity, which printf()s do you find noisy?

Quoted 4 lines

Many boards I look at don't use TABS. In particular Freescale reference platforms. Many boards even have different amounts of spaces:) I don't see the value of indentation in this case as it doesn't increase readability and just makes already long lines even longer.

Really? Which ones? We should clean those up!

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

drivers/gpio/ds4510.c new file +2 -0

+int do_ds4510(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{

drivers/gpio/ds4510.c new file +1 -0

+	case 5:

drivers/gpio/ds4510.c new file +1 -0

+	case 7:

drivers/gpio/ds4510.c new file +9 -0

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

drivers/gpio/ds4510.c new file +14 -0

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

Dear Peter Tyser,

Quoted 7 lines · 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]>

Quoted 8 lines

---
 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/

drivers/gpio/ds4510.c line 45

Quoted 2 lines

drivers/gpio/ds4510.c new file +2 -0

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

Quoted 3 lines

drivers/gpio/ds4510.c new file +2 -0

+int do_ds4510(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{

...

drivers/gpio/ds4510.c line 256

Quoted 10 lines

drivers/gpio/ds4510.c new file +10 -0

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

Quoted 1 line

drivers/gpio/ds4510.c new file +1 -0

+	case 5:

...

drivers/gpio/ds4510.c line 271

Quoted 8 lines

drivers/gpio/ds4510.c new file +8 -0

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

Quoted 1 line

drivers/gpio/ds4510.c new file +1 -0

+	case 7:

...

Quoted 9 lines

drivers/gpio/ds4510.c new file +9 -0

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

drivers/gpio/ds4510.c line 303

Quoted 10 lines

drivers/gpio/ds4510.c new file +10 -0

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

Quoted 18 lines

drivers/gpio/ds4510.c new file +18 -0

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

drivers/gpio/ds4510.c line 342

Quoted 6 lines

drivers/gpio/ds4510.c new file +6 -0

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

Quoted 5 lines

Quoted 3 lines

 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.

Quoted 16 lines

Quoted 16 lines

drivers/gpio/ds4510.c new file +16 -0

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

Quoted 8 lines

drivers/gpio/ds4510.c line 342

Quoted 6 lines

drivers/gpio/ds4510.c new file +6 -0

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

Initial support for NXP's 4 and 8 bit I2C gpio expanders (eg pca9537, pca9557, etc). The CONFIG_PCA953X define enables support for the devices while the CONFIG_CMD_PCA953X define enables the pca953x command.

Dear Peter Tyser,

Quoted 4 lines · In message <[email protected]> you wrote:

Initial support for NXP's 4 and 8 bit I2C gpio expanders (eg pca9537, pca9557, etc). The CONFIG_PCA953X define enables support for the devices while the CONFIG_CMD_PCA953X define enables the pca953x command.

I'm not sure if we need such a detailed level of access to these chips. We already have a couple of similar I/O expanders in some boards, for example implemente in some PICs. See for example the "pic read" / "pic write" conde in board/lwmon/lwmon.c etc.

I think we should generalize this - and more than read / write isn't really needed, or is it?

Quoted 8 lines

I'm not sure if we need such a detailed level of access to these chips. We already have a couple of similar I/O expanders in some boards, for example implemente in some PICs. See for example the "pic read" / "pic write" conde in board/lwmon/lwmon.c etc.

I think we should generalize this - and more than read / write isn't really needed, or is it?

What do you mean by "we should generalize this"?

I view the read/write/invert commands as necessary to easily configure/read/write the GPIO pins, and the info command is nice, but not necessary.

The info command is nice to get an idea of what's going on at a high level - eg is the input on pin 2 being inverted? What pins are currently being driven low? etc. I've found the iopinfo command useful for the 82xx boards and tried to replicate similar functionality in this driver. I like the info command but understand if others want it removed.

This patch series adds support for the XPedite5370 SBC. Its an MPC8572-based VPX card with a PMC/XMC site. The XPedite5370 includes a significant number of I2C GPIO devices (5) which are used for board configuration. I added support for 2 new I2C gpio devices in a new drivers/gpio directory. I'm not sure if this is the preferred location/method, so let me know

if others have have different preferences. It'd be nice to have a more generic GPIO framework (like Linux's) at some point, but