[U-Boot] [PATCH 0/4] Add support for TI PCF8575 I2C GPIO expander

[U-Boot] [PATCH 0/4] Add support for TI PCF8575 I2C GPIO expander

[Vignesh R]

This series adds support for TI's PCF8575 I2C GPIO expander[1] based on
Linux Kernel driver for the same.  Also adds support for ethernet to use
new PCF8575 driver to select appropriate cpsw slaves on dra72.

Depends on I2C DM support [2]

Tested on DRA72 EVM.

[1]http://www.ti.com/lit/ds/symlink/pcf8575.pdf
[2]https://www.mail-archive.com/u-boot at lists.denx.de/msg218923.html

Vignesh R (4):
  gpio: Add driver for TI PCF8575 I2C GPIO expander
  ARM: dra7xx_evm: Enable support for TI PCF8575
  net: cpsw: Add support to drive gpios for ethernet to be functional
  ARM: dts: dra72-evm: Add mode-gpios entry for mac node

 arch/arm/dts/dra72-evm.dts                     |   2 +-
 doc/device-tree-bindings/gpio/gpio-pcf857x.txt |  71 +++++++++
 drivers/gpio/Kconfig                           |   7 +
 drivers/gpio/Makefile                          |   1 +
 drivers/gpio/pcf8575_gpio.c                    | 199 +++++++++++++++++++++++++
 drivers/net/cpsw.c                             |  13 ++
 include/configs/dra7xx_evm.h                   |   5 +
 7 files changed, 297 insertions(+), 1 deletion(-)
 create mode 100644 doc/device-tree-bindings/gpio/gpio-pcf857x.txt
 create mode 100644 drivers/gpio/pcf8575_gpio.c

-- 
2.9.2

[U-Boot] [PATCH 1/4] gpio: Add driver for TI PCF8575 I2C GPIO expander

[Vignesh R]

TI's PCF8575 is a 16-bit I2C GPIO expander.The device features a
16-bit quasi-bidirectional I/O ports. Each quasi-bidirectional I/O can
be used as an input or output without the use of a data-direction
control signal. The I/Os should be high before being used as inputs.
Read the device documentation for more details[1].

This driver is based on pcf857x driver available in Linux v4.7 kernel.
It supports basic reading and writing of gpio pins.

[1] http://www.ti.com/lit/ds/symlink/pcf8575.pdf

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 doc/device-tree-bindings/gpio/gpio-pcf857x.txt |  71 +++++++++
 drivers/gpio/Kconfig                           |   7 +
 drivers/gpio/Makefile                          |   1 +
 drivers/gpio/pcf8575_gpio.c                    | 199 +++++++++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 doc/device-tree-bindings/gpio/gpio-pcf857x.txt
 create mode 100644 drivers/gpio/pcf8575_gpio.c

diff --git a/doc/device-tree-bindings/gpio/gpio-pcf857x.txt b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
new file mode 100644
index 000000000000..ada4e2973323
--- /dev/null
+++ b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
@@ -0,0 +1,71 @@
+* PCF857x-compatible I/O expanders
+
+The PCF857x-compatible chips have "quasi-bidirectional" I/O lines that can be
+driven high by a pull-up current source or driven low to ground. This combines
+the direction and output level into a single bit per line, which can't be read
+back. We can't actually know at initialization time whether a line is configured
+(a) as output and driving the signal low/high, or (b) as input and reporting a
+low/high value, without knowing the last value written since the chip came out
+of reset (if any). The only reliable solution for setting up line direction is
+thus to do it explicitly.
+
+Required Properties:
+
+  - compatible: should be one of the following.
+    - "maxim,max7328": For the Maxim MAX7378
+    - "maxim,max7329": For the Maxim MAX7329
+    - "nxp,pca8574": For the NXP PCA8574
+    - "nxp,pca8575": For the NXP PCA8575
+    - "nxp,pca9670": For the NXP PCA9670
+    - "nxp,pca9671": For the NXP PCA9671
+    - "nxp,pca9672": For the NXP PCA9672
+    - "nxp,pca9673": For the NXP PCA9673
+    - "nxp,pca9674": For the NXP PCA9674
+    - "nxp,pca9675": For the NXP PCA9675
+    - "nxp,pcf8574": For the NXP PCF8574
+    - "nxp,pcf8574a": For the NXP PCF8574A
+    - "nxp,pcf8575": For the NXP PCF8575
+    - "ti,tca9554": For the TI TCA9554
+
+  - reg: I2C slave address.
+
+  - gpio-controller: Marks the device node as a gpio controller.
+  - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
+    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
+    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
+
+Optional Properties:
+
+  - lines-initial-states: Bitmask that specifies the initial state of each
+  line. When a bit is set to zero, the corresponding line will be initialized to
+  the input (pulled-up) state. When the  bit is set to one, the line will be
+  initialized the low-level output state. If the property is not specified
+  all lines will be initialized to the input state.
+
+  The I/O expander can detect input state changes, and thus optionally act as
+  an interrupt controller. When the expander interrupt line is connected all the
+  following properties must be set. For more information please see the
+  interrupt controller device tree bindings documentation available at
+  Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
+
+  - interrupt-controller: Identifies the node as an interrupt controller.
+  - #interrupt-cells: Number of cells to encode an interrupt source, shall be 2.
+  - interrupt-parent: phandle of the parent interrupt controller.
+  - interrupts: Interrupt specifier for the controllers interrupt.
+
+
+Please refer to gpio.txt in this directory for details of the common GPIO
+bindings used by client devices.
+
+Example: PCF8575 I/O expander node
+
+	pcf8575: gpio at 20 {
+		compatible = "nxp,pcf8575";
+		reg = <0x20>;
+		interrupt-parent = <&irqpin2>;
+		interrupts = <3 0>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 73b862dc0b21..1af05358ec76 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -79,6 +79,13 @@ config PM8916_GPIO
 	  Power and reset buttons are placed in "pm8916_key" bank and
           have gpio numbers 0 and 1 respectively.
 
+config PCF8575_GPIO
+	bool "PCF8575 I2C GPIO Expander driver"
+	depends on DM_GPIO && DM_I2C
+	help
+	 Support for PCF8575 I2C 16 bit GPIO expander. Most of these
+	 chips are from NXP and TI.
+
 config ROCKCHIP_GPIO
 	bool "Rockchip GPIO driver"
 	depends on DM_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 792d19186aad..8f426c82cdca 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
 obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
 obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
 obj-$(CONFIG_PM8916_GPIO)	+= pm8916_gpio.o
+obj-$(CONFIG_PCF8575_GPIO)	+= pcf8575_gpio.o
diff --git a/drivers/gpio/pcf8575_gpio.c b/drivers/gpio/pcf8575_gpio.c
new file mode 100644
index 000000000000..3ca97eee11db
--- /dev/null
+++ b/drivers/gpio/pcf8575_gpio.c
@@ -0,0 +1,199 @@
+/*
+ * PCF8575 I2C GPIO EXPANDER DRIVER
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Vignesh R <vigneshr@ti.com>
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ *
+ *
+ * Driver for TI PCF-8575 16 bit I2C gpio expander. Based on
+ * gpio-pcf857x Linux Kernel(v4.7) driver.
+ *
+ * Copyright (C) 2007 David Brownell
+ *
+ */
+
+/*
+ * NOTE: The driver and devicetree bindings are borrowed from Linux
+ * Kernel, but driver does not support all PCF857x devices. It currently
+ * supports PCF8575 16Bit expander by TI and NXP.
+ *
+ * TODO:
+ * Support 8 bit PCF857x compatible expanders.
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+#include <asm-generic/gpio.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct pcf8575_chip {
+	int gpio_count;		/* No GPIOs supported by the chip */
+	unsigned int out;	/* software latch */
+	const char *bank_name;	/* Name of the expander bank */
+};
+
+/* Read/Write to 16-bit I/O expander */
+
+static int pcf8575_i2c_write_le16(struct udevice *dev, unsigned int word)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	struct i2c_msg msg;
+	u8 buf[2] = { word & 0xff, word >> 8, };
+	int ret;
+
+	msg.addr = chip->chip_addr;
+	msg.buf = buf;
+	msg.flags = 0;
+	msg.len = 2;
+	ret = dm_i2c_xfer(dev, &msg, 1);
+
+	if (ret)
+		printf("%s i2c write failed to addr %x\n", __func__, msg.addr);
+
+	return ret;
+}
+
+static int pcf8575_i2c_read_le16(struct udevice *dev)
+{
+	struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
+	struct i2c_msg msg;
+	u8 buf[2];
+	int ret;
+
+	msg.addr = chip->chip_addr;
+	msg.buf = buf;
+	msg.flags = I2C_M_RD;
+	msg.len = 2;
+
+	ret = dm_i2c_xfer(dev, &msg, 1);
+	if (ret) {
+		printf("%s i2c read failed to addr %x\n", __func__, msg.addr);
+		return ret;
+	}
+
+	return (msg.buf[1] << 8) | msg.buf[0];
+}
+
+static int pcf8575_direction_input(struct udevice *dev, unsigned offset)
+{
+	struct pcf8575_chip *pc = dev_get_platdata(dev);
+	int status;
+
+	pc->out |= BIT(offset);
+	status = pcf8575_i2c_write_le16(dev, pc->out);
+
+	return status;
+}
+
+static int pcf8575_direction_output(struct udevice *dev,
+				    unsigned int offset, int value)
+{
+	struct pcf8575_chip *pc = dev_get_platdata(dev);
+	int ret;
+
+	if (value)
+		pc->out |= BIT(offset);
+	else
+		pc->out &= ~BIT(offset);
+
+	ret = pcf8575_i2c_write_le16(dev, pc->out);
+
+	return ret;
+}
+
+static int pcf8575_get_value(struct udevice *dev, unsigned int offset)
+{
+	int             value;
+
+	value = pcf8575_i2c_read_le16(dev);
+
+	return (value < 0) ? value : ((value & BIT(offset)) >> offset);
+}
+
+static int pcf8575_set_value(struct udevice *dev, unsigned int offset,
+			     int value)
+{
+	return pcf8575_direction_output(dev, offset, value);
+}
+
+static int pcf8575_ofdata_platdata(struct udevice *dev)
+{
+	struct pcf8575_chip *pc = dev_get_platdata(dev);
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+
+	int n_latch;
+
+	uc_priv->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+					     "gpio-count", 16);
+	uc_priv->bank_name = fdt_getprop(gd->fdt_blob, dev->of_offset,
+					 "gpio-bank-name", NULL);
+	if (!uc_priv->bank_name)
+		uc_priv->bank_name = fdt_get_name(gd->fdt_blob,
+						  dev->of_offset, NULL);
+
+	/* NOTE:  these chips have strange "quasi-bidirectional" I/O pins.
+	 * We can't actually know whether a pin is configured (a) as output
+	 * and driving the signal low, or (b) as input and reporting a low
+	 * value ... without knowing the last value written since the chip
+	 * came out of reset (if any).  We can't read the latched output.
+	 * In short, the only reliable solution for setting up pin direction
+	 * is to do it explicitly.
+	 *
+	 * Using n_latch avoids that trouble.  When left initialized to zero,
+	 * our software copy of the "latch" then matches the chip's all-ones
+	 * reset state.  Otherwise it flags pins to be driven low.
+	 */
+
+	n_latch = fdtdec_get_uint(gd->fdt_blob, dev->of_offset,
+				  "lines-initial-states", 0);
+	pc->out = ~n_latch;
+
+	return 0;
+}
+
+static int pcf8575_gpio_probe(struct udevice  *dev)
+{
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+
+	debug("%s GPIO controller with %d gpios probed\n",
+	      uc_priv->bank_name, uc_priv->gpio_count);
+
+	return 0;
+}
+
+static const struct dm_gpio_ops pcf8575_gpio_ops = {
+	.direction_input	= pcf8575_direction_input,
+	.direction_output	= pcf8575_direction_output,
+	.get_value		= pcf8575_get_value,
+	.set_value		= pcf8575_set_value,
+};
+
+static const struct udevice_id pcf8575_gpio_ids[] = {
+	{ .compatible = "nxp,pcf8575" },
+	{ .compatible = "ti,pcf8575" },
+	{ }
+};
+
+U_BOOT_DRIVER(gpio_pcf8575) = {
+	.name	= "gpio_pcf8575",
+	.id	= UCLASS_GPIO,
+	.ops	= &pcf8575_gpio_ops,
+	.of_match = pcf8575_gpio_ids,
+	.ofdata_to_platdata = pcf8575_ofdata_platdata,
+	.probe	= pcf8575_gpio_probe,
+	.platdata_auto_alloc_size = sizeof(struct pcf8575_chip),
+};
-- 
2.9.2

[U-Boot] [PATCH 2/4] ARM: dra7xx_evm: Enable support for TI PCF8575

[Vignesh R]

On DRA7, pcf chip present at address 0x21 on i2c1, is used to
switch between cpsw slave0 and slave1. Hence, enable PCF
driver for the same.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 include/configs/dra7xx_evm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/configs/dra7xx_evm.h b/include/configs/dra7xx_evm.h
index 0d51aeb86909..4ad8beaf2f75 100644
--- a/include/configs/dra7xx_evm.h
+++ b/include/configs/dra7xx_evm.h
@@ -328,4 +328,9 @@
 #define CONFIG_EEPROM_CHIP_ADDRESS 0x50
 #define CONFIG_EEPROM_BUS_ADDRESS 0
 
+/* PCF Support */
+#ifndef CONFIG_SPL_BUILD
+#define CONFIG_PCF8575_GPIO
+#endif
+
 #endif /* __CONFIG_DRA7XX_EVM_H */
-- 
2.9.2

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[Vignesh R]

On DRA72 EVM, cpsw slaves may be muxed with other modules. This
selection is controlled by a pcf gpio line. Add support for cpsw driver
to acquire mode-gpios and select the appropriate slave using gpio APIs.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/net/cpsw.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c
index 2ce4ec69f1df..83be336cf39c 100644
--- a/drivers/net/cpsw.c
+++ b/drivers/net/cpsw.c
@@ -22,6 +22,7 @@
 #include <netdev.h>
 #include <cpsw.h>
 #include <asm/errno.h>
+#include <asm/gpio.h>
 #include <asm/io.h>
 #include <phy.h>
 #include <asm/arch/cpu.h>
@@ -1152,12 +1153,14 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	struct cpsw_priv *priv = dev_get_priv(dev);
+	struct gpio_desc *mode_gpios;
 	const char *phy_mode;
 	const void *fdt = gd->fdt_blob;
 	int node = dev->of_offset;
 	int subnode;
 	int slave_index = 0;
 	int active_slave;
+	int num_mode_gpios;
 	int ret;
 
 	pdata->iobase = dev_get_addr(dev);
@@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
 		return -ENOENT;
 	}
 
+
+	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
+	if (num_mode_gpios > 0) {
+		mode_gpios = malloc(sizeof(struct gpio_desc) *
+				    num_mode_gpios);
+		gpio_request_list_by_name(dev, "mode-gpios", mode_gpios,
+					  num_mode_gpios, GPIOD_IS_OUT);
+		free(mode_gpios);
+	}
+
 	active_slave = fdtdec_get_int(fdt, node, "active_slave", 0);
 	priv->data.active_slave = active_slave;
 
-- 
2.9.2

[U-Boot] [PATCH 4/4] ARM: dts: dra72-evm: Add mode-gpios entry for mac node

[Vignesh R]

On DRA72 EVM, cpsw slave1 is muxed with VIN2A, hence switch to cpsw
slave0 for ethernet. This is controlled by pcf gpio line. Add
appropriate mode-gpios DT entry so that driver can select the required
slave.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 arch/arm/dts/dra72-evm.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/dra72-evm.dts b/arch/arm/dts/dra72-evm.dts
index ced2f1166d8c..aee570520aa6 100644
--- a/arch/arm/dts/dra72-evm.dts
+++ b/arch/arm/dts/dra72-evm.dts
@@ -576,6 +576,7 @@
 	pinctrl-names = "default", "sleep";
 	pinctrl-0 = <&cpsw_default>;
 	pinctrl-1 = <&cpsw_sleep>;
+	mode-gpios = <&pcf_gpio_21 4 GPIO_ACTIVE_HIGH>;
 };
 
 &cpsw_emac1 {
@@ -587,7 +588,6 @@
 	pinctrl-names = "default", "sleep";
 	pinctrl-0 = <&davinci_mdio_default>;
 	pinctrl-1 = <&davinci_mdio_sleep>;
-	active_slave = <1>;
 };
 
 &dcan1 {
-- 
2.9.2

[U-Boot] [PATCH 1/4] gpio: Add driver for TI PCF8575 I2C GPIO expander

[Tom Rini]

On Mon, Jul 25, 2016 at 06:40:20PM +0530, Vignesh R wrote:

> TI's PCF8575 is a 16-bit I2C GPIO expander.The device features a
> 16-bit quasi-bidirectional I/O ports. Each quasi-bidirectional I/O can
> be used as an input or output without the use of a data-direction
> control signal. The I/Os should be high before being used as inputs.
> Read the device documentation for more details[1].
> 
> This driver is based on pcf857x driver available in Linux v4.7 kernel.
> It supports basic reading and writing of gpio pins.
> 
> [1] http://www.ti.com/lit/ds/symlink/pcf8575.pdf
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
[snip]
> @@ -0,0 +1,199 @@
> +/*
> + * PCF8575 I2C GPIO EXPANDER DRIVER
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Vignesh R <vigneshr@ti.com>
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * SPDX-License-Identifier:	GPL-2.0

With the SPDX tag we don't need the rest of the GPL boilerplate.

> + *
> + *
> + * Driver for TI PCF-8575 16 bit I2C gpio expander. Based on
> + * gpio-pcf857x Linux Kernel(v4.7) driver.
> + *
> + * Copyright (C) 2007 David Brownell

But of course do keep the above.

Otherwise:
Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160725/d85bea79/attachment.sig>

[U-Boot] [PATCH 2/4] ARM: dra7xx_evm: Enable support for TI PCF8575

[Tom Rini]

On Mon, Jul 25, 2016 at 06:40:21PM +0530, Vignesh R wrote:

> On DRA7, pcf chip present at address 0x21 on i2c1, is used to
> switch between cpsw slave0 and slave1. Hence, enable PCF
> driver for the same.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  include/configs/dra7xx_evm.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/configs/dra7xx_evm.h b/include/configs/dra7xx_evm.h
> index 0d51aeb86909..4ad8beaf2f75 100644
> --- a/include/configs/dra7xx_evm.h
> +++ b/include/configs/dra7xx_evm.h
> @@ -328,4 +328,9 @@
>  #define CONFIG_EEPROM_CHIP_ADDRESS 0x50
>  #define CONFIG_EEPROM_BUS_ADDRESS 0
>  
> +/* PCF Support */
> +#ifndef CONFIG_SPL_BUILD
> +#define CONFIG_PCF8575_GPIO
> +#endif

We need to invert the logic and undef on SPL, and pick this up normally
via Kconfig.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160725/abb7d706/attachment.sig>

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[Tom Rini]

On Mon, Jul 25, 2016 at 06:40:22PM +0530, Vignesh R wrote:

> On DRA72 EVM, cpsw slaves may be muxed with other modules. This
> selection is controlled by a pcf gpio line. Add support for cpsw driver
> to acquire mode-gpios and select the appropriate slave using gpio APIs.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

Minor nit below:

> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
>  		return -ENOENT;
>  	}
>  
> +
> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");

Extra blank line added.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160725/241cb11c/attachment.sig>

[U-Boot] [PATCH 4/4] ARM: dts: dra72-evm: Add mode-gpios entry for mac node

[Tom Rini]

On Mon, Jul 25, 2016 at 06:40:23PM +0530, Vignesh R wrote:

> On DRA72 EVM, cpsw slave1 is muxed with VIN2A, hence switch to cpsw
> slave0 for ethernet. This is controlled by pcf gpio line. Add
> appropriate mode-gpios DT entry so that driver can select the required
> slave.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160725/e305115c/attachment.sig>

[U-Boot] [PATCH 2/4] ARM: dra7xx_evm: Enable support for TI PCF8575

[Vignesh R]

On Monday 25 July 2016 07:08 PM, Tom Rini wrote:
> On Mon, Jul 25, 2016 at 06:40:21PM +0530, Vignesh R wrote:
> 
>> On DRA7, pcf chip present at address 0x21 on i2c1, is used to
>> switch between cpsw slave0 and slave1. Hence, enable PCF
>> driver for the same.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  include/configs/dra7xx_evm.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/configs/dra7xx_evm.h b/include/configs/dra7xx_evm.h
>> index 0d51aeb86909..4ad8beaf2f75 100644
>> --- a/include/configs/dra7xx_evm.h
>> +++ b/include/configs/dra7xx_evm.h
>> @@ -328,4 +328,9 @@
>>  #define CONFIG_EEPROM_CHIP_ADDRESS 0x50
>>  #define CONFIG_EEPROM_BUS_ADDRESS 0
>>  
>> +/* PCF Support */
>> +#ifndef CONFIG_SPL_BUILD
>> +#define CONFIG_PCF8575_GPIO
>> +#endif
> 
> We need to invert the logic and undef on SPL, and pick this up normally
> via Kconfig.
> 

I tried that initially but, the logic does not seem to work. Since, the
driver is DM only driver, undef'ing CONFIG_PCF8575_GPIO on
CONFIG_SPL_BUILD does seem to take any effect, the driver is still being
compiled for SPL build and results in compile time errors. It seems that
undef in include/configs/* file does not seem to take effect in
Makefile, but only takes effect in the code. Any idea what am I missing?

-- 
Regards
Vignesh

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[R, Vignesh]

On 7/25/2016 7:08 PM, Tom Rini wrote:
> On Mon, Jul 25, 2016 at 06:40:22PM +0530, Vignesh R wrote:
> 
>> On DRA72 EVM, cpsw slaves may be muxed with other modules. This
>> selection is controlled by a pcf gpio line. Add support for cpsw driver
>> to acquire mode-gpios and select the appropriate slave using gpio APIs.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> Minor nit below:
> 
>> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
>>  		return -ENOENT;
>>  	}
>>  
>> +
>> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
> 
> Extra blank line added.

I added blank line for readability. So, that code block handling
toggling of "mode-gpios" is separated from the rest. I can remove that
if that seems unnecessary.

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[Wolfgang Denk]

Dear "R, Vignesh",

In message <a210f61e-bf51-a946-013c-dd98fd9a17b0@ti.com> you wrote:
> 
> >> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
> >>  		return -ENOENT;
> >>  	}
> >>  
> >> +
> >> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
> > 
> > Extra blank line added.
> 
> I added blank line for readability. So, that code block handling
> toggling of "mode-gpios" is separated from the rest. I can remove that
> if that seems unnecessary.

There was already a blank line which provides enough separation. No
need to have multiple consecutive blank lines.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
In an infinite universe all things are possible, including the possi-
bility that the universe does not exist.
                        - Terry Pratchett, _The Dark Side of the Sun_

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[R, Vignesh]

On 7/26/2016 12:26 PM, Wolfgang Denk wrote:
> Dear "R, Vignesh",
> 
> In message <a210f61e-bf51-a946-013c-dd98fd9a17b0@ti.com> you wrote:
>>
>>>> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
>>>>  		return -ENOENT;
>>>>  	}
>>>>  
>>>> +
>>>> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
>>>
>>> Extra blank line added.
>>
>> I added blank line for readability. So, that code block handling
>> toggling of "mode-gpios" is separated from the rest. I can remove that
>> if that seems unnecessary.
> 
> There was already a blank line which provides enough separation. No
> need to have multiple consecutive blank lines.

Sorry, will clean that up in v2. Thanks!

Regards
Vignesh

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[Tom Rini]

On Tue, Jul 26, 2016 at 12:07:26PM +0530, R, Vignesh wrote:
> 
> 
> On 7/25/2016 7:08 PM, Tom Rini wrote:
> > On Mon, Jul 25, 2016 at 06:40:22PM +0530, Vignesh R wrote:
> > 
> >> On DRA72 EVM, cpsw slaves may be muxed with other modules. This
> >> selection is controlled by a pcf gpio line. Add support for cpsw driver
> >> to acquire mode-gpios and select the appropriate slave using gpio APIs.
> >>
> >> Signed-off-by: Vignesh R <vigneshr@ti.com>
> > 
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > 
> > Minor nit below:
> > 
> >> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
> >>  		return -ENOENT;
> >>  	}
> >>  
> >> +
> >> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
> > 
> > Extra blank line added.
> 
> I added blank line for readability. So, that code block handling
> toggling of "mode-gpios" is separated from the rest. I can remove that
> if that seems unnecessary.

Two empty lines in a row just isn't done so yes please, thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160726/65cab7be/attachment.sig>

[U-Boot] [PATCH 3/4] net: cpsw: Add support to drive gpios for ethernet to be functional

[Vignesh R]

On Tuesday 26 July 2016 05:43 PM, Tom Rini wrote:
> On Tue, Jul 26, 2016 at 12:07:26PM +0530, R, Vignesh wrote:
>>
>>
>> On 7/25/2016 7:08 PM, Tom Rini wrote:
>>> On Mon, Jul 25, 2016 at 06:40:22PM +0530, Vignesh R wrote:
>>>
>>>> On DRA72 EVM, cpsw slaves may be muxed with other modules. This
>>>> selection is controlled by a pcf gpio line. Add support for cpsw driver
>>>> to acquire mode-gpios and select the appropriate slave using gpio APIs.
>>>>
>>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>>
>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>>
>>> Minor nit below:
>>>
>>>> @@ -1203,6 +1206,16 @@ static int cpsw_eth_ofdata_to_platdata(struct udevice *dev)
>>>>  		return -ENOENT;
>>>>  	}
>>>>  
>>>> +
>>>> +	num_mode_gpios = gpio_get_list_count(dev, "mode-gpios");
>>>
>>> Extra blank line added.
>>
>> I added blank line for readability. So, that code block handling
>> toggling of "mode-gpios" is separated from the rest. I can remove that
>> if that seems unnecessary.
> 
> Two empty lines in a row just isn't done so yes please, thanks.

Sorry, I overlooked two empty lines.  Will fix this in v2. Thanks for
the review.

-- 
Regards
Vignesh

[U-Boot] [PATCH 1/4] gpio: Add driver for TI PCF8575 I2C GPIO expander

[Simon Glass]

Hi,

On 25 July 2016 at 07:10, Vignesh R <vigneshr@ti.com> wrote:
> TI's PCF8575 is a 16-bit I2C GPIO expander.The device features a
> 16-bit quasi-bidirectional I/O ports. Each quasi-bidirectional I/O can
> be used as an input or output without the use of a data-direction
> control signal. The I/Os should be high before being used as inputs.
> Read the device documentation for more details[1].
>
> This driver is based on pcf857x driver available in Linux v4.7 kernel.
> It supports basic reading and writing of gpio pins.
>
> [1] http://www.ti.com/lit/ds/symlink/pcf8575.pdf
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  doc/device-tree-bindings/gpio/gpio-pcf857x.txt |  71 +++++++++
>  drivers/gpio/Kconfig                           |   7 +
>  drivers/gpio/Makefile                          |   1 +
>  drivers/gpio/pcf8575_gpio.c                    | 199 +++++++++++++++++++++++++
>  4 files changed, 278 insertions(+)
>  create mode 100644 doc/device-tree-bindings/gpio/gpio-pcf857x.txt
>  create mode 100644 drivers/gpio/pcf8575_gpio.c

Reviewed-by: Simon Glass <sjg@chromium.org>

A few nits below.

>
> diff --git a/doc/device-tree-bindings/gpio/gpio-pcf857x.txt b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
> new file mode 100644
> index 000000000000..ada4e2973323
> --- /dev/null
> +++ b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
> @@ -0,0 +1,71 @@
> +* PCF857x-compatible I/O expanders
> +
> +The PCF857x-compatible chips have "quasi-bidirectional" I/O lines that can be
> +driven high by a pull-up current source or driven low to ground. This combines
> +the direction and output level into a single bit per line, which can't be read
> +back. We can't actually know at initialization time whether a line is configured
> +(a) as output and driving the signal low/high, or (b) as input and reporting a
> +low/high value, without knowing the last value written since the chip came out
> +of reset (if any). The only reliable solution for setting up line direction is
> +thus to do it explicitly.
> +
> +Required Properties:
> +
> +  - compatible: should be one of the following.
> +    - "maxim,max7328": For the Maxim MAX7378
> +    - "maxim,max7329": For the Maxim MAX7329
> +    - "nxp,pca8574": For the NXP PCA8574
> +    - "nxp,pca8575": For the NXP PCA8575
> +    - "nxp,pca9670": For the NXP PCA9670
> +    - "nxp,pca9671": For the NXP PCA9671
> +    - "nxp,pca9672": For the NXP PCA9672
> +    - "nxp,pca9673": For the NXP PCA9673
> +    - "nxp,pca9674": For the NXP PCA9674
> +    - "nxp,pca9675": For the NXP PCA9675
> +    - "nxp,pcf8574": For the NXP PCF8574
> +    - "nxp,pcf8574a": For the NXP PCF8574A
> +    - "nxp,pcf8575": For the NXP PCF8575
> +    - "ti,tca9554": For the TI TCA9554
> +
> +  - reg: I2C slave address.
> +
> +  - gpio-controller: Marks the device node as a gpio controller.
> +  - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
> +    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
> +    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> +
> +Optional Properties:
> +
> +  - lines-initial-states: Bitmask that specifies the initial state of each
> +  line. When a bit is set to zero, the corresponding line will be initialized to
> +  the input (pulled-up) state. When the  bit is set to one, the line will be
> +  initialized the low-level output state. If the property is not specified
> +  all lines will be initialized to the input state.
> +
> +  The I/O expander can detect input state changes, and thus optionally act as
> +  an interrupt controller. When the expander interrupt line is connected all the
> +  following properties must be set. For more information please see the
> +  interrupt controller device tree bindings documentation available at
> +  Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
> +
> +  - interrupt-controller: Identifies the node as an interrupt controller.
> +  - #interrupt-cells: Number of cells to encode an interrupt source, shall be 2.
> +  - interrupt-parent: phandle of the parent interrupt controller.
> +  - interrupts: Interrupt specifier for the controllers interrupt.
> +
> +
> +Please refer to gpio.txt in this directory for details of the common GPIO
> +bindings used by client devices.
> +
> +Example: PCF8575 I/O expander node
> +
> +       pcf8575: gpio at 20 {
> +               compatible = "nxp,pcf8575";
> +               reg = <0x20>;
> +               interrupt-parent = <&irqpin2>;
> +               interrupts = <3 0>;
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +               interrupt-controller;
> +               #interrupt-cells = <2>;
> +       };
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 73b862dc0b21..1af05358ec76 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -79,6 +79,13 @@ config PM8916_GPIO
>           Power and reset buttons are placed in "pm8916_key" bank and
>            have gpio numbers 0 and 1 respectively.
>
> +config PCF8575_GPIO
> +       bool "PCF8575 I2C GPIO Expander driver"
> +       depends on DM_GPIO && DM_I2C
> +       help
> +        Support for PCF8575 I2C 16 bit GPIO expander. Most of these

16-bit

> +        chips are from NXP and TI.

You mention a single chip and then say 'most of these chips'. Is there
a series of chips, or are there lot of different chips that are
compatible? Can you update the help to be a bit more specific?

Can you briefly summarise the features of the driver? E.g. can set
each to input or output (I think). Anything else?

> +
>  config ROCKCHIP_GPIO
>         bool "Rockchip GPIO driver"
>         depends on DM_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 792d19186aad..8f426c82cdca 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_PIC32_GPIO)      += pic32_gpio.o
>  obj-$(CONFIG_MVEBU_GPIO)       += mvebu_gpio.o
>  obj-$(CONFIG_MSM_GPIO)         += msm_gpio.o
>  obj-$(CONFIG_PM8916_GPIO)      += pm8916_gpio.o
> +obj-$(CONFIG_PCF8575_GPIO)     += pcf8575_gpio.o

Can you move that up one line to maintain order?

> diff --git a/drivers/gpio/pcf8575_gpio.c b/drivers/gpio/pcf8575_gpio.c
> new file mode 100644
> index 000000000000..3ca97eee11db
> --- /dev/null
> +++ b/drivers/gpio/pcf8575_gpio.c
> @@ -0,0 +1,199 @@
> +/*
> + * PCF8575 I2C GPIO EXPANDER DRIVER
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Vignesh R <vigneshr@ti.com>
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + *
> + *
> + * Driver for TI PCF-8575 16 bit I2C gpio expander. Based on
> + * gpio-pcf857x Linux Kernel(v4.7) driver.
> + *
> + * Copyright (C) 2007 David Brownell
> + *
> + */
> +
> +/*
> + * NOTE: The driver and devicetree bindings are borrowed from Linux
> + * Kernel, but driver does not support all PCF857x devices. It currently
> + * supports PCF8575 16Bit expander by TI and NXP.
> + *
> + * TODO:

TODO(email)

> + * Support 8 bit PCF857x compatible expanders.
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <asm-generic/gpio.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct pcf8575_chip {
> +       int gpio_count;         /* No GPIOs supported by the chip */

s/No/Number of/

or s/No/No./

> +       unsigned int out;       /* software latch */

Can you explain that one a bit more?

> +       const char *bank_name;  /* Name of the expander bank */
> +};
> +
> +/* Read/Write to 16-bit I/O expander */
> +
> +static int pcf8575_i2c_write_le16(struct udevice *dev, unsigned int word)
> +{
> +       struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> +       struct i2c_msg msg;
> +       u8 buf[2] = { word & 0xff, word >> 8, };
> +       int ret;
> +
> +       msg.addr = chip->chip_addr;
> +       msg.buf = buf;
> +       msg.flags = 0;
> +       msg.len = 2;
> +       ret = dm_i2c_xfer(dev, &msg, 1);

Can you use dm_i2c_write()?

> +
> +       if (ret)
> +               printf("%s i2c write failed to addr %x\n", __func__, msg.addr);
> +
> +       return ret;
> +}
> +
> +static int pcf8575_i2c_read_le16(struct udevice *dev)
> +{
> +       struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
> +       struct i2c_msg msg;
> +       u8 buf[2];
> +       int ret;
> +
> +       msg.addr = chip->chip_addr;
> +       msg.buf = buf;
> +       msg.flags = I2C_M_RD;
> +       msg.len = 2;
> +
> +       ret = dm_i2c_xfer(dev, &msg, 1);
> +       if (ret) {
> +               printf("%s i2c read failed to addr %x\n", __func__, msg.addr);
> +               return ret;
> +       }
> +
> +       return (msg.buf[1] << 8) | msg.buf[0];

Does dm_i2c_read() work instead of calling dm_i2c_xfer()?

> +}
> +
> +static int pcf8575_direction_input(struct udevice *dev, unsigned offset)
> +{
> +       struct pcf8575_chip *pc = dev_get_platdata(dev);
> +       int status;
> +
> +       pc->out |= BIT(offset);
> +       status = pcf8575_i2c_write_le16(dev, pc->out);
> +
> +       return status;
> +}
> +
> +static int pcf8575_direction_output(struct udevice *dev,
> +                                   unsigned int offset, int value)
> +{
> +       struct pcf8575_chip *pc = dev_get_platdata(dev);
> +       int ret;
> +
> +       if (value)
> +               pc->out |= BIT(offset);
> +       else
> +               pc->out &= ~BIT(offset);
> +
> +       ret = pcf8575_i2c_write_le16(dev, pc->out);
> +
> +       return ret;
> +}
> +
> +static int pcf8575_get_value(struct udevice *dev, unsigned int offset)
> +{
> +       int             value;
> +
> +       value = pcf8575_i2c_read_le16(dev);
> +
> +       return (value < 0) ? value : ((value & BIT(offset)) >> offset);
> +}
> +
> +static int pcf8575_set_value(struct udevice *dev, unsigned int offset,
> +                            int value)
> +{
> +       return pcf8575_direction_output(dev, offset, value);
> +}
> +
> +static int pcf8575_ofdata_platdata(struct udevice *dev)
> +{
> +       struct pcf8575_chip *pc = dev_get_platdata(dev);

s/pc/plat/

(it's what most drivers use)

> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +
> +       int n_latch;
> +
> +       uc_priv->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> +                                            "gpio-count", 16);
> +       uc_priv->bank_name = fdt_getprop(gd->fdt_blob, dev->of_offset,
> +                                        "gpio-bank-name", NULL);
> +       if (!uc_priv->bank_name)
> +               uc_priv->bank_name = fdt_get_name(gd->fdt_blob,
> +                                                 dev->of_offset, NULL);
> +
> +       /* NOTE:  these chips have strange "quasi-bidirectional" I/O pins.
> +        * We can't actually know whether a pin is configured (a) as output
> +        * and driving the signal low, or (b) as input and reporting a low
> +        * value ... without knowing the last value written since the chip
> +        * came out of reset (if any).  We can't read the latched output.
> +        * In short, the only reliable solution for setting up pin direction
> +        * is to do it explicitly.
> +        *
> +        * Using n_latch avoids that trouble.  When left initialized to zero,
> +        * our software copy of the "latch" then matches the chip's all-ones
> +        * reset state.  Otherwise it flags pins to be driven low.
> +        */
> +
> +       n_latch = fdtdec_get_uint(gd->fdt_blob, dev->of_offset,
> +                                 "lines-initial-states", 0);
> +       pc->out = ~n_latch;
> +
> +       return 0;
> +}
> +
> +static int pcf8575_gpio_probe(struct udevice  *dev)
> +{
> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +
> +       debug("%s GPIO controller with %d gpios probed\n",
> +             uc_priv->bank_name, uc_priv->gpio_count);
> +
> +       return 0;
> +}
> +
> +static const struct dm_gpio_ops pcf8575_gpio_ops = {
> +       .direction_input        = pcf8575_direction_input,
> +       .direction_output       = pcf8575_direction_output,
> +       .get_value              = pcf8575_get_value,
> +       .set_value              = pcf8575_set_value,
> +};
> +
> +static const struct udevice_id pcf8575_gpio_ids[] = {
> +       { .compatible = "nxp,pcf8575" },
> +       { .compatible = "ti,pcf8575" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(gpio_pcf8575) = {
> +       .name   = "gpio_pcf8575",
> +       .id     = UCLASS_GPIO,
> +       .ops    = &pcf8575_gpio_ops,
> +       .of_match = pcf8575_gpio_ids,
> +       .ofdata_to_platdata = pcf8575_ofdata_platdata,
> +       .probe  = pcf8575_gpio_probe,
> +       .platdata_auto_alloc_size = sizeof(struct pcf8575_chip),
> +};
> --
> 2.9.2
>

Regards,
Simon

[U-Boot] [PATCH 2/4] ARM: dra7xx_evm: Enable support for TI PCF8575

[Simon Glass]

Hi Vignesh,

On 26 July 2016 at 00:33, Vignesh R <vigneshr@ti.com> wrote:
>
>
> On Monday 25 July 2016 07:08 PM, Tom Rini wrote:
>> On Mon, Jul 25, 2016 at 06:40:21PM +0530, Vignesh R wrote:
>>
>>> On DRA7, pcf chip present at address 0x21 on i2c1, is used to
>>> switch between cpsw slave0 and slave1. Hence, enable PCF
>>> driver for the same.
>>>
>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>> ---
>>>  include/configs/dra7xx_evm.h | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/include/configs/dra7xx_evm.h b/include/configs/dra7xx_evm.h
>>> index 0d51aeb86909..4ad8beaf2f75 100644
>>> --- a/include/configs/dra7xx_evm.h
>>> +++ b/include/configs/dra7xx_evm.h
>>> @@ -328,4 +328,9 @@
>>>  #define CONFIG_EEPROM_CHIP_ADDRESS 0x50
>>>  #define CONFIG_EEPROM_BUS_ADDRESS 0
>>>
>>> +/* PCF Support */
>>> +#ifndef CONFIG_SPL_BUILD
>>> +#define CONFIG_PCF8575_GPIO
>>> +#endif
>>
>> We need to invert the logic and undef on SPL, and pick this up normally
>> via Kconfig.
>>
>
> I tried that initially but, the logic does not seem to work. Since, the
> driver is DM only driver, undef'ing CONFIG_PCF8575_GPIO on
> CONFIG_SPL_BUILD does seem to take any effect, the driver is still being
> compiled for SPL build and results in compile time errors. It seems that
> undef in include/configs/* file does not seem to take effect in
> Makefile, but only takes effect in the code. Any idea what am I missing?

Can you have CONFIG_PCF8575_GPIO and CONFIG_SPL_PCF8575_GPIO?

Then

obj-CONFIG_$(SPL_)PCF8575_GPIO += ...

Regards,
Simon

[U-Boot] [PATCH 2/4] ARM: dra7xx_evm: Enable support for TI PCF8575

[Vignesh R]

Hi,

On Monday 01 August 2016 06:31 AM, Simon Glass wrote:
[...]
>>>> +/* PCF Support */
>>>> +#ifndef CONFIG_SPL_BUILD
>>>> +#define CONFIG_PCF8575_GPIO
>>>> +#endif
>>>
>>> We need to invert the logic and undef on SPL, and pick this up normally
>>> via Kconfig.
>>>
>>
>> I tried that initially but, the logic does not seem to work. Since, the
>> driver is DM only driver, undef'ing CONFIG_PCF8575_GPIO on
>> CONFIG_SPL_BUILD does seem to take any effect, the driver is still being
>> compiled for SPL build and results in compile time errors. It seems that
>> undef in include/configs/* file does not seem to take effect in
>> Makefile, but only takes effect in the code. Any idea what am I missing?
> 
> Can you have CONFIG_PCF8575_GPIO and CONFIG_SPL_PCF8575_GPIO?
> 
> Then
> 
> obj-CONFIG_$(SPL_)PCF8575_GPIO += ...

Thanks! Will make changes accordingly...

-- 
Regards
Vignesh

[U-Boot] [PATCH 1/4] gpio: Add driver for TI PCF8575 I2C GPIO expander

[Vignesh R]

On Monday 01 August 2016 06:31 AM, Simon Glass wrote:
> Hi,
> 
> On 25 July 2016 at 07:10, Vignesh R <vigneshr@ti.com> wrote:
>> TI's PCF8575 is a 16-bit I2C GPIO expander.The device features a
>> 16-bit quasi-bidirectional I/O ports. Each quasi-bidirectional I/O can
>> be used as an input or output without the use of a data-direction
>> control signal. The I/Os should be high before being used as inputs.
>> Read the device documentation for more details[1].
>>
>> This driver is based on pcf857x driver available in Linux v4.7 kernel.
>> It supports basic reading and writing of gpio pins.
>>
>> [1] http://www.ti.com/lit/ds/symlink/pcf8575.pdf
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  doc/device-tree-bindings/gpio/gpio-pcf857x.txt |  71 +++++++++
>>  drivers/gpio/Kconfig                           |   7 +
>>  drivers/gpio/Makefile                          |   1 +
>>  drivers/gpio/pcf8575_gpio.c                    | 199 +++++++++++++++++++++++++
>>  4 files changed, 278 insertions(+)
>>  create mode 100644 doc/device-tree-bindings/gpio/gpio-pcf857x.txt
>>  create mode 100644 drivers/gpio/pcf8575_gpio.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> A few nits below.
> 
>>
>> diff --git a/doc/device-tree-bindings/gpio/gpio-pcf857x.txt b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
>> new file mode 100644
>> index 000000000000..ada4e2973323
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/gpio/gpio-pcf857x.txt
>> @@ -0,0 +1,71 @@
>> +* PCF857x-compatible I/O expanders
>> +
>> +The PCF857x-compatible chips have "quasi-bidirectional" I/O lines that can be
>> +driven high by a pull-up current source or driven low to ground. This combines
>> +the direction and output level into a single bit per line, which can't be read
>> +back. We can't actually know at initialization time whether a line is configured
>> +(a) as output and driving the signal low/high, or (b) as input and reporting a
>> +low/high value, without knowing the last value written since the chip came out
>> +of reset (if any). The only reliable solution for setting up line direction is
>> +thus to do it explicitly.
>> +
>> +Required Properties:
>> +
>> +  - compatible: should be one of the following.
>> +    - "maxim,max7328": For the Maxim MAX7378
>> +    - "maxim,max7329": For the Maxim MAX7329
>> +    - "nxp,pca8574": For the NXP PCA8574
>> +    - "nxp,pca8575": For the NXP PCA8575
>> +    - "nxp,pca9670": For the NXP PCA9670
>> +    - "nxp,pca9671": For the NXP PCA9671
>> +    - "nxp,pca9672": For the NXP PCA9672
>> +    - "nxp,pca9673": For the NXP PCA9673
>> +    - "nxp,pca9674": For the NXP PCA9674
>> +    - "nxp,pca9675": For the NXP PCA9675
>> +    - "nxp,pcf8574": For the NXP PCF8574
>> +    - "nxp,pcf8574a": For the NXP PCF8574A
>> +    - "nxp,pcf8575": For the NXP PCF8575
>> +    - "ti,tca9554": For the TI TCA9554
>> +
>> +  - reg: I2C slave address.
>> +
>> +  - gpio-controller: Marks the device node as a gpio controller.
>> +  - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
>> +    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
>> +    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
>> +
>> +Optional Properties:
>> +
>> +  - lines-initial-states: Bitmask that specifies the initial state of each
>> +  line. When a bit is set to zero, the corresponding line will be initialized to
>> +  the input (pulled-up) state. When the  bit is set to one, the line will be
>> +  initialized the low-level output state. If the property is not specified
>> +  all lines will be initialized to the input state.
>> +
>> +  The I/O expander can detect input state changes, and thus optionally act as
>> +  an interrupt controller. When the expander interrupt line is connected all the
>> +  following properties must be set. For more information please see the
>> +  interrupt controller device tree bindings documentation available at
>> +  Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
>> +
>> +  - interrupt-controller: Identifies the node as an interrupt controller.
>> +  - #interrupt-cells: Number of cells to encode an interrupt source, shall be 2.
>> +  - interrupt-parent: phandle of the parent interrupt controller.
>> +  - interrupts: Interrupt specifier for the controllers interrupt.
>> +
>> +
>> +Please refer to gpio.txt in this directory for details of the common GPIO
>> +bindings used by client devices.
>> +
>> +Example: PCF8575 I/O expander node
>> +
>> +       pcf8575: gpio at 20 {
>> +               compatible = "nxp,pcf8575";
>> +               reg = <0x20>;
>> +               interrupt-parent = <&irqpin2>;
>> +               interrupts = <3 0>;
>> +               gpio-controller;
>> +               #gpio-cells = <2>;
>> +               interrupt-controller;
>> +               #interrupt-cells = <2>;
>> +       };
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 73b862dc0b21..1af05358ec76 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -79,6 +79,13 @@ config PM8916_GPIO
>>           Power and reset buttons are placed in "pm8916_key" bank and
>>            have gpio numbers 0 and 1 respectively.
>>
>> +config PCF8575_GPIO
>> +       bool "PCF8575 I2C GPIO Expander driver"
>> +       depends on DM_GPIO && DM_I2C
>> +       help
>> +        Support for PCF8575 I2C 16 bit GPIO expander. Most of these
> 
> 16-bit

Ok.

> 
>> +        chips are from NXP and TI.
> 
> You mention a single chip and then say 'most of these chips'. Is there
> a series of chips, or are there lot of different chips that are
> compatible? Can you update the help to be a bit more specific?
> 
> Can you briefly summarise the features of the driver? E.g. can set
> each to input or output (I think). Anything else?
> 
>> +
>>  config ROCKCHIP_GPIO
>>         bool "Rockchip GPIO driver"
>>         depends on DM_GPIO
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 792d19186aad..8f426c82cdca 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -57,3 +57,4 @@ obj-$(CONFIG_PIC32_GPIO)      += pic32_gpio.o
>>  obj-$(CONFIG_MVEBU_GPIO)       += mvebu_gpio.o
>>  obj-$(CONFIG_MSM_GPIO)         += msm_gpio.o
>>  obj-$(CONFIG_PM8916_GPIO)      += pm8916_gpio.o
>> +obj-$(CONFIG_PCF8575_GPIO)     += pcf8575_gpio.o
> 
> Can you move that up one line to maintain order?

Looking at drivers/gpio/Makefile, there does not seem to be alphabetical
order(or any other order) at all. Hence, I appended this to the end of
the list. Anyways, I will move it one line.

> 
>> diff --git a/drivers/gpio/pcf8575_gpio.c b/drivers/gpio/pcf8575_gpio.c
>> new file mode 100644
>> index 000000000000..3ca97eee11db
>> --- /dev/null
>> +++ b/drivers/gpio/pcf8575_gpio.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * PCF8575 I2C GPIO EXPANDER DRIVER
>> + *
>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * Vignesh R <vigneshr@ti.com>
>> + *
>> + * 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 version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0
>> + *
>> + *
>> + * Driver for TI PCF-8575 16 bit I2C gpio expander. Based on
>> + * gpio-pcf857x Linux Kernel(v4.7) driver.
>> + *
>> + * Copyright (C) 2007 David Brownell
>> + *
>> + */
>> +
>> +/*
>> + * NOTE: The driver and devicetree bindings are borrowed from Linux
>> + * Kernel, but driver does not support all PCF857x devices. It currently
>> + * supports PCF8575 16Bit expander by TI and NXP.
>> + *
>> + * TODO:
> 
> TODO(email)

ok.

> 
>> + * Support 8 bit PCF857x compatible expanders.
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <i2c.h>
>> +#include <asm-generic/gpio.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +struct pcf8575_chip {
>> +       int gpio_count;         /* No GPIOs supported by the chip */
> 
> s/No/Number of/
> 
> or s/No/No./

Will use this.

> 
>> +       unsigned int out;       /* software latch */
> 
> Can you explain that one a bit more?

I kept the explanation in pcf8575_ofdata_platdata, will move that here.

> 
>> +       const char *bank_name;  /* Name of the expander bank */
>> +};
>> +
>> +/* Read/Write to 16-bit I/O expander */
>> +
>> +static int pcf8575_i2c_write_le16(struct udevice *dev, unsigned int word)
>> +{
>> +       struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
>> +       struct i2c_msg msg;
>> +       u8 buf[2] = { word & 0xff, word >> 8, };
>> +       int ret;
>> +
>> +       msg.addr = chip->chip_addr;
>> +       msg.buf = buf;
>> +       msg.flags = 0;
>> +       msg.len = 2;
>> +       ret = dm_i2c_xfer(dev, &msg, 1);
> 
> Can you use dm_i2c_write(?

I thought dm_i2c_xfer() is the preferred interface. Moreover, PCF8575
I2C chip does not have any register hence offset param in dm_i2c_read()
needs to be ignored. Looks like, this can be achieved by using DT property:
	u-boot,i2c-offset-len = <0>;

I will do the changes to use dm_i2c_write()/dm_i2c_read() and send v3.

> 
>> +
>> +       if (ret)
>> +               printf("%s i2c write failed to addr %x\n", __func__, msg.addr);
>> +
>> +       return ret;
>> +}
>> +
>> +static int pcf8575_i2c_read_le16(struct udevice *dev)
>> +{
>> +       struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
>> +       struct i2c_msg msg;
>> +       u8 buf[2];
>> +       int ret;
>> +
>> +       msg.addr = chip->chip_addr;
>> +       msg.buf = buf;
>> +       msg.flags = I2C_M_RD;
>> +       msg.len = 2;
>> +
>> +       ret = dm_i2c_xfer(dev, &msg, 1);
>> +       if (ret) {
>> +               printf("%s i2c read failed to addr %x\n", __func__, msg.addr);
>> +               return ret;
>> +       }
>> +
>> +       return (msg.buf[1] << 8) | msg.buf[0];
> 
> Does dm_i2c_read() work instead of calling dm_i2c_xfer()?
> 
>> +}
>> +
>> +static int pcf8575_direction_input(struct udevice *dev, unsigned offset)
>> +{
>> +       struct pcf8575_chip *pc = dev_get_platdata(dev);
>> +       int status;
>> +
>> +       pc->out |= BIT(offset);
>> +       status = pcf8575_i2c_write_le16(dev, pc->out);
>> +
>> +       return status;
>> +}
>> +
>> +static int pcf8575_direction_output(struct udevice *dev,
>> +                                   unsigned int offset, int value)
>> +{
>> +       struct pcf8575_chip *pc = dev_get_platdata(dev);
>> +       int ret;
>> +
>> +       if (value)
>> +               pc->out |= BIT(offset);
>> +       else
>> +               pc->out &= ~BIT(offset);
>> +
>> +       ret = pcf8575_i2c_write_le16(dev, pc->out);
>> +
>> +       return ret;
>> +}
>> +
>> +static int pcf8575_get_value(struct udevice *dev, unsigned int offset)
>> +{
>> +       int             value;
>> +
>> +       value = pcf8575_i2c_read_le16(dev);
>> +
>> +       return (value < 0) ? value : ((value & BIT(offset)) >> offset);
>> +}
>> +
>> +static int pcf8575_set_value(struct udevice *dev, unsigned int offset,
>> +                            int value)
>> +{
>> +       return pcf8575_direction_output(dev, offset, value);
>> +}
>> +
>> +static int pcf8575_ofdata_platdata(struct udevice *dev)
>> +{
>> +       struct pcf8575_chip *pc = dev_get_platdata(dev);
> 
> s/pc/plat/
> 
> (it's what most drivers use)

Ok.

Thanks for the review!

-- 
Regards
Vignesh