| Message ID | 20190130203156.19774-2-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
śr., 30 sty 2019 o 21:32 Linus Walleij <linus.walleij@linaro.org> 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@openwrt.org> > Cc: Tim Harvey <tharvey@gateworks.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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@linaro.org> > + * > + * 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@linaro.org>"); > -- > 2.20.1 > Best regards, Bartosz Golaszewski [1] https://lkml.org/lkml/2017/11/2/715
Hi Bartosz, thanks for your review! On Thu, Jan 31, 2019 at 9:28 AM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > a couple remarks below. Fixed most! One comment: > > +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 can't (AFAICT) beacuse, i2c regmap is for registers, which have register offsets. It accesses the devices with two standard transfers, one telling the register, and the second doing the actual byte read or write. This thing has only one read and one write register so it doesn't send any register offset, just the value. So I doubt I can make it any simpler. I realized I forgot to use the BIT() macros that I usually insist on myself though. Yours, Linus Walleij
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@linaro.org> + * + * Based on code and know-how from the OpenWrt driver: + * Copyright (C) 2009 Gateworks Corporation + * Authors: Chris Lang, Imre Kaloz + */ +#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; +}; + +/* + * 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); +} + +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@linaro.org>");
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@openwrt.org> Cc: Tim Harvey <tharvey@gateworks.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- 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 -- 2.20.1