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

[Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>]

śr., 30 sty 2019 o 21:32 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.
>

Hi Linus,

a couple remarks below.

> Cc: Imre Kaloz <kaloz@xxxxxxxxxxx>
> Cc: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig       |   7 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpio-gw-pld.c | 137 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+)
>  create mode 100644 drivers/gpio/gpio-gw-pld.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 77785f4b94bb..fe4a47e49a24 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -842,6 +842,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 289ad2727bef..cf66523b5eec 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..7ba8dd102f5a
> --- /dev/null
> +++ b/drivers/gpio/gpio-gw-pld.c
> @@ -0,0 +1,137 @@
> +// 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
> + */

I was told to use C++-style comments for the entire copyright header
in the reviews for my max77650 submission on two separate occasions.
While I wasn't aware of that policy it seems to be right[1] and I'd
say we should try to keep the code base uniform.

> +#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;
> +       unsigned out;

Checkpatch should complain about using unsigned instead of unsigned int.

> +};
> +
> +/*
> + * 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 offset)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +
> +       gw->out |= (1 << offset);
> +
> +       return i2c_smbus_write_byte(gw->client, gw->out);

May I suggest using the i2c regmap for that? You wouldn't even need to
keep track of the out variable - you could use the
regmap_update_bits() helper.

I often advocate the use of regmap whenever possible because it really
is superior to using the low-level primitives for subsystems. Just
take a look at the code shrink of at24 after porting it to regmap!

> +}
> +
> +static int gw_pld_get8(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +       s32 value;
> +
> +       value = i2c_smbus_read_byte(gw->client);
> +
> +       return (value < 0) ? 0 : (value & (1 << offset));
> +}
> +
> +static int gw_pld_output8(struct gpio_chip *gc, unsigned offset, int value)
> +{
> +       struct gw_pld *gw = gpiochip_get_data(gc);
> +       unsigned bit = 1 << offset;
> +
> +       if (value)
> +               gw->out |= bit;
> +       else
> +               gw->out &= ~bit;
> +
> +       return i2c_smbus_write_byte(gw->client, gw->out);
> +}
> +
> +static void gw_pld_set8(struct gpio_chip *gc, unsigned 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
>

Best regards,
Bartosz Golaszewski

[1] https://lkml.org/lkml/2017/11/2/715