[U-Boot] [PATCH 1/4] gpio: Add driver for TI PCF8575 I2C GPIO expander
Vignesh R
vigneshr at ti.com
Mon Aug 1 10:54:44 CEST 2016
On Monday 01 August 2016 06:31 AM, Simon Glass wrote:
> Hi,
>
> On 25 July 2016 at 07:10, Vignesh R <vigneshr at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list