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 <[email protected]>
---
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
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 <[email protected]>
+ *
+ * Based on gpio-tpic2810.c
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ * Andrew F. Davis <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("GPIO expander driver for PCA9570");
+MODULE_LICENSE("GPL v2");
--
2.27.0
On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <[email protected]> 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 <[email protected]>
> ---
> 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
<[email protected]> wrote:>
> On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <[email protected]> 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 <[email protected]>
> 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!
--
With Best Regards,
Andy Shevchenko
On Sun, Jul 5, 2020 at 4:31 PM Sungbo Eo <[email protected]> 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 <[email protected]>
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).
--
With Best Regards,
Andy Shevchenko
On 20. 7. 6. 오후 9:00, Andy Shevchenko wrote:
> On Mon, Jul 6, 2020 at 2:21 PM Bartosz Golaszewski
> <[email protected]> wrote:>
>> On Sun, Jul 5, 2020 at 3:31 PM Sungbo Eo <[email protected]> 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 <[email protected]>
>
>> 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 <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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.