Skip to contentSkip to conversation

Support for XPedite5370 and misc GPIO

PATCH Superseded Peter Tyser 3 participants 8 messages Dec 1, 2008 – Dec 15, 2008

Subsystems (3)

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v4 cover Support for XPedite5370 and misc GPIO

Hello,
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 figured this was a step in the right direction by providing a place for generic GPIO devices in drivers/gpio.

Thanks,
Peter

Changes since v1:
- Rebased to u-boot-mpc85xx tree
- Updated NOR flash remapping method
- Added CONFIG_SYS_I2C2_OFFSET define to XPEDITE5370.h
- Removed CONFIG_SYS_EEPROM_PAGE_WRITE_ENABLE from XPEDITE5370.h

Changes since v2:
pca953x.c:
- Use cmd_tbl_t for subcommand parsing
- Made 'info' command dependent on CONFIG_CMD_PCA953X_INFO
- Removed chip command parameters, added 'device' subcommand

ds4510.c:
- Use cmd_tbl_t for subcommand parsing
- Made 'info' command dependent on CONFIG_CMD_DS4510_INFO
- Made memory commands dependent on CONFIG_CMD_DS4510_MEM
- Made 'rst' command dependent on CONFIG_CMD_DS4510_RST
- Removed chip command parameters, added 'device' subcommand
- Fixed multiline comment style
- moved to drivers/misc

xpedite5370:
- Changed alignment by spaces to alignment by tabs
- Fixed gur->devdisr typo for PCIe2
- Fixed some lines over 80 chars
- Fixed multiline comment style
- Cleaned up environment defines

Changes since v3:
- Removed CONFIG_L1_INIT_RAM reference

Peter Tyser (3):
Add support for PCA953x I2C gpio devices
Add support for Maxim's DS4510 I2C device
XPedite5370 board support

 MAINTAINERS                         |    3 +
 MAKEALL                             |    1 +
 Makefile                            |    5 +
 README                              |   13 +
 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     |  265 ++++++++++++++++
 board/xes/xpedite5370/Makefile      |   45 +++
 board/xes/xpedite5370/config.mk     |   35 ++
 board/xes/xpedite5370/ddr.c         |  270 ++++++++++++++++
 board/xes/xpedite5370/law.c         |   54 ++++
 board/xes/xpedite5370/tlb.c         |   94 ++++++
 board/xes/xpedite5370/u-boot.lds    |  145 +++++++++
 board/xes/xpedite5370/xpedite5370.c |  128 ++++++++
 drivers/gpio/Makefile               |   47 +++
 drivers/gpio/pca953x.c              |  218 +++++++++++++
 drivers/misc/Makefile               |    1 +
 drivers/misc/ds4510.c               |  400 ++++++++++++++++++++++++
 include/configs/XPEDITE5370.h       |  589 +++++++++++++++++++++++++++++++++++
 include/gpio/ds4510.h               |   75 +++++
 include/gpio/pca953x.h              |   39 +++
 22 files changed, 2627 insertions(+), 0 deletions(-)
 create mode 100644 board/xes/common/Makefile
 create mode 100644 board/xes/common/fsl_8572_clk.c
 create mode 100644 board/xes/common/fsl_85xx_ddr.c
 create mode 100644 board/xes/common/fsl_85xx_pci.c
 create mode 100644 board/xes/xpedite5370/Makefile
 create mode 100644 board/xes/xpedite5370/config.mk
 create mode 100644 board/xes/xpedite5370/ddr.c
 create mode 100644 board/xes/xpedite5370/law.c
 create mode 100644 board/xes/xpedite5370/tlb.c
 create mode 100644 board/xes/xpedite5370/u-boot.lds
 create mode 100644 board/xes/xpedite5370/xpedite5370.c
 create mode 100644 drivers/gpio/Makefile
 create mode 100644 drivers/gpio/pca953x.c
 create mode 100644 drivers/misc/ds4510.c
 create mode 100644 include/configs/XPEDITE5370.h
 create mode 100644 include/gpio/ds4510.h
 create mode 100644 include/gpio/pca953x.h
#1 lore raw

PATCH v4 1/3 Add support for PCA953x I2C gpio devices

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. The CONFIG_CMD_PCA953X_INFO define enables an 'info' sub-command which provides summary information for the given pca953x device.

---
 Makefile               |    2 +
 README                 |    9 ++
 drivers/gpio/Makefile  |   47 ++++++++++
 drivers/gpio/pca953x.c |  218 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/gpio/pca953x.h |   39 +++++++++
 5 files changed, 315 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/Makefile
 create mode 100644 drivers/gpio/pca953x.c
 create mode 100644 include/gpio/pca953x.h
Makefile +2 -0
index fd521b6..0c5dd61 100644
@@ -221,6 +221,7 @@ LIBS += disk/libdisk.a
 LIBS += drivers/bios_emulator/libatibiosemu.a
 LIBS += drivers/block/libblock.a
 LIBS += drivers/dma/libdma.a
+LIBS += drivers/gpio/libgpio.a
 LIBS += drivers/hwmon/libhwmon.a
 LIBS += drivers/i2c/libi2c.a
 LIBS += drivers/input/libinput.a
@@ -396,6 +397,7 @@ TAG_SUBDIRS += disk
 TAG_SUBDIRS += common
 TAG_SUBDIRS += drivers/bios_emulator
 TAG_SUBDIRS += drivers/block
+TAG_SUBDIRS += drivers/gpio
 TAG_SUBDIRS += drivers/hwmon
 TAG_SUBDIRS += drivers/i2c
 TAG_SUBDIRS += drivers/input
README +9 -0
index 9455fa7..bca8061 100644
@@ -603,6 +603,8 @@ The following options need to be configured:
 		CONFIG_CMD_MII		* MII utility commands
 		CONFIG_CMD_NAND		* NAND support
 		CONFIG_CMD_NET		  bootp, tftpboot, rarpboot
+		CONFIG_CMD_PCA953X	* PCA953x I2C gpio commands
+		CONFIG_CMD_PCA953X_INFO	* PCA953x I2C gpio info command
 		CONFIG_CMD_PCI		* pciinfo
 		CONFIG_CMD_PCMCIA		* PCMCIA support
 		CONFIG_CMD_PING		* send ICMP ECHO_REQUEST to network
@@ -680,6 +682,13 @@ The following options need to be configured:
 		Note that if the RTC uses I2C, then the I2C interface
 		must also be configured. See I2C Support, below.
 
+- GPIO Support:
+		CONFIG_PCA953X		- use NXP's PCA953X series I2C GPIO
+		CONFIG_PCA953X_INFO	- enable pca953x info command
+
+		Note that if the GPIO device uses I2C, then the I2C interface
+		must also be configured. See I2C Support, below.
+
 - Timestamp Support:
 
 		When CONFIG_TIMESTAMP is selected, the timestamp
drivers/gpio/Makefile new file +47 -0

47 lines not on this page · raw message

drivers/gpio/pca953x.c new file +218 -0

218 lines not on this page · raw message

include/gpio/pca953x.h new file +39 -0
new file mode 100644
index 0000000..6c2b58c
@@ -0,0 +1,39 @@
+/*
+ * Copyright 2008 Extreme Engineering Solutions, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * Version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef __PCA953X_H_
+#define __PCA953X_H_
+
+#define PCA953X_IN		0x00
+#define PCA953X_OUT		0x01
+#define PCA953X_POL		0x02
+#define PCA953X_CONF		0x03
+
+#define PCA953X_OUT_LOW		0
+#define PCA953X_OUT_HIGH	1
+#define PCA953X_POL_NORMAL	0
+#define PCA953X_POL_INVERT	1
+#define PCA953X_DIR_OUT		0
+#define PCA953X_DIR_IN		1
+
+int pca953x_set_val(u8 chip, uint mask, uint data);
+int pca953x_set_pol(u8 chip, uint mask, uint data);
+int pca953x_set_dir(u8 chip, uint mask, uint data);
+int pca953x_get_val(u8 chip);
+
+#endif /* __PCA953X_H_ */
Signature · 1 line
1.6.0.2.GIT
#2 lore raw

PATCH v4 2/3 Add support for Maxim's DS4510 I2C device

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. The additional CONFIG_DS4510_INFO, CONFIG_DS4510_MEM, and CONFIG_DS4510_RST defines add additional sub-commands to the ds4510 command when defined.

---
 README                |    4 +
 drivers/misc/Makefile |    1 +
 drivers/misc/ds4510.c |  400 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/gpio/ds4510.h |   75 +++++++++
 4 files changed, 480 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/ds4510.c
 create mode 100644 include/gpio/ds4510.h
README +4 -0
index bca8061..cea057d 100644
@@ -574,6 +574,10 @@ The following options need to be configured:
 		CONFIG_CMD_DHCP		* DHCP support
 		CONFIG_CMD_DIAG		* Diagnostics
 		CONFIG_CMD_DOC		* Disk-On-Chip Support
+		CONFIG_CMD_DS4510	* ds4510 I2C gpio commands
+		CONFIG_CMD_DS4510_INFO	* ds4510 I2C info command
+		CONFIG_CMD_DS4510_MEM	* ds4510 I2C eeprom/sram commansd
+		CONFIG_CMD_DS4510_RST	* ds4510 I2C rst command
 		CONFIG_CMD_DTT		* Digital Therm and Thermostat
 		CONFIG_CMD_ECHO		  echo arguments
 		CONFIG_CMD_EEPROM	* EEPROM read/write support
drivers/misc/Makefile +1 -0
index 01e0f39..ea2bf87 100644
@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
 LIB	:= $(obj)libmisc.a
 
 COBJS-$(CONFIG_ALI152X) += ali512x.o
+COBJS-$(CONFIG_DS4510)  += ds4510.o
 COBJS-$(CONFIG_FSL_LAW) += fsl_law.o
 COBJS-$(CONFIG_NS87308) += ns87308.o
 COBJS-$(CONFIG_STATUS_LED) += status_led.o
drivers/misc/ds4510.c new file +400 -0

400 lines not on this page · raw message

include/gpio/ds4510.h new file +75 -0

75 lines not on this page · raw message

Signature · 1 line
1.6.0.2.GIT
#3 lore raw

PATCH v4 3/3 XPedite5370 board support

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

---
 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     |  265 ++++++++++++++++
 board/xes/xpedite5370/Makefile      |   45 +++
 board/xes/xpedite5370/config.mk     |   35 ++
 board/xes/xpedite5370/ddr.c         |  270 ++++++++++++++++
 board/xes/xpedite5370/law.c         |   54 ++++
 board/xes/xpedite5370/tlb.c         |   94 ++++++
 board/xes/xpedite5370/u-boot.lds    |  145 +++++++++
 board/xes/xpedite5370/xpedite5370.c |  128 ++++++++
 include/configs/XPEDITE5370.h       |  589 +++++++++++++++++++++++++++++++++++
 15 files changed, 1832 insertions(+), 0 deletions(-)
 create mode 100644 board/xes/common/Makefile
 create mode 100644 board/xes/common/fsl_8572_clk.c
 create mode 100644 board/xes/common/fsl_85xx_ddr.c
 create mode 100644 board/xes/common/fsl_85xx_pci.c
 create mode 100644 board/xes/xpedite5370/Makefile
 create mode 100644 board/xes/xpedite5370/config.mk
 create mode 100644 board/xes/xpedite5370/ddr.c
 create mode 100644 board/xes/xpedite5370/law.c
 create mode 100644 board/xes/xpedite5370/tlb.c
 create mode 100644 board/xes/xpedite5370/u-boot.lds
 create mode 100644 board/xes/xpedite5370/xpedite5370.c
 create mode 100644 include/configs/XPEDITE5370.h
MAINTAINERS +3 -0
index 127604b..ee98824 100644
@@ -411,6 +411,9 @@ Rune Torgersen <[email protected]>
 
 	MPC8266ADS	MPC8266
 
+Peter Tyser <[email protected]>
+
+	XPEDITE5370	MPC8572
 
 David Updegraff <[email protected]>
 
MAKEALL +1 -0
index dbed268..1328514 100755
@@ -376,6 +376,7 @@ LIST_85xx="		\
 	TQM8548		\
 	TQM8555		\
 	TQM8560		\
+	XPEDITE5370	\
 "
 
 #########################################################################
Makefile +3 -0
index 0c5dd61..ae3ec22 100644
@@ -2454,6 +2454,9 @@ TQM8560_config:		unconfig
 	echo "#define CONFIG_BOARDNAME \"TQM$${CTYPE}\"">>$(obj)include/config.h;
 	@$(MKCONFIG) -a TQM85xx ppc mpc85xx tqm85xx tqc
 
+XPEDITE5370_config:	unconfig
+	@$(MKCONFIG) $(@:_config=) ppc mpc85xx xpedite5370 xes
+
 #########################################################################
 ## MPC86xx Systems
 #########################################################################
board/xes/common/Makefile new file +56 -0

56 lines not on this page · raw message

board/xes/common/fsl_8572_clk.c new file +51 -0

51 lines not on this page · raw message

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

93 lines not on this page · raw message

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

265 lines not on this page · raw message

board/xes/xpedite5370/Makefile new file +45 -0

45 lines not on this page · raw message

board/xes/xpedite5370/config.mk new file +35 -0
new file mode 100644
index 0000000..39469b2
@@ -0,0 +1,35 @@
+#
+# Copyright 2008 Extreme Engineering Solutions, Inc.
+# Copyright 2007-2008 Freescale Semiconductor, Inc.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+#
+# xpedite5370 board
+#
+ifndef TEXT_BASE
+TEXT_BASE = 0xfff80000
+endif
+
+PLATFORM_RELFLAGS += -mrelocatable
+
+PLATFORM_CPPFLAGS += -DCONFIG_E500=1
+PLATFORM_CPPFLAGS += -DCONFIG_MPC85xx=1
+PLATFORM_CPPFLAGS += -DCONFIG_MPC8572=1
board/xes/xpedite5370/ddr.c new file +270 -0

270 lines not on this page · raw message

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

54 lines not on this page · raw message

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

94 lines not on this page · raw message

board/xes/xpedite5370/u-boot.lds new file +145 -0

145 lines not on this page · raw message

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

128 lines not on this page · raw message

include/configs/XPEDITE5370.h new file +589 -0

589 lines not on this page · raw message

Signature · 1 line
1.6.0.2.GIT
#4 lore raw

PATCH v4 3/3 XPedite5370 board support

+ }
+#else
+ gur->devdisr |= MPC85xx_DEVDISR_PCIE1; /* disable */
+#endif /* CONFIG_PCIE2 */

MPC85xx_DEVDISR_PCIE1 isn't defined anywhere. Did you miss some changes you made to header files?

Andy

#5 lore raw

PATCH v4 3/3 XPedite5370 board support

Quoted 8 lines · Andy Fleming
board/xes/common/fsl_85xx_pci.c new file +4 -0
+       }
+#else
+       gur->devdisr |= MPC85xx_DEVDISR_PCIE1; /* disable */
+#endif /* CONFIG_PCIE2 */

MPC85xx_DEVDISR_PCIE1 isn't defined anywhere. Did you miss some changes you made to header files?

Thanks for catching that. I noticed the same DEVDISR_PCIEx value was being used for PCIE2 and PCIE3 in v2 of the patch so quickly "fixed" it in v3 by changing the DEVDISR value for PCIE2. That fix should have been changing the DEVDISR value for PCIE3, not PCIE2. I'll resubmit.

Thanks,
Peter

#6 lore raw

PATCH v4 2/3 Add support for Maxim's DS4510 I2C device

In reply to Peter Tyser

Dear Peter Tyser,

Quoted 8 lines · In message <300544b7901dacbe6b9e3edd052629f612b92735.1228160312.git.ptyser@xes…

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. The additional CONFIG_DS4510_INFO, CONFIG_DS4510_MEM, and CONFIG_DS4510_RST defines add additional sub-commands to the ds4510 command when defined.

General note: the code shows a serious lack of error detection and error handling.

Quoted 9 lines
---
 README                |    4 +
 drivers/misc/Makefile |    1 +
 drivers/misc/ds4510.c |  400 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/gpio/ds4510.h |   75 +++++++++
 4 files changed, 480 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/ds4510.c
 create mode 100644 include/gpio/ds4510.h

The driver would probably be better placed in the drivers/hwmon/ directory.

Also, I don't see a reason to create a new include/gpio/ directory here, especially since this is not primarily a GPIO device.

README The following options need to be configured: · line 580

Quoted 12 lines
README +4 -0
index bca8061..cea057d 100644
@@ -574,6 +574,10 @@ The following options need to be configured:
 		CONFIG_CMD_DHCP		* DHCP support
 		CONFIG_CMD_DIAG		* Diagnostics
 		CONFIG_CMD_DOC		* Disk-On-Chip Support
+		CONFIG_CMD_DS4510	* ds4510 I2C gpio commands
+		CONFIG_CMD_DS4510_INFO	* ds4510 I2C info command
+		CONFIG_CMD_DS4510_MEM	* ds4510 I2C eeprom/sram commansd
+		CONFIG_CMD_DS4510_RST	* ds4510 I2C rst command

Do we really need such granularity? If you have the device on your board and suppoort it in U-Boot, you probably always want to have CONFIG_CMD_DS4510_INFO and CONFIG_CMD_DS4510_RST. And CONFIG_CMD_DS4510_MEM seems reduindand - that should already be covered by the CONFIG_CMD_EEPROM setting.

Quoted 5 lines
drivers/misc/ds4510.c new file +0 -0
new file mode 100644
index 0000000..572dcb7

...

Quoted 4 lines
drivers/misc/ds4510.c new file +4 -0
+/*
+ * Write to DS4510, taking page boundaries into account
+ */
+int ds4510_mem_write(uint8_t chip, int offset, uint8_t *buf, int count)

drivers/misc/ds4510.c line 60

Quoted 10 lines
drivers/misc/ds4510.c new file +10 -0
+{
+	int wrlen;
+	int i = 0;
+
+	do {
+		wrlen = DS4510_EEPROM_PAGE_SIZE -
+			DS4510_EEPROM_PAGE_OFFSET(offset);
+		if (count < wrlen)
+			wrlen = count;
+		i2c_write(chip, offset, 1, &buf[i], wrlen);

i2c_write() can fail - it returns error codes. I'm missing error handling here and elsewhere. I'll flag a few locations, but please check all the code.

drivers/misc/ds4510.c line 151

Quoted 7 lines
drivers/misc/ds4510.c new file +7 -0
+int ds4510_gpio_write(uint8_t chip, uint8_t val)
+{
+	uint8_t data;
+	int i;
+
+	for (i = 0; i < DS4510_NUM_IO; i++) {
+		i2c_read(chip, DS4510_IO0 - i, 1, &data, 1);

Error handling?

drivers/misc/ds4510.c line 158

Quoted 6 lines
drivers/misc/ds4510.c new file +6 -0
+		if (val & (0x1 << i))
+			data |= 0x1;
+		else
+			data &= ~0x1;
+
+		ds4510_mem_write(chip, DS4510_IO0 - i, &data, 1);

Error handling?

drivers/misc/ds4510.c line 174

Quoted 8 lines
drivers/misc/ds4510.c new file +8 -0
+int ds4510_gpio_read(uint8_t chip)
+{
+	uint8_t data;
+	int val = 0;
+	int i;
+
+	for (i = 0; i < DS4510_NUM_IO; i++) {
+		i2c_read(chip, DS4510_IO0 - i, 1, &data, 1);

Error handling?

drivers/misc/ds4510.c line 209

Quoted 9 lines
drivers/misc/ds4510.c new file +9 -0
+static int ds4510_info(uint8_t chip)
+{
+	int i;
+	int tmp;
+	uint8_t data;
+
+	printf("DS4510 @ 0x%x:\n\n", chip);
+
+	i2c_read(chip, DS4510_RSTDELAY, 1, &data, 1);

error handling?

drivers/misc/ds4510.c line 212

Quoted 3 lines
drivers/misc/ds4510.c new file +3 -0
+	printf("rstdelay = 0x%x\n\n", data & DS4510_RSTDELAY_MASK);
+
+	i2c_read(chip, DS4510_CFG, 1, &data, 1);

error handling?

drivers/misc/ds4510.c line 246

Quoted 2 lines
drivers/misc/ds4510.c new file +2 -0
+/* Use 'maxargs' field as minimum number of args to ease error checking  */
+cmd_tbl_t cmd_ds4510[] = {

Please do not mis-use variables with a clearly defined meaning in such a way. This is not acceptable.

drivers/misc/ds4510.c line 398

Quoted 11 lines
drivers/misc/ds4510.c new file +11 -0
+#ifdef CONFIG_CMD_DS4510_MEM
+	"ds4510 eeprom read addr off cnt\n"
+	"ds4510 eeprom write addr off cnt\n"
+	"	- read/write 'cnt' bytes at EEPROM offset 'off'\n"
+	"ds4510 seeprom read addr off cnt\n"
+	"ds4510 seeprom write addr off cnt\n"
+	"	- read/write 'cnt' bytes at SRAM-shadowed EEPROM offset 'off'\n"
+	"ds4510 sram read addr off cnt\n"
+	"ds4510 sram write addr off cnt\n"
+	"	- read/write 'cnt' bytes at SRAM offset 'off'\n"
+#endif

We should not need a separate EEPROM command here, I think.

And cannot this SRAM be written using plain "cp" commands?

Best regards,

Wolfgang Denk

Signature · 5 lines
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
For every problem there is one solution which is  simple,  neat,  and
wrong.                                                - H. L. Mencken
#7 lore raw

PATCH v4 2/3 Add support for Maxim's DS4510 I2C device

Hi Wolfgang,
Thanks for the comments.

Quoted 12 lines · Wolfgang Denk

Dear Peter Tyser,

Quoted 8 lines · In message <300544b7901dacbe6b9e3edd052629f612b92735.1228160312.git.ptyser@xes…

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. The additional CONFIG_DS4510_INFO, CONFIG_DS4510_MEM, and CONFIG_DS4510_RST defines add additional sub-commands to the ds4510 command when defined.

Quoted 2 lines

General note: the code shows a serious lack of error detection and error handling.

OK, I'll fix it.

Quoted 10 lines
Quoted 9 lines
---
 README                |    4 +
 drivers/misc/Makefile |    1 +
 drivers/misc/ds4510.c |  400 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/gpio/ds4510.h |   75 +++++++++
 4 files changed, 480 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/ds4510.c
 create mode 100644 include/gpio/ds4510.h
Quoted 2 lines

The driver would probably be better placed in the drivers/hwmon/ directory.

You commented in the original version of the patch:
"I think drivers/misc/ is probably the best place we have at the moment, indeed."

I think drivers/misc makes the most sense, but hwmon is fine too. Let me know what you prefer.

Quoted 2 lines

Also, I don't see a reason to create a new include/gpio/ directory here, especially since this is not primarily a GPIO device.

The original patch series had the pca953x.h (a purely GPIO device) and ds4510.h in include/gpio/. You're right that the ds4510.h doesn't belong in include/gpio, I'll move it to include/. I'm assuming you'd also like the pca953x.h in this patch series to be moved from include/gpio to include?

Quoted 10 lines
Quoted 10 lines
README +2 -0
index bca8061..cea057d 100644
@@ -574,6 +574,10 @@ The following options need to be configured:
 		CONFIG_CMD_DHCP		* DHCP support
 		CONFIG_CMD_DIAG		* Diagnostics
 		CONFIG_CMD_DOC		* Disk-On-Chip Support
+		CONFIG_CMD_DS4510	* ds4510 I2C gpio commands
+		CONFIG_CMD_DS4510_INFO	* ds4510 I2C info command
Quoted 8 lines

README The following options need to be configured: · line 580

Quoted 2 lines
README +2 -0
+		CONFIG_CMD_DS4510_MEM	* ds4510 I2C eeprom/sram commansd
+		CONFIG_CMD_DS4510_RST	* ds4510 I2C rst command

Do we really need such granularity? If you have the device on your board and suppoort it in U-Boot, you probably always want to have CONFIG_CMD_DS4510_INFO and CONFIG_CMD_DS4510_RST. And CONFIG_CMD_DS4510_MEM seems reduindand - that should already be covered by the CONFIG_CMD_EEPROM setting.

I added finer granularity based on a comment you made about the pca953x "info" command:
"Don't remove it, but maybe make it a selectable option so those who like it can include it while others that are tryting to minimize the memory footprint don't suffer from it."

If the "info" command could be enabled/disabled I reasoned the other functions of the chip should also be.

The original patch didn't have any command-enabling granularity. For our hardware setups, we only use the DS4510 as a non-volatile GPIO device, the EEPROM, SRAM, and reset monitor functionality are not used (I know, the part seems a bit overkill for what we use it for, but I didn't design the hardware:).

The standard EEPROM commands could be used on the device, but I think it would get confusing as part of the device's memory space is EEPROM only, part SRAM only, and the other part EEPROM or SRAM depending on register setting in the device. Thus the eeprom command's functionality would depend on the settings of the DS4510 device (or be limited only to the EEPROM portion of the device), and the ds4510 command would support the SRAM, but not EEPROM which is confusing in my opinion.

So in any case, all I really care about is the GPIO functionality of the chip as is - I added EEPROM, reset monitor, and SRAM support to cover all uses of the chip. I'm fine with removing the finer granularity command enabling/disabling, or even removing non-GPIO functionality if you'd prefer:) I'd vote for leaving the command enabling/disabling implementation as is, let me know if you'd like it changed.

Quoted 5 lines

README The following options need to be configured: · line 582

Quoted 2 lines
README +2 -0
+/* Use 'maxargs' field as minimum number of args to ease error checking  */
+cmd_tbl_t cmd_ds4510[] = {

Please do not mis-use variables with a clearly defined meaning in such a way. This is not acceptable.

Alright, I'll rework this and also the pca953x.c implementation in this patchset.

Quoted 5 lines
Quoted 5 lines
README +5 -0
+#ifdef CONFIG_CMD_DS4510_MEM
+	"ds4510 eeprom read addr off cnt\n"
+	"ds4510 eeprom write addr off cnt\n"
+	"	- read/write 'cnt' bytes at EEPROM offset 'off'\n"
+	"ds4510 seeprom read addr off cnt\n"
Quoted 8 lines

README

Quoted 6 lines
README +6 -0
+	"ds4510 seeprom write addr off cnt\n"
+	"	- read/write 'cnt' bytes at SRAM-shadowed EEPROM offset 'off'\n"
+	"ds4510 sram read addr off cnt\n"
+	"ds4510 sram write addr off cnt\n"
+	"	- read/write 'cnt' bytes at SRAM offset 'off'\n"
+#endif

We should not need a separate EEPROM command here, I think.

See comments above.

Quoted 1 line

And cannot this SRAM be written using plain "cp" commands?

The device is only accessible via I2C, so the "cp" commands would not work.

Thanks for the comments,
Peter