2020-06-23 06:10:43

by Sungbo Eo

[permalink] [raw]
Subject: [PATCH] gpio: add GPO driver for PCA9570

This patch adds support for the PCA9570 I2C GPO expander.

Signed-off-by: Sungbo Eo <[email protected]>
---
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. (Do I also need to document the DT compatible string?)

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 | 159 ++++++++++++++++++++++++++++++++++++
3 files changed, 168 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..9ed01554f5df
--- /dev/null
+++ b/drivers/gpio/gpio-pca9570.c
@@ -0,0 +1,159 @@
+// 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>
+#include <linux/mutex.h>
+
+/**
+ * struct pca9570 - GPIO driver data
+ * @chip: GPIO controller chip
+ * @client: I2C device pointer
+ * @buffer: Buffer for device register
+ * @lock: Protects write sequences
+ */
+struct pca9570 {
+ struct gpio_chip chip;
+ struct i2c_client *client;
+ u8 buffer;
+ struct mutex lock;
+};
+
+static void pca9570_set(struct gpio_chip *chip, unsigned offset, int value);
+
+static int pca9570_get_direction(struct gpio_chip *chip,
+ unsigned offset)
+{
+ /* This device always output */
+ return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int pca9570_direction_input(struct gpio_chip *chip,
+ unsigned offset)
+{
+ /* This device is output only */
+ return -EINVAL;
+}
+
+static int pca9570_direction_output(struct gpio_chip *chip,
+ unsigned offset, int value)
+{
+ /* This device always output */
+ pca9570_set(chip, offset, value);
+ return 0;
+}
+
+static void pca9570_set_mask_bits(struct gpio_chip *chip, u8 mask, u8 bits)
+{
+ struct pca9570 *gpio = gpiochip_get_data(chip);
+ u8 buffer;
+ int err;
+
+ mutex_lock(&gpio->lock);
+
+ buffer = gpio->buffer & ~mask;
+ buffer |= (mask & bits);
+
+ err = i2c_smbus_write_byte(gpio->client, buffer);
+ if (!err)
+ gpio->buffer = buffer;
+
+ mutex_unlock(&gpio->lock);
+}
+
+static void pca9570_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ pca9570_set_mask_bits(chip, BIT(offset), value ? BIT(offset) : 0);
+}
+
+static void pca9570_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ pca9570_set_mask_bits(chip, *mask, *bits);
+}
+
+static const struct gpio_chip template_chip = {
+ .label = "pca9570",
+ .owner = THIS_MODULE,
+ .get_direction = pca9570_get_direction,
+ .direction_input = pca9570_direction_input,
+ .direction_output = pca9570_direction_output,
+ .set = pca9570_set,
+ .set_multiple = pca9570_set_multiple,
+ .base = -1,
+ .ngpio = 4,
+ .can_sleep = true,
+};
+
+static const struct of_device_id pca9570_of_match_table[] = {
+ { .compatible = "nxp,pca9570" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pca9570_of_match_table);
+
+static int pca9570_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct pca9570 *gpio;
+ int ret;
+
+ gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, gpio);
+
+ gpio->chip = template_chip;
+ gpio->chip.parent = &client->dev;
+
+ gpio->client = client;
+
+ mutex_init(&gpio->lock);
+
+ ret = gpiochip_add_data(&gpio->chip, gpio);
+ if (ret < 0) {
+ dev_err(&client->dev, "Unable to register gpiochip\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int pca9570_remove(struct i2c_client *client)
+{
+ struct pca9570 *gpio = i2c_get_clientdata(client);
+
+ gpiochip_remove(&gpio->chip);
+
+ return 0;
+}
+
+static const struct i2c_device_id pca9570_id_table[] = {
+ { "pca9570", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, pca9570_id_table);
+
+static struct i2c_driver pca9570_driver = {
+ .driver = {
+ .name = "pca9570",
+ .of_match_table = pca9570_of_match_table,
+ },
+ .probe = pca9570_probe,
+ .remove = pca9570_remove,
+ .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


2020-06-23 08:34:10

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

Hi Sungbo,

Am 2020-06-23 08:05, schrieb Sungbo Eo:
> This patch adds support for the PCA9570 I2C GPO expander.
>
> Signed-off-by: Sungbo Eo <[email protected]>
> ---
> 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. (Do I also
> need to document the DT compatible string?)

Did you have a look at drivers/gpio/gpio-regmap.c ? Your driver seems
to be simple enough to be easily integrated with that. If you need a
blueprint; because at the moment there is no driver in the kernel
using that, you could have a look at:
https://lore.kernel.org/linux-gpio/[email protected]/

-michael

2020-06-23 12:24:56

by Sungbo Eo

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

Hi Michael,

On 2020-06-23 17:31, Michael Walle wrote:
> Hi Sungbo,
>
> Am 2020-06-23 08:05, schrieb Sungbo Eo:
>> This patch adds support for the PCA9570 I2C GPO expander.
>>
>> Signed-off-by: Sungbo Eo <[email protected]>
>> ---
>> 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. (Do I also
>> need to document the DT compatible string?)
>
> Did you have a look at drivers/gpio/gpio-regmap.c ? Your driver seems
> to be simple enough to be easily integrated with that. If you need a
> blueprint; because at the moment there is no driver in the kernel
> using that, you could have a look at:
> https://lore.kernel.org/linux-gpio/[email protected]/

Thanks for your advice. I didn't really know what regmap is for...
It seems gpio-regmap is for gpio controllers having val/dir registers.
But pca9570 does not use port registers. The master only sends a data
byte without reg address. I'm not sure how to apply gpio-regmap or
regmap-i2c here.
I'll try to investigate if setting reg_size or reg_bits to zero is possible.

Please correct me if I'm in the wrong direction.

Thanks.

>
> -michael

2020-06-23 12:49:39

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

Hi Sungbo,

Am 2020-06-23 14:22, schrieb Sungbo Eo:
> On 2020-06-23 17:31, Michael Walle wrote:
>> Am 2020-06-23 08:05, schrieb Sungbo Eo:
>>> This patch adds support for the PCA9570 I2C GPO expander.
>>>
>>> Signed-off-by: Sungbo Eo <[email protected]>
>>> ---
>>> 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. (Do I also
>>> need to document the DT compatible string?)
>>
>> Did you have a look at drivers/gpio/gpio-regmap.c ? Your driver seems
>> to be simple enough to be easily integrated with that. If you need a
>> blueprint; because at the moment there is no driver in the kernel
>> using that, you could have a look at:
>> https://lore.kernel.org/linux-gpio/[email protected]/
>
> Thanks for your advice. I didn't really know what regmap is for...
> It seems gpio-regmap is for gpio controllers having val/dir registers.
> But pca9570 does not use port registers. The master only sends a data
> byte without reg address.

Ahh I missed that :(

> I'm not sure how to apply gpio-regmap or
> regmap-i2c here.
> I'll try to investigate if setting reg_size or reg_bits to zero is
> possible.
>
> Please correct me if I'm in the wrong direction.

That won't work because the underlying regmap expects the address bits
to be either 8 or 16. In this case I'd guess gpio-regmap, doesn't make
sense, because there is actually no real gain.

-michael

2020-06-24 12:33:27

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

wt., 23 cze 2020 o 08:05 Sungbo Eo <[email protected]> napisaƂ(a):
>
> This patch adds support for the PCA9570 I2C GPO expander.
>
> Signed-off-by: Sungbo Eo <[email protected]>

Hi Sungbo, this looks pretty good, but there are some nits listed below.

> ---
> 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. (Do I also need to document the DT compatible string?)
>

Yes, you should send a separate patch to DT maintainers with DT
binding document (preferably in YAML). Please see
Documentations/devicetree/bindings for more info.

> 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 | 159 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 168 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..9ed01554f5df
> --- /dev/null
> +++ b/drivers/gpio/gpio-pca9570.c
> @@ -0,0 +1,159 @@
> +// 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>
> +#include <linux/mutex.h>
> +
> +/**
> + * struct pca9570 - GPIO driver data
> + * @chip: GPIO controller chip
> + * @client: I2C device pointer
> + * @buffer: Buffer for device register
> + * @lock: Protects write sequences
> + */
> +struct pca9570 {
> + struct gpio_chip chip;
> + struct i2c_client *client;
> + u8 buffer;
> + struct mutex lock;
> +};
> +
> +static void pca9570_set(struct gpio_chip *chip, unsigned offset, int value);
> +

Please just move this function here instead of declaring it and
implementing it later.

[snip!]

> +
> +static int pca9570_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct pca9570 *gpio;
> + int ret;
> +
> + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, gpio);
> +
> + gpio->chip = template_chip;
> + gpio->chip.parent = &client->dev;
> +
> + gpio->client = client;
> +
> + mutex_init(&gpio->lock);
> +
> + ret = gpiochip_add_data(&gpio->chip, gpio);

Why not devm_gpiochip_add_data()? You could drop the remove callback.

> + if (ret < 0) {
> + dev_err(&client->dev, "Unable to register gpiochip\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int pca9570_remove(struct i2c_client *client)
> +{
> + struct pca9570 *gpio = i2c_get_clientdata(client);
> +
> + gpiochip_remove(&gpio->chip);
> +
> + return 0;
> +}
> +

[snip!]

Bartosz

2020-06-24 13:06:03

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

On Tue, Jun 23, 2020 at 9:06 AM Sungbo Eo <[email protected]> wrote:
>
> This patch adds support for the PCA9570 I2C GPO expander.

> 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. (Do I also need to document the DT compatible string?)
>
> 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.

My very first questions to such (simple) driver submissions is: Have
you conducted research of existing drivers and found that none is
suitable for this chip?
Do this and answer in a commit message, if it will be still valid.

--
With Best Regards,
Andy Shevchenko

2020-06-24 13:36:52

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <[email protected]> wrote:
> Am 2020-06-23 14:22, schrieb Sungbo Eo:
> > On 2020-06-23 17:31, Michael Walle wrote:
> >> Am 2020-06-23 08:05, schrieb Sungbo Eo:

...

> >> Did you have a look at drivers/gpio/gpio-regmap.c ? Your driver seems
> >> to be simple enough to be easily integrated with that. If you need a
> >> blueprint; because at the moment there is no driver in the kernel
> >> using that, you could have a look at:
> >> https://lore.kernel.org/linux-gpio/[email protected]/
> >
> > Thanks for your advice. I didn't really know what regmap is for...
> > It seems gpio-regmap is for gpio controllers having val/dir registers.
> > But pca9570 does not use port registers. The master only sends a data
> > byte without reg address.
>
> Ahh I missed that :(
>
> > I'm not sure how to apply gpio-regmap or
> > regmap-i2c here.
> > I'll try to investigate if setting reg_size or reg_bits to zero is
> > possible.
> >
> > Please correct me if I'm in the wrong direction.
>
> That won't work because the underlying regmap expects the address bits
> to be either 8 or 16. In this case I'd guess gpio-regmap, doesn't make
> sense, because there is actually no real gain.

From the DS:
"The device acknowledges and the master sends the data byte for P7 to
P0 and is acknowledged by the device. Writes to P7 to P4 are ignored
in the PCA9570 as only P3 through P0 are available. The 4-bit data is
presented on the port lines after it has been acknowledged by the
device. The number of data bytes that can be sent successively is not
limited. The previous data is overwritten every time a data byte has
been sent."

So, basically writing to the register the value of register can
simulate register map, but the question is do we gain anything from
that abstraction because it means that all 256 (or 16 for 4-bit
variant) registers are possible?


--
With Best Regards,
Andy Shevchenko

2020-06-24 13:48:56

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

Hi Andy,

Am 2020-06-24 15:33, schrieb Andy Shevchenko:
> On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <[email protected]> wrote:
>> Am 2020-06-23 14:22, schrieb Sungbo Eo:
>> > On 2020-06-23 17:31, Michael Walle wrote:
>> >> Am 2020-06-23 08:05, schrieb Sungbo Eo:
>
> ...
>
>> >> Did you have a look at drivers/gpio/gpio-regmap.c ? Your driver seems
>> >> to be simple enough to be easily integrated with that. If you need a
>> >> blueprint; because at the moment there is no driver in the kernel
>> >> using that, you could have a look at:
>> >> https://lore.kernel.org/linux-gpio/[email protected]/
>> >
>> > Thanks for your advice. I didn't really know what regmap is for...
>> > It seems gpio-regmap is for gpio controllers having val/dir registers.
>> > But pca9570 does not use port registers. The master only sends a data
>> > byte without reg address.
>>
>> Ahh I missed that :(
>>
>> > I'm not sure how to apply gpio-regmap or
>> > regmap-i2c here.
>> > I'll try to investigate if setting reg_size or reg_bits to zero is
>> > possible.
>> >
>> > Please correct me if I'm in the wrong direction.
>>
>> That won't work because the underlying regmap expects the address bits
>> to be either 8 or 16. In this case I'd guess gpio-regmap, doesn't make
>> sense, because there is actually no real gain.
>
> From the DS:
> "The device acknowledges and the master sends the data byte for P7 to
> P0 and is acknowledged by the device. Writes to P7 to P4 are ignored
> in the PCA9570 as only P3 through P0 are available. The 4-bit data is
> presented on the port lines after it has been acknowledged by the
> device. The number of data bytes that can be sent successively is not
> limited. The previous data is overwritten every time a data byte has
> been sent."
>
> So, basically writing to the register the value of register can
> simulate register map, but the question is do we gain anything from
> that abstraction because it means that all 256 (or 16 for 4-bit
> variant) registers are possible?

Mh? I can't follow you. Port means a physical I/O port, if I
read the datasheet correctly. And because that is a 4 port IO
expander only the lower four bits are used. I'd guess if it is
a 8 port IO expander all bits would be used. (Actually, its
output only.)

So you just write one byte of data (or you might repeat it, but
that is just as if you start a new i2c transaction, just that
you save the i2c addressing).

-michael

2020-06-24 14:53:53

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

On Wed, Jun 24, 2020 at 4:46 PM Michael Walle <[email protected]> wrote:
> Am 2020-06-24 15:33, schrieb Andy Shevchenko:
> > On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <[email protected]> wrote:
> >> Am 2020-06-23 14:22, schrieb Sungbo Eo:
> >> > On 2020-06-23 17:31, Michael Walle wrote:
> >> >> Am 2020-06-23 08:05, schrieb Sungbo Eo:

...

> >> That won't work because the underlying regmap expects the address bits
> >> to be either 8 or 16. In this case I'd guess gpio-regmap, doesn't make
> >> sense, because there is actually no real gain.
> >
> > From the DS:
> > "The device acknowledges and the master sends the data byte for P7 to
> > P0 and is acknowledged by the device. Writes to P7 to P4 are ignored
> > in the PCA9570 as only P3 through P0 are available. The 4-bit data is
> > presented on the port lines after it has been acknowledged by the
> > device. The number of data bytes that can be sent successively is not
> > limited. The previous data is overwritten every time a data byte has
> > been sent."
> >
> > So, basically writing to the register the value of register can
> > simulate register map, but the question is do we gain anything from
> > that abstraction because it means that all 256 (or 16 for 4-bit
> > variant) registers are possible?
>
> Mh? I can't follow you. Port means a physical I/O port, if I
> read the datasheet correctly. And because that is a 4 port IO
> expander only the lower four bits are used. I'd guess if it is
> a 8 port IO expander all bits would be used. (Actually, its
> output only.)
>
> So you just write one byte of data (or you might repeat it, but
> that is just as if you start a new i2c transaction, just that
> you save the i2c addressing).

You can write the same value twice.
It means that the first byte can represent the register address. But
it's still too volatile.
Perhaps regmap can gain something like register-less communication.

--
With Best Regards,
Andy Shevchenko

2020-06-25 07:42:44

by Sungbo Eo

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

On 2020-06-24 22:01, Andy Shevchenko wrote:
> On Tue, Jun 23, 2020 at 9:06 AM Sungbo Eo <[email protected]> wrote:
>>
>> This patch adds support for the PCA9570 I2C GPO expander.
>
>> 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. (Do I also need to document the DT compatible string?)
>>
>> 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.
>
> My very first questions to such (simple) driver submissions is: Have
> you conducted research of existing drivers and found that none is
> suitable for this chip?
> Do this and answer in a commit message, if it will be still valid.
>

I've done a more extensive research for i2c expanders and found out that
gw-pld, max732x, pcf857x also use i2c_smbus_write_byte() without reg
address. But their architectures are more complex than pca9570 and I'm
not sure if I can make them compatible with pca9570. (I still don't
understand what "quasi-bidirectional" in pcf857x means...)

2020-06-27 17:00:21

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] gpio: add GPO driver for PCA9570

Hi,

Am 2020-06-24 16:52, schrieb Andy Shevchenko:
> On Wed, Jun 24, 2020 at 4:46 PM Michael Walle <[email protected]> wrote:

[..]

>> So you just write one byte of data (or you might repeat it, but
>> that is just as if you start a new i2c transaction, just that
>> you save the i2c addressing).
>
> You can write the same value twice.
> It means that the first byte can represent the register address. But
> it's still too volatile.

In this case the output pin will hold the value of the "fake" address
register for a short period of time. No?

> Perhaps regmap can gain something like register-less communication.

I don't know if that is worth it (and if that is an intended use of
regmap).

-michael