[PATCH v1 0/2] gpio: fxl6408: add I2C GPIO expander driver

[PATCH v1 0/2] gpio: fxl6408: add I2C GPIO expander driver

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add support for Fairchild (now ON Semiconductor) fxl6408 8-bit I2C-controlled
GPIO expander, see data-sheet [0].

[0] https://www.onsemi.com/download/data-sheet/pdf/fxl6408-d.pdf

Emanuele Ghidoli (2):
  dt-bindings: gpio: add fcs,fxl6408-gpio binding document
  gpio: fxl6408: add I2C GPIO expander driver

 .../bindings/gpio/fcs,fxl6408-gpio.yaml       |  73 ++++++++
 drivers/gpio/Kconfig                          |   6 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-fxl6408.c                   | 160 ++++++++++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
 create mode 100644 drivers/gpio/gpio-fxl6408.c

-- 
2.25.1

[PATCH v1 1/2] dt-bindings: gpio: add fcs,fxl6408-gpio binding document

[Francesco Dolcini]

From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Add binding document for Fairchild FXL6408 GPIO expander.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
---
 .../bindings/gpio/fcs,fxl6408-gpio.yaml       | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
new file mode 100644
index 000000000000..ccf946040d00
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/fcs,fxl6408-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: FXL6408 GPIO driver
+
+maintainers:
+  - Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
+
+description: |
+  Driver for Fairchild FXL6408 GPIO expander
+
+properties:
+  compatible:
+    enum:
+      - fcs,fxl6408
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  gpio-line-names:
+    minItems: 1
+    maxItems: 8
+
+patternProperties:
+  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      input: true
+      output-high: true
+      output-low: true
+      line-name: true
+
+    required:
+      - gpio-hog
+      - gpios
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        gpio_expander_43: gpio-expander@43 {
+            compatible = "fcs,fxl6408";
+            gpio-controller;
+            #gpio-cells = <2>;
+            reg = <0x43>;
+            gpio-line-names = "Wi-Fi_W_DISABLE", "Wi-Fi_WKUP_WLAN",
+              "PWR_EN_+V3.3_WiFi_N", "PCIe_REF_CLK_EN",
+              "USB_RESET_N", "USB_BYPASS_N", "Wi-Fi_PDn",
+              "Wi-Fi_WKUP_BT";
+        };
+    };
-- 
2.25.1

[PATCH v1 2/2] gpio: fxl6408: add I2C GPIO expander driver

[Francesco Dolcini]

From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines
and is controlled by I2C bus.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
---
 drivers/gpio/Kconfig        |   6 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-fxl6408.c | 160 ++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/gpio/gpio-fxl6408.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..34ed748dba5c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1000,6 +1000,12 @@ config GPIO_ADNP
 	  enough to represent all pins, but the driver will assume a
 	  register layout for 64 pins (8 registers).
 
+config GPIO_FXL6408
+	tristate "FXL6408 I2C GPIO expander"
+	select GPIO_REGMAP
+	help
+	  GPIO driver for Fairchild Semiconductor FXL6408 GPIO expander
+
 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..12027f4c3bee 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_GPIO_EP93XX)		+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_EXAR)			+= gpio-exar.o
 obj-$(CONFIG_GPIO_F7188X)		+= gpio-f7188x.o
 obj-$(CONFIG_GPIO_FTGPIO010)		+= gpio-ftgpio010.o
+obj-$(CONFIG_GPIO_FXL6408)		+= gpio-fxl6408.o
 obj-$(CONFIG_GPIO_GE_FPGA)		+= gpio-ge.o
 obj-$(CONFIG_GPIO_GPIO_MM)		+= gpio-gpio-mm.o
 obj-$(CONFIG_GPIO_GRGPIO)		+= gpio-grgpio.o
diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c
new file mode 100644
index 000000000000..7b28b09c7b66
--- /dev/null
+++ b/drivers/gpio/gpio-fxl6408.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * FXL6408 GPIO driver
+ *
+ * Copyright 2023 Toradex
+ *
+ * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/gpio/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+
+#define FXL6408_REG_DEVICE_ID		0x01
+#define FXL6408_MF_FAIRCHILD		0b101
+#define FXL6408_MF_SHIFT		5
+
+/* Bits set here indicate that the GPIO is an output. */
+#define FXL6408_REG_IO_DIR		0x03
+
+/* Bits set here, when the corresponding bit of IO_DIR is set, drive
+ * the output high instead of low.
+ */
+#define FXL6408_REG_OUTPUT		0x05
+
+/* Bits here make the output High-Z, instead of the OUTPUT value. */
+#define FXL6408_REG_OUTPUT_HIGH_Z	0x07
+
+/* Returns the current status (1 = HIGH) of the input pins. */
+#define FXL6408_REG_INPUT_STATUS	0x0f
+
+#define FXL6408_MAX_REGISTER		0x13
+
+#define FXL6408_NGPIO			8
+
+static const struct regmap_range rd_range[] = {
+	{ FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
+	{ FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
+	{ FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS }
+};
+
+static const struct regmap_range wr_range[] = {
+	{ FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
+	{ FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
+	{ FXL6408_REG_OUTPUT_HIGH_Z, FXL6408_REG_OUTPUT_HIGH_Z }
+};
+
+static const struct regmap_range volatile_range[] = {
+	{ FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
+	{ FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS }
+};
+
+static const struct regmap_access_table rd_table = {
+	.yes_ranges = rd_range,
+	.n_yes_ranges = ARRAY_SIZE(rd_range)
+};
+
+static const struct regmap_access_table wr_table = {
+	.yes_ranges = wr_range,
+	.n_yes_ranges = ARRAY_SIZE(wr_range)
+};
+
+static const struct regmap_access_table volatile_table = {
+	.yes_ranges = volatile_range,
+	.n_yes_ranges = ARRAY_SIZE(volatile_range)
+};
+
+static const struct regmap_config regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = FXL6408_MAX_REGISTER,
+	.wr_table = &wr_table,
+	.rd_table = &rd_table,
+	.volatile_table = &volatile_table,
+
+	.cache_type = REGCACHE_RBTREE,
+	.num_reg_defaults_raw = FXL6408_MAX_REGISTER + 1
+};
+
+static int fxl6408_identify(struct device *dev, struct regmap *regmap)
+{
+	int val, ret;
+
+	ret = regmap_read(regmap, FXL6408_REG_DEVICE_ID, &val);
+	if (ret) {
+		dev_err(dev, "error %d reading DEVICE_ID\n", ret);
+	} else if (val >> FXL6408_MF_SHIFT != FXL6408_MF_FAIRCHILD) {
+		dev_err(dev, "invalid device id 0x%02x\n", val);
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static int fxl6408_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	int ret;
+	struct gpio_regmap_config gpio_config = {
+		.parent = dev,
+		.ngpio = FXL6408_NGPIO,
+		.reg_dat_base = GPIO_REGMAP_ADDR(FXL6408_REG_INPUT_STATUS),
+		.reg_set_base = GPIO_REGMAP_ADDR(FXL6408_REG_OUTPUT),
+		.reg_dir_out_base = GPIO_REGMAP_ADDR(FXL6408_REG_IO_DIR),
+		.ngpio_per_reg = FXL6408_NGPIO
+	};
+	gpio_config.regmap = devm_regmap_init_i2c(client, &regmap);
+
+	if (IS_ERR(gpio_config.regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(gpio_config.regmap);
+	}
+
+	ret = fxl6408_identify(dev, gpio_config.regmap);
+	if (ret)
+		return ret;
+
+	/* Disable High-Z of outputs, so that our OUTPUT updates
+	 * actually take effect.
+	 */
+	ret = regmap_write(gpio_config.regmap, FXL6408_REG_OUTPUT_HIGH_Z, 0);
+	if (ret) {
+		dev_err(dev, "failed to write 'output high Z' register\n");
+		return ret;
+	}
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
+}
+
+static const __maybe_unused struct of_device_id fxl6408_dt_ids[] = {
+	{ .compatible = "fcs,fxl6408" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, fxl6408_dt_ids);
+
+static const struct i2c_device_id fxl6408_id[] = {
+	{ "fxl6408", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, fxl6408_id);
+
+static struct i2c_driver fxl6408_driver = {
+	.driver = {
+		.name	= "fxl6408",
+		.of_match_table = fxl6408_dt_ids,
+	},
+	.probe_new	= fxl6408_probe,
+	.id_table	= fxl6408_id,
+};
+
+module_i2c_driver(fxl6408_driver);
+
+MODULE_AUTHOR("Emanuele Ghidoli <emanuele.ghidoli@toradex.com>");
+MODULE_DESCRIPTION("FXL6408 GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v1 1/2] dt-bindings: gpio: add fcs,fxl6408-gpio binding document

[Krzysztof Kozlowski]

On 06/03/2023 09:34, Francesco Dolcini wrote:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Add binding document for Fairchild FXL6408 GPIO expander.

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

> 
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

Missing SoB.

> ---
>  .../bindings/gpio/fcs,fxl6408-gpio.yaml       | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> new file mode 100644
> index 000000000000..ccf946040d00
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/fcs,fxl6408-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FXL6408 GPIO driver

If "driver" means Linux driver, then drop.

Fairchild FXL6408 GPIO Expander

> +
> +maintainers:
> +  - Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> +
> +description: |
> +  Driver for Fairchild FXL6408 GPIO expander

This is not a driver. Drop entire description as it is duplicating title
or add here something useful.


> +
> +properties:
> +  compatible:
> +    enum:
> +      - fcs,fxl6408
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  gpio-line-names:
> +    minItems: 1
> +    maxItems: 8
> +
> +patternProperties:
> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":

From here....

> +    type: object
> +    properties:
> +      gpio-hog: true
> +      gpios: true
> +      input: true
> +      output-high: true
> +      output-low: true
> +      line-name: true
> +
> +    required:
> +      - gpio-hog
> +      - gpios
> +
> +    additionalProperties: false

To here, all this can be simpler:

  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
    required:
      - gpio-hog

which selects gpio hog schema.

> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        gpio_expander_43: gpio-expander@43 {
> +            compatible = "fcs,fxl6408";
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +            reg = <0x43>;
> +            gpio-line-names = "Wi-Fi_W_DISABLE", "Wi-Fi_WKUP_WLAN",
> +              "PWR_EN_+V3.3_WiFi_N", "PCIe_REF_CLK_EN",

Align with previous line/entries.

> +              "USB_RESET_N", "USB_BYPASS_N", "Wi-Fi_PDn",
> +              "Wi-Fi_WKUP_BT";
> +        };
> +    };

Best regards,
Krzysztof

Re: [PATCH v1 2/2] gpio: fxl6408: add I2C GPIO expander driver

[Francesco Dolcini]

On Mon, Mar 06, 2023 at 09:34:46AM +0100, Francesco Dolcini wrote:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines
> and is controlled by I2C bus.
> 
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Whoops. As already noticed by Krzysztof in patch 1/2 I forgot my SoB,
will fix in v2.

Francesco

Re: [PATCH v1 2/2] gpio: fxl6408: add I2C GPIO expander driver

[andy.shevchenko]

Mon, Mar 06, 2023 at 09:34:46AM +0100, Francesco Dolcini kirjoitti:
> From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines
> and is controlled by I2C bus.

Is it really GPIO expander and not a (semi-)featured pin control with GPIO
capability?

Can we have a Datasheet: tag here?

> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

...

> +	help
> +	  GPIO driver for Fairchild Semiconductor FXL6408 GPIO expander

Checkpatch usually complains on the help < 3 lines.
You may add the module name for M choice.

...

> + * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

> + *

Unneeded blank line.

...

> +#include <linux/gpio.h>

No way. This must not be in any code.

...

> +#include <linux/of_platform.h>

Why?
For discrete components make sure you have not an OF-centric code.

...

> +static const struct regmap_range rd_range[] = {
> +	{ FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID },
> +	{ FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT },
> +	{ FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS }

In all definitions where the entry is _not_ a terminator, leave the trailing
comma in place.

> +};

...

> +	};

> +	gpio_config.regmap = devm_regmap_init_i2c(client, &regmap);

> +

This blank line is misplaced. Should be before devm_regmap_init_i2c() call.

> +	if (IS_ERR(gpio_config.regmap)) {

> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(gpio_config.regmap);

	return dev_err_probe();

> +	}

...

> +	/* Disable High-Z of outputs, so that our OUTPUT updates
> +	 * actually take effect.
> +	 */

/*
 * This is correct style for multi-line
 * comments. Yours needs to be fixed.
 */

...

> +	ret = regmap_write(gpio_config.regmap, FXL6408_REG_OUTPUT_HIGH_Z, 0);
> +	if (ret) {

> +		dev_err(dev, "failed to write 'output high Z' register\n");
> +		return ret;

	return dev_err_probe(...);

> +	}

...

> +static const struct i2c_device_id fxl6408_id[] = {
> +	{ "fxl6408", 0 },
> +	{ },

But no comma for a terminator entry.
 
> +};

...

> +

Unneeded blank line.

> +module_i2c_driver(fxl6408_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] dt-bindings: gpio: add fcs,fxl6408-gpio binding document

[Rob Herring]

On Mon, Mar 06, 2023 at 09:41:50AM +0100, Krzysztof Kozlowski wrote:
> On 06/03/2023 09:34, Francesco Dolcini wrote:
> > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> > 
> > Add binding document for Fairchild FXL6408 GPIO expander.
> 
> Subject: drop second/last, redundant "binding document". The
> "dt-bindings" prefix is already stating that these are bindings.
> 
> > 
> > Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> 
> Missing SoB.
> 
> > ---
> >  .../bindings/gpio/fcs,fxl6408-gpio.yaml       | 73 +++++++++++++++++++

Also, match the compatible string here dropping '-gpio'.

> >  1 file changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> > new file mode 100644
> > index 000000000000..ccf946040d00
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/fcs,fxl6408-gpio.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/fcs,fxl6408-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: FXL6408 GPIO driver
> 
> If "driver" means Linux driver, then drop.
> 
> Fairchild FXL6408 GPIO Expander
> 
> > +
> > +maintainers:
> > +  - Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> > +
> > +description: |
> > +  Driver for Fairchild FXL6408 GPIO expander
> 
> This is not a driver. Drop entire description as it is duplicating title
> or add here something useful.
> 
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fcs,fxl6408
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +  gpio-line-names:
> > +    minItems: 1
> > +    maxItems: 8
> > +
> > +patternProperties:
> > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> 
> >From here....
> 
> > +    type: object
> > +    properties:
> > +      gpio-hog: true
> > +      gpios: true
> > +      input: true
> > +      output-high: true
> > +      output-low: true
> > +      line-name: true
> > +
> > +    required:
> > +      - gpio-hog
> > +      - gpios
> > +
> > +    additionalProperties: false
> 
> To here, all this can be simpler:
> 
>   "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
>     required:
>       - gpio-hog
> 
> which selects gpio hog schema.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        gpio_expander_43: gpio-expander@43 {
> > +            compatible = "fcs,fxl6408";
> > +            gpio-controller;
> > +            #gpio-cells = <2>;
> > +            reg = <0x43>;
> > +            gpio-line-names = "Wi-Fi_W_DISABLE", "Wi-Fi_WKUP_WLAN",
> > +              "PWR_EN_+V3.3_WiFi_N", "PCIe_REF_CLK_EN",
> 
> Align with previous line/entries.
> 
> > +              "USB_RESET_N", "USB_BYPASS_N", "Wi-Fi_PDn",
> > +              "Wi-Fi_WKUP_BT";
> > +        };
> > +    };
> 
> Best regards,
> Krzysztof
>