[PATCH v2 0/2] Add DS4520 GPIO Expander Support

[PATCH v2 0/2] Add DS4520 GPIO Expander Support

[Okan Sahin]

GPIO expander driver and bindings for DS4520.
The patches are required to be applied in sequence.

Changes in v2:
* Patch 1: "dt-bindings: gpio: ds4520: Add ADI DS4520"
  * Drop label for gpio node
  * Use consistent quotes
  * Fix filename matching compatible.
* Patch 2: "gpio: ds4520: Add ADI DS4520 GPIO Expander Support"
  * Use regmap_gpio framework
  * Fix typo in Kconfig
  * Fix commit message

Okan Sahin (2):
  dt-bindings: gpio: ds4520: Add ADI DS4520
  gpio: ds4520: Add ADI DS4520 GPIO Expander Support

 .../bindings/gpio/adi,ds4520-gpio.yaml        | 45 ++++++++++
 drivers/gpio/Kconfig                          | 11 +++
 drivers/gpio/Makefile                         |  1 +
 drivers/gpio/gpio-ds4520.c                    | 83 +++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml
 create mode 100644 drivers/gpio/gpio-ds4520.c

-- 
2.30.2

[PATCH v2 1/2] dt-bindings: gpio: ds4520: Add ADI DS4520

[Okan Sahin]

Add ADI DS4520 devicetree document.

Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
 .../bindings/gpio/adi,ds4520-gpio.yaml        | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml
new file mode 100644
index 000000000000..ea2cfbebda9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/adi,ds4520-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DS4520 I2C GPIO expander
+
+maintainers:
+  - Okan Sahin <okan.sahin@analog.com>
+
+properties:
+  compatible:
+    enum:
+      - adi,ds4520-gpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        gpio@50 {
+            compatible = "adi,ds4520-gpio";
+            reg = <0x50>;
+            gpio-controller;
+            #gpio-cells = <2>;
+        };
+    };
-- 
2.30.2

Re: [PATCH v2 1/2] dt-bindings: gpio: ds4520: Add ADI DS4520

[Krzysztof Kozlowski]

On 02/05/2023 01:05, Okan Sahin wrote:
> Add ADI DS4520 devicetree document.
> 
> Signed-off-by: Okan Sahin <okan.sahin@analog.com>
> ---
>  .../bindings/gpio/adi,ds4520-gpio.yaml        | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: gpio: ds4520: Add ADI DS4520

[Linus Walleij]

On Tue, May 2, 2023 at 1:05 AM Okan Sahin <okan.sahin@analog.com> wrote:

> Add ADI DS4520 devicetree document.
>
> Signed-off-by: Okan Sahin <okan.sahin@analog.com>

Looks reasonable.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

[Okan Sahin]

The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
It offers users a digitally programmable alternative
to hardware jumpers and mechanical switches that are
being used to control digital logic node.

Signed-off-by: Okan Sahin <okan.sahin@analog.com>
---
 drivers/gpio/Kconfig       | 11 +++++
 drivers/gpio/Makefile      |  1 +
 drivers/gpio/gpio-ds4520.c | 83 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)
 create mode 100644 drivers/gpio/gpio-ds4520.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..5f89e46d6411 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1000,6 +1000,17 @@ config GPIO_ADNP
 	  enough to represent all pins, but the driver will assume a
 	  register layout for 64 pins (8 registers).
 
+config GPIO_DS4520
+	tristate "DS4520 I2C GPIO expander"
+	select REGMAP_I2C
+	select GPIO_REGMAP
+	help
+	  GPIO driver for ADI DS4520 I2C-based GPIO expander.
+	  Say yes here to enable the GPIO driver for the ADI DS4520 chip.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio-ds4520.
+
 config GPIO_GW_PLD
 	tristate "Gateworks PLD GPIO Expander"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c048ba003367..6f8656d5d617 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_GPIO_DA9052)		+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)		+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)		+= gpio-davinci.o
 obj-$(CONFIG_GPIO_DLN2)			+= gpio-dln2.o
+obj-$(CONFIG_GPIO_DS4520)		+= gpio-ds4520.o
 obj-$(CONFIG_GPIO_DWAPB)		+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EIC_SPRD)		+= gpio-eic-sprd.o
 obj-$(CONFIG_GPIO_EM)			+= gpio-em.o
diff --git a/drivers/gpio/gpio-ds4520.c b/drivers/gpio/gpio-ds4520.c
new file mode 100644
index 000000000000..3358c2faa787
--- /dev/null
+++ b/drivers/gpio/gpio-ds4520.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Analog Devices, Inc.
+ * Driver for the DS4520 I/O Expander
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#define NUMBER_OF_GPIO	9
+
+#define PULLUP0		0xF0
+#define IO_CONTROL0	0xF2
+#define IO_STATUS0	0xF8
+
+static const struct regmap_config ds4520_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ds4520_gpio_probe(struct i2c_client *client)
+{
+	struct gpio_regmap_config config = {0};
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	u32 ngpio;
+	u32 base;
+	int ret;
+
+	ngpio = NUMBER_OF_GPIO;
+
+	ret = device_property_read_u32(dev, "reg", &base);
+	if (ret)
+		return -EINVAL;
+	regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err_probe(dev, ret,
+			      "Failed to allocate register map\n");
+		return ret;
+	}
+
+	config.regmap = regmap;
+	config.parent = dev;
+	config.ngpio = ngpio;
+
+	config.reg_dat_base = base + IO_STATUS0;
+	config.reg_set_base = base + PULLUP0;
+	config.reg_dir_out_base = base + IO_CONTROL0;
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &config));
+}
+
+static const struct of_device_id ds4520_gpio_of_match_table[] = {
+	{
+		.compatible = "adi,ds4520-gpio"
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ds4520_gpio_of_match_table);
+
+static const struct i2c_device_id ds4520_gpio_id_table[] = {
+	{ "ds4520-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ds4520_gpio_id_table);
+
+static struct i2c_driver ds4520_gpio_driver = {
+	.driver = {
+		.name = "ds4520-gpio",
+		.of_match_table = ds4520_gpio_of_match_table,
+	},
+	.probe_new = ds4520_gpio_probe,
+	.id_table = ds4520_gpio_id_table,
+};
+module_i2c_driver(ds4520_gpio_driver);
+
+MODULE_DESCRIPTION("DS4520 I/O Expander");
+MODULE_AUTHOR("Okan Sahin <okan.sahin@analog.com>");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

[Michael Walle]

[Please include any former reviewers in new versions.]

> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.

Ok, what I just noticed is that this is an open-drain output buffer
with an optional pull-up, that should really go into the commit
message.

Also the commit message is misleading "it offers users a digitally
programmable alternative to hardware jumpers". While the hardware is
capable of that, this driver doesn't make use of it.

> +	config.reg_dat_base = base + IO_STATUS0;
> +	config.reg_set_base = base + PULLUP0;
> +	config.reg_dir_out_base = base + IO_CONTROL0;

Given the above, I don't think this is correct. You pull the line low if
the line is in input mode (?). The line will be pulled low if the
corresponding bit in IO_CONTROL is zero. A one means, the pin is
floating. With open-drain buffers there are usually an external pull-ups,
so I'd treat the internal pull-up as optional and it is not necessary to
switch the actual line state.

In that case the following should be sufficient:
	config.reg_dat_base = base + IO_STATUS0;
	config.reg_set_base = base + IO_CONTROL0;

I'm not sure about the direction though. Technically speaking there is
no direction register. I'm not familiar with how open drain output are
modeled in linux. I'd expect the above is enough. Bartosz/Linus/Andy?

To enable the optional pull-up, you should refer to .set_config.
(You don't need to disable the pull-up if you pull the line low).

Regarding the SEE bit and wear out: The SEE bit seem to be shadowed by the
EEPROM, so if someone is setting the SEE bit it will be persisent. Changing
direction or output value will result in an EEPROM write and might wear out
the EEPROM. I'd like to hear others opinion on that. The worst case write
cycles are 50000. Fail the probe if the SEE bit is set seems not ideal.
Just ignoring that problem for now (or at least warn the user)?

-michael

Re: [PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

[Linus Walleij]

On Tue, May 2, 2023 at 10:44 AM Michael Walle <michael@walle.cc> wrote:

> I'm not sure about the direction though. Technically speaking there is
> no direction register. I'm not familiar with how open drain output are
> modeled in linux. I'd expect the above is enough. Bartosz/Linus/Andy?

Linux has no special concept of open drain/source, it just sees the
.set(), .set_multiple() callbacks in gpio_chip to set the output value(s).
How that happens physically is an electrical question, Linux just expects
it to happen.

.get_direction() is however partly a software concept here, so you might
need to maintain a state in the driver for this. Linux will emulate open
drain when requested by simply putting the line into input (high-impedance)
mode for a high output, but only actively drive it low, which in most cases
is physically the same as open drain.

I imagine the direction would be unknown at probe() and only go into
a definitive input/output state after .set_direction() is called so some
tri-state enum?

.set_config() can be called with the special parameter
PIN_CONFIG_DRIVE_OPEN_DRAIN if the driver on the line can be
put explicitly into open drain state. If the hardware is permanently like
that, there is not much point in implementing it.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

[andy.shevchenko]

Tue, May 02, 2023 at 02:05:16AM +0300, Okan Sahin kirjoitti:
> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.

...

> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/i2c.h>

Missing property.h.

> +#include <linux/regmap.h>

...

> +#define NUMBER_OF_GPIO	9
> +
> +#define PULLUP0		0xF0
> +#define IO_CONTROL0	0xF2
> +#define IO_STATUS0	0xF8

No namespace for the above?

...

> +	struct gpio_regmap_config config = {0};

0 is not needed.

> +	ngpio = NUMBER_OF_GPIO;

Do you really need this? Can Device Tree be sufficient here? (We have a
GPIO-wide property for that).

...

> +	ret = device_property_read_u32(dev, "reg", &base);
> +	if (ret)
> +		return -EINVAL;

Why shadowing error?

...

> +	regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
> +	if (IS_ERR(regmap)) {

> +		ret = PTR_ERR(regmap);
> +		dev_err_probe(dev, ret,
> +			      "Failed to allocate register map\n");
> +		return ret;

	return dev_err_probe();

> +	}

...

> +	config.ngpio = ngpio;

Why do you use temporary variable ngpio and not assign directly here?

-- 
With Best Regards,
Andy Shevchenko