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

[PATCH v1 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.

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

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

-- 
2.30.2

[PATCH v1 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..69f90e59d415
--- /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
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ds4520: gpio@50 {
+            compatible = "adi,ds4520-gpio";
+            reg = <0x50>;
+            gpio-controller;
+            #gpio-cells = <2>;
+        };
+    };
-- 
2.30.2

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

[Rob Herring]

On Mon, 27 Mar 2023 16:00:06 +0300, 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.example.dtb: /example-0/i2c0/gpio@50: failed to match any schema with compatible: ['adi,ds4520-gpio']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230327130010.8342-2-okan.sahin@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Krzysztof Kozlowski]

On 27/03/2023 15:00, 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
> 
> 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..69f90e59d415
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/adi,ds4520-gpio.yaml

Filename matching compatible.

> @@ -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
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':

Use consistent quotes - either ' or ".

> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {

i2c


> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ds4520: gpio@50 {

Drop the label, not used here.

> +            compatible = "adi,ds4520-gpio";

As Rob's bot pointed...

Best regards,
Krzysztof

[PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Okan Sahin]

Gpio I/O expander.

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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..20aa28e208d5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1000,6 +1000,16 @@ 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
+	help
+	  GPIO driver for Maxim MAX7300 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..8f878e7824b8
--- /dev/null
+++ b/drivers/gpio/gpio-ds4520.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Analog Devices, Inc.
+ * Driver for the DS4520 I/O Expander
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/gpio/driver.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
+
+#define REG_SIZE	8
+
+struct ds4520_gpio_priv {
+	struct regmap *regmap;
+	struct gpio_chip gpio_chip;
+};
+
+static int ds4520_gpio_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct ds4520_gpio_priv *priv = gpiochip_get_data(chip);
+	u32 val_io_control = 0, val_pullup = 0;
+	u8 reg_offset = (offset / REG_SIZE);
+	int ret;
+
+	ret = regmap_set_bits(priv->regmap, 0xF4, 0x01);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(priv->regmap, IO_CONTROL0 + reg_offset,
+			  &val_io_control);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(priv->regmap, PULLUP0 + reg_offset, &val_pullup);
+	if (ret)
+		return ret;
+
+	if ((val_io_control & (1 << (offset % 8)))
+	    == (val_pullup & (1 << (offset % 8))))
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int ds4520_gpio_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	struct ds4520_gpio_priv *priv = gpiochip_get_data(chip);
+	u8 reg_offset = (offset / REG_SIZE);
+	u8 mask = BIT(offset % 8);
+	int ret;
+
+	ret = regmap_clear_bits(priv->regmap, IO_CONTROL0 + reg_offset, mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(priv->regmap, PULLUP0 + reg_offset, mask);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ds4520_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ds4520_gpio_priv *priv = gpiochip_get_data(chip);
+	u8 reg_offset = (offset / REG_SIZE);
+	u8 mask = BIT(offset % 8);
+	u32 val = 0;
+	int ret;
+
+	ret = regmap_read(priv->regmap, IO_STATUS0 + reg_offset, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & mask);
+}
+
+static void ds4520_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int value)
+{
+	struct ds4520_gpio_priv *priv = gpiochip_get_data(chip);
+	u8 reg_offset = (offset / REG_SIZE);
+	u8 mask = BIT(offset % 8);
+
+	regmap_update_bits(priv->regmap, PULLUP0 + reg_offset, mask,
+			   value ? mask : 0);
+	regmap_update_bits(priv->regmap, IO_CONTROL0 + reg_offset, mask,
+			   value ? mask : 0);
+}
+
+static int ds4520_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	struct ds4520_gpio_priv *priv = gpiochip_get_data(chip);
+	u8 reg_offset = (offset / REG_SIZE);
+	u8 mask = BIT(offset % 8);
+	int ret;
+
+	ret = regmap_clear_bits(priv->regmap, IO_CONTROL0 + reg_offset, mask);
+	if (ret)
+		return ret;
+
+	ds4520_gpio_set(chip, offset, value);
+
+	return 0;
+}
+
+static const struct regmap_config ds4520_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct gpio_chip ds4520_chip = {
+	.label			= "ds4520-gpio",
+	.owner			= THIS_MODULE,
+	.get_direction		= ds4520_gpio_get_direction,
+	.direction_input	= ds4520_gpio_direction_input,
+	.direction_output	= ds4520_gpio_direction_output,
+	.get			= ds4520_gpio_get,
+	.set			= ds4520_gpio_set,
+	.base			= -1,
+	.can_sleep		= true,
+};
+
+static int ds4520_gpio_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ds4520_gpio_priv *priv;
+	u32 ngpio;
+	int ret;
+
+	ngpio = NUMBER_OF_GPIO;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->gpio_chip = ds4520_chip;
+	priv->gpio_chip.label = "ds4520-gpio";
+	priv->gpio_chip.ngpio = ngpio;
+	priv->gpio_chip.parent = dev;
+
+	priv->regmap = devm_regmap_init_i2c(client, &ds4520_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err_probe(dev, ret,
+			      "Failed to allocate register map\n");
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(dev, &priv->gpio_chip, priv);
+	if (ret) {
+		dev_err_probe(dev, ret, "Unable to register gpiochip\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, priv);
+
+	return 0;
+}
+
+static const struct of_device_id ds4520_gpio_of_match_table[] = {
+	{
+		.compatible = "adi,ds4520-gpio"
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ds4520_gpio_of_match_table);
+
+static const struct i2c_device_id ds4520_gpio_id_table[] = {
+	{ "ds4520-gpio" },
+	{ /* sentinel */ }
+};
+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 v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Linus Walleij]

Hi Okan,

thanks for your patch!

First: why is the word "Regulator" in the subject? I don't quite get it.

On Mon, Mar 27, 2023 at 3:01 PM Okan Sahin <okan.sahin@analog.com> wrote:
>
> Gpio I/O expander.
>
> Signed-off-by: Okan Sahin <okan.sahin@analog.com>

This commit log is too terse. Write a bit about what this hardware is.

> +config GPIO_DS4520
> +       tristate "DS4520 I2C GPIO expander"
> +       select REGMAP_I2C
> +       help
> +         GPIO driver for Maxim MAX7300 I2C-based GPIO expander.

Is it MAX7300, I don't get this, it seems super-confused.

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

(...)

The driver is pretty straight-forward, but I think this can use the
generic GPIO_REGMAP helpers in
drivers/gpio/gpio-regmap.c
check other drivers selecting this helper library for inspiration.

Yours,
Linus Walleij

RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Sahin, Okan]

>Hi Okan,
>
>thanks for your patch!
>
>First: why is the word "Regulator" in the subject? I don't quite get it.
>
>On Mon, Mar 27, 2023 at 3:01 PM Okan Sahin <okan.sahin@analog.com> wrote:
>>
>> Gpio I/O expander.
>>
>> Signed-off-by: Okan Sahin <okan.sahin@analog.com>
>
>This commit log is too terse. Write a bit about what this hardware is.
>
>> +config GPIO_DS4520
>> +       tristate "DS4520 I2C GPIO expander"
>> +       select REGMAP_I2C
>> +       help
>> +         GPIO driver for Maxim MAX7300 I2C-based GPIO expander.
>
>Is it MAX7300, I don't get this, it seems super-confused.
>
>> +         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.
>
>(...)
>
>The driver is pretty straight-forward, but I think this can use the generic
>GPIO_REGMAP helpers in drivers/gpio/gpio-regmap.c check other drivers selecting
>this helper library for inspiration.
>
>Yours,
>Linus Walleij

Hi Linus,

Thank you for your contribution. Should I add select GPIO_REGMAP into Kconfig?
I am trying to understand completely before sending new patch.
I will fix your other comments.

Regards,
Okan Sahin

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Linus Walleij]

On Tue, Apr 4, 2023 at 4:36 PM Sahin, Okan <Okan.Sahin@analog.com> wrote:

> >The driver is pretty straight-forward, but I think this can use the generic
> >GPIO_REGMAP helpers in drivers/gpio/gpio-regmap.c check other drivers selecting
> >this helper library for inspiration.
(..)
> Thank you for your contribution. Should I add select GPIO_REGMAP into Kconfig?

Yes but that is not all, you also need to make use of the library helpers
provided in include/linux/gpio/regmap.h.

Find examples of other drivers doing this by e.g.:
git grep gpio_regmap_register

drivers/gpio/gpio-sl28cpld.c:   return
PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
drivers/gpio/gpio-tn48m.c:      return
PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
drivers/pinctrl/bcm/pinctrl-bcm63xx.c:  return
PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));

^Look what these are doing

Yours,
Linus Walleij

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Michael Walle]

Am 2023-04-05 15:20, schrieb Linus Walleij:
> On Tue, Apr 4, 2023 at 4:36 PM Sahin, Okan <Okan.Sahin@analog.com> 
> wrote:
> 
>> >The driver is pretty straight-forward, but I think this can use the generic
>> >GPIO_REGMAP helpers in drivers/gpio/gpio-regmap.c check other drivers selecting
>> >this helper library for inspiration.
> (..)
>> Thank you for your contribution. Should I add select GPIO_REGMAP into 
>> Kconfig?
> 
> Yes but that is not all, you also need to make use of the library 
> helpers
> provided in include/linux/gpio/regmap.h.
> 
> Find examples of other drivers doing this by e.g.:
> git grep gpio_regmap_register
> 
> drivers/gpio/gpio-sl28cpld.c:   return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> drivers/gpio/gpio-tn48m.c:      return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> drivers/pinctrl/bcm/pinctrl-bcm63xx.c:  return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));
> 
> ^Look what these are doing

This driver is doing two register writes/reads for setting the
direction, that's something which isn't supported in GPIO_REGMAP.
OTOH I'm not sure the driver is doing it correctly, because it also
seems to switch the pullup resisters together with the direction.
I'm not sure that is correct. So there might be just one register
involved after all and the GPIO_REGMAP should work again.

Also, according to the datasheet this has some nv memory (to set the
initial state of the GPIOs [?]). So it should really be a multi-function
device. I'm not sure if this has to be considered right from the
beginning or if the device support can start with GPIO only and later
be transitioned to a full featured MFD (probably with nvmem support).

-michael

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Linus Walleij]

On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> wrote:

> OTOH I'm not sure the driver is doing it correctly, because it also
> seems to switch the pullup resisters together with the direction.
> I'm not sure that is correct. So there might be just one register
> involved after all and the GPIO_REGMAP should work again.

I'm pretty sure that should be in the .set_config() callback.

> Also, according to the datasheet this has some nv memory (to set the
> initial state of the GPIOs [?]). So it should really be a multi-function
> device. I'm not sure if this has to be considered right from the
> beginning or if the device support can start with GPIO only and later
> be transitioned to a full featured MFD (probably with nvmem support).

That's a bit of a soft definition.

If the chip is *only* doing GPIO and nvram it can be a GPIO-only
device I think.

The precedent is a ton of ethernet drivers with nvram for storing
e.g. the MAC address. We don't make all of those into MFDs,
as the nvram is closely tied to the one and only function of the
block.

Yours,
Linus Walleij

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[andy.shevchenko]

Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> wrote:
> 
> > OTOH I'm not sure the driver is doing it correctly, because it also
> > seems to switch the pullup resisters together with the direction.
> > I'm not sure that is correct. So there might be just one register
> > involved after all and the GPIO_REGMAP should work again.
> 
> I'm pretty sure that should be in the .set_config() callback.
> 
> > Also, according to the datasheet this has some nv memory (to set the
> > initial state of the GPIOs [?]). So it should really be a multi-function
> > device. I'm not sure if this has to be considered right from the
> > beginning or if the device support can start with GPIO only and later
> > be transitioned to a full featured MFD (probably with nvmem support).
> 
> That's a bit of a soft definition.
> 
> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
> device I think.
> 
> The precedent is a ton of ethernet drivers with nvram for storing
> e.g. the MAC address. We don't make all of those into MFDs,
> as the nvram is closely tied to the one and only function of the
> block.

I agree with Linus. This should be part of the actual (main) driver for
the chip as many do (like USB to serial adapters that have GPIO capability).
Also this code lacks of proper locking and has style issues.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Sahin, Okan]

>Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> wrote:
>>
>> > OTOH I'm not sure the driver is doing it correctly, because it also
>> > seems to switch the pullup resisters together with the direction.
>> > I'm not sure that is correct. So there might be just one register
>> > involved after all and the GPIO_REGMAP should work again.
>>
>> I'm pretty sure that should be in the .set_config() callback.
>>
>> > Also, according to the datasheet this has some nv memory (to set the
>> > initial state of the GPIOs [?]). So it should really be a
>> > multi-function device. I'm not sure if this has to be considered
>> > right from the beginning or if the device support can start with
>> > GPIO only and later be transitioned to a full featured MFD (probably with nvmem
>support).
>>
>> That's a bit of a soft definition.
>>
>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>> device I think.
>>
>> The precedent is a ton of ethernet drivers with nvram for storing e.g.
>> the MAC address. We don't make all of those into MFDs, as the nvram is
>> closely tied to the one and only function of the block.
>
>I agree with Linus. This should be part of the actual (main) driver for the chip as many
>do (like USB to serial adapters that have GPIO capability).
>Also this code lacks of proper locking and has style issues.
>
>--
>With Best Regards,
>Andy Shevchenko
>

Hi Linus,

I think gpio_regmap is not suitable for this driver as Michael stated. 
https://www.analog.com/media/en/technical-documentation/data-sheets/ds4520.pdf
Please check block diagram. There are two input registers that control gpio state
so gpio_regmap does not look ok for this. Am I missing something?

Hi Michael,

I tested driver for both writing and reading, it seems fine. Initially, this question
confused me too, but after examining other drivers with nvmem, my opinion is 
same as both Linus and Andy. Also, at this point I am not planning to add
nvmem support.

Hi Andy,

Could you give more detail related to locking and style issues?

Regards,
Okan Sahin

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Michael Walle]

Am 2023-04-09 16:25, schrieb Sahin, Okan:
>> Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> 
>>> wrote:
>>> 
>>> > OTOH I'm not sure the driver is doing it correctly, because it also
>>> > seems to switch the pullup resisters together with the direction.
>>> > I'm not sure that is correct. So there might be just one register
>>> > involved after all and the GPIO_REGMAP should work again.
>>> 
>>> I'm pretty sure that should be in the .set_config() callback.
>>> 
>>> > Also, according to the datasheet this has some nv memory (to set the
>>> > initial state of the GPIOs [?]). So it should really be a
>>> > multi-function device. I'm not sure if this has to be considered
>>> > right from the beginning or if the device support can start with
>>> > GPIO only and later be transitioned to a full featured MFD (probably with nvmem
>> support).
>>> 
>>> That's a bit of a soft definition.
>>> 
>>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>>> device I think.
>>> 
>>> The precedent is a ton of ethernet drivers with nvram for storing 
>>> e.g.
>>> the MAC address. We don't make all of those into MFDs, as the nvram 
>>> is
>>> closely tied to the one and only function of the block.
>> 
>> I agree with Linus. This should be part of the actual (main) driver 
>> for the chip as many
>> do (like USB to serial adapters that have GPIO capability).

You mean the gpio driver is calling nvmem_register()? Yeah I agree, that
should work.

> I think gpio_regmap is not suitable for this driver as Michael stated.
> https://www.analog.com/media/en/technical-documentation/data-sheets/ds4520.pdf
> Please check block diagram. There are two input registers that control
> gpio state
> so gpio_regmap does not look ok for this. Am I missing something?

You mean F8/F9? That will work as they are for different GPIOs. What
doesn't work with gpio-regmap is when you need to modify two different
registers for one GPIO. Have a look at gpio_regmap_get() and
gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't 
work
you can use your own .xlate() op.

> Also, at this point I am not planning to add nvmem support.

That is a pity, because that is the whole use case for this gpio 
expander,
no? "Programmable Replacement for Mechanical Jumpers and Switches"

-michael

RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Sahin, Okan]

>Am 2023-04-09 16:25, schrieb Sahin, Okan:
>>> Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>>>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc>
>>>> wrote:
>>>>
>>>> > OTOH I'm not sure the driver is doing it correctly, because it also
>>>> > seems to switch the pullup resisters together with the direction.
>>>> > I'm not sure that is correct. So there might be just one register
>>>> > involved after all and the GPIO_REGMAP should work again.
>>>>
>>>> I'm pretty sure that should be in the .set_config() callback.
>>>>
>>>> > Also, according to the datasheet this has some nv memory (to set the
>>>> > initial state of the GPIOs [?]). So it should really be a
>>>> > multi-function device. I'm not sure if this has to be considered
>>>> > right from the beginning or if the device support can start with
>>>> > GPIO only and later be transitioned to a full featured MFD (probably with
>nvmem
>>> support).
>>>>
>>>> That's a bit of a soft definition.
>>>>
>>>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>>>> device I think.
>>>>
>>>> The precedent is a ton of ethernet drivers with nvram for storing
>>>> e.g.
>>>> the MAC address. We don't make all of those into MFDs, as the nvram
>>>> is
>>>> closely tied to the one and only function of the block.
>>>
>>> I agree with Linus. This should be part of the actual (main) driver
>>> for the chip as many
>>> do (like USB to serial adapters that have GPIO capability).
>
>You mean the gpio driver is calling nvmem_register()? Yeah I agree, that
>should work.
>
>> I think gpio_regmap is not suitable for this driver as Michael stated.
>> https://www.analog.com/media/en/technical-documentation/data-
>sheets/ds4520.pdf
>> Please check block diagram. There are two input registers that control
>> gpio state
>> so gpio_regmap does not look ok for this. Am I missing something?
>
>You mean F8/F9? That will work as they are for different GPIOs. What
>doesn't work with gpio-regmap is when you need to modify two different
>registers for one GPIO. Have a look at gpio_regmap_get() and
>gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't
>work
>you can use your own .xlate() op.
>

Actually, I checked the functions that you suggested, but as far as I understand
they might work if there would be one bit to set direction or value. However,
this is not the case for ds4520. In other words, if I want to set the gpio direction
as output, I need to set a corresponding bit for both F0 and F1 registers.
In the document, you can see block diagram. I do not know why, but design is
not standard that’s why I think I can not use gpio-regmap.

>> Also, at this point I am not planning to add nvmem support.
>
>That is a pity, because that is the whole use case for this gpio
>expander,
>no? "Programmable Replacement for Mechanical Jumpers and Switches"

I can set "SEE" bit as "0" in the Configuration Register to write EEPROM so it might solve
issue.

>
>-michael

Regards,
Okan Sahin

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Michael Walle]

Am 2023-04-24 17:39, schrieb Sahin, Okan:
>> Am 2023-04-09 16:25, schrieb Sahin, Okan:
>>>> Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>>>>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc>
>>>>> wrote:
>>>>> 
>>>>> > OTOH I'm not sure the driver is doing it correctly, because it also
>>>>> > seems to switch the pullup resisters together with the direction.
>>>>> > I'm not sure that is correct. So there might be just one register
>>>>> > involved after all and the GPIO_REGMAP should work again.
>>>>> 
>>>>> I'm pretty sure that should be in the .set_config() callback.
>>>>> 
>>>>> > Also, according to the datasheet this has some nv memory (to set the
>>>>> > initial state of the GPIOs [?]). So it should really be a
>>>>> > multi-function device. I'm not sure if this has to be considered
>>>>> > right from the beginning or if the device support can start with
>>>>> > GPIO only and later be transitioned to a full featured MFD (probably with
>> nvmem
>>>> support).
>>>>> 
>>>>> That's a bit of a soft definition.
>>>>> 
>>>>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>>>>> device I think.
>>>>> 
>>>>> The precedent is a ton of ethernet drivers with nvram for storing
>>>>> e.g.
>>>>> the MAC address. We don't make all of those into MFDs, as the nvram
>>>>> is
>>>>> closely tied to the one and only function of the block.
>>>> 
>>>> I agree with Linus. This should be part of the actual (main) driver
>>>> for the chip as many
>>>> do (like USB to serial adapters that have GPIO capability).
>> 
>> You mean the gpio driver is calling nvmem_register()? Yeah I agree, 
>> that
>> should work.
>> 
>>> I think gpio_regmap is not suitable for this driver as Michael 
>>> stated.
>>> https://www.analog.com/media/en/technical-documentation/data-
>> sheets/ds4520.pdf
>>> Please check block diagram. There are two input registers that 
>>> control
>>> gpio state
>>> so gpio_regmap does not look ok for this. Am I missing something?
>> 
>> You mean F8/F9? That will work as they are for different GPIOs. What
>> doesn't work with gpio-regmap is when you need to modify two different
>> registers for one GPIO. Have a look at gpio_regmap_get() and
>> gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't
>> work
>> you can use your own .xlate() op.
>> 
> 
> Actually, I checked the functions that you suggested, but as far as I 
> understand
> they might work if there would be one bit to set direction or value. 
> However,
> this is not the case for ds4520. In other words, if I want to set the
> gpio direction
> as output, I need to set a corresponding bit for both F0 and F1 
> registers.

I can't follow. F0/F1 is for the pull-up. That was actually my initial
question and Linus said, that should probably be done in a seperate
.set_config operation not together with a direction change.

> In the document, you can see block diagram. I do not know why, but 
> design is
> not standard that’s why I think I can not use gpio-regmap.
> 
>>> Also, at this point I am not planning to add nvmem support.
>> 
>> That is a pity, because that is the whole use case for this gpio
>> expander,
>> no? "Programmable Replacement for Mechanical Jumpers and Switches"
> 
> I can set "SEE" bit as "0" in the Configuration Register to write
> EEPROM so it might solve
> issue.

If you do that unconditionally, that might wear out the EEPROM,
though.

-michael

RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Sahin, Okan]

>Am 2023-04-24 17:39, schrieb Sahin, Okan:
>>> Am 2023-04-09 16:25, schrieb Sahin, Okan:
>>>>> Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>>>>>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc>
>>>>>> wrote:
>>>>>>
>>>>>> > OTOH I'm not sure the driver is doing it correctly, because it also
>>>>>> > seems to switch the pullup resisters together with the direction.
>>>>>> > I'm not sure that is correct. So there might be just one register
>>>>>> > involved after all and the GPIO_REGMAP should work again.
>>>>>>
>>>>>> I'm pretty sure that should be in the .set_config() callback.
>>>>>>
>>>>>> > Also, according to the datasheet this has some nv memory (to set the
>>>>>> > initial state of the GPIOs [?]). So it should really be a
>>>>>> > multi-function device. I'm not sure if this has to be considered
>>>>>> > right from the beginning or if the device support can start with
>>>>>> > GPIO only and later be transitioned to a full featured MFD (probably with
>>> nvmem
>>>>> support).
>>>>>>
>>>>>> That's a bit of a soft definition.
>>>>>>
>>>>>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>>>>>> device I think.
>>>>>>
>>>>>> The precedent is a ton of ethernet drivers with nvram for storing
>>>>>> e.g.
>>>>>> the MAC address. We don't make all of those into MFDs, as the nvram
>>>>>> is
>>>>>> closely tied to the one and only function of the block.
>>>>>
>>>>> I agree with Linus. This should be part of the actual (main) driver
>>>>> for the chip as many
>>>>> do (like USB to serial adapters that have GPIO capability).
>>>
>>> You mean the gpio driver is calling nvmem_register()? Yeah I agree,
>>> that
>>> should work.
>>>
>>>> I think gpio_regmap is not suitable for this driver as Michael
>>>> stated.
>>>> https://www.analog.com/media/en/technical-documentation/data-
>>> sheets/ds4520.pdf
>>>> Please check block diagram. There are two input registers that
>>>> control
>>>> gpio state
>>>> so gpio_regmap does not look ok for this. Am I missing something?
>>>
>>> You mean F8/F9? That will work as they are for different GPIOs. What
>>> doesn't work with gpio-regmap is when you need to modify two different
>>> registers for one GPIO. Have a look at gpio_regmap_get() and
>>> gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't
>>> work
>>> you can use your own .xlate() op.
>>>
>>
>> Actually, I checked the functions that you suggested, but as far as I
>> understand
>> they might work if there would be one bit to set direction or value.
>> However,
>> this is not the case for ds4520. In other words, if I want to set the
>> gpio direction
>> as output, I need to set a corresponding bit for both F0 and F1
>> registers.
>
>I can't follow. F0/F1 is for the pull-up. That was actually my initial
>question and Linus said, that should probably be done in a seperate
>.set_config operation not together with a direction change.

I think I understand what you are trying to say so far. I did not have too much
experience related to gpio. I will set pull_up register in .set_config
However, I did not understand where its parameters come from.
set_config(struct gpio_chip *chip, unsigned int offset,
	      unsigned long config)
It might be trivial question, but Where does config come from?

At the end, I should rewrite the code using regmap_gpio, right? So if I rewrite 
code using regmap_gpio, how can I replace set_config(...)?

>
>> In the document, you can see block diagram. I do not know why, but
>> design is
>> not standard that’s why I think I can not use gpio-regmap.
>>
>>>> Also, at this point I am not planning to add nvmem support.
>>>
>>> That is a pity, because that is the whole use case for this gpio
>>> expander,
>>> no? "Programmable Replacement for Mechanical Jumpers and Switches"
>>
>> I can set "SEE" bit as "0" in the Configuration Register to write
>> EEPROM so it might solve
>> issue.
>
>If you do that unconditionally, that might wear out the EEPROM,
>though.
>
>-michael

Hi Michael,

Thank you for your support.

Regards,
Okan Sahin

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Michael Walle]

Hi,

> I think I understand what you are trying to say so far. I did not have 
> too much
> experience related to gpio. I will set pull_up register in .set_config
> However, I did not understand where its parameters come from.
> set_config(struct gpio_chip *chip, unsigned int offset,
> 	      unsigned long config)
> It might be trivial question, but Where does config come from?

Others have to answer that one as I don't have that much experience 
either.

> At the end, I should rewrite the code using regmap_gpio, right? So if I 
> rewrite
> code using regmap_gpio, how can I replace set_config(...)?

You'd have to add a .set_config to gpio_regmap_config and then in

gpio_regmap_register():
   gpio->set_config = config->set_config;

I don't think it makes sense to have a default implementation in 
gpio-regmap,
the variances between "simple" gpio controllers might be too broad.

-michael

RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Sahin, Okan]

>> I think I understand what you are trying to say so far. I did not have
>> too much
>> experience related to gpio. I will set pull_up register in .set_config
>> However, I did not understand where its parameters come from.
>> set_config(struct gpio_chip *chip, unsigned int offset,
>> 	      unsigned long config)
>> It might be trivial question, but Where does config come from?
>
>Others have to answer that one as I don't have that much experience
>either.
>
>> At the end, I should rewrite the code using regmap_gpio, right? So if I
>> rewrite
>> code using regmap_gpio, how can I replace set_config(...)?
>
>You'd have to add a .set_config to gpio_regmap_config and then in
>
>gpio_regmap_register():
>   gpio->set_config = config->set_config;
>
>I don't think it makes sense to have a default implementation in
>gpio-regmap,
>the variances between "simple" gpio controllers might be too broad.
>
>-michael

Hi,

One last question, as ds4520 IC has 9 I/O pins so I need to set registers like 
below
struct gpio_regmap *gpio;
config.reg_dir_out_base = IO_CONTROL0; (get_direction and setting direction)
config.reg_dat_base = IO_STATUS0; (for .get)
config.reg_set_base = IO_STATUS0; (for .set)

As I have both directions, I must set both reg_dat_base and
reg_set_base.

https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/regmap.h#L52

In this case, I am able to use only pull_up register to set output value to high
as default. Is that okay? I am asking again and again to minimize number of 
patch. I want to be sure before sending new patch. Thank you for your contribution.

Regards,
Okan Sahin

Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Andy Shevchenko]

On Sun, Apr 9, 2023 at 5:25 PM Sahin, Okan <Okan.Sahin@analog.com> wrote:
> >Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
> >> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> wrote:

...

> >> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
> >> device I think.

I have read a datasheet, it has the pre-boot settings, but it doesn't
matter from the Linux point of view. The only thing which we need to
take care of is to have the EEPROM disabled during GPIO interaction.
However, there is a question as to how this device should actually
commit into EEPROM the state to survive the cold/warm power cycle.
What is the persistent flag for, btw, I haven't been familiar with it?

> >> The precedent is a ton of ethernet drivers with nvram for storing e.g.
> >> the MAC address. We don't make all of those into MFDs, as the nvram is
> >> closely tied to the one and only function of the block.
> >
> >I agree with Linus. This should be part of the actual (main) driver for the chip as many
> >do (like USB to serial adapters that have GPIO capability).
> >Also this code lacks of proper locking and has style issues.

...

> Could you give more detail related to locking and style issues?

You use a few IO accessors in a row without locking, which means at
any point of this flow some other CPUs may access the chip using the
same or other APIs of this driver.

-- 
With Best Regards,
Andy Shevchenko