Message ID | 20200705133038.161547-1-mans0n@gorani.run |
---|---|
State | New |
Headers | show |
Series | [v5,1/2] gpio: add GPO driver for PCA9570 | expand |
On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <mans0n@gorani.run> wrote: > > NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. > Its ports are controlled only by a data byte without register address. > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf > > Signed-off-by: Sungbo Eo <mans0n@gorani.run> > --- > v5: > * amended the commit message > * removed unnecessary castings > * added data to of_match_table > > v4: > * removed ->direction_input() and ->direction_output() > (Seems unnecessary to me) > * removed ->set_multiple() > (I'm not sure this implementation is really correct) > * added ->get() > (DS says we can read the status from the device) > * read current status during probe > > v3: > * remove mutex > * rename buffer to out > * simplify return statements > * replace ->probe() to ->probe_new() > * move ngpio to driver_data > (PCA9571 is 8-bit so I thought making ngpio configurable is a good idea) This driver looks nice now but why did you remove the mutex in v3? I think when Andy commented on that, he meant not understanding why the error check is protected, not the i2c operations. Are you sure you don't need this lock? Bartosz
On Mon, Jul 6, 2020 at 2:21 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote:> > On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <mans0n@gorani.run> wrote: > > > > NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. > > Its ports are controlled only by a data byte without register address. > > > > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf > > No blank line here. > > Signed-off-by: Sungbo Eo <mans0n@gorani.run> > This driver looks nice now but why did you remove the mutex in v3? I > think when Andy commented on that, he meant not understanding why the > error check is protected, not the i2c operations. Right. > Are you sure you don't need this lock? It's a good point!
On Sun, Jul 5, 2020 at 4:31 PM Sungbo Eo <mans0n@gorani.run> wrote: > > NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. > Its ports are controlled only by a data byte without register address. Thank you for an update, my comments (besides what Bart has) below. > Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf > > Signed-off-by: Sungbo Eo <mans0n@gorani.run> No blank line in between. ... > +#include <linux/gpio/driver.h> > +#include <linux/i2c.h> > +#include <linux/module.h> It also needs property.h. ... > +struct pca9570 { > + struct gpio_chip chip; > + struct i2c_client *client; This basically a duplication of reference to a parent device of GPIO chip. See below. > + u8 out; > +}; > + > +static int pca9570_read(struct pca9570 *gpio, u8 *value) > +{ struct i2c_client *client = to_i2c_client(gpio->chip.parent); > + int ret; > + > + ret = i2c_smbus_read_byte(gpio->client); > + if (ret < 0) > + return ret; > + > + *value = ret; > + return 0; > +} ... > +static const struct i2c_device_id pca9570_id_table[] = { > + { "pca9570", 4 }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(i2c, pca9570_id_table); > + > +static const struct of_device_id pca9570_of_match_table[] = { > + { .compatible = "nxp,pca9570", .data = (void *)4 }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pca9570_of_match_table); These structures now can be located closer to the end of the driver (exactly before first use). ... > + gpio->chip = template_chip; I'm not sure why we need an additional structure to just memcpy() to the chip. But here I have no strong opinion (perhaps the compiler even optimizes this).
On 20. 7. 6. 오후 9:00, Andy Shevchenko wrote: > On Mon, Jul 6, 2020 at 2:21 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote:> >> On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <mans0n@gorani.run> wrote: >>> >>> NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. >>> Its ports are controlled only by a data byte without register address. >>> > >>> Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf >>> > > No blank line here. > >>> Signed-off-by: Sungbo Eo <mans0n@gorani.run> > >> This driver looks nice now but why did you remove the mutex in v3? I >> think when Andy commented on that, he meant not understanding why the >> error check is protected, not the i2c operations. > > Right. Oh, probably I misunderstood the comment... :( But I don't really understand what mutex does here. The driver does not need consecutive commands, it only sends/receives only one byte at a time. And AFAIK each i2c_smbus function is already protected by a mutex. So what should be exactly inside the lock? Should we protect the output buffer as well? I'm not an expert on this so please enlighten me. Thanks for your kind reviews, as always. :) > >> Are you sure you don't need this lock? > > It's a good point! >
On Tue, Jul 7, 2020 at 5:03 PM Sungbo Eo <mans0n@gorani.run> wrote: > On 20. 7. 6. 오후 9:00, Andy Shevchenko wrote: ... > But I don't really understand what mutex does here. The driver does not > need consecutive commands, it only sends/receives only one byte at a > time. And AFAIK each i2c_smbus function is already protected by a mutex. > So what should be exactly inside the lock? Should we protect the output > buffer as well? I'm not an expert on this so please enlighten me. There are questions, answering them will give you a solution: - Since we have two functions doing i2c communications, can they clash? If so, does the i2c framework guarantee the serialisation? - Since we have a shared resource (buf), can accessors clash? How do we guarantee serialization?
Thanks, it made me think about it deeper... On 20. 7. 8. 오전 12:07, Andy Shevchenko wrote: > On Tue, Jul 7, 2020 at 5:03 PM Sungbo Eo <mans0n@gorani.run> wrote: >> On 20. 7. 6. 오후 9:00, Andy Shevchenko wrote: > > ... > >> But I don't really understand what mutex does here. The driver does not >> need consecutive commands, it only sends/receives only one byte at a >> time. And AFAIK each i2c_smbus function is already protected by a mutex. >> So what should be exactly inside the lock? Should we protect the output >> buffer as well? I'm not an expert on this so please enlighten me. > > There are questions, answering them will give you a solution: > - Since we have two functions doing i2c communications, can they > clash? If so, does the i2c framework guarantee the serialisation? I think it does. > - Since we have a shared resource (buf), can accessors clash? How do > we guarantee serialization? > But the output buffer should be tied to the i2c operations. So I guess it requires a mutex here. pca9570_get() does not access gpio->out so it does not need to be locked. On the other hand, the whole pca9570_set() function should be protected, from reading gpio->out to rewriting to gpio->out. So pca9570_write() error check should be inside the lock as well. Am I right? Thanks.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index c6b5c65c8405..d10dcb81b841 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -962,6 +962,14 @@ config GPIO_PCA953X_IRQ Say yes here to enable the pca953x to be used as an interrupt controller. It requires the driver to be built in the kernel. +config GPIO_PCA9570 + tristate "PCA9570 4-Bit I2C GPO expander" + help + Say yes here to enable the GPO driver for the NXP PCA9570 chip. + + To compile this driver as a module, choose M here: the module will + be called gpio-pca9570. + config GPIO_PCF857X tristate "PCF857x, PCA{85,96}7x, and MAX732[89] I2C GPIO expanders" select GPIOLIB_IRQCHIP diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 1e4894e0bf0f..33cb40c28a61 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -110,6 +110,7 @@ obj-$(CONFIG_GPIO_OCTEON) += gpio-octeon.o obj-$(CONFIG_GPIO_OMAP) += gpio-omap.o obj-$(CONFIG_GPIO_PALMAS) += gpio-palmas.o obj-$(CONFIG_GPIO_PCA953X) += gpio-pca953x.o +obj-$(CONFIG_GPIO_PCA9570) += gpio-pca9570.o obj-$(CONFIG_GPIO_PCF857X) += gpio-pcf857x.o obj-$(CONFIG_GPIO_PCH) += gpio-pch.o obj-$(CONFIG_GPIO_PCIE_IDIO_24) += gpio-pcie-idio-24.o diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c new file mode 100644 index 000000000000..d420c4f55766 --- /dev/null +++ b/drivers/gpio/gpio-pca9570.c @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for PCA9570 I2C GPO expander + * + * Copyright (C) 2020 Sungbo Eo <mans0n@gorani.run> + * + * Based on gpio-tpic2810.c + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/ + * Andrew F. Davis <afd@ti.com> + */ + +#include <linux/gpio/driver.h> +#include <linux/i2c.h> +#include <linux/module.h> + +/** + * struct pca9570 - GPIO driver data + * @chip: GPIO controller chip + * @client: I2C device pointer + * @out: Buffer for device register + */ +struct pca9570 { + struct gpio_chip chip; + struct i2c_client *client; + u8 out; +}; + +static int pca9570_read(struct pca9570 *gpio, u8 *value) +{ + int ret; + + ret = i2c_smbus_read_byte(gpio->client); + if (ret < 0) + return ret; + + *value = ret; + return 0; +} + +static int pca9570_write(struct pca9570 *gpio, u8 value) +{ + return i2c_smbus_write_byte(gpio->client, value); +} + +static int pca9570_get_direction(struct gpio_chip *chip, + unsigned offset) +{ + /* This device always output */ + return GPIO_LINE_DIRECTION_OUT; +} + +static int pca9570_get(struct gpio_chip *chip, unsigned offset) +{ + struct pca9570 *gpio = gpiochip_get_data(chip); + u8 buffer; + int ret; + + ret = pca9570_read(gpio, &buffer); + if (ret) + return ret; + + return !!(buffer & BIT(offset)); +} + +static void pca9570_set(struct gpio_chip *chip, unsigned offset, int value) +{ + struct pca9570 *gpio = gpiochip_get_data(chip); + u8 buffer = gpio->out; + int ret; + + if (value) + buffer |= BIT(offset); + else + buffer &= ~BIT(offset); + + ret = pca9570_write(gpio, buffer); + if (ret) + return; + + gpio->out = buffer; +} + +static const struct gpio_chip template_chip = { + .label = "pca9570", + .owner = THIS_MODULE, + .get_direction = pca9570_get_direction, + .get = pca9570_get, + .set = pca9570_set, + .base = -1, + .can_sleep = true, +}; + +static const struct i2c_device_id pca9570_id_table[] = { + { "pca9570", 4 }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(i2c, pca9570_id_table); + +static const struct of_device_id pca9570_of_match_table[] = { + { .compatible = "nxp,pca9570", .data = (void *)4 }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, pca9570_of_match_table); + +static int pca9570_probe(struct i2c_client *client) +{ + struct pca9570 *gpio; + + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL); + if (!gpio) + return -ENOMEM; + + gpio->chip = template_chip; + gpio->chip.parent = &client->dev; + gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev); + gpio->client = client; + + /* Read the current output level */ + pca9570_read(gpio, &gpio->out); + + i2c_set_clientdata(client, gpio); + + return devm_gpiochip_add_data(&client->dev, &gpio->chip, gpio); +} + +static struct i2c_driver pca9570_driver = { + .driver = { + .name = "pca9570", + .of_match_table = pca9570_of_match_table, + }, + .probe_new = pca9570_probe, + .id_table = pca9570_id_table, +}; +module_i2c_driver(pca9570_driver); + +MODULE_AUTHOR("Sungbo Eo <mans0n@gorani.run>"); +MODULE_DESCRIPTION("GPIO expander driver for PCA9570"); +MODULE_LICENSE("GPL v2");
NXP PCA9570 is a 4-bit I2C GPO expander without interrupt functionality. Its ports are controlled only by a data byte without register address. Datasheet: https://www.nxp.com/docs/en/data-sheet/PCA9570.pdf Signed-off-by: Sungbo Eo <mans0n@gorani.run> --- v5: * amended the commit message * removed unnecessary castings * added data to of_match_table v4: * removed ->direction_input() and ->direction_output() (Seems unnecessary to me) * removed ->set_multiple() (I'm not sure this implementation is really correct) * added ->get() (DS says we can read the status from the device) * read current status during probe v3: * remove mutex * rename buffer to out * simplify return statements * replace ->probe() to ->probe_new() * move ngpio to driver_data (PCA9571 is 8-bit so I thought making ngpio configurable is a good idea) v2: * move the direction functions below the set functions * use devm_gpiochip_add_data() and remove the remove callback v1: Tested in kernel 5.4 on an ipq40xx platform. This is my first time submitting a whole driver patch, and I'm not really familiar with this PCA expander series. Please let me know how I can improve this patch further. FYI there's an unmerged patch for this chip. http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2017-May/105602.html I don't have PCA9571 either so I didn't add support for it. --- drivers/gpio/Kconfig | 8 +++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-pca9570.c | 138 ++++++++++++++++++++++++++++++++++++ 3 files changed, 147 insertions(+) create mode 100644 drivers/gpio/gpio-pca9570.c