linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: add GPO driver for PCA9570
@ 2020-06-23  6:05 Sungbo Eo
  2020-06-23  8:31 ` Michael Walle
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sungbo Eo @ 2020-06-23  6:05 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio; +Cc: Sungbo Eo

This patch adds support for the PCA9570 I2C GPO expander.

Signed-off-by: Sungbo Eo <mans0n@gorani.run>
---
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 <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>
+#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 <mans0n@gorani.run>");
+MODULE_DESCRIPTION("GPIO expander driver for PCA9570");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23  6:05 [PATCH] gpio: add GPO driver for PCA9570 Sungbo Eo
@ 2020-06-23  8:31 ` Michael Walle
  2020-06-23 12:22   ` Sungbo Eo
  2020-06-24 12:29 ` Bartosz Golaszewski
  2020-06-24 13:01 ` Andy Shevchenko
  2 siblings, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-06-23  8:31 UTC (permalink / raw)
  To: Sungbo Eo; +Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

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 <mans0n@gorani.run>
> ---
> 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/20200604211039.12689-7-michael@walle.cc/

-michael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23  8:31 ` Michael Walle
@ 2020-06-23 12:22   ` Sungbo Eo
  2020-06-23 12:47     ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Sungbo Eo @ 2020-06-23 12:22 UTC (permalink / raw)
  To: Michael Walle
  Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

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 <mans0n@gorani.run>
>> ---
>> 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/20200604211039.12689-7-michael@walle.cc/

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23 12:22   ` Sungbo Eo
@ 2020-06-23 12:47     ` Michael Walle
  2020-06-24 13:33       ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-06-23 12:47 UTC (permalink / raw)
  To: Sungbo Eo; +Cc: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio

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 <mans0n@gorani.run>
>>> ---
>>> 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/20200604211039.12689-7-michael@walle.cc/
> 
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23  6:05 [PATCH] gpio: add GPO driver for PCA9570 Sungbo Eo
  2020-06-23  8:31 ` Michael Walle
@ 2020-06-24 12:29 ` Bartosz Golaszewski
  2020-06-24 13:01 ` Andy Shevchenko
  2 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2020-06-24 12:29 UTC (permalink / raw)
  To: Sungbo Eo; +Cc: Linus Walleij, LKML, linux-gpio

wt., 23 cze 2020 o 08:05 Sungbo Eo <mans0n@gorani.run> napisał(a):
>
> This patch adds support for the PCA9570 I2C GPO expander.
>
> Signed-off-by: Sungbo Eo <mans0n@gorani.run>

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 <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>
> +#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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23  6:05 [PATCH] gpio: add GPO driver for PCA9570 Sungbo Eo
  2020-06-23  8:31 ` Michael Walle
  2020-06-24 12:29 ` Bartosz Golaszewski
@ 2020-06-24 13:01 ` Andy Shevchenko
  2020-06-25  7:34   ` Sungbo Eo
  2 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-24 13:01 UTC (permalink / raw)
  To: Sungbo Eo
  Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM

On Tue, Jun 23, 2020 at 9:06 AM Sungbo Eo <mans0n@gorani.run> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-23 12:47     ` Michael Walle
@ 2020-06-24 13:33       ` Andy Shevchenko
  2020-06-24 13:46         ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-24 13:33 UTC (permalink / raw)
  To: Michael Walle
  Cc: Sungbo Eo, Linus Walleij, Bartosz Golaszewski,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM

On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <michael@walle.cc> 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/20200604211039.12689-7-michael@walle.cc/
> >
> > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-24 13:33       ` Andy Shevchenko
@ 2020-06-24 13:46         ` Michael Walle
  2020-06-24 14:52           ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-06-24 13:46 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sungbo Eo, Linus Walleij, Bartosz Golaszewski,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM

Hi Andy,

Am 2020-06-24 15:33, schrieb Andy Shevchenko:
> On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <michael@walle.cc> 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/20200604211039.12689-7-michael@walle.cc/
>> >
>> > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-24 13:46         ` Michael Walle
@ 2020-06-24 14:52           ` Andy Shevchenko
  2020-06-27 16:58             ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-06-24 14:52 UTC (permalink / raw)
  To: Michael Walle
  Cc: Sungbo Eo, Linus Walleij, Bartosz Golaszewski,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM

On Wed, Jun 24, 2020 at 4:46 PM Michael Walle <michael@walle.cc> wrote:
> Am 2020-06-24 15:33, schrieb Andy Shevchenko:
> > On Tue, Jun 23, 2020 at 3:48 PM Michael Walle <michael@walle.cc> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-24 13:01 ` Andy Shevchenko
@ 2020-06-25  7:34   ` Sungbo Eo
  0 siblings, 0 replies; 11+ messages in thread
From: Sungbo Eo @ 2020-06-25  7:34 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM

On 2020-06-24 22:01, Andy Shevchenko wrote:
> On Tue, Jun 23, 2020 at 9:06 AM Sungbo Eo <mans0n@gorani.run> 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...)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] gpio: add GPO driver for PCA9570
  2020-06-24 14:52           ` Andy Shevchenko
@ 2020-06-27 16:58             ` Michael Walle
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Walle @ 2020-06-27 16:58 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sungbo Eo, Linus Walleij, Bartosz Golaszewski,
	Linux Kernel Mailing List, open list:GPIO SUBSYSTEM

Hi,

Am 2020-06-24 16:52, schrieb Andy Shevchenko:
> On Wed, Jun 24, 2020 at 4:46 PM Michael Walle <michael@walle.cc> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-06-27 16:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23  6:05 [PATCH] gpio: add GPO driver for PCA9570 Sungbo Eo
2020-06-23  8:31 ` Michael Walle
2020-06-23 12:22   ` Sungbo Eo
2020-06-23 12:47     ` Michael Walle
2020-06-24 13:33       ` Andy Shevchenko
2020-06-24 13:46         ` Michael Walle
2020-06-24 14:52           ` Andy Shevchenko
2020-06-27 16:58             ` Michael Walle
2020-06-24 12:29 ` Bartosz Golaszewski
2020-06-24 13:01 ` Andy Shevchenko
2020-06-25  7:34   ` Sungbo Eo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).