On Monday 01 August 2016 06:31 AM, Simon Glass wrote: > Hi, > > On 25 July 2016 at 07:10, Vignesh R <vigne...@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 <vigne...@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 <s...@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@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 <vigne...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot