[PATCH v2 1/2] dt-bindings: gpio: max7317: add spi gpio extender documentation

[PATCH v2 1/2] dt-bindings: gpio: max7317: add spi gpio extender documentation

[Edmund Berenson]

Add driver documentation for the maxim max7317 spi
gpio expander.

Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
---
 .../bindings/gpio/gpio-max7317.yaml           | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max7317.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml
new file mode 100644
index 000000000000..ad5a796c704e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-max7317.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7317 SPI-Interfaced I/O Expander
+
+maintainers:
+  - Edmund Berenson <edmund.berenson@emlix.com>
+
+description:
+  Bindings for 10-Port Maxim MAX7317 SPI GPIO expanders.
+
+properties:
+  compatible:
+    const: maxim,max7317
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+unevaluatedProperties: false
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+examples:
+  - |
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            gpio@0 {
+                    compatible = "maxim,max7317";
+                    reg = <0>;
+                    gpio-controller;
+                    #gpio-cells = <2>;
+            };
+    };
-- 
2.39.2

[PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Edmund Berenson]

Add driver for maxim MAX7317 SPI-Interfaced 10 Port
GPIO Expander.

v2: adjust driver to use regmap

Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
---
 drivers/gpio/Kconfig        |  11 +++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-max7317.c | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/gpio/gpio-max7317.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..109cf09d8c05 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1615,6 +1615,17 @@ config GPIO_MAX7301
 	help
 	  GPIO driver for Maxim MAX7301 SPI-based GPIO expander.
 
+config GPIO_MAX7317
+	tristate "Maxim MAX7317 GPIO expander"
+	select REGMAP_SPI
+	help
+	  GPIO driver for Maxim MAX7317 SPI-based GPIO expander.
+	  The MAX7317 is a serial-interfaced gpio extender, with
+	  10 ports.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-max7317.
+
 config GPIO_MC33880
 	tristate "Freescale MC33880 high-side/low-side switch"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c048ba003367..8dce549bb6c5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_GPIO_MAX7300)		+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)		+= gpio-max7301.o
 obj-$(CONFIG_GPIO_MAX730X)		+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX732X)		+= gpio-max732x.o
+obj-$(CONFIG_GPIO_MAX7317)		+= gpio-max7317.o
 obj-$(CONFIG_GPIO_MAX77620)		+= gpio-max77620.o
 obj-$(CONFIG_GPIO_MAX77650)		+= gpio-max77650.o
 obj-$(CONFIG_GPIO_MB86S7X)		+= gpio-mb86s7x.o
diff --git a/drivers/gpio/gpio-max7317.c b/drivers/gpio/gpio-max7317.c
new file mode 100644
index 000000000000..65824efaad6c
--- /dev/null
+++ b/drivers/gpio/gpio-max7317.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021, Lukasz Zemla, Woodward Inc.
+ */
+
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#define MAX7317_PIN_NUMBER 10
+#define REG_CODE_READ_PORTS_7_TO_0	((u8)0x0E)
+#define REG_CODE_READ_PORTS_9_TO_8	((u8)0x0F)
+
+struct max7317 {
+	struct gpio_chip	chip;
+	struct regmap		*regmap;
+	int			direction[MAX7317_PIN_NUMBER];
+};
+
+struct max7317_platform_data {
+	unsigned int	gpio_base;
+};
+
+static const struct regmap_config max7317_regmap_cfg =  {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int max7317_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+	u8 reg = (offset < 8) ? REG_CODE_READ_PORTS_7_TO_0 : REG_CODE_READ_PORTS_9_TO_8;
+
+	ret = regmap_read(ts->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return val & BIT(offset % 8);
+}
+
+/*
+ * After writing the register an additional read is performed in order for
+ * changes to take effect.
+ */
+static void max7317_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+
+	if (regmap_write(ts->regmap, offset, value ? 1 : 0))
+		dev_err(chip->parent, "Failed to set pin: %d\n", offset);
+	max7317_get(chip, offset);
+}
+
+static int max7317_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+
+	max7317_set(chip, offset, 1);
+	ts->direction[offset] = GPIO_LINE_DIRECTION_IN;
+	return 0;
+}
+
+static int max7317_direction_output(struct gpio_chip *chip, unsigned int offset,
+				    int value)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+
+	ts->direction[offset] = GPIO_LINE_DIRECTION_OUT;
+	return 0;
+}
+
+static int max7317_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct max7317 *ts = gpiochip_get_data(chip);
+
+	return ts->direction[offset];
+}
+
+static int max7317_probe(struct spi_device *spi)
+{
+	struct max7317 *ts;
+	struct device *dev;
+	struct max7317_platform_data *pdata;
+	int i;
+
+	ts = devm_kzalloc(&spi->dev, sizeof(struct max7317), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+	dev_set_drvdata(&spi->dev, ts);
+
+	pdata = dev_get_platdata(dev);
+	if (pdata)
+		ts->chip.base = pdata->gpio_base;
+	else
+		ts->chip.base = -1;
+
+	ts->chip.label = dev_name(&spi->dev);
+	ts->chip.direction_input = max7317_direction_input;
+	ts->chip.get = max7317_get;
+	ts->chip.direction_output = max7317_direction_output;
+	ts->chip.set = max7317_set;
+	ts->chip.get_direction = max7317_get_direction;
+
+	ts->chip.ngpio = MAX7317_PIN_NUMBER;
+	ts->chip.can_sleep = true;
+	ts->chip.parent = &spi->dev;
+	ts->chip.owner = THIS_MODULE;
+
+	for (i = 0; i < MAX7317_PIN_NUMBER; i++)
+		ts->direction[i] = GPIO_LINE_DIRECTION_IN;
+
+	ts->regmap = devm_regmap_init_spi(spi, &max7317_regmap_cfg);
+	if (IS_ERR(ts->regmap))
+		return PTR_ERR(ts->regmap);
+
+	return devm_gpiochip_add_data(&spi->dev, &ts->chip, ts);
+}
+
+static void max7317_remove(struct spi_device *spi)
+{
+	struct max7317 *ts = dev_get_drvdata(&spi->dev);
+
+	if (!ts)
+		return;
+
+	gpiochip_remove(&ts->chip);
+}
+
+static const struct spi_device_id max7317_id[] = {
+	{ "max7317", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max7317_id);
+
+static const struct of_device_id max7317_dt_ids[] = {
+	{ .compatible = "maxim,max7317" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max7317_dt_ids);
+
+static struct spi_driver max7317_driver = {
+	.driver = {
+		.name = "max7317",
+		.of_match_table	= max7317_dt_ids,
+	},
+	.probe = max7317_probe,
+	.remove = max7317_remove,
+	.id_table = max7317_id,
+};
+module_spi_driver(max7317_driver);
+
+MODULE_AUTHOR("Lukasz Zemla");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MAX7317 GPIO-Expander");
-- 
2.39.2

Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Bartosz Golaszewski]

On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
<edmund.berenson@emlix.com> wrote:
>
> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
> GPIO Expander.
>
> v2: adjust driver to use regmap
>
> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
> ---
>  drivers/gpio/Kconfig        |  11 +++
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-max7317.c | 161 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 173 insertions(+)
>  create mode 100644 drivers/gpio/gpio-max7317.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..109cf09d8c05 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1615,6 +1615,17 @@ config GPIO_MAX7301
>         help
>           GPIO driver for Maxim MAX7301 SPI-based GPIO expander.
>
> +config GPIO_MAX7317
> +       tristate "Maxim MAX7317 GPIO expander"
> +       select REGMAP_SPI
> +       help
> +         GPIO driver for Maxim MAX7317 SPI-based GPIO expander.
> +         The MAX7317 is a serial-interfaced gpio extender, with
> +         10 ports.
> +
> +         This driver can also be built as a module. If so, the module will be
> +         called gpio-max7317.
> +
>  config GPIO_MC33880
>         tristate "Freescale MC33880 high-side/low-side switch"
>         help
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c048ba003367..8dce549bb6c5 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -91,6 +91,7 @@ obj-$(CONFIG_GPIO_MAX7300)            += gpio-max7300.o
>  obj-$(CONFIG_GPIO_MAX7301)             += gpio-max7301.o
>  obj-$(CONFIG_GPIO_MAX730X)             += gpio-max730x.o
>  obj-$(CONFIG_GPIO_MAX732X)             += gpio-max732x.o
> +obj-$(CONFIG_GPIO_MAX7317)             += gpio-max7317.o
>  obj-$(CONFIG_GPIO_MAX77620)            += gpio-max77620.o
>  obj-$(CONFIG_GPIO_MAX77650)            += gpio-max77650.o
>  obj-$(CONFIG_GPIO_MB86S7X)             += gpio-mb86s7x.o
> diff --git a/drivers/gpio/gpio-max7317.c b/drivers/gpio/gpio-max7317.c
> new file mode 100644
> index 000000000000..65824efaad6c
> --- /dev/null
> +++ b/drivers/gpio/gpio-max7317.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021, Lukasz Zemla, Woodward Inc.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +
> +#define MAX7317_PIN_NUMBER 10
> +#define REG_CODE_READ_PORTS_7_TO_0     ((u8)0x0E)
> +#define REG_CODE_READ_PORTS_9_TO_8     ((u8)0x0F)
> +
> +struct max7317 {

Please call it max7317_gpio or something similar.

> +       struct gpio_chip        chip;
> +       struct regmap           *regmap;
> +       int                     direction[MAX7317_PIN_NUMBER];

Don't use tabs in structs, it doesn't really add to readability in
small declarations.

> +};
> +
> +struct max7317_platform_data {
> +       unsigned int    gpio_base;
> +};
> +
> +static const struct regmap_config max7317_regmap_cfg =  {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +};
> +
> +static int max7317_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +       unsigned int val;
> +       int ret;
> +       u8 reg = (offset < 8) ? REG_CODE_READ_PORTS_7_TO_0 : REG_CODE_READ_PORTS_9_TO_8;
> +
> +       ret = regmap_read(ts->regmap, reg, &val);
> +       if (ret)
> +               return ret;
> +
> +       return val & BIT(offset % 8);
> +}
> +
> +/*
> + * After writing the register an additional read is performed in order for
> + * changes to take effect.
> + */
> +static void max7317_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +
> +       if (regmap_write(ts->regmap, offset, value ? 1 : 0))
> +               dev_err(chip->parent, "Failed to set pin: %d\n", offset);
> +       max7317_get(chip, offset);

Don't you need to protect both the write and read operations from
concurrent access here?


> +}
> +
> +static int max7317_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +
> +       max7317_set(chip, offset, 1);
> +       ts->direction[offset] = GPIO_LINE_DIRECTION_IN;
> +       return 0;
> +}
> +
> +static int max7317_direction_output(struct gpio_chip *chip, unsigned int offset,
> +                                   int value)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +
> +       ts->direction[offset] = GPIO_LINE_DIRECTION_OUT;
> +       return 0;
> +}
> +
> +static int max7317_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct max7317 *ts = gpiochip_get_data(chip);
> +
> +       return ts->direction[offset];
> +}
> +
> +static int max7317_probe(struct spi_device *spi)
> +{
> +       struct max7317 *ts;
> +       struct device *dev;
> +       struct max7317_platform_data *pdata;

Where does this come from?

> +       int i;
> +
> +       ts = devm_kzalloc(&spi->dev, sizeof(struct max7317), GFP_KERNEL);
> +       if (!ts)
> +               return -ENOMEM;
> +       dev_set_drvdata(&spi->dev, ts);
> +
> +       pdata = dev_get_platdata(dev);
> +       if (pdata)
> +               ts->chip.base = pdata->gpio_base;

Don't encourage setting a hard-coded base via platform data. Please
use -1 unconditionally in new drivers.

> +       else
> +               ts->chip.base = -1;
> +
> +       ts->chip.label = dev_name(&spi->dev);
> +       ts->chip.direction_input = max7317_direction_input;
> +       ts->chip.get = max7317_get;
> +       ts->chip.direction_output = max7317_direction_output;
> +       ts->chip.set = max7317_set;
> +       ts->chip.get_direction = max7317_get_direction;
> +
> +       ts->chip.ngpio = MAX7317_PIN_NUMBER;
> +       ts->chip.can_sleep = true;
> +       ts->chip.parent = &spi->dev;
> +       ts->chip.owner = THIS_MODULE;
> +
> +       for (i = 0; i < MAX7317_PIN_NUMBER; i++)
> +               ts->direction[i] = GPIO_LINE_DIRECTION_IN;
> +
> +       ts->regmap = devm_regmap_init_spi(spi, &max7317_regmap_cfg);
> +       if (IS_ERR(ts->regmap))
> +               return PTR_ERR(ts->regmap);
> +
> +       return devm_gpiochip_add_data(&spi->dev, &ts->chip, ts);
> +}
> +
> +static void max7317_remove(struct spi_device *spi)
> +{
> +       struct max7317 *ts = dev_get_drvdata(&spi->dev);
> +
> +       if (!ts)
> +               return;
> +
> +       gpiochip_remove(&ts->chip);

This is clearly wrong - you registered the chip using
devm_gpiochip_add_data() precisely to avoid having to call
gpiochip_remove(). And checking for !it makes no sense either - you
should have set it yourself, which you didn't.

Bart

> +}
> +
> +static const struct spi_device_id max7317_id[] = {
> +       { "max7317", 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(spi, max7317_id);
> +
> +static const struct of_device_id max7317_dt_ids[] = {
> +       { .compatible = "maxim,max7317" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, max7317_dt_ids);
> +
> +static struct spi_driver max7317_driver = {
> +       .driver = {
> +               .name = "max7317",
> +               .of_match_table = max7317_dt_ids,
> +       },
> +       .probe = max7317_probe,
> +       .remove = max7317_remove,
> +       .id_table = max7317_id,
> +};
> +module_spi_driver(max7317_driver);
> +
> +MODULE_AUTHOR("Lukasz Zemla");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MAX7317 GPIO-Expander");
> --
> 2.39.2
>

Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Linus Walleij]

On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
<edmund.berenson@emlix.com> wrote:

> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
> GPIO Expander.
>
> v2: adjust driver to use regmap
>
> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>

Notwithstanding the other comments from Bartosz, this seems like
a driver that should be using the regmap GPIO helper library.
git grep GPIO_REGMAP will show you examples of other drivers
that use this and how it is used.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Edmund Berenson]

On 4/4/23 16:05, Linus Walleij wrote:
> On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
> <edmund.berenson@emlix.com> wrote:
> 
>> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
>> GPIO Expander.
>>
>> v2: adjust driver to use regmap
>>
>> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
>> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
>> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
> 
> Notwithstanding the other comments from Bartosz, this seems like
> a driver that should be using the regmap GPIO helper library.
> git grep GPIO_REGMAP will show you examples of other drivers
> that use this and how it is used.
> 
> Yours,
> Linus Walleij

Hi,

thanks for the review and suggestion. I tried following your suggestion and use
GPIO_REGMAP to implement the driver.

Unfortunately I ran into two issues
1. reg_set_base == 0: for the devcie reg_set base is 0x0. In gpio-regmap there
are several tests for !reg_set_base. There doesn't seem a way to distinguish
between is set to 0 and is not set. :)

2. input/output direction: to set a gpio pin to input one has to write 0x1 to
the corresponding output register. The issue starts when I configure a port to
be an output, set output to 0x1, check the direction of the pin, doing so trough
sysfs the system will now assume the pin is an input and I can't set its values
anymore. Avoiding this I would like to track the direction of the pin separately
from the device register, which is atm done in the corresponding bespoke in/out
functions.

I could probably solve both of these issues trough the reg_mask_xlate function
but I believe this would introduce unneeded obscurity in the driver.

I do not believe there are any other easy obvious/better fixes for this. (or
maybe you prove me wrong :))
Would you be okay for this driver to stick with direct regmap usage? (obviously
fixing the review suggestions)

BR
Edmund

-- 
Edmund Berenson, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Michael Walle]

Am 2023-05-03 10:37, schrieb Edmund Berenson:
> On 4/4/23 16:05, Linus Walleij wrote:
>> On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
>> <edmund.berenson@emlix.com> wrote:
>> 
>>> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
>>> GPIO Expander.
>>> 
>>> v2: adjust driver to use regmap
>>> 
>>> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
>>> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
>>> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
>> 
>> Notwithstanding the other comments from Bartosz, this seems like
>> a driver that should be using the regmap GPIO helper library.
>> git grep GPIO_REGMAP will show you examples of other drivers
>> that use this and how it is used.
>> 
>> Yours,
>> Linus Walleij
> 
> Hi,
> 
> thanks for the review and suggestion. I tried following your suggestion 
> and use
> GPIO_REGMAP to implement the driver.
> 
> Unfortunately I ran into two issues
> 1. reg_set_base == 0: for the devcie reg_set base is 0x0. In 
> gpio-regmap there
> are several tests for !reg_set_base. There doesn't seem a way to 
> distinguish
> between is set to 0 and is not set. :)

That's what GPIO_REGMAP_ADDR(addr) is for :)

> 2. input/output direction: to set a gpio pin to input one has to write 
> 0x1 to
> the corresponding output register. The issue starts when I configure a 
> port to
> be an output, set output to 0x1, check the direction of the pin, doing 
> so trough
> sysfs the system will now assume the pin is an input and I can't set 
> its values
> anymore. Avoiding this I would like to track the direction of the pin 
> separately
> from the device register, which is atm done in the corresponding 
> bespoke in/out
> functions.

I can't follow you here exactly. But I've briefly looked at the 
datasheet
and the output is an open drain one. Just like the one we are currently
discussing in [1]. Here too, there seems to be no direction register,
although it is mentioned in the datasheet. But Table 1. Register Address 
Map
is super confusing, so please correct me if I'm wrong.

Since we now have already two different chips with this OD output and 
always
active input buffer, it makes sense to add support to gpio-regmap for 
these
kind of devices. To me it is still unclear wether we need the direction 
at
all. Linus answered my question yesterday, but I haven't found time to 
dig
into that topic myself. Please go ahead and make some suggestions :)

> I could probably solve both of these issues trough the reg_mask_xlate 
> function
> but I believe this would introduce unneeded obscurity in the driver.
> 
> I do not believe there are any other easy obvious/better fixes for 
> this. (or
> maybe you prove me wrong :))
> Would you be okay for this driver to stick with direct regmap usage? 
> (obviously
> fixing the review suggestions)

-michael

[1] https://lore.kernel.org/r/20230502084406.3529645-1-michael@walle.cc/

Re: [PATCH v2 1/2] dt-bindings: gpio: max7317: add spi gpio extender documentation

[Krzysztof Kozlowski]

On 03/04/2023 13:40, Edmund Berenson wrote:
> Add driver documentation for the maxim max7317 spi
> gpio expander.

Subject: drop second/last, redundant "documentation". The "dt-bindings"
prefix is already stating that these are bindings and documentation.

> 
> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com>
> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com>
> ---

This is v2, so where is the changelog? No cover letter, either.

>  .../bindings/gpio/gpio-max7317.yaml           | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max7317.yaml

Filename must be like compatible. "gpio" is not a vendor prefix.
maxim,max7317.yaml


> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml
> new file mode 100644
> index 000000000000..ad5a796c704e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-max7317.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7317 SPI-Interfaced I/O Expander
> +
> +maintainers:
> +  - Edmund Berenson <edmund.berenson@emlix.com>
> +
> +description:
> +  Bindings for 10-Port Maxim MAX7317 SPI GPIO expanders.

Describe the hardware, not the "bindings". Drop "Bindings for".

> +
> +properties:
> +  compatible:
> +    const: maxim,max7317
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':

Use consistent quotes, either ' or ".

> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +unevaluatedProperties: false
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#

Fix ordering. allOf goes either before properties or before
unevaluatedProps. See for example example-schema.

> +
> +examples:
> +  - |
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;

Use 4 spaces for example indentation.

> +


Best regards,
Krzysztof