Re: [PATCH 2/2 v3] gpio: Add a Gateworks PLD GPIO driver

[Bartosz Golaszewski <brgl@xxxxxxxx>]

pt., 1 lut 2019 o 21:44 Linus Walleij <linus.walleij@xxxxxxxxxx> napisał(a):
>
> This adds a driver for Gateworks PLD GPIO, that exist in
> two instances on the Gateworks Cambria GW2358-4 router
> platform at least.
>
> Cc: Imre Kaloz <kaloz@xxxxxxxxxxx>
> Cc: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - Fix more unsigned -> unsigned int warnings, I was too trigger
>   happy with v2 :/
> ChangeLog v1->v2:
> - Use BIT() macro for bitshifted masks.
> - Use an u8 to keep track of the output value.
> - Use C99 comment style in the header.
> ---
>  drivers/gpio/Kconfig       |   7 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpio-gw-pld.c | 136 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/gpio/gpio-gw-pld.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b5a2845347ec..699a8118c433 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -830,6 +830,13 @@ config GPIO_ADNP
>           enough to represent all pins, but the driver will assume a
>           register layout for 64 pins (8 registers).
>
> +config GPIO_GW_PLD
> +       tristate "Gateworks PLD GPIO Expander"
> +       depends on OF_GPIO
> +       help
> +         Say yes here to provide access to the Gateworks I2C PLD GPIO
> +         Expander. This is used at least on the Cambria GW2358-4.
> +
>  config GPIO_MAX7300
>         tristate "Maxim MAX7300 GPIO expander"
>         select GPIO_MAX730X
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 37628f8dbf70..0568bbe6fe68 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_GPIO_FTGPIO010)  += gpio-ftgpio010.o
>  obj-$(CONFIG_GPIO_GE_FPGA)     += gpio-ge.o
>  obj-$(CONFIG_GPIO_GPIO_MM)     += gpio-gpio-mm.o
>  obj-$(CONFIG_GPIO_GRGPIO)      += gpio-grgpio.o
> +obj-$(CONFIG_GPIO_GW_PLD)      += gpio-gw-pld.o
>  obj-$(CONFIG_GPIO_HLWD)                += gpio-hlwd.o
>  obj-$(CONFIG_HTC_EGPIO)                += gpio-htc-egpio.o
>  obj-$(CONFIG_GPIO_ICH)         += gpio-ich.o
> diff --git a/drivers/gpio/gpio-gw-pld.c b/drivers/gpio/gpio-gw-pld.c
> new file mode 100644
> index 000000000000..b3d9baec5464
> --- /dev/null
> +++ b/drivers/gpio/gpio-gw-pld.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Gateworks I2C PLD GPIO expander
> +//
> +// Copyright (C) 2019 Linus Walleij <linus.walleij@xxxxxxxxxx>
> +//
> +// Based on code and know-how from the OpenWrt driver:
> +// Copyright (C) 2009 Gateworks Corporation
> +// Authors: Chris Lang, Imre Kaloz
> +
> +#include <linux/bits.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +
> +/**
> + * struct gw_pld - State container for Gateworks PLD
> + * @chip: GPIO chip instance
> + * @client: I2C client
> + * @out: shadow register for the output bute
> + */
> +struct gw_pld {
> +       struct gpio_chip chip;
> +       struct i2c_client *client;
> +       u8 out;
> +};
> +
> +/*
> + * The Gateworks I2C PLD chip only expose one read and one write register.
> + * Writing a "one" bit (to match the reset state) lets that pin be used as an
> + * input. It is an open-drain model.
> + */
> +static int gw_pld_input8(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +
> +       gw->out |= BIT(offset);
> +       return i2c_smbus_write_byte(gw->client, gw->out);
> +}
> +
> +static int gw_pld_get8(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +       s32 val;
> +
> +       val = i2c_smbus_read_byte(gw->client);
> +
> +       return (val < 0) ? 0 : !!(val & BIT(offset));
> +}
> +
> +static int gw_pld_output8(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +
> +       if (value)
> +               gw->out |= BIT(offset);
> +       else
> +               gw->out &= ~BIT(offset);
> +
> +       return i2c_smbus_write_byte(gw->client, gw->out);
> +}
> +
> +static void gw_pld_set8(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +       gw_pld_output8(gc, offset, value);
> +}
> +
> +static int gw_pld_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{
> +       struct device *dev = &client->dev;
> +       struct device_node *np = dev->of_node;
> +       struct gw_pld *gw;
> +       int ret;
> +
> +       gw = devm_kzalloc(dev, sizeof(*gw), GFP_KERNEL);
> +       if (!gw)
> +               return -ENOMEM;
> +
> +       gw->chip.base = -1;
> +       gw->chip.can_sleep = true;
> +       gw->chip.parent = dev;
> +       gw->chip.of_node = np;
> +       gw->chip.owner = THIS_MODULE;
> +       gw->chip.label = dev_name(dev);
> +       gw->chip.ngpio = 8;
> +       gw->chip.direction_input = gw_pld_input8;
> +       gw->chip.get = gw_pld_get8;
> +       gw->chip.direction_output = gw_pld_output8;
> +       gw->chip.set = gw_pld_set8;
> +       gw->client = client;
> +
> +       /*
> +        * The Gateworks I2C PLD chip does not properly send the acknowledge
> +        * bit at all times, but we can still use the standard i2c_smbus
> +        * functions by simply ignoring this bit.
> +        */
> +       client->flags |= I2C_M_IGNORE_NAK;
> +       gw->out = 0xFF;
> +
> +       i2c_set_clientdata(client, gw);
> +
> +       ret = devm_gpiochip_add_data(dev, &gw->chip, gw);
> +       if (ret)
> +               return ret;
> +
> +       dev_info(dev, "registered Gateworks PLD GPIO device\n");
> +
> +       return 0;
> +}
> +
> +static const struct i2c_device_id gw_pld_id[] = {
> +       { "gw-pld", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, gw_pld_id);
> +
> +static const struct of_device_id gw_pld_dt_ids[] = {
> +       { .compatible = "gateworks,pld-gpio", },
> +};
> +MODULE_DEVICE_TABLE(of, gw_pld_dt_ids);
> +
> +static struct i2c_driver gw_pld_driver = {
> +       .driver = {
> +               .name = "gw_pld",
> +               .of_match_table = gw_pld_dt_ids,
> +       },
> +       .probe = gw_pld_probe,
> +       .id_table = gw_pld_id,
> +};
> +module_i2c_driver(gw_pld_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@xxxxxxxxxx>");
> --
> 2.20.1
>

I missed the fact about this driver's i2c protocol. Now looks good.

Reviewed-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>