[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