[PATCH v3 0/7] Add support for MAX7360

[PATCH v3 0/7] Add support for MAX7360

[Mathieu Dubois-Briand]

This series implements a set of drivers allowing to support the Maxim
Integrated MAX7360 device.

The MAX7360 is an I2C key-switch and led controller, with following
functionalities:
- Keypad controller for a key matrix of up to 8 rows and 8 columns.
- Rotary encoder support, for a single rotary encoder.
- Up to 8 PWM outputs.
- Up to 8 GPIOs with support for interrupts and 6 GPOs.

Chipset pins are shared between all functionalities, so all cannot be
used at the same time.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
Changes in v3:
- Fix MFD device tree binding to add gpio child nodes.
- Fix various small issues in device tree bindings.
- Add missing line returns in error messages.
- Use dev_err_probe() when possible.
- Link to v2: https://lore.kernel.org/r/20241223-mdb-max7360-support-v2-0-37a8d22c36ed@bootlin.com

Changes in v2:
- Removing device tree subnodes for keypad, rotary encoder and pwm
  functionalities.
- Fixed dt-bindings syntax and naming.
- Fixed missing handling of requested period in PWM driver.
- Cleanup of the code
- Link to v1: https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-0-8e8317584121@bootlin.com

---
Kamel Bouhara (2):
      mfd: Add max7360 support
      pwm: max7360: Add MAX7360 PWM support

Mathieu Dubois-Briand (5):
      dt-bindings: mfd: gpio: Add MAX7360
      gpio: max7360: Add MAX7360 gpio support
      input: keyboard: Add support for MAX7360 keypad
      input: misc: Add support for MAX7360 rotary
      MAINTAINERS: Add entry on MAX7360 driver

 .../bindings/gpio/maxim,max7360-gpio.yaml          |  90 ++++
 .../devicetree/bindings/mfd/maxim,max7360.yaml     | 140 +++++++
 MAINTAINERS                                        |  12 +
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-max7360.c                        | 454 +++++++++++++++++++++
 drivers/input/keyboard/Kconfig                     |  12 +
 drivers/input/keyboard/Makefile                    |   1 +
 drivers/input/keyboard/max7360-keypad.c            | 284 +++++++++++++
 drivers/input/misc/Kconfig                         |  11 +
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/max7360-rotary.c                | 183 +++++++++
 drivers/mfd/Kconfig                                |  12 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/max7360.c                              | 296 ++++++++++++++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-max7360.c                          | 220 ++++++++++
 include/linux/mfd/max7360.h                        | 109 +++++
 19 files changed, 1850 insertions(+)
---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241219-mdb-max7360-support-223a8ce45ba3

Best regards,
-- 
Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>

[PATCH v3 1/7] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

Add device tree bindings for Maxim Integrated MAX7360 device with
support for keypad, rotary, gpios and pwm functionalities.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 .../bindings/gpio/maxim,max7360-gpio.yaml          |  90 +++++++++++++
 .../devicetree/bindings/mfd/maxim,max7360.yaml     | 140 +++++++++++++++++++++
 2 files changed, 230 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
new file mode 100644
index 000000000000..95e9ccd455c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/maxim,max7360-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7360 GPIO controller
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+
+description: |
+  Maxim MAX7360 GPIO controller, in MAX7360 chipset
+  https://www.analog.com/en/products/max7360.html
+
+  The device provide two series of GPIOs, referred here as GPIOs and GPOs.
+
+  PORT0 to PORT7 pins can be used as GPIOs, with support for interrupts and
+  constant-current mode. These pins will also be used by the torary encoder and
+  PWM functionalities.
+
+  COL2 to COL7 pins can be used as GPOs, there is no input capability. COL pins
+  will be partitionned, with the first pins being affected to the keypad
+  functionality and the last ones as GPOs. This partioning must be described
+  here using the ngpios property.
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-gpio
+      - maxim,max7360-gpo
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  ngpios:
+    minimum: 0
+    maximum: 8
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  maxim,constant-current-disable:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: >
+      Bit field, each bit disables constant-current output of the associated
+      GPIO, starting from the least significant bit for the first GPIO.
+
+required:
+  - compatible
+  - gpio-controller
+  - ngpios
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - maxim,max7360-gpio
+then:
+  required:
+    - interrupt-controller
+else:
+  properties:
+    interrupt-controller: false
+    maxim,constant-current-disable: false
+
+    ngpios:
+      maximum: 6
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+      compatible = "maxim,max7360-gpio";
+
+      gpio-controller;
+      #gpio-cells = <2>;
+      ngpios = <8>;
+      maxim,constant-current-disable = <0x06>;
+
+      interrupt-controller;
+      #interrupt-cells = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
new file mode 100644
index 000000000000..7c50bab8c6d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/maxim,max7360.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7360 Keypad, Rotary encoder, PWM and GPIO controller
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+
+description: |
+  Maxim MAX7360 device, with following functions:
+    - keypad controller
+    - rotary controller
+    - GPIO and GPO controller
+    - PWM controller
+
+  https://www.analog.com/en/products/max7360.html
+
+allOf:
+  - $ref: /schemas/input/matrix-keymap.yaml#
+  - $ref: /schemas/input/input.yaml#
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    items:
+      - const: inti
+      - const: intk
+
+  keypad-debounce-delay-ms:
+    description: Keypad debounce delay in ms
+    minimum: 9
+    maximum: 40
+    default: 9
+
+  autorepeat: true
+
+  rotary-debounce-delay-ms:
+    description: Rotary encoder debounce delay in ms
+    minimum: 0
+    maximum: 15
+    default: 0
+
+  linux,axis:
+    description: The input subsystem axis to map to this rotary encoder.
+
+  "#pwm-cells":
+    const: 3
+
+  gpio:
+    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
+    description: >
+      PORT0 to PORT7 general purpose input/output pins configuration.
+
+  gpo:
+    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
+    description: >
+      COL2 to COL7 general purpose output pins configuration.
+      Allows to use unused keypad columns as outputs.
+      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. Value
+      of ngpios must be coherent with the value of keypad,num-columns, as their
+      sum must not exceed the number of physical lines.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - linux,keymap
+  - linux,axis
+  - "#pwm-cells"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      io-expander@38 {
+        compatible = "maxim,max7360";
+        reg = <0x38>;
+
+        interrupt-parent = <&gpio1>;
+        interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
+                     <24 IRQ_TYPE_LEVEL_LOW>;
+        interrupt-names = "inti", "intk";
+
+        keypad,num-rows = <8>;
+        keypad,num-columns = <4>;
+        linux,keymap = <
+          MATRIX_KEY(0x00, 0x00, KEY_F5)
+          MATRIX_KEY(0x01, 0x00, KEY_F4)
+          MATRIX_KEY(0x02, 0x01, KEY_F6)
+          >;
+        keypad-debounce-delay-ms = <10>;
+        autorepeat;
+
+        rotary-debounce-delay-ms = <2>;
+        linux,axis = <0>; /* REL_X */
+
+        #pwm-cells = <3>;
+
+        max7360_gpio: gpio {
+          compatible = "maxim,max7360-gpio";
+
+          gpio-controller;
+          #gpio-cells = <2>;
+          ngpios = <8>;
+          maxim,constant-current-disable = <0x06>;
+
+          interrupt-controller;
+          #interrupt-cells = <0x2>;
+        };
+
+        max7360_gpo: gpo {
+          compatible = "maxim,max7360-gpo";
+
+          gpio-controller;
+          #gpio-cells = <2>;
+          ngpios = <4>;
+        };
+      };
+    };

-- 
2.39.5

Re: [PATCH v3 1/7] dt-bindings: mfd: gpio: Add MAX7360

[Krzysztof Kozlowski]

On Mon, Jan 13, 2025 at 01:42:25PM +0100, Mathieu Dubois-Briand wrote:
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  maxim,constant-current-disable:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: >

Drop >

> +      Bit field, each bit disables constant-current output of the associated
> +      GPIO, starting from the least significant bit for the first GPIO.

maximum: 0xff?

> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - ngpios
> +

allOf: here, so you won't re-indent it later.

> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - maxim,max7360-gpio
> +then:
> +  required:
> +    - interrupt-controller
> +else:
> +  properties:
> +    interrupt-controller: false
> +    maxim,constant-current-disable: false
> +
> +    ngpios:
> +      maximum: 6
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio {
> +      compatible = "maxim,max7360-gpio";
> +
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      ngpios = <8>;
> +      maxim,constant-current-disable = <0x06>;
> +
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +    };

...

> +  interrupt-names:
> +    items:
> +      - const: inti
> +      - const: intk
> +
> +  keypad-debounce-delay-ms:
> +    description: Keypad debounce delay in ms
> +    minimum: 9
> +    maximum: 40
> +    default: 9
> +
> +  autorepeat: true

Drop, not needed.

> +
> +  rotary-debounce-delay-ms:
> +    description: Rotary encoder debounce delay in ms
> +    minimum: 0
> +    maximum: 15
> +    default: 0
> +
> +  linux,axis:
> +    description: The input subsystem axis to map to this rotary encoder.

Missing type. I guess you wanted to reference rotary encoder schema,
next to input and matrix-keymap?

> +
> +  "#pwm-cells":
> +    const: 3
> +
> +  gpio:
> +    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
> +    description: >

Drop >

> +      PORT0 to PORT7 general purpose input/output pins configuration.
> +
> +  gpo:
> +    $ref: /schemas/gpio/maxim,max7360-gpio.yaml#
> +    description: >

Drop >

> +      COL2 to COL7 general purpose output pins configuration.
> +      Allows to use unused keypad columns as outputs.
> +      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. Value
> +      of ngpios must be coherent with the value of keypad,num-columns, as their
> +      sum must not exceed the number of physical lines.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - linux,keymap
> +  - linux,axis
> +  - "#pwm-cells"

gpio and gpo nodes are optional? How would the driver behave? I assume
you need to define the partition between GPIOs, especially that 'ngpios'
are a required property in their schema.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/input/input.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      io-expander@38 {
> +        compatible = "maxim,max7360";
> +        reg = <0x38>;
> +
> +        interrupt-parent = <&gpio1>;
> +        interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
> +                     <24 IRQ_TYPE_LEVEL_LOW>;
> +        interrupt-names = "inti", "intk";
> +
> +        keypad,num-rows = <8>;
> +        keypad,num-columns = <4>;
> +        linux,keymap = <
> +          MATRIX_KEY(0x00, 0x00, KEY_F5)
> +          MATRIX_KEY(0x01, 0x00, KEY_F4)
> +          MATRIX_KEY(0x02, 0x01, KEY_F6)
> +          >;
> +        keypad-debounce-delay-ms = <10>;
> +        autorepeat;
> +
> +        rotary-debounce-delay-ms = <2>;
> +        linux,axis = <0>; /* REL_X */
> +
> +        #pwm-cells = <3>;
> +
> +        max7360_gpio: gpio {
> +          compatible = "maxim,max7360-gpio";
> +
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          ngpios = <8>;
> +          maxim,constant-current-disable = <0x06>;
> +
> +          interrupt-controller;
> +          #interrupt-cells = <0x2>;
> +        };
> +
> +        max7360_gpo: gpo {
> +          compatible = "maxim,max7360-gpo";
> +
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          ngpios = <4>;
> +        };
> +      };
> +    };
> 
> -- 
> 2.39.5
> 

Re: [PATCH v3 1/7] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

On Tue Jan 14, 2025 at 9:11 AM CET, Krzysztof Kozlowski wrote:
> On Mon, Jan 13, 2025 at 01:42:25PM +0100, Mathieu Dubois-Briand wrote:
> > +
> > +  rotary-debounce-delay-ms:
> > +    description: Rotary encoder debounce delay in ms
> > +    minimum: 0
> > +    maximum: 15
> > +    default: 0
> > +
> > +  linux,axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
>
> Missing type. I guess you wanted to reference rotary encoder schema,
> next to input and matrix-keymap?
>

I'm not sure I fully understood your suggestion. Do you mean adding a
reference to rotary-encoder.yaml, at the root of the document? Like:

allOf:
  - $ref: /schemas/input/matrix-keymap.yaml#
  - $ref: /schemas/input/input.yaml#
  - $ref: /schemas/input/rotary-encoder.yaml#

I did base the schema of the rotary encoder part on rotary-encoder.yaml,
but I believe we cannot reference it directly: it adds some properties
that do not make sense here (gpios, rotary-encoder,steps...) and also
some of them are mandatory.

Yet I see that I'm not referring to any type here. Also I did not
specify the default value. Would the following be OK?

  linux,axis:
    description: The input subsystem axis to map to the rotary encoder.
    $ref: /schemas/types.yaml#/definitions/uint32
    default: 0

> > +      COL2 to COL7 general purpose output pins configuration.
> > +      Allows to use unused keypad columns as outputs.
> > +      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. Value
> > +      of ngpios must be coherent with the value of keypad,num-columns, as their
> > +      sum must not exceed the number of physical lines.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - linux,keymap
> > +  - linux,axis
> > +  - "#pwm-cells"
>
> gpio and gpo nodes are optional? How would the driver behave? I assume
> you need to define the partition between GPIOs, especially that 'ngpios'
> are a required property in their schema.
>

No, you are right. In my mind it was optional, but current driver
implementation will complain if the gpo node is missing. I could make it
optional in the code, but it's probably better to make it required in
the device tree, so the hardware is correctly described.

> > 
> > -- 
> > 2.39.5
> > 

I have fixed the other points listed in your mail. Thanks for your review.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 2/7] mfd: Add max7360 support

[mathieu.dubois-briand]

From: Kamel Bouhara <kamel.bouhara@bootlin.com>

Add core driver to support MAX7360 i2c chip, multi function device
with keypad, gpio, pwm, gpo and rotary encoder submodules.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/mfd/Kconfig         |  12 ++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/max7360.c       | 296 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max7360.h | 109 ++++++++++++++++
 4 files changed, 418 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ae23b317a64e..c1ddda480922 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2414,5 +2414,17 @@ config MFD_RSMU_SPI
 	  Additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_MAX7360
+	tristate "Maxim MAX7360 Support"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  Say yes here to add support for Maxim MAX7360.
+	  This driver provides common support for accessing
+	  the device; additional drivers must be enabled in
+	  order to use the functionality of the device.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e057d6d6faef..6cd55504106d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_DA9063)	+= da9063.o
 obj-$(CONFIG_MFD_DA9150)	+= da9150-core.o
 
 obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
+obj-$(CONFIG_MFD_MAX7360)	+= max7360.o
 obj-$(CONFIG_MFD_MAX77541)	+= max77541.o
 obj-$(CONFIG_MFD_MAX77620)	+= max77620.o
 obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
diff --git a/drivers/mfd/max7360.c b/drivers/mfd/max7360.c
new file mode 100644
index 000000000000..e2751b4f68b2
--- /dev/null
+++ b/drivers/mfd/max7360.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX7360 Core Driver
+ *
+ * Copyright (C) 2024 Kamel Bouhara
+ * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+static DEFINE_SPINLOCK(request_lock);
+
+struct max7360_mfd {
+	struct regmap *regmap;
+	unsigned int requested_ports;
+	struct device *dev;
+};
+
+#define GPO_COMPATIBLE "maxim,max7360-gpo"
+#define GPIO_COMPATIBLE "maxim,max7360-gpio"
+
+static const struct mfd_cell max7360_cells[] = {
+	{
+		.name           = MAX7360_DRVNAME_PWM,
+	},
+	{
+		.name           = MAX7360_DRVNAME_GPO,
+		.of_compatible	= GPO_COMPATIBLE,
+	},
+	{
+		.name           = MAX7360_DRVNAME_GPIO,
+		.of_compatible	= GPIO_COMPATIBLE,
+	},
+	{
+		.name           = MAX7360_DRVNAME_KEYPAD,
+	},
+	{
+		.name           = MAX7360_DRVNAME_ROTARY,
+	},
+};
+
+static const struct regmap_range max7360_volatile_ranges[] = {
+	{
+		.range_min = MAX7360_REG_KEYFIFO,
+		.range_max = MAX7360_REG_KEYFIFO,
+	}, {
+		.range_min = 0x48,
+		.range_max = 0x4a,
+	},
+};
+
+static const struct regmap_access_table max7360_volatile_table = {
+	.yes_ranges = max7360_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(max7360_volatile_ranges),
+};
+
+static const struct regmap_config max7360_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.volatile_table = &max7360_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int max7360_set_gpos_count(struct max7360_mfd *max7360_mfd)
+{
+	/*
+	 * Max7360 COL0 to COL7 pins can be used either as keypad columns,
+	 * general purpose output or a mix of both.
+	 * Get the number of pins requested by the corresponding drivers, ensure
+	 * they are compatible with each others and apply the corresponding
+	 * configuration.
+	 */
+	struct device_node *np;
+	u32 gpos = 0;
+	u32 columns = 0;
+	unsigned int val;
+	int ret;
+
+	np = of_get_compatible_child(max7360_mfd->dev->of_node, GPO_COMPATIBLE);
+	if (np) {
+		ret = of_property_read_u32(np, "ngpios", &gpos);
+		if (ret < 0) {
+			dev_err(max7360_mfd->dev, "Failed to read gpos count\n");
+			return ret;
+		}
+	}
+
+	ret = device_property_read_u32(max7360_mfd->dev,
+				       "keypad,num-columns", &columns);
+	if (ret < 0) {
+		dev_err(max7360_mfd->dev, "Failed to read columns count\n");
+		return ret;
+	}
+
+	if (gpos > MAX7360_MAX_GPO ||
+	    (gpos + columns > MAX7360_MAX_KEY_COLS)) {
+		dev_err(max7360_mfd->dev,
+			"Incompatible gpos and columns count (%u, %u)\n",
+			gpos, columns);
+		return -EINVAL;
+	}
+
+	/*
+	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
+	 * timings and gpos/keypad columns repartition. Only the later is
+	 * modified here.
+	 */
+	val = FIELD_PREP(MAX7360_PORTS, gpos);
+	ret = regmap_write_bits(max7360_mfd->regmap, MAX7360_REG_DEBOUNCE,
+				MAX7360_PORTS, val);
+	if (ret) {
+		dev_err(max7360_mfd->dev,
+			"Failed to write max7360 columns/gpos configuration");
+		return ret;
+	}
+
+	return 0;
+}
+
+int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)
+{
+	struct i2c_client *client;
+	struct max7360_mfd *max7360_mfd;
+	unsigned long flags;
+	int ret = 0;
+
+	client = to_i2c_client(dev);
+	max7360_mfd = i2c_get_clientdata(client);
+
+	spin_lock_irqsave(&request_lock, flags);
+	if (request) {
+		if (max7360_mfd->requested_ports & BIT(pin))
+			ret = -EBUSY;
+		else
+			max7360_mfd->requested_ports |= BIT(pin);
+	} else {
+		max7360_mfd->requested_ports &= ~BIT(pin);
+	}
+	spin_unlock_irqrestore(&request_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(max7360_port_pin_request);
+
+static int max7360_mask_irqs(struct max7360_mfd *max7360_mfd)
+{
+	unsigned int i;
+	unsigned int val;
+	int ret;
+
+	/*
+	 * GPIO/PWM interrupts are not masked on reset: mask the during probe,
+	 * avoiding repeated spurious interrupts if the corresponding drivers
+	 * are not present.
+	 */
+	for (i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
+		ret = regmap_write_bits(max7360_mfd->regmap,
+					MAX7360_REG_PWMCFG + i,
+					MAX7360_PORT_CFG_INTERRUPT_MASK,
+					MAX7360_PORT_CFG_INTERRUPT_MASK);
+		if (ret) {
+			dev_err(max7360_mfd->dev,
+				"failed to write max7360 port configuration");
+			return ret;
+		}
+	}
+
+	/* Read gpio in register, to ack any pending IRQ.
+	 */
+	ret = regmap_read(max7360_mfd->regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret) {
+		dev_err(max7360_mfd->dev, "Failed to read gpio values: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_reset(struct max7360_mfd *max7360_mfd)
+{
+	int err;
+
+	/*
+	 * Set back the default values.
+	 * We do not use GPIO reset function here, as it does not work reliably.
+	 */
+	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIODEB, 0x00);
+	if (err) {
+		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
+		return err;
+	}
+
+	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOCURR, MAX7360_REG_GPIOCURR_FIXED);
+	if (err) {
+		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
+		return err;
+	}
+
+	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOOUTM, 0x00);
+	if (err) {
+		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
+		return err;
+	}
+
+	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_PWMCOM, 0x00);
+	if (err) {
+		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
+		return err;
+	}
+
+	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_SLEEP, 0);
+	if (err) {
+		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int max7360_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	struct max7360_mfd *max7360_mfd;
+	int err;
+
+	regmap = devm_regmap_init_i2c(client, &max7360_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialise regmap\n");
+
+	max7360_mfd = devm_kzalloc(dev, sizeof(*max7360_mfd), GFP_KERNEL);
+	if (!max7360_mfd)
+		return -ENOMEM;
+
+	max7360_mfd->regmap = regmap;
+	max7360_mfd->dev = dev;
+	i2c_set_clientdata(client, max7360_mfd);
+
+	err = max7360_reset(max7360_mfd);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to reset device\n");
+
+	err = max7360_set_gpos_count(max7360_mfd);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to set GPOS pin count\n");
+
+	/*
+	 * Get the device out of shutdown mode.
+	 */
+	err = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_GPIO_EN,
+				MAX7360_GPIO_CFG_GPIO_EN);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to set device out of shutdown\n");
+
+	err = max7360_mask_irqs(max7360_mfd);
+	if (err)
+		return dev_err_probe(dev, err, "could not mask interrupts\n");
+
+	err =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    max7360_cells, ARRAY_SIZE(max7360_cells),
+				    NULL, 0, NULL);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register child devices\n");
+
+	return 0;
+}
+
+static const struct of_device_id max7360_dt_match[] = {
+	{ .compatible = "maxim,max7360" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max7360_dt_match);
+
+static struct i2c_driver max7360_driver = {
+	.driver = {
+		.name = "max7360",
+		.of_match_table = max7360_dt_match,
+	},
+	.probe = max7360_probe,
+};
+module_i2c_driver(max7360_driver);
+
+MODULE_DESCRIPTION("Maxim MAX7360 MFD core driver");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max7360.h b/include/linux/mfd/max7360.h
new file mode 100644
index 000000000000..2665a8e6b0f0
--- /dev/null
+++ b/include/linux/mfd/max7360.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_MFD_MAX7360_H
+#define __LINUX_MFD_MAX7360_H
+#include <linux/bitfield.h>
+#include <linux/device.h>
+
+#define MAX7360_MAX_KEY_ROWS	8
+#define MAX7360_MAX_KEY_COLS	8
+#define MAX7360_MAX_KEY_NUM	(MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS)
+#define MAX7360_ROW_SHIFT	3
+
+#define MAX7360_MAX_GPIO 8
+#define MAX7360_MAX_GPO 6
+#define MAX7360_PORT_PWM_COUNT	8
+#define MAX7360_PORT_RTR_PIN	(MAX7360_PORT_PWM_COUNT - 1)
+/*
+ * MAX7360 registers
+ */
+#define MAX7360_REG_KEYFIFO	0x00
+#define MAX7360_REG_CONFIG	0x01
+#define MAX7360_REG_DEBOUNCE	0x02
+#define MAX7360_REG_INTERRUPT	0x03
+#define MAX7360_REG_PORTS	0x04
+#define MAX7360_REG_KEYREP	0x05
+#define MAX7360_REG_SLEEP	0x06
+
+/*
+ * MAX7360 registers
+ */
+#define MAX7360_REG_GPIOCFG	0x40
+#define MAX7360_REG_GPIOCTRL	0x41
+#define MAX7360_REG_GPIODEB	0x42
+#define MAX7360_REG_GPIOCURR	0x43
+#define MAX7360_REG_GPIOOUTM	0x44
+#define MAX7360_REG_PWMCOM	0x45
+#define MAX7360_REG_RTRCFG	0x46
+#define MAX7360_REG_GPIOIN	0x49
+#define MAX7360_REG_RTR_CNT	0x4A
+#define MAX7360_REG_PWMBASE	0x50
+#define MAX7360_REG_PWMCFG	0x58
+
+#define MAX7360_REG_PORTCFGBASE 0x58
+
+/*
+ * Configuration register bits
+ */
+#define MAX7360_FIFO_EMPTY	0x3f
+#define MAX7360_FIFO_OVERFLOW	0x7f
+#define MAX7360_FIFO_RELEASE	BIT(6)
+#define MAX7360_FIFO_COL	GENMASK(5, 3)
+#define MAX7360_FIFO_ROW	GENMASK(2, 0)
+
+#define MAX7360_CFG_SLEEP	BIT(7)
+#define MAX7360_CFG_INTERRUPT	BIT(5)
+#define MAX7360_CFG_KEY_RELEASE	BIT(3)
+#define MAX7360_CFG_WAKEUP	BIT(1)
+#define MAX7360_CFG_TIMEOUT	BIT(0)
+
+#define MAX7360_DEBOUNCE	GENMASK(4, 0)
+#define MAX7360_DEBOUNCE_MIN	9
+#define MAX7360_DEBOUNCE_MAX	40
+#define MAX7360_PORTS		GENMASK(8, 5)
+
+#define MAX7360_INTERRUPT_TIME_MASK GENMASK(4, 0)
+#define MAX7360_INTERRUPT_FIFO_MASK GENMASK(7, 5)
+
+#define MAX7360_PORT_CFG_INTERRUPT_MASK BIT(7)
+#define MAX7360_PORT_CFG_INTERRUPT_EDGES BIT(6)
+
+#define MAX7360_REG_GPIOCURR_FIXED 0xC0
+
+/*
+ * Autosleep register values (ms)
+ */
+#define MAX7360_AUTOSLEEP_8192	0x01
+#define MAX7360_AUTOSLEEP_4096	0x02
+#define MAX7360_AUTOSLEEP_2048	0x03
+#define MAX7360_AUTOSLEEP_1024	0x04
+#define MAX7360_AUTOSLEEP_512	0x05
+#define MAX7360_AUTOSLEEP_256	0x06
+
+#define MAX7360_GPIO_CFG_RTR_EN		BIT(7)
+#define MAX7360_GPIO_CFG_GPIO_EN	BIT(4)
+#define MAX7360_GPIO_CFG_GPIO_RST	BIT(3)
+
+#define MAX7360_ROT_DEBOUNCE	GENMASK(3, 0)
+#define MAX7360_ROT_DEBOUNCE_MIN 0
+#define MAX7360_ROT_DEBOUNCE_MAX 15
+#define MAX7360_ROT_INTCNT	GENMASK(6, 4)
+#define MAX7360_ROT_INTCNT_DLY	BIT(7)
+
+#define MAX7360_INT_INTI	0
+#define MAX7360_INT_INTK	1
+
+#define MAX7360_INT_GPIO   0
+#define MAX7360_INT_KEYPAD 1
+#define MAX7360_INT_ROTARY 2
+
+#define MAX7360_NR_INTERNAL_IRQS	3
+
+#define MAX7360_DRVNAME_PWM	"max7360-pwm"
+#define MAX7360_DRVNAME_GPO	"max7360-gpo"
+#define MAX7360_DRVNAME_GPIO	"max7360-gpio"
+#define MAX7360_DRVNAME_KEYPAD	"max7360-keypad"
+#define MAX7360_DRVNAME_ROTARY	"max7360-rotary"
+
+int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request);
+
+#endif

-- 
2.39.5

Re: [PATCH v3 2/7] mfd: Add max7360 support

[Lee Jones]

On Mon, 13 Jan 2025, mathieu.dubois-briand@bootlin.com wrote:

> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> Add core driver to support MAX7360 i2c chip, multi function device
> with keypad, gpio, pwm, gpo and rotary encoder submodules.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/mfd/Kconfig         |  12 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/max7360.c       | 296 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max7360.h | 109 ++++++++++++++++
>  4 files changed, 418 insertions(+)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e..c1ddda480922 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2414,5 +2414,17 @@ config MFD_RSMU_SPI
>  	  Additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_MAX7360
> +	tristate "Maxim MAX7360 Support"

What is it?

> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  Say yes here to add support for Maxim MAX7360.

60 chars is an odd place to break.

> +	  This driver provides common support for accessing
> +	  the device; additional drivers must be enabled in
> +	  order to use the functionality of the device.
> +
>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef..6cd55504106d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_DA9063)	+= da9063.o
>  obj-$(CONFIG_MFD_DA9150)	+= da9150-core.o
>  
>  obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
> +obj-$(CONFIG_MFD_MAX7360)	+= max7360.o
>  obj-$(CONFIG_MFD_MAX77541)	+= max77541.o
>  obj-$(CONFIG_MFD_MAX77620)	+= max77620.o
>  obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
> diff --git a/drivers/mfd/max7360.c b/drivers/mfd/max7360.c
> new file mode 100644
> index 000000000000..e2751b4f68b2
> --- /dev/null
> +++ b/drivers/mfd/max7360.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Maxim MAX7360 Core Driver
> + *
> + * Copyright (C) 2024 Kamel Bouhara
> + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +static DEFINE_SPINLOCK(request_lock);
> +
> +struct max7360_mfd {

Drop references to "mfd".

> +	struct regmap *regmap;
> +	unsigned int requested_ports;
> +	struct device *dev;

Pop dev at the top.

> +};
> +
> +#define GPO_COMPATIBLE "maxim,max7360-gpo"
> +#define GPIO_COMPATIBLE "maxim,max7360-gpio"

Please use the raw strings instead.

> +static const struct mfd_cell max7360_cells[] = {
> +	{
> +		.name           = MAX7360_DRVNAME_PWM,

Raw strings please.

> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_GPO,
> +		.of_compatible	= GPO_COMPATIBLE,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_GPIO,
> +		.of_compatible	= GPIO_COMPATIBLE,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_KEYPAD,
> +	},
> +	{
> +		.name           = MAX7360_DRVNAME_ROTARY,
> +	},
> +};
> +
> +static const struct regmap_range max7360_volatile_ranges[] = {
> +	{
> +		.range_min = MAX7360_REG_KEYFIFO,
> +		.range_max = MAX7360_REG_KEYFIFO,
> +	}, {
> +		.range_min = 0x48,
> +		.range_max = 0x4a,
> +	},
> +};
> +
> +static const struct regmap_access_table max7360_volatile_table = {
> +	.yes_ranges = max7360_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(max7360_volatile_ranges),
> +};
> +
> +static const struct regmap_config max7360_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +	.volatile_table = &max7360_volatile_table,
> +	.cache_type = REGCACHE_RBTREE,

A lot of these are being replaced with REGCACHE_MAPLE.

Are you sure you shouldn't be using that too?

> +};
> +
> +static int max7360_set_gpos_count(struct max7360_mfd *max7360_mfd)

Pass the dev pointer around instead.

> +{
> +	/*
> +	 * Max7360 COL0 to COL7 pins can be used either as keypad columns,

MAX?

> +	 * general purpose output or a mix of both.
> +	 * Get the number of pins requested by the corresponding drivers, ensure
> +	 * they are compatible with each others and apply the corresponding
> +	 * configuration.
> +	 */

What are you documenting here?

Comments should go with their code snippets.

> +	struct device_node *np;

np usually implies our own node.

> +	u32 gpos = 0;
> +	u32 columns = 0;
> +	unsigned int val;
> +	int ret;
> +
> +	np = of_get_compatible_child(max7360_mfd->dev->of_node, GPO_COMPATIBLE);

Why don't you do all of this in the GPO driver?

> +	if (np) {
> +		ret = of_property_read_u32(np, "ngpios", &gpos);
> +		if (ret < 0) {
> +			dev_err(max7360_mfd->dev, "Failed to read gpos count\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = device_property_read_u32(max7360_mfd->dev,
> +				       "keypad,num-columns", &columns);
> +	if (ret < 0) {
> +		dev_err(max7360_mfd->dev, "Failed to read columns count\n");
> +		return ret;
> +	}
> +
> +	if (gpos > MAX7360_MAX_GPO ||
> +	    (gpos + columns > MAX7360_MAX_KEY_COLS)) {
> +		dev_err(max7360_mfd->dev,
> +			"Incompatible gpos and columns count (%u, %u)\n",
> +			gpos, columns);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> +	 * timings and gpos/keypad columns repartition. Only the later is
> +	 * modified here.
> +	 */
> +	val = FIELD_PREP(MAX7360_PORTS, gpos);
> +	ret = regmap_write_bits(max7360_mfd->regmap, MAX7360_REG_DEBOUNCE,
> +				MAX7360_PORTS, val);
> +	if (ret) {
> +		dev_err(max7360_mfd->dev,
> +			"Failed to write max7360 columns/gpos configuration");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)

This whole function is rough.  What are you trying to achieve?

> +{
> +	struct i2c_client *client;
> +	struct max7360_mfd *max7360_mfd;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	client = to_i2c_client(dev);
> +	max7360_mfd = i2c_get_clientdata(client);
> +
> +	spin_lock_irqsave(&request_lock, flags);
> +	if (request) {
> +		if (max7360_mfd->requested_ports & BIT(pin))
> +			ret = -EBUSY;
> +		else
> +			max7360_mfd->requested_ports |= BIT(pin);
> +	} else {
> +		max7360_mfd->requested_ports &= ~BIT(pin);
> +	}
> +	spin_unlock_irqrestore(&request_lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(max7360_port_pin_request);
> +
> +static int max7360_mask_irqs(struct max7360_mfd *max7360_mfd)
> +{
> +	unsigned int i;

Use `for (int i;` instead.

> +	unsigned int val;
> +	int ret;
> +
> +	/*
> +	 * GPIO/PWM interrupts are not masked on reset: mask the during probe,
> +	 * avoiding repeated spurious interrupts if the corresponding drivers
> +	 * are not present.
> +	 */
> +	for (i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
> +		ret = regmap_write_bits(max7360_mfd->regmap,
> +					MAX7360_REG_PWMCFG + i,
> +					MAX7360_PORT_CFG_INTERRUPT_MASK,
> +					MAX7360_PORT_CFG_INTERRUPT_MASK);
> +		if (ret) {
> +			dev_err(max7360_mfd->dev,
> +				"failed to write max7360 port configuration");

You can use 100-chars to avoid the line break.

> +			return ret;
> +		}
> +	}
> +
> +	/* Read gpio in register, to ack any pending IRQ.

GPIO, ACK

> +	 */

Wrong format.

> +	ret = regmap_read(max7360_mfd->regmap, MAX7360_REG_GPIOIN, &val);
> +	if (ret) {
> +		dev_err(max7360_mfd->dev, "Failed to read gpio values: %d\n",

GPIO

> +			ret);

No wrap needed.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_reset(struct max7360_mfd *max7360_mfd)
> +{
> +	int err;
> +
> +	/*
> +	 * Set back the default values.
> +	 * We do not use GPIO reset function here, as it does not work reliably.

Why?  What's wrong with it?

> +	 */
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIODEB, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");

Use a goto to only print this out once.

> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOCURR, MAX7360_REG_GPIOCURR_FIXED);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIOOUTM, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_PWMCOM, 0x00);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_SLEEP, 0);
> +	if (err) {
> +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +	struct max7360_mfd *max7360_mfd;
> +	int err;
> +
> +	regmap = devm_regmap_init_i2c(client, &max7360_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to initialise regmap\n");
> +
> +	max7360_mfd = devm_kzalloc(dev, sizeof(*max7360_mfd), GFP_KERNEL);
> +	if (!max7360_mfd)
> +		return -ENOMEM;
> +
> +	max7360_mfd->regmap = regmap;
> +	max7360_mfd->dev = dev;
> +	i2c_set_clientdata(client, max7360_mfd);
> +
> +	err = max7360_reset(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to reset device\n");
> +
> +	err = max7360_set_gpos_count(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to set GPOS pin count\n");
> +
> +	/*
> +	 * Get the device out of shutdown mode.
> +	 */

Single line comment please.

> +	err = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG,
> +				MAX7360_GPIO_CFG_GPIO_EN,
> +				MAX7360_GPIO_CFG_GPIO_EN);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to set device out of shutdown\n");

This doesn't read well.

> +	err = max7360_mask_irqs(max7360_mfd);
> +	if (err)
> +		return dev_err_probe(dev, err, "could not mask interrupts\n");

Some messages start with an uppercase char, others do not.

I suggest you use one for consistency.

> +	err =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				    max7360_cells, ARRAY_SIZE(max7360_cells),
> +				    NULL, 0, NULL);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to register child devices\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max7360_dt_match[] = {
> +	{ .compatible = "maxim,max7360" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max7360_dt_match);
> +
> +static struct i2c_driver max7360_driver = {
> +	.driver = {
> +		.name = "max7360",
> +		.of_match_table = max7360_dt_match,
> +	},
> +	.probe = max7360_probe,
> +};
> +module_i2c_driver(max7360_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX7360 MFD core driver");

There is no such thing as an MFD driver.  What does it do?

> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max7360.h b/include/linux/mfd/max7360.h
> new file mode 100644
> index 000000000000..2665a8e6b0f0
> --- /dev/null
> +++ b/include/linux/mfd/max7360.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

'\n'

> +#ifndef __LINUX_MFD_MAX7360_H
> +#define __LINUX_MFD_MAX7360_H

'\n'

> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +
> +#define MAX7360_MAX_KEY_ROWS	8
> +#define MAX7360_MAX_KEY_COLS	8
> +#define MAX7360_MAX_KEY_NUM	(MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS)
> +#define MAX7360_ROW_SHIFT	3
> +
> +#define MAX7360_MAX_GPIO 8
> +#define MAX7360_MAX_GPO 6
> +#define MAX7360_PORT_PWM_COUNT	8
> +#define MAX7360_PORT_RTR_PIN	(MAX7360_PORT_PWM_COUNT - 1)

'\n'

> +/*
> + * MAX7360 registers
> + */
> +#define MAX7360_REG_KEYFIFO	0x00
> +#define MAX7360_REG_CONFIG	0x01
> +#define MAX7360_REG_DEBOUNCE	0x02
> +#define MAX7360_REG_INTERRUPT	0x03
> +#define MAX7360_REG_PORTS	0x04
> +#define MAX7360_REG_KEYREP	0x05
> +#define MAX7360_REG_SLEEP	0x06
> +
> +/*
> + * MAX7360 registers
> + */

How is this different to the ones above?

> +#define MAX7360_REG_GPIOCFG	0x40
> +#define MAX7360_REG_GPIOCTRL	0x41
> +#define MAX7360_REG_GPIODEB	0x42
> +#define MAX7360_REG_GPIOCURR	0x43
> +#define MAX7360_REG_GPIOOUTM	0x44
> +#define MAX7360_REG_PWMCOM	0x45
> +#define MAX7360_REG_RTRCFG	0x46
> +#define MAX7360_REG_GPIOIN	0x49
> +#define MAX7360_REG_RTR_CNT	0x4A
> +#define MAX7360_REG_PWMBASE	0x50
> +#define MAX7360_REG_PWMCFG	0x58
> +
> +#define MAX7360_REG_PORTCFGBASE 0x58
> +
> +/*
> + * Configuration register bits
> + */
> +#define MAX7360_FIFO_EMPTY	0x3f
> +#define MAX7360_FIFO_OVERFLOW	0x7f
> +#define MAX7360_FIFO_RELEASE	BIT(6)
> +#define MAX7360_FIFO_COL	GENMASK(5, 3)
> +#define MAX7360_FIFO_ROW	GENMASK(2, 0)
> +
> +#define MAX7360_CFG_SLEEP	BIT(7)
> +#define MAX7360_CFG_INTERRUPT	BIT(5)
> +#define MAX7360_CFG_KEY_RELEASE	BIT(3)
> +#define MAX7360_CFG_WAKEUP	BIT(1)
> +#define MAX7360_CFG_TIMEOUT	BIT(0)
> +
> +#define MAX7360_DEBOUNCE	GENMASK(4, 0)
> +#define MAX7360_DEBOUNCE_MIN	9
> +#define MAX7360_DEBOUNCE_MAX	40
> +#define MAX7360_PORTS		GENMASK(8, 5)
> +
> +#define MAX7360_INTERRUPT_TIME_MASK GENMASK(4, 0)
> +#define MAX7360_INTERRUPT_FIFO_MASK GENMASK(7, 5)
> +
> +#define MAX7360_PORT_CFG_INTERRUPT_MASK BIT(7)
> +#define MAX7360_PORT_CFG_INTERRUPT_EDGES BIT(6)
> +
> +#define MAX7360_REG_GPIOCURR_FIXED 0xC0
> +
> +/*
> + * Autosleep register values (ms)
> + */
> +#define MAX7360_AUTOSLEEP_8192	0x01
> +#define MAX7360_AUTOSLEEP_4096	0x02
> +#define MAX7360_AUTOSLEEP_2048	0x03
> +#define MAX7360_AUTOSLEEP_1024	0x04
> +#define MAX7360_AUTOSLEEP_512	0x05
> +#define MAX7360_AUTOSLEEP_256	0x06
> +
> +#define MAX7360_GPIO_CFG_RTR_EN		BIT(7)
> +#define MAX7360_GPIO_CFG_GPIO_EN	BIT(4)
> +#define MAX7360_GPIO_CFG_GPIO_RST	BIT(3)
> +
> +#define MAX7360_ROT_DEBOUNCE	GENMASK(3, 0)
> +#define MAX7360_ROT_DEBOUNCE_MIN 0
> +#define MAX7360_ROT_DEBOUNCE_MAX 15
> +#define MAX7360_ROT_INTCNT	GENMASK(6, 4)
> +#define MAX7360_ROT_INTCNT_DLY	BIT(7)
> +
> +#define MAX7360_INT_INTI	0
> +#define MAX7360_INT_INTK	1
> +
> +#define MAX7360_INT_GPIO   0
> +#define MAX7360_INT_KEYPAD 1
> +#define MAX7360_INT_ROTARY 2
> +
> +#define MAX7360_NR_INTERNAL_IRQS	3
> +
> +#define MAX7360_DRVNAME_PWM	"max7360-pwm"
> +#define MAX7360_DRVNAME_GPO	"max7360-gpo"
> +#define MAX7360_DRVNAME_GPIO	"max7360-gpio"
> +#define MAX7360_DRVNAME_KEYPAD	"max7360-keypad"
> +#define MAX7360_DRVNAME_ROTARY	"max7360-rotary"

Nope.

> +
> +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request);
> +
> +#endif
> 
> -- 
> 2.39.5
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 2/7] mfd: Add max7360 support

[Mathieu Dubois-Briand]

On Wed Jan 15, 2025 at 4:42 PM CET, Lee Jones wrote:
> On Mon, 13 Jan 2025, mathieu.dubois-briand@bootlin.com wrote:
>
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > 
> > Add core driver to support MAX7360 i2c chip, multi function device
> > with keypad, gpio, pwm, gpo and rotary encoder submodules.
> > 
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > ---
...
> > +static int max7360_set_gpos_count(struct max7360_mfd *max7360_mfd)
> > +{
> > +	/*
> > +	 * Max7360 COL0 to COL7 pins can be used either as keypad columns,
> > +	 * general purpose output or a mix of both.
> > +	 * Get the number of pins requested by the corresponding drivers, ensure
> > +	 * they are compatible with each others and apply the corresponding
> > +	 * configuration.
> > +	 */
> > +	struct device_node *np;
> > +	u32 gpos = 0;
> > +	u32 columns = 0;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	np = of_get_compatible_child(max7360_mfd->dev->of_node, GPO_COMPATIBLE);
>
> Why don't you do all of this in the GPO driver?
>

I first did this here, so the configuration was still done if the GPO
driver was missing. But on a second thought, we can just set the GPO
count to 0 here, and let the GPO driver handle all of this.

I will move this function to the GPO driver.

> > +	if (np) {
> > +		ret = of_property_read_u32(np, "ngpios", &gpos);
> > +		if (ret < 0) {
> > +			dev_err(max7360_mfd->dev, "Failed to read gpos count\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	ret = device_property_read_u32(max7360_mfd->dev,
> > +				       "keypad,num-columns", &columns);
> > +	if (ret < 0) {
> > +		dev_err(max7360_mfd->dev, "Failed to read columns count\n");
> > +		return ret;
> > +	}
> > +
> > +	if (gpos > MAX7360_MAX_GPO ||
> > +	    (gpos + columns > MAX7360_MAX_KEY_COLS)) {
> > +		dev_err(max7360_mfd->dev,
> > +			"Incompatible gpos and columns count (%u, %u)\n",
> > +			gpos, columns);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > +	 * timings and gpos/keypad columns repartition. Only the later is
> > +	 * modified here.
> > +	 */
> > +	val = FIELD_PREP(MAX7360_PORTS, gpos);
> > +	ret = regmap_write_bits(max7360_mfd->regmap, MAX7360_REG_DEBOUNCE,
> > +				MAX7360_PORTS, val);
> > +	if (ret) {
> > +		dev_err(max7360_mfd->dev,
> > +			"Failed to write max7360 columns/gpos configuration");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)
>
> This whole function is rough.  What are you trying to achieve?
>

Some pins can be used either for PWM, rotary encoder or GPIO. The goal
here is to allow corresponding drivers to request the pin, making sure
only one driver use a given pin at some point.

> > +{
> > +	struct i2c_client *client;
> > +	struct max7360_mfd *max7360_mfd;
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	client = to_i2c_client(dev);
> > +	max7360_mfd = i2c_get_clientdata(client);
> > +
> > +	spin_lock_irqsave(&request_lock, flags);
> > +	if (request) {
> > +		if (max7360_mfd->requested_ports & BIT(pin))
> > +			ret = -EBUSY;
> > +		else
> > +			max7360_mfd->requested_ports |= BIT(pin);
> > +	} else {
> > +		max7360_mfd->requested_ports &= ~BIT(pin);
> > +	}
> > +	spin_unlock_irqrestore(&request_lock, flags);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(max7360_port_pin_request);
...
> > +static int max7360_reset(struct max7360_mfd *max7360_mfd)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Set back the default values.
> > +	 * We do not use GPIO reset function here, as it does not work reliably.
>
> Why?  What's wrong with it?
>

I was going to update this comment to add details, but after some extra
testing, this was wrong actually. Reset function of the chip is working
correctly, it was just a caching issue. I will rework the whole
function.

> > +	 */
> > +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIODEB, 0x00);
> > +	if (err) {
> > +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> > -- 
> > 2.39.5
> > 

Thanks for your review. I fixed all other points listed in your mail.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[mathieu.dubois-briand]

From: Kamel Bouhara <kamel.bouhara@bootlin.com>

Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
8 independent PWM outputs.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/pwm/Kconfig       |  11 +++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-max7360.c | 220 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 232 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..399dc3f76e92 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -745,4 +745,15 @@ config PWM_XILINX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-xilinx.
 
+config PWM_MAX7360
+	tristate "MAX7360 PWMs"
+	depends on MFD_MAX7360
+	depends on OF_GPIO
+	help
+	  PWM driver for Maxim Integrated MAX7360 multifunction device, with
+	  support for up to 8 PWM outputs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-max7360.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..ae8908ffc892 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MAX7360)	+= pwm-max7360.o
 obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
new file mode 100644
index 000000000000..e76a8aa05fc4
--- /dev/null
+++ b/drivers/pwm/pwm-max7360.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ *
+ * Limitations:
+ * - Only supports normal polarity.
+ * - The period is fixed to 2 ms.
+ * - Only the duty cycle can be changed, new values are applied at the beginning
+ *   of the next cycle.
+ * - When disabled, the output is put in Hi-Z.
+ */
+#include <linux/math.h>
+#include <linux/mfd/max7360.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define MAX7360_NUM_PWMS			8
+#define MAX7360_PWM_MAX_RES			256
+#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
+#define MAX7360_PWM_COMMON_PWN			BIT(5)
+#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
+#define MAX7360_PWM_PORT(n)			BIT(n)
+
+struct max7360_pwm {
+	struct device *parent;
+	struct regmap *regmap;
+};
+
+static inline struct max7360_pwm *to_max7360_pwm(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+	int ret;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
+				       true);
+	if (ret) {
+		dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_pwm->regmap,
+				MAX7360_REG_PWMCFG + pwm->hwpwm,
+				MAX7360_PWM_COMMON_PWN,
+				0);
+	if (ret) {
+		dev_warn(&chip->dev,
+			 "failed to write pwm-%d cfg register, error %d\n",
+			 pwm->hwpwm, ret);
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
+				MAX7360_PWM_PORT(pwm->hwpwm),
+				MAX7360_PWM_PORT(pwm->hwpwm));
+	if (ret) {
+		dev_warn(&chip->dev,
+			 "failed to write pwm-%d ports register, error %d\n",
+			 pwm->hwpwm, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+	int ret;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
+				0);
+	if (ret)
+		dev_warn(&chip->dev, "failed to disable pwm-%d , error %d\n",
+			 pwm->hwpwm, ret);
+
+	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
+				 false);
+}
+
+static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct max7360_pwm *max7360_pwm;
+	u64 duty_steps;
+	int ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period != MAX7360_PWM_PERIOD_NS) {
+		dev_warn(&chip->dev,
+			 "unsupported pwm period: %llu, should be %u\n",
+			 state->period, MAX7360_PWM_PERIOD_NS);
+		return -EINVAL;
+	}
+
+	duty_steps = mul_u64_u64_div_u64(state->duty_cycle, MAX7360_PWM_MAX_RES,
+					 MAX7360_PWM_PERIOD_NS);
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
+	if (ret) {
+		dev_warn(&chip->dev, "failed to enable pwm-%d , error %d\n",
+			 pwm->hwpwm, ret);
+		return ret;
+	}
+
+	ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
+			   duty_steps >= 255 ? 255 : duty_steps);
+	if (ret) {
+		dev_warn(&chip->dev,
+			 "failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
+			 duty_steps, pwm->hwpwm, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct max7360_pwm *max7360_pwm;
+	unsigned int val;
+	int ret;
+
+	max7360_pwm = to_max7360_pwm(chip);
+
+	state->period = MAX7360_PWM_PERIOD_NS;
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
+	if (ret) {
+		dev_warn(&chip->dev,
+			 "failed to read pwm configuration on pwm-%d, error %d\n",
+			 pwm->hwpwm, ret);
+		return ret;
+	}
+	state->enabled = !!(val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
+
+	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
+			  &val);
+	if (ret) {
+		dev_warn(&chip->dev,
+			 "failed to read pwm duty_cycle on pwm-%d, error %d\n",
+			 pwm->hwpwm, ret);
+		return ret;
+	}
+	state->duty_cycle = mul_u64_u64_div_u64(val, MAX7360_PWM_PERIOD_NS,
+						MAX7360_PWM_MAX_RES);
+
+	return 0;
+}
+
+static const struct pwm_ops max7360_pwm_ops = {
+	.request = max7360_pwm_request,
+	.free = max7360_pwm_free,
+	.apply = max7360_pwm_apply,
+	.get_state = max7360_pwm_get_state,
+};
+
+static int max7360_pwm_probe(struct platform_device *pdev)
+{
+	struct max7360_pwm *max7360_pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "no parent device\n");
+
+	chip = devm_pwmchip_alloc(pdev->dev.parent, MAX7360_NUM_PWMS,
+				  sizeof(*max7360_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	chip->ops = &max7360_pwm_ops;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	max7360_pwm->parent = pdev->dev.parent;
+
+	max7360_pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_pwm->regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "could not get parent regmap\n");
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret != 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver max7360_pwm_driver = {
+	.driver = {
+		.name = MAX7360_DRVNAME_PWM,
+	},
+	.probe = max7360_pwm_probe,
+};
+module_platform_driver(max7360_pwm_driver);
+
+MODULE_DESCRIPTION("MAX7360 PWM driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_ALIAS("platform:" MAX7360_DRVNAME_PWM);
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8325 bytes --]

Hello Mathieu,

On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
> 8 independent PWM outputs.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/pwm/Kconfig       |  11 +++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-max7360.c | 220 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 232 insertions(+)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..399dc3f76e92 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -745,4 +745,15 @@ config PWM_XILINX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-xilinx.
>  
> +config PWM_MAX7360
> +	tristate "MAX7360 PWMs"
> +	depends on MFD_MAX7360
> +	depends on OF_GPIO
> +	help
> +	  PWM driver for Maxim Integrated MAX7360 multifunction device, with
> +	  support for up to 8 PWM outputs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-max7360.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..ae8908ffc892 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
>  obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
>  obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MAX7360)	+= pwm-max7360.o
>  obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..e76a8aa05fc4
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *
> + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
> + *
> + * Limitations:
> + * - Only supports normal polarity.
> + * - The period is fixed to 2 ms.
> + * - Only the duty cycle can be changed, new values are applied at the beginning
> + *   of the next cycle.
> + * - When disabled, the output is put in Hi-Z.
> + */
> +#include <linux/math.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define MAX7360_NUM_PWMS			8
> +#define MAX7360_PWM_MAX_RES			256
> +#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
> +#define MAX7360_PWM_COMMON_PWN			BIT(5)
> +#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
> +#define MAX7360_PWM_PORT(n)			BIT(n)
> +
> +struct max7360_pwm {
> +	struct device *parent;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct max7360_pwm *to_max7360_pwm(struct pwm_chip *chip)

Please stick to the common function prefix also here. So something like
max7360_pwm_from_chip would work.

> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	int ret;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
> +				       true);

The statement fits on a single line just fine.

> +	if (ret) {
> +		dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_pwm->regmap,
> +				MAX7360_REG_PWMCFG + pwm->hwpwm,

Can you make MAX7360_REG_PWMCFG a macro accepting pwm->hwpwm as
parameter please?

> +				MAX7360_PWM_COMMON_PWN,
> +				0);
> +	if (ret) {
> +		dev_warn(&chip->dev,
> +			 "failed to write pwm-%d cfg register, error %d\n",
> +			 pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
> +				MAX7360_PWM_PORT(pwm->hwpwm),
> +				MAX7360_PWM_PORT(pwm->hwpwm));
> +	if (ret) {
> +		dev_warn(&chip->dev,
> +			 "failed to write pwm-%d ports register, error %d\n",
> +			 pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	int ret;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> +				0);
> +	if (ret)
> +		dev_warn(&chip->dev, "failed to disable pwm-%d , error %d\n",
> +			 pwm->hwpwm, ret);

.free is not supposed to stop the PWM. Though I admit this concept is a
bit fuzzy, because when unconfiguring the pin function this is kind of
moot. Still it's the responsibility of the consumer to stop the PWM
before pwm_put().

Also s/ ,/,/ and use %pe for error codes.

> +	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
> +				 false);
> +}
> +
> +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	u64 duty_steps;
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (state->period != MAX7360_PWM_PERIOD_NS) {
> +		dev_warn(&chip->dev,
> +			 "unsupported pwm period: %llu, should be %u\n",
> +			 state->period, MAX7360_PWM_PERIOD_NS);
> +		return -EINVAL;

Please don't emit error messages in .apply(). Also a driver is supposed
to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
accepted.

Also note that you might want to implement the waveform callbacks
instead of .apply() and .get_state() for the more modern abstraction
(with slightly different rounding rules).

> +	}
> +
> +	duty_steps = mul_u64_u64_div_u64(state->duty_cycle, MAX7360_PWM_MAX_RES,
> +					 MAX7360_PWM_PERIOD_NS);
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> +	if (ret) {
> +		dev_warn(&chip->dev, "failed to enable pwm-%d , error %d\n",
> +			 pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> +			   duty_steps >= 255 ? 255 : duty_steps);
> +	if (ret) {
> +		dev_warn(&chip->dev,
> +			 "failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
> +			 duty_steps, pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	unsigned int val;
> +	int ret;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +
> +	state->period = MAX7360_PWM_PERIOD_NS;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
> +	if (ret) {
> +		dev_warn(&chip->dev,
> +			 "failed to read pwm configuration on pwm-%d, error %d\n",
> +			 pwm->hwpwm, ret);

Similar to .apply() please no messages in .get_state(). Just fail
silently.

> +		return ret;
> +	}
> +	state->enabled = !!(val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> +
> +	ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> +			  &val);
> +	if (ret) {
> +		dev_warn(&chip->dev,
> +			 "failed to read pwm duty_cycle on pwm-%d, error %d\n",
> +			 pwm->hwpwm, ret);
> +		return ret;
> +	}
> +	state->duty_cycle = mul_u64_u64_div_u64(val, MAX7360_PWM_PERIOD_NS,
> +						MAX7360_PWM_MAX_RES);

You have to round up here. I would expect that the checks in the core
(with PWM_DEBUG=1) help you catching this type of error. In your case
changing the configuration to

	.period = 2000000,
	.duty_cycle = 234379,

should yield some hint in the kernel log.

> +	return 0;
> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Fri Jan 17, 2025 at 10:33 AM CET, Uwe Kleine-König wrote:
> Hello Mathieu,
>
> On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
...
> > +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			     const struct pwm_state *state)
> > +{
> > +	struct max7360_pwm *max7360_pwm;
> > +	u64 duty_steps;
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	if (state->period != MAX7360_PWM_PERIOD_NS) {
> > +		dev_warn(&chip->dev,
> > +			 "unsupported pwm period: %llu, should be %u\n",
> > +			 state->period, MAX7360_PWM_PERIOD_NS);
> > +		return -EINVAL;
>
> Please don't emit error messages in .apply(). Also a driver is supposed
> to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
> accepted.
>
> Also note that you might want to implement the waveform callbacks
> instead of .apply() and .get_state() for the more modern abstraction
> (with slightly different rounding rules).
>

Sure, I just switched to the waveform callbacks, it was quite
straightforward.

> > +static int max7360_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				 struct pwm_state *state)
> > +{
...
> > +	state->duty_cycle = mul_u64_u64_div_u64(val, MAX7360_PWM_PERIOD_NS,
> > +						MAX7360_PWM_MAX_RES);
>
> You have to round up here. I would expect that the checks in the core
> (with PWM_DEBUG=1) help you catching this type of error. In your case
> changing the configuration to
>
> 	.period = 2000000,
> 	.duty_cycle = 234379,
>
> should yield some hint in the kernel log.
>

Thanks for the reproduce steps: I saw the bug and fixed it. Also
MAX7360_PWM_MAX_RES had to be set to 255 and not 256...

> > +	return 0;
> > +}
>
> Best regards
> Uwe

I also fixed all other points mentioned in your mail. Thanks again for your review.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

On Fri, Jan 17, 2025 at 03:11:29PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Jan 17, 2025 at 10:33 AM CET, Uwe Kleine-König wrote:
> > Hello Mathieu,
> >
> > On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ...
> > > +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			     const struct pwm_state *state)
> > > +{
> > > +	struct max7360_pwm *max7360_pwm;
> > > +	u64 duty_steps;
> > > +	int ret;
> > > +
> > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	if (state->period != MAX7360_PWM_PERIOD_NS) {
> > > +		dev_warn(&chip->dev,
> > > +			 "unsupported pwm period: %llu, should be %u\n",
> > > +			 state->period, MAX7360_PWM_PERIOD_NS);
> > > +		return -EINVAL;
> >
> > Please don't emit error messages in .apply(). Also a driver is supposed
> > to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
> > accepted.
> >
> > Also note that you might want to implement the waveform callbacks
> > instead of .apply() and .get_state() for the more modern abstraction
> > (with slightly different rounding rules).
> >
> 
> Sure, I just switched to the waveform callbacks, it was quite
> straightforward.

sounds great. Note that the detail in rounding that is different for
waveforms is that a value that cannot be round down to a valid value
(because it's too small) is round up. This is a bit ugly in the drivers
but simplifies usage considerably. So you never return -EINVAL because
the values don't fit.

> Thanks for the reproduce steps: I saw the bug and fixed it. Also
> MAX7360_PWM_MAX_RES had to be set to 255 and not 256...

A good test (for a driver doing .apply/.get_state) is a sequence of
increasing settings. So something like:

	for p in range(1000, 10000):
	    pwm_apply(period=p, duty_cycle=0, ...)

and also do the same for duty_cycle and also try decreasing series.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Fri Jan 17, 2025 at 3:40 PM CET, Uwe Kleine-König wrote:
> On Fri, Jan 17, 2025 at 03:11:29PM +0100, Mathieu Dubois-Briand wrote:
> > On Fri Jan 17, 2025 at 10:33 AM CET, Uwe Kleine-König wrote:
> > > Hello Mathieu,
> > >
> > > On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > > > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ...
> > > > +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			     const struct pwm_state *state)
> > > > +{
> > > > +	struct max7360_pwm *max7360_pwm;
> > > > +	u64 duty_steps;
> > > > +	int ret;
> > > > +
> > > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (state->period != MAX7360_PWM_PERIOD_NS) {
> > > > +		dev_warn(&chip->dev,
> > > > +			 "unsupported pwm period: %llu, should be %u\n",
> > > > +			 state->period, MAX7360_PWM_PERIOD_NS);
> > > > +		return -EINVAL;
> > >
> > > Please don't emit error messages in .apply(). Also a driver is supposed
> > > to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
> > > accepted.
> > >
> > > Also note that you might want to implement the waveform callbacks
> > > instead of .apply() and .get_state() for the more modern abstraction
> > > (with slightly different rounding rules).
> > >
> > 
> > Sure, I just switched to the waveform callbacks, it was quite
> > straightforward.
>
> sounds great. Note that the detail in rounding that is different for
> waveforms is that a value that cannot be round down to a valid value
> (because it's too small) is round up. This is a bit ugly in the drivers
> but simplifies usage considerably. So you never return -EINVAL because
> the values don't fit.
>

Sorry, I'm not sure I got it right. Does this affect the three members
of pwm_waveform (period_length_ns, duty_offset_ns, duty_length_ns) ? So
on this device where the period is fixed and I cannot define an offset,
does that mean I will silently accept any value for period_length_ns and
duty_offset_ns ?

Best regards,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

Hello Mathieu,

On Fri, Jan 17, 2025 at 04:47:45PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Jan 17, 2025 at 3:40 PM CET, Uwe Kleine-König wrote:
> > sounds great. Note that the detail in rounding that is different for
> > waveforms is that a value that cannot be round down to a valid value
> > (because it's too small) is round up. This is a bit ugly in the drivers
> > but simplifies usage considerably. So you never return -EINVAL because
> > the values don't fit.
> 
> Sorry, I'm not sure I got it right. Does this affect the three members
> of pwm_waveform (period_length_ns, duty_offset_ns, duty_length_ns) ? So
> on this device where the period is fixed and I cannot define an offset,
> does that mean I will silently accept any value for period_length_ns and
> duty_offset_ns ?

Yes. The fromhw callback obviously always fills the respective constants
into .period_length_ns and .duty_offset_ns and the tohw callback
essentially only looks at .duty_length_ns.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Mon Jan 20, 2025 at 3:13 PM CET, Uwe Kleine-König wrote:
> Hello Mathieu,
>
> On Fri, Jan 17, 2025 at 04:47:45PM +0100, Mathieu Dubois-Briand wrote:
> > On Fri Jan 17, 2025 at 3:40 PM CET, Uwe Kleine-König wrote:
> > > sounds great. Note that the detail in rounding that is different for
> > > waveforms is that a value that cannot be round down to a valid value
> > > (because it's too small) is round up. This is a bit ugly in the drivers
> > > but simplifies usage considerably. So you never return -EINVAL because
> > > the values don't fit.
> > 
> > Sorry, I'm not sure I got it right. Does this affect the three members
> > of pwm_waveform (period_length_ns, duty_offset_ns, duty_length_ns) ? So
> > on this device where the period is fixed and I cannot define an offset,
> > does that mean I will silently accept any value for period_length_ns and
> > duty_offset_ns ?
>
> Yes. The fromhw callback obviously always fills the respective constants
> into .period_length_ns and .duty_offset_ns and the tohw callback
> essentially only looks at .duty_length_ns.
>
> Best regards
> Uwe

Ok, thanks! I will make these changes for the next version.

Best regards,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

Two sets of GPIOs are provided by the device:
- Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
  These GPIOs also provide interrupts on input changes.
- Up to 6 GPOs, on unused keypad columns pins.

Co-developed-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/Kconfig        |  11 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-max7360.c | 454 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 466 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 93ee3aa092f8..efe07e21c442 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1444,6 +1444,17 @@ config GPIO_MADERA
 	help
 	  Support for GPIOs on Cirrus Logic Madera class codecs.
 
+config GPIO_MAX7360
+	tristate "MAX7360 GPIO support"
+	depends on MFD_MAX7360
+	depends on OF_GPIO
+	help
+	  Allows to use MAX7360 I/O Expander PWM lines as GPIO and keypad COL
+	  lines as GPO.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-max7360.
+
 config GPIO_MAX77620
 	tristate "GPIO support for PMIC MAX77620 and MAX20024"
 	depends on MFD_MAX77620
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index af3ba4d81b58..581341b3e3e4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -100,6 +100,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_MAX7360)		+= gpio-max7360.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-max7360.c b/drivers/gpio/gpio-max7360.c
new file mode 100644
index 000000000000..9b701246689c
--- /dev/null
+++ b/drivers/gpio/gpio-max7360.c
@@ -0,0 +1,454 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX7360_GPIO_PORT	1
+#define MAX7360_GPIO_COL	2
+
+struct max7360_gpio {
+	struct gpio_chip chip;
+	struct device *dev;
+	struct regmap *regmap;
+	unsigned long gpio_function;
+
+	/*
+	 * Interrupts handling data: only used when gpio_function is
+	 * MAX7360_GPIO_PORT.
+	 */
+	u8 masked_interrupts;
+	u8 in_values;
+	unsigned int irq_types[MAX7360_MAX_GPIO];
+};
+
+static void max7360_gpio_set_value(struct gpio_chip *gc,
+				   unsigned int pin, int state)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+	int ret;
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
+		int off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
+
+		ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS,
+					BIT(off), state ? BIT(off) : 0);
+	} else {
+		ret = regmap_write(max7360_gpio->regmap,
+				   MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0);
+	}
+
+	if (ret)
+		dev_err(max7360_gpio->dev,
+			"failed to set value %d on gpio-%d", state, pin);
+}
+
+static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+	unsigned int val;
+	int off;
+	int ret;
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
+		off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
+
+		ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val);
+	} else {
+		off = pin;
+		ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
+	}
+
+	if (ret) {
+		dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin);
+		return ret;
+	}
+
+	return !!(val & BIT(off));
+}
+
+static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+	unsigned int val;
+	int ret;
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val);
+	if (ret) {
+		dev_err(max7360_gpio->dev, "failed to read gpio-%d direction",
+			pin);
+		return ret;
+	}
+
+	if (val & BIT(pin))
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+	int ret;
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
+		return -EIO;
+
+	ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL,
+				BIT(pin), 0);
+	if (ret) {
+		dev_err(max7360_gpio->dev, "failed to set gpio-%d direction",
+			pin);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin,
+					 int state)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+	int ret;
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
+		ret = regmap_write_bits(max7360_gpio->regmap,
+					MAX7360_REG_GPIOCTRL, BIT(pin),
+					BIT(pin));
+		if (ret) {
+			dev_err(max7360_gpio->dev,
+				"failed to set gpio-%d direction", pin);
+			return ret;
+		}
+	}
+
+	max7360_gpio_set_value(gc, pin, state);
+
+	return 0;
+}
+
+static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+
+	/*
+	 * GPOs on COL pins (keypad columns) can always be requested: this
+	 * driver has full access to them, up to the number set in chip.ngpio.
+	 * GPIOs on PORT pins are shared with the PWM and rotary encoder
+	 * drivers: they have to be requested from the MFD driver.
+	 */
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
+		return 0;
+
+	return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true);
+}
+
+static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
+{
+	struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
+		return;
+
+	max7360_port_pin_request(max7360_gpio->dev->parent, pin, false);
+}
+
+static struct gpio_chip max7360_gpio_chip = {
+	.label                  = "max7360",
+	.request		= max7360_gpio_request,
+	.free			= max7360_gpio_free,
+	.get_direction		= max7360_gpio_get_direction,
+	.direction_input	= max7360_gpio_direction_input,
+	.direction_output	= max7360_gpio_direction_output,
+	.get                    = max7360_gpio_get_value,
+	.set                    = max7360_gpio_set_value,
+	.base                   = -1,
+	.can_sleep              = true,
+};
+
+static irqreturn_t max7360_gpio_irq(int irq, void *data)
+{
+	struct max7360_gpio *max7360_gpio = data;
+	unsigned long pending;
+	unsigned int gpio_irq;
+	unsigned int type;
+	unsigned int count = 0;
+	int val;
+	int irqn;
+	int ret;
+
+	ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret) {
+		dev_err(max7360_gpio->dev, "Failed to read gpio values: %d\n",
+			ret);
+		return IRQ_NONE;
+	}
+
+	/* MAX7360 generates interrupts but does not report which pins changed:
+	 * compare the pin value with the value they had in previous interrupt
+	 * and report interrupt if the change match the set IRQ type.
+	 */
+	pending = val ^ max7360_gpio->in_values;
+	for_each_set_bit(irqn, &pending, max7360_gpio->chip.ngpio) {
+		type = max7360_gpio->irq_types[irqn];
+		if (max7360_gpio->masked_interrupts & BIT(irqn))
+			continue;
+		if ((val & BIT(irqn)) && type == IRQ_TYPE_EDGE_FALLING)
+			continue;
+		if (!(val & BIT(irqn)) && type == IRQ_TYPE_EDGE_RISING)
+			continue;
+		gpio_irq = irq_find_mapping(max7360_gpio->chip.irq.domain, irqn);
+		handle_nested_irq(gpio_irq);
+		count++;
+	}
+
+	max7360_gpio->in_values = val;
+
+	if (count == 0)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static void max7360_gpio_irq_unmask(struct irq_data *data)
+{
+	struct max7360_gpio *max7360_gpio;
+	unsigned int pin = irqd_to_hwirq(data);
+	unsigned int val;
+	int ret;
+
+	max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+	/* Read current pin value, so we know if the pin changed in the next
+	 * interrupt.
+	 * No lock should be needed regarding the interrupt handler: as long as
+	 * the corresponding bit has not been cleared in masked_interrupts, this
+	 * gpio is ignored.
+	 */
+	ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret)
+		dev_err(max7360_gpio->dev, "Failed to read gpio values: %d\n",
+			ret);
+
+	max7360_gpio->in_values &= ~BIT(pin);
+	max7360_gpio->in_values |= val & BIT(pin);
+
+	ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin,
+				MAX7360_PORT_CFG_INTERRUPT_MASK, 0);
+
+	if (ret)
+		dev_err(max7360_gpio->dev, "failed to unmask gpio-%d", pin);
+
+	max7360_gpio->masked_interrupts &= ~BIT(pin);
+}
+
+static void max7360_gpio_irq_mask(struct irq_data *data)
+{
+	struct max7360_gpio *max7360_gpio;
+	unsigned int pin = irqd_to_hwirq(data);
+	int ret;
+
+	max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+	max7360_gpio->masked_interrupts |= BIT(pin);
+
+	ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin,
+				MAX7360_PORT_CFG_INTERRUPT_MASK,
+				MAX7360_PORT_CFG_INTERRUPT_MASK);
+
+	if (ret)
+		dev_err(max7360_gpio->dev, "failed to mask gpio-%d", pin);
+}
+
+static void max7360_gpio_irq_enable(struct irq_data *data)
+{
+	max7360_gpio_irq_unmask(data);
+}
+
+static void max7360_gpio_irq_disable(struct irq_data *data)
+{
+	max7360_gpio_irq_mask(data);
+}
+
+static int max7360_gpio_irq_set_type(struct irq_data *data,
+				     unsigned int flow_type)
+{
+	struct max7360_gpio *max7360_gpio;
+	unsigned int pin;
+	unsigned int val;
+	int ret;
+
+	pin = irqd_to_hwirq(data);
+	max7360_gpio = gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+	switch (flow_type) {
+	case IRQ_TYPE_EDGE_RISING:
+		val = 0;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_EDGE_BOTH:
+		val = MAX7360_PORT_CFG_INTERRUPT_EDGES;
+		break;
+	default:
+		return -EINVAL;
+	}
+	ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PWMCFG + pin,
+				MAX7360_PORT_CFG_INTERRUPT_EDGES, val);
+
+	if (ret)
+		dev_err(max7360_gpio->dev, "failed to unmask gpio-%d", pin);
+
+	max7360_gpio->irq_types[pin] = flow_type;
+
+	return 0;
+}
+
+static const struct irq_chip max7360_gpio_irqchip = {
+	.name = "max7360",
+	.irq_enable = max7360_gpio_irq_enable,
+	.irq_disable = max7360_gpio_irq_disable,
+	.irq_mask = max7360_gpio_irq_mask,
+	.irq_unmask = max7360_gpio_irq_unmask,
+	.irq_set_type = max7360_gpio_irq_set_type,
+	.flags = IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int max7360_gpio_probe(struct platform_device *pdev)
+{
+	struct max7360_gpio *max7360_gpio;
+	struct platform_device *parent;
+	unsigned int ngpios;
+	unsigned int outconf;
+	struct gpio_irq_chip *girq;
+	unsigned long flags;
+	int irq;
+	int ret;
+
+	if (!pdev->dev.parent) {
+		dev_err(&pdev->dev, "no parent device\n");
+		return -ENODEV;
+	}
+	parent = to_platform_device(pdev->dev.parent);
+
+	max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(*max7360_gpio),
+				    GFP_KERNEL);
+	if (!max7360_gpio)
+		return -ENOMEM;
+
+	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
+		dev_err(&pdev->dev, "Missing ngpios OF property\n");
+		return -ENODEV;
+	}
+
+	max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_gpio->regmap) {
+		dev_err(&pdev->dev, "could not get parent regmap\n");
+		return -ENODEV;
+	}
+
+	max7360_gpio->dev = &pdev->dev;
+	max7360_gpio->chip = max7360_gpio_chip;
+	max7360_gpio->chip.ngpio = ngpios;
+	max7360_gpio->chip.parent = &pdev->dev;
+	max7360_gpio->chip.base = -1;
+	max7360_gpio->gpio_function = (uintptr_t)device_get_match_data(&pdev->dev);
+
+	dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio);
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
+		/* Port GPIOs: set output mode configuration (constant-current
+		 * or not).
+		 * This property is optional.
+		 */
+		outconf = 0;
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "maxim,constant-current-disable",
+					   &outconf);
+		if (ret && (ret != -EINVAL)) {
+			dev_err(&pdev->dev,
+				"Failed to read maxim,constant-current-disable OF property\n");
+			return -ENODEV;
+		}
+
+	    regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf);
+	}
+
+	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT &&
+	    of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
+		/* Port GPIOs: declare IRQ chip, if configuration was provided.
+		 */
+		irq = platform_get_irq_byname(parent, "inti");
+		if (irq < 0)
+			return dev_err_probe(&pdev->dev, irq,
+					     "Failed to get IRQ\n");
+
+		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
+		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+						max7360_gpio_irq, flags,
+						"max7360-gpio", max7360_gpio);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to register interrupt\n");
+
+		girq = &max7360_gpio->chip.irq;
+		gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip);
+		girq->parent_handler = NULL;
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->threaded = true;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_simple_irq;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio);
+	if (ret) {
+		return dev_err_probe(&pdev->dev, ret,
+				     "unable to add gpiochip\n");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id max7360_gpio_of_match[] = {
+	{
+		.compatible = "maxim,max7360-gpo",
+		.data = (void *)MAX7360_GPIO_COL
+	}, {
+		.compatible = "maxim,max7360-gpio",
+		.data = (void *)MAX7360_GPIO_PORT
+	}, {
+	}
+};
+MODULE_DEVICE_TABLE(of, max7360_gpio_of_match);
+
+static struct platform_driver max7360_gpio_driver = {
+	.driver = {
+		.name	= MAX7360_DRVNAME_GPIO,
+		.of_match_table = of_match_ptr(max7360_gpio_of_match),
+	},
+	.probe		= max7360_gpio_probe,
+};
+module_platform_driver(max7360_gpio_driver);
+
+MODULE_DESCRIPTION("MAX7360 GPIO driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_ALIAS("platform:" MAX7360_DRVNAME_GPIO);
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Linus Walleij]

Hi Mathieu,

thanks for your patch!

On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
<mathieu.dubois-briand@bootlin.com> wrote:

> Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
>
> Two sets of GPIOs are provided by the device:
> - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
>   These GPIOs also provide interrupts on input changes.
> - Up to 6 GPOs, on unused keypad columns pins.
>
> Co-developed-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
(...)
> +#include <linux/gpio/consumer.h>

Why?

My most generic feedback is if you have looked at using
select GPIO_REGMAP for this driver?

The regmap utility library is very helpful, look how other driver
selecting GPIO_REGMAP gets default implementations
from the library just git grep GPIO_REGMAP drivers/gpio/

> +static void max7360_gpio_set_value(struct gpio_chip *gc,
> +                                  unsigned int pin, int state)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {

OK some custom stuff...

> +               int off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> +
> +               ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS,
> +                                       BIT(off), state ? BIT(off) : 0);

Fairly standard.

> +       } else {
> +               ret = regmap_write(max7360_gpio->regmap,
> +                                  MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0);
> +       }

Some custom stuff.

> +static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       unsigned int val;
> +       int off;
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
> +               off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> +
> +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val);
> +       } else {
> +               off = pin;
> +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
> +       }
> +
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin);
> +               return ret;
> +       }
> +
> +       return !!(val & BIT(off));
> +}

Looks like stock template regmap-gpio.

> +static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       unsigned int val;
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return GPIO_LINE_DIRECTION_OUT;
> +
> +       ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val);
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to read gpio-%d direction",
> +                       pin);
> +               return ret;
> +       }
> +
> +       if (val & BIT(pin))
> +               return GPIO_LINE_DIRECTION_OUT;
> +
> +       return GPIO_LINE_DIRECTION_IN;
> +}

Dito.

> +static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return -EIO;
> +
> +       ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL,
> +                               BIT(pin), 0);
> +       if (ret) {
> +               dev_err(max7360_gpio->dev, "failed to set gpio-%d direction",
> +                       pin);
> +               return ret;
> +       }
> +
> +       return 0;
> +}

Dito.

> +static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin,
> +                                        int state)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +       int ret;
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
> +               ret = regmap_write_bits(max7360_gpio->regmap,
> +                                       MAX7360_REG_GPIOCTRL, BIT(pin),
> +                                       BIT(pin));
> +               if (ret) {
> +                       dev_err(max7360_gpio->dev,
> +                               "failed to set gpio-%d direction", pin);
> +                       return ret;
> +               }
> +       }
> +
> +       max7360_gpio_set_value(gc, pin, state);
> +
> +       return 0;
> +}

Dito.

> +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +
> +       /*
> +        * GPOs on COL pins (keypad columns) can always be requested: this
> +        * driver has full access to them, up to the number set in chip.ngpio.
> +        * GPIOs on PORT pins are shared with the PWM and rotary encoder
> +        * drivers: they have to be requested from the MFD driver.
> +        */
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return 0;
> +
> +       return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true);
> +}
> +
> +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> +{
> +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> +
> +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> +               return;
> +
> +       max7360_port_pin_request(max7360_gpio->dev->parent, pin, false);
> +}

The pin request looks a bit like a custom pin control implementation...

But I think it's fine, pin control can be a bit heavy to implement on simple
devices, but if there is elaborate muxing and config going on, pin control
should be used.

Yours,
Linus Walleij

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote:
> Hi Mathieu,
>
> thanks for your patch!
>
> On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
> <mathieu.dubois-briand@bootlin.com> wrote:
>
> > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
> >
> > Two sets of GPIOs are provided by the device:
> > - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
> >   These GPIOs also provide interrupts on input changes.
> > - Up to 6 GPOs, on unused keypad columns pins.
> >
> > Co-developed-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> (...)
> > +#include <linux/gpio/consumer.h>
>
> Why?
>
> My most generic feedback is if you have looked at using
> select GPIO_REGMAP for this driver?
>
> The regmap utility library is very helpful, look how other driver
> selecting GPIO_REGMAP gets default implementations
> from the library just git grep GPIO_REGMAP drivers/gpio/
>

Thanks, I was not aware of that. I tested it and I should be able to get
rid of a lot of code using GPIO_REGMAP.

My main concern so far is with the request()/free() functions, as I
believe I will not be able to define them as callback anymore.

I also saw the equivalent REGMAP_IRQ, but I'm not sure I will be able to
use it, as I believe I would need to have registers identifying the
exact GPIO source of the IRQ.

> > +static void max7360_gpio_set_value(struct gpio_chip *gc,
> > +                                  unsigned int pin, int state)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +       int ret;
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
>
> OK some custom stuff...
>
> > +               int off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> > +
> > +               ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_PORTS,
> > +                                       BIT(off), state ? BIT(off) : 0);
>
> Fairly standard.
>
> > +       } else {
> > +               ret = regmap_write(max7360_gpio->regmap,
> > +                                  MAX7360_REG_PWMBASE + pin, state ? 0xFF : 0);
> > +       }
>
> Some custom stuff.
>
> > +static int max7360_gpio_get_value(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +       unsigned int val;
> > +       int off;
> > +       int ret;
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL) {
> > +               off = MAX7360_MAX_GPIO - (gc->ngpio - pin);
> > +
> > +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_PORTS, &val);
> > +       } else {
> > +               off = pin;
> > +               ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOIN, &val);
> > +       }
> > +
> > +       if (ret) {
> > +               dev_err(max7360_gpio->dev, "failed to read gpio-%d", pin);
> > +               return ret;
> > +       }
> > +
> > +       return !!(val & BIT(off));
> > +}
>
> Looks like stock template regmap-gpio.
>
> > +static int max7360_gpio_get_direction(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +       unsigned int val;
> > +       int ret;
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> > +               return GPIO_LINE_DIRECTION_OUT;
> > +
> > +       ret = regmap_read(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL, &val);
> > +       if (ret) {
> > +               dev_err(max7360_gpio->dev, "failed to read gpio-%d direction",
> > +                       pin);
> > +               return ret;
> > +       }
> > +
> > +       if (val & BIT(pin))
> > +               return GPIO_LINE_DIRECTION_OUT;
> > +
> > +       return GPIO_LINE_DIRECTION_IN;
> > +}
>
> Dito.
>
> > +static int max7360_gpio_direction_input(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +       int ret;
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> > +               return -EIO;
> > +
> > +       ret = regmap_write_bits(max7360_gpio->regmap, MAX7360_REG_GPIOCTRL,
> > +                               BIT(pin), 0);
> > +       if (ret) {
> > +               dev_err(max7360_gpio->dev, "failed to set gpio-%d direction",
> > +                       pin);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
>
> Dito.
>
> > +static int max7360_gpio_direction_output(struct gpio_chip *gc, unsigned int pin,
> > +                                        int state)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +       int ret;
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
> > +               ret = regmap_write_bits(max7360_gpio->regmap,
> > +                                       MAX7360_REG_GPIOCTRL, BIT(pin),
> > +                                       BIT(pin));
> > +               if (ret) {
> > +                       dev_err(max7360_gpio->dev,
> > +                               "failed to set gpio-%d direction", pin);
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       max7360_gpio_set_value(gc, pin, state);
> > +
> > +       return 0;
> > +}
>
> Dito.
>
> > +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +
> > +       /*
> > +        * GPOs on COL pins (keypad columns) can always be requested: this
> > +        * driver has full access to them, up to the number set in chip.ngpio.
> > +        * GPIOs on PORT pins are shared with the PWM and rotary encoder
> > +        * drivers: they have to be requested from the MFD driver.
> > +        */
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> > +               return 0;
> > +
> > +       return max7360_port_pin_request(max7360_gpio->dev->parent, pin, true);
> > +}
> > +
> > +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +       struct max7360_gpio *max7360_gpio = gpiochip_get_data(gc);
> > +
> > +       if (max7360_gpio->gpio_function == MAX7360_GPIO_COL)
> > +               return;
> > +
> > +       max7360_port_pin_request(max7360_gpio->dev->parent, pin, false);
> > +}
>
> The pin request looks a bit like a custom pin control implementation...
>
> But I think it's fine, pin control can be a bit heavy to implement on simple
> devices, but if there is elaborate muxing and config going on, pin control
> should be used.

Just so remove any doubt, all this does is request the pin for the
exclusive use of this driver, preventing the PWM or rotary encoder
drivers to use it. There is no hardware configuration done here.

Yet I agree that this does look a bit like some pin muxing.

>
> Yours,
> Linus Walleij

Thanks for your review.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote:
> On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
> My most generic feedback is if you have looked at using
> select GPIO_REGMAP for this driver?
>
> The regmap utility library is very helpful, look how other driver
> selecting GPIO_REGMAP gets default implementations
> from the library just git grep GPIO_REGMAP drivers/gpio/
>

I tried to switch to GPIO_REGMAP and I really like it overall, as it
does simplify a lot the code. However, I identified two features that I
was not able to port so far: the request()/free() callbacks and the
interrupts.

So for the request()/free() callbacks, I cannot add them anymore, as
they are set on the gpio_chip structure, and this structure is hidden
behind the gpio_regmap structure. I could easily modify the
gpio_regmap_config structure and gpio_regmap_register() to allow to
provide these callbacks, but is this acceptable? Or should I switch to a
different way to prevent concurrent use of the same pin? I saw you
mentioned the possibility of defining pin control.

On the IRQ side, before switching to GPIO_REGMAP, I was able to define
the IRQ configuration using the irq member of the gpio_chip structure.
This does create the IRQ domain for me in a quite straightforward way.
Again, I will not be able to do that anymore, as the gpio_chip structure
is hidden. 

I saw I can specify my own irq_domain in gpio_regmap_config, so that
would be a way, but I was wondering if there is any way to have
something as easy as previously.

I had a quick look at existing drivers using GPIO_REGMAP and providing
IRQ support: I believe they are all using REGMAP_IRQ. And I believe I
cannot use REGMAP_IRQ here, as if I understood correctly, I would need
to have a register telling me exactly on which GPIO I have a pending
interrupt and I don't have such a thing: all I know is there was an
interrupt related to the GPIOs, and then I have to compare each GPIO
with the previous known state to know which pin is affected.

Do you have any thought about this?

Best regards,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Linus Walleij]

Hi Mathieu,

On Fri, Jan 17, 2025 at 4:22 PM Mathieu Dubois-Briand
<mathieu.dubois-briand@bootlin.com> wrote:

> I tried to switch to GPIO_REGMAP and I really like it overall, as it
> does simplify a lot the code. However, I identified two features that I
> was not able to port so far: the request()/free() callbacks and the
> interrupts.

Thanks for looking into this!

> So for the request()/free() callbacks, I cannot add them anymore, as
> they are set on the gpio_chip structure, and this structure is hidden
> behind the gpio_regmap structure. I could easily modify the
> gpio_regmap_config structure and gpio_regmap_register() to allow to
> provide these callbacks, but is this acceptable?

Of course. The template is to be used for setting it up, just
modify it if we need more from it.

> On the IRQ side, before switching to GPIO_REGMAP, I was able to define
> the IRQ configuration using the irq member of the gpio_chip structure.
> This does create the IRQ domain for me in a quite straightforward way.
> Again, I will not be able to do that anymore, as the gpio_chip structure
> is hidden.
(...)
> I had a quick look at existing drivers using GPIO_REGMAP and providing
> IRQ support: I believe they are all using REGMAP_IRQ. And I believe I
> cannot use REGMAP_IRQ here,

Then we need to modify GPIO_REGMAP so it's possible to pass
in our own gpio_irq_chip, maybe another member in
the config, simply?

Yours,
Linus Walleij

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 04:22:33PM +0100, Mathieu Dubois-Briand wrote:
> On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote:
> > On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
> > My most generic feedback is if you have looked at using
> > select GPIO_REGMAP for this driver?
> >
> > The regmap utility library is very helpful, look how other driver
> > selecting GPIO_REGMAP gets default implementations
> > from the library just git grep GPIO_REGMAP drivers/gpio/
> 
> I tried to switch to GPIO_REGMAP and I really like it overall, as it
> does simplify a lot the code. However, I identified two features that I
> was not able to port so far: the request()/free() callbacks and the
> interrupts.

You can easily extend the config to provide both if needed.
So, update gpio-regmap.c itself as a prerequisite.

> So for the request()/free() callbacks, I cannot add them anymore, as
> they are set on the gpio_chip structure, and this structure is hidden
> behind the gpio_regmap structure. I could easily modify the
> gpio_regmap_config structure and gpio_regmap_register() to allow to
> provide these callbacks, but is this acceptable? Or should I switch to a
> different way to prevent concurrent use of the same pin? I saw you
> mentioned the possibility of defining pin control.
> 
> On the IRQ side, before switching to GPIO_REGMAP, I was able to define
> the IRQ configuration using the irq member of the gpio_chip structure.
> This does create the IRQ domain for me in a quite straightforward way.
> Again, I will not be able to do that anymore, as the gpio_chip structure
> is hidden. 

Look how it's done in, e.g., drivers/gpio/gpio-104-idi-48.c.

It's pretty straightforward. You create and register an IRQ chip with
devm_regmap_add_irq_chip(). It creates a domain for you.

> I saw I can specify my own irq_domain in gpio_regmap_config, so that
> would be a way, but I was wondering if there is any way to have
> something as easy as previously.
> 
> I had a quick look at existing drivers using GPIO_REGMAP and providing
> IRQ support: I believe they are all using REGMAP_IRQ. And I believe I
> cannot use REGMAP_IRQ here, as if I understood correctly, I would need
> to have a register telling me exactly on which GPIO I have a pending
> interrupt and I don't have such a thing: all I know is there was an
> interrupt related to the GPIOs, and then I have to compare each GPIO
> with the previous known state to know which pin is affected.
> 
> Do you have any thought about this?

I briefly looked at the code of regmap IRQ and I don't see big impediments
for your case. So, you seems to have mask, unmask, and input registers.
What you most likely want to do is to use input as status (regmap IRQ
supports configuration without status registers).

What seems to be needed is the logic that uses previous state to handle a new
one. This is not only this chip that may need this type of handling, so it
will be beneficial for others that may be converted to use gpio-regmap.c.

So, I don't think we need full bypass of the GPIO IRQ chip through
gpio-regmap.c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
> 
> Two sets of GPIOs are provided by the device:
> - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
>   These GPIOs also provide interrupts on input changes.
> - Up to 6 GPOs, on unused keypad columns pins.

...

> +	depends on OF_GPIO

This is wrong. You don't use it.

...

> +#include <linux/of.h>

Shouldn't be here. Instead it should be linux/property.h.

...

> +	/* MAX7360 generates interrupts but does not report which pins changed:
> +	 * compare the pin value with the value they had in previous interrupt
> +	 * and report interrupt if the change match the set IRQ type.
> +	 */

/*
 * Wrong multi-line comment style. Consider using
 * this one as an example. This applies to all the comments
 * in the entire series.
 */

> +	pending = val ^ max7360_gpio->in_values;
> +	for_each_set_bit(irqn, &pending, max7360_gpio->chip.ngpio) {
> +		type = max7360_gpio->irq_types[irqn];
> +		if (max7360_gpio->masked_interrupts & BIT(irqn))
> +			continue;
> +		if ((val & BIT(irqn)) && type == IRQ_TYPE_EDGE_FALLING)
> +			continue;
> +		if (!(val & BIT(irqn)) && type == IRQ_TYPE_EDGE_RISING)
> +			continue;
> +		gpio_irq = irq_find_mapping(max7360_gpio->chip.irq.domain, irqn);
> +		handle_nested_irq(gpio_irq);
> +		count++;

You can also look how gpio-pca953x.c does this. It uses bitmaps all over the
place. But with the gpio-regmap.c in use this should be much more simpler.

> +	}

> +	max7360_gpio->in_values = val;

> +	if (count == 0)

count is redundant, Checking pending against 0 is enough (or in case of
bitmaps, if it's longer than unsigned long, bitmap_empty() is what is here).

> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;

...

> +static int max7360_gpio_probe(struct platform_device *pdev)
> +{
> +	struct max7360_gpio *max7360_gpio;
> +	struct platform_device *parent;
> +	unsigned int ngpios;
> +	unsigned int outconf;
> +	struct gpio_irq_chip *girq;
> +	unsigned long flags;
> +	int irq;
> +	int ret;

> +	if (!pdev->dev.parent) {
> +		dev_err(&pdev->dev, "no parent device\n");
> +		return -ENODEV;
> +	}

Probably redundant as one of the calls below may fail if parent is absent (see
below for more).

> +	parent = to_platform_device(pdev->dev.parent);

Why do you need this? Can't the fwnode be propagated to the children and then
the respective APIs to be used?

> +	max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(*max7360_gpio),
> +				    GFP_KERNEL);

With

	struct device *dev = &pdev->dev;

at the beginning of the function this and other lines will become neater and
shorter, in particular this becomes one line even for the strict 80 characters
limit (however we have it being relaxed to 100 nowadays).

> +	if (!max7360_gpio)
> +		return -ENOMEM;

> +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> +		return -ENODEV;
> +	}

This is not needed, it is already done in GPIOLIB core.

> +	max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!max7360_gpio->regmap) {
> +		dev_err(&pdev->dev, "could not get parent regmap\n");
> +		return -ENODEV;

Here and everywhere in the ->probe() and related use

	return dev_err_probe(...);

And drop unneeded curly braces.

> +	}

> +	max7360_gpio->dev = &pdev->dev;

So, why do you need this dup? You can easily get it via GPIO chip, no?

> +	max7360_gpio->chip = max7360_gpio_chip;
> +	max7360_gpio->chip.ngpio = ngpios;
> +	max7360_gpio->chip.parent = &pdev->dev;
> +	max7360_gpio->chip.base = -1;
> +	max7360_gpio->gpio_function = (uintptr_t)device_get_match_data(&pdev->dev);
> +
> +	dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio);
> +
> +	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
> +		/* Port GPIOs: set output mode configuration (constant-current
> +		 * or not).
> +		 * This property is optional.
> +		 */
> +		outconf = 0;
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					   "maxim,constant-current-disable",
> +					   &outconf);

device_property_read_u32()

> +		if (ret && (ret != -EINVAL)) {
> +			dev_err(&pdev->dev,
> +				"Failed to read maxim,constant-current-disable OF property\n");
> +			return -ENODEV;
> +		}
> +
> +	    regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf);
> +	}
> +
> +	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT &&
> +	    of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
> +		/* Port GPIOs: declare IRQ chip, if configuration was provided.
> +		 */

> +		irq = platform_get_irq_byname(parent, "inti");

fwnode_irq_get_byname() with the propagated firmware node will give you
the same result.

> +		if (irq < 0)
> +			return dev_err_probe(&pdev->dev, irq,
> +					     "Failed to get IRQ\n");
> +
> +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						max7360_gpio_irq, flags,
> +						"max7360-gpio", max7360_gpio);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to register interrupt\n");
> +
> +		girq = &max7360_gpio->chip.irq;
> +		gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip);
> +		girq->parent_handler = NULL;
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->threaded = true;
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_simple_irq;
> +	}
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio);
> +	if (ret) {
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "unable to add gpiochip\n");
> +	}
> +
> +	return 0;
> +}

...

> +static struct platform_driver max7360_gpio_driver = {
> +	.driver = {
> +		.name	= MAX7360_DRVNAME_GPIO,

> +		.of_match_table = of_match_ptr(max7360_gpio_of_match),

No of_match_ptr() or ACPI_PTR() in new code, please. It appears to be more
harmful than helpful.

> +	},
> +	.probe		= max7360_gpio_probe,
> +};
> +module_platform_driver(max7360_gpio_driver);

...

> +MODULE_ALIAS("platform:" MAX7360_DRVNAME_GPIO);

Why? It doesn't look like it can be instantiated via board files.

...

Overall seems many of my comments are applicable to your entire series (all
patches), please address and feel free to Cc me on v4 for review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

Hi Andy,

Thanks for your review. I've been addressing most of your comments in
this mail and the ones related to regmap-irq. I should be able to send a
new version in a few days.

However I have a few questions regarding some of the points.

On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
> > +	parent = to_platform_device(pdev->dev.parent);
>
> Why do you need this? Can't the fwnode be propagated to the children and then
> the respective APIs to be used?
>

I'm not sure to understand this correctly, what do you mean by
propagating the fwnode to the children?

Just a quick summary of the situation and what I try to do. The device
tree looks like this, only keeping the interesting properties:

io-expander@38 {
  ...
  interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
               <24 IRQ_TYPE_LEVEL_LOW>;
  interrupt-names = "inti", "intk";

  max7360_gpio: gpio {
    ...
  };

  max7360_gpo: gpo {
    ...
  };
};

Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from
our parent device points to "io-expander@38". Here we need to get the
"inti" interrupt from the parent node. What would be the correct way to
do it?

> > +	if (!max7360_gpio)
> > +		return -ENOMEM;
>
> > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > +		return -ENODEV;
> > +	}
>
> This is not needed, it is already done in GPIOLIB core.
>

I believe this is still needed:
- For gpos, we need the gpio count to correctly set the partition
  between gpo and keypad columns in max7360_set_gpos_count().
- For gpios, we need the gpio count to setup the IRQs.

Best regards,
Mathieu

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +	parent = to_platform_device(pdev->dev.parent);
> >
> > Why do you need this? Can't the fwnode be propagated to the children and then
> > the respective APIs to be used?
> 
> I'm not sure to understand this correctly, what do you mean by
> propagating the fwnode to the children?
> 
> Just a quick summary of the situation and what I try to do. The device
> tree looks like this, only keeping the interesting properties:
> 
> io-expander@38 {
>   ...
>   interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
>                <24 IRQ_TYPE_LEVEL_LOW>;
>   interrupt-names = "inti", "intk";
> 
>   max7360_gpio: gpio {
>     ...
>   };
> 
>   max7360_gpo: gpo {
>     ...
>   };
> };
> 
> Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from
> our parent device points to "io-expander@38". Here we need to get the
> "inti" interrupt from the parent node. What would be the correct way to
> do it?

Ah, I see now. This is being used only for IRQs, but don't you want to call
actually fwnode_irq_get_byname()? It will makes the intention clearer.

...

> > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > +		return -ENODEV;
> > > +	}
> >
> > This is not needed, it is already done in GPIOLIB core.
> 
> I believe this is still needed:
> - For gpos, we need the gpio count to correctly set the partition
>   between gpo and keypad columns in max7360_set_gpos_count().

Shouldn't be that done somewhere in the GPIO valid mask initialisation?

> - For gpios, we need the gpio count to setup the IRQs.

Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
and other init callbacks?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > > +	parent = to_platform_device(pdev->dev.parent);
> > >
> > > Why do you need this? Can't the fwnode be propagated to the children and then
> > > the respective APIs to be used?
> > 
> > I'm not sure to understand this correctly, what do you mean by
> > propagating the fwnode to the children?
> > 
> > Just a quick summary of the situation and what I try to do. The device
> > tree looks like this, only keeping the interesting properties:
> > 
> > io-expander@38 {
> >   ...
> >   interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
> >                <24 IRQ_TYPE_LEVEL_LOW>;
> >   interrupt-names = "inti", "intk";
> > 
> >   max7360_gpio: gpio {
> >     ...
> >   };
> > 
> >   max7360_gpo: gpo {
> >     ...
> >   };
> > };
> > 
> > Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from
> > our parent device points to "io-expander@38". Here we need to get the
> > "inti" interrupt from the parent node. What would be the correct way to
> > do it?
>
> Ah, I see now. This is being used only for IRQs, but don't you want to call
> actually fwnode_irq_get_byname()? It will makes the intention clearer.
>

Sure! I can definitely call fwnode_irq_get_byname().

>
> > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > +		return -ENODEV;
> > > > +	}
> > >
> > > This is not needed, it is already done in GPIOLIB core.
> > 
> > I believe this is still needed:
> > - For gpos, we need the gpio count to correctly set the partition
> >   between gpo and keypad columns in max7360_set_gpos_count().
>
> Shouldn't be that done somewhere in the GPIO valid mask initialisation?
>
> > - For gpios, we need the gpio count to setup the IRQs.
>
> Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> and other init callbacks?

No, I believe I have to register the IRQ before registering the GPIO, so
I can get the IRQ domain.

Right now I have something like:

irq_chip->num_irqs = ngpios;
devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, irq, flags, 0, irq_chip, &irq_chip_data);
gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
devm_gpio_regmap_register(dev, &gpio_config);

Also, gpiolib will store ngpios in the gpio_chip structure, but while
using gpio-regmap, this structure is masked behind the opaque
gpio_regmap structure. So I believe there is no easy way to retrieve its
value.

This part of the code changed a lot, maybe it would be easier if I push
a new version of the series and we continue the discussion there?

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:

...

> > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > >
> > > > This is not needed, it is already done in GPIOLIB core.
> > > 
> > > I believe this is still needed:
> > > - For gpos, we need the gpio count to correctly set the partition
> > >   between gpo and keypad columns in max7360_set_gpos_count().
> >
> > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> >
> > > - For gpios, we need the gpio count to setup the IRQs.
> >
> > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > and other init callbacks?
> 
> No, I believe I have to register the IRQ before registering the GPIO, so
> I can get the IRQ domain.
> 
> Right now I have something like:
> 
> irq_chip->num_irqs = ngpios;
> devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> irq, flags, 0, irq_chip, &irq_chip_data);
> gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> devm_gpio_regmap_register(dev, &gpio_config);
> 
> Also, gpiolib will store ngpios in the gpio_chip structure, but while
> using gpio-regmap, this structure is masked behind the opaque
> gpio_regmap structure. So I believe there is no easy way to retrieve its
> value.
> 
> This part of the code changed a lot, maybe it would be easier if I push
> a new version of the series and we continue the discussion there?

So, what seems need to be added is some flag to GPIO regmap configuration
data structure and a code that is called after gpiochip_add_data() in
gpio_regmap_register() to create a domain. This will solve the above issue
and helps other drivers to get rid of potential duplication of
devm_regmap_add_irq_chip_fwnode() calls.

Have you researched this path?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > > +		return -ENODEV;
> > > > > > +	}
> > > > >
> > > > > This is not needed, it is already done in GPIOLIB core.
> > > > 
> > > > I believe this is still needed:
> > > > - For gpos, we need the gpio count to correctly set the partition
> > > >   between gpo and keypad columns in max7360_set_gpos_count().
> > >
> > > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> > >
> > > > - For gpios, we need the gpio count to setup the IRQs.
> > >
> > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > > and other init callbacks?
> > 
> > No, I believe I have to register the IRQ before registering the GPIO, so
> > I can get the IRQ domain.
> > 
> > Right now I have something like:
> > 
> > irq_chip->num_irqs = ngpios;
> > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> > irq, flags, 0, irq_chip, &irq_chip_data);
> > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> > devm_gpio_regmap_register(dev, &gpio_config);
> > 
> > Also, gpiolib will store ngpios in the gpio_chip structure, but while
> > using gpio-regmap, this structure is masked behind the opaque
> > gpio_regmap structure. So I believe there is no easy way to retrieve its
> > value.
> > 
> > This part of the code changed a lot, maybe it would be easier if I push
> > a new version of the series and we continue the discussion there?
>
> So, what seems need to be added is some flag to GPIO regmap configuration
> data structure and a code that is called after gpiochip_add_data() in
> gpio_regmap_register() to create a domain. This will solve the above issue
> and helps other drivers to get rid of potential duplication of
> devm_regmap_add_irq_chip_fwnode() calls.
>
> Have you researched this path?

OK, so looking at the code, I believe it would need to:
- Add some flag in gpio_regmap_config structure, so
  gpio_regmap_register() creates a new IRQ domain.
- Add a function allowing to retrieve this domain out of the gpio_regmap
  structure.
- Allow to pass a domain in the regmap_irq_chip structure, so
  regmap_add_irq_chip_fwnode() use this domain instead of calling
  regmap_irq_create_domain().
- Make sure this domain is still populated with the IRQ data: number of
  IRQs, IRQ base but also a pointer on the regmap_irq_chip_data
  structure in .host_data. I believe this will be a bit tricky.
- Add a function allowing to retrieve ngpio out of the
  gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and
  other places of the driver.

I'm sorry, but I feel like this is a lot of changes to solve this point.
I've been thinking about it, and I can suggest a different solution.

For gpios, I will remove the ngpios property of the device tree and use
a fixed value:
- For the today version of the chip, this is always 8.
- I a chip variant or a similar chip ever arise later with a different
  number of gpios, the fixed value can be set according to the
  "compatible" value.
- This removes any issue with the IRQ setup.

For gpos, we have to keep ngpios, as it depends of the implementation on
the board. That means ngpios will be used:
- For the gpio chip configuration: we let gpiolib retrieve it from the
  device tree.
- In gpio-regmap reg_mask_xlate callback: I can add a function allowing
  to retrieve it from gpio_regmap.gpio_chip, as suggested above.
- In max7360_set_gpos_count() to validate the coherency between
  requested gpios and keypad columns and set the correct configuration
  on the chip:
  - I can also retrieve the value from gpio_regmap.gpio_chip, but that
    means the check is made after the call to
    devm_gpio_regmap_register().
  - Or I will still need to retrieve it using device_property_read_u32()
    here.

How do you feel about this solution?

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote:
> On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
> >
> > ...
> >
> > > > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > > > +		return -ENODEV;
> > > > > > > +	}
> > > > > >
> > > > > > This is not needed, it is already done in GPIOLIB core.
> > > > > 
> > > > > I believe this is still needed:
> > > > > - For gpos, we need the gpio count to correctly set the partition
> > > > >   between gpo and keypad columns in max7360_set_gpos_count().
> > > >
> > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> > > >
> > > > > - For gpios, we need the gpio count to setup the IRQs.
> > > >
> > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > > > and other init callbacks?
> > > 
> > > No, I believe I have to register the IRQ before registering the GPIO, so
> > > I can get the IRQ domain.
> > > 
> > > Right now I have something like:
> > > 
> > > irq_chip->num_irqs = ngpios;
> > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> > > irq, flags, 0, irq_chip, &irq_chip_data);
> > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> > > devm_gpio_regmap_register(dev, &gpio_config);
> > > 
> > > Also, gpiolib will store ngpios in the gpio_chip structure, but while
> > > using gpio-regmap, this structure is masked behind the opaque
> > > gpio_regmap structure. So I believe there is no easy way to retrieve its
> > > value.
> > > 
> > > This part of the code changed a lot, maybe it would be easier if I push
> > > a new version of the series and we continue the discussion there?
> >
> > So, what seems need to be added is some flag to GPIO regmap configuration
> > data structure and a code that is called after gpiochip_add_data() in
> > gpio_regmap_register() to create a domain. This will solve the above issue
> > and helps other drivers to get rid of potential duplication of
> > devm_regmap_add_irq_chip_fwnode() calls.
> >
> > Have you researched this path?
>
> OK, so looking at the code, I believe it would need to:
> - Add some flag in gpio_regmap_config structure, so
>   gpio_regmap_register() creates a new IRQ domain.
> - Add a function allowing to retrieve this domain out of the gpio_regmap
>   structure.
> - Allow to pass a domain in the regmap_irq_chip structure, so
>   regmap_add_irq_chip_fwnode() use this domain instead of calling
>   regmap_irq_create_domain().
> - Make sure this domain is still populated with the IRQ data: number of
>   IRQs, IRQ base but also a pointer on the regmap_irq_chip_data
>   structure in .host_data. I believe this will be a bit tricky.
> - Add a function allowing to retrieve ngpio out of the
>   gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and
>   other places of the driver.
>
> I'm sorry, but I feel like this is a lot of changes to solve this point.
> I've been thinking about it, and I can suggest a different solution.
>
> For gpios, I will remove the ngpios property of the device tree and use
> a fixed value:
> - For the today version of the chip, this is always 8.
> - I a chip variant or a similar chip ever arise later with a different
>   number of gpios, the fixed value can be set according to the
>   "compatible" value.
> - This removes any issue with the IRQ setup.
>
> For gpos, we have to keep ngpios, as it depends of the implementation on
> the board. That means ngpios will be used:
> - For the gpio chip configuration: we let gpiolib retrieve it from the
>   device tree.
> - In gpio-regmap reg_mask_xlate callback: I can add a function allowing
>   to retrieve it from gpio_regmap.gpio_chip, as suggested above.
> - In max7360_set_gpos_count() to validate the coherency between
>   requested gpios and keypad columns and set the correct configuration
>   on the chip:
>   - I can also retrieve the value from gpio_regmap.gpio_chip, but that
>     means the check is made after the call to
>     devm_gpio_regmap_register().
>   - Or I will still need to retrieve it using device_property_read_u32()
>     here.
>
> How do you feel about this solution?

Actually there is an additional issue: today, relying on gpiolib to
parse the "ngpios" property does not work with gpio-regmap.

The gpiochip_get_ngpios() function in gpiolib is called in
gpiochip_add_data_with_key(), but when using gpio_regmap_register(),
we first ensure ngpio is set correctly before calling anything.

Yet I believe this check can safely be removed, allowing the magic in
gpiolib happen as expected.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Thu, Feb 13, 2025 at 02:45:31PM +0100, Mathieu Dubois-Briand wrote:
> On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote:
> > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote:
> > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:

...

> > > > > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > > > > +		return -ENODEV;
> > > > > > > > +	}
> > > > > > >
> > > > > > > This is not needed, it is already done in GPIOLIB core.
> > > > > > 
> > > > > > I believe this is still needed:
> > > > > > - For gpos, we need the gpio count to correctly set the partition
> > > > > >   between gpo and keypad columns in max7360_set_gpos_count().
> > > > >
> > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> > > > >
> > > > > > - For gpios, we need the gpio count to setup the IRQs.
> > > > >
> > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > > > > and other init callbacks?
> > > > 
> > > > No, I believe I have to register the IRQ before registering the GPIO, so
> > > > I can get the IRQ domain.
> > > > 
> > > > Right now I have something like:
> > > > 
> > > > irq_chip->num_irqs = ngpios;
> > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> > > > irq, flags, 0, irq_chip, &irq_chip_data);
> > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> > > > devm_gpio_regmap_register(dev, &gpio_config);
> > > > 
> > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while
> > > > using gpio-regmap, this structure is masked behind the opaque
> > > > gpio_regmap structure. So I believe there is no easy way to retrieve its
> > > > value.

Would it be needed in your driver ->probe() after all? (See also below)

> > > > This part of the code changed a lot, maybe it would be easier if I push
> > > > a new version of the series and we continue the discussion there?
> > >
> > > So, what seems need to be added is some flag to GPIO regmap configuration
> > > data structure and a code that is called after gpiochip_add_data() in
> > > gpio_regmap_register() to create a domain. This will solve the above issue
> > > and helps other drivers to get rid of potential duplication of
> > > devm_regmap_add_irq_chip_fwnode() calls.
> > >
> > > Have you researched this path?
> >
> > OK, so looking at the code, I believe it would need to:
> > - Add some flag in gpio_regmap_config structure, so
> >   gpio_regmap_register() creates a new IRQ domain.

Easy.

> > - Add a function allowing to retrieve this domain out of the gpio_regmap
> >   structure.

Easy, as there is an API available for regmaps, so it looks like one-liner.

> > - Allow to pass a domain in the regmap_irq_chip structure, so
> >   regmap_add_irq_chip_fwnode() use this domain instead of calling
> >   regmap_irq_create_domain().

You need this because of...? (Please, remind me what the obstacle is there
that requires this to be done)

> > - Make sure this domain is still populated with the IRQ data: number of
> >   IRQs, IRQ base but also a pointer on the regmap_irq_chip_data
> >   structure in .host_data. I believe this will be a bit tricky.

Hmm... But wouldn't gpio-regmap internals have all this information available?

> > - Add a function allowing to retrieve ngpio out of the
> >   gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and
> >   other places of the driver.

I'm not sure where it can be needed.

> > I'm sorry, but I feel like this is a lot of changes to solve this point.
> > I've been thinking about it, and I can suggest a different solution.
> >
> > For gpios, I will remove the ngpios property of the device tree and use
> > a fixed value:
> > - For the today version of the chip, this is always 8.
> > - I a chip variant or a similar chip ever arise later with a different
> >   number of gpios, the fixed value can be set according to the
> >   "compatible" value.
> > - This removes any issue with the IRQ setup.
> >
> > For gpos, we have to keep ngpios, as it depends of the implementation on
> > the board. That means ngpios will be used:
> > - For the gpio chip configuration: we let gpiolib retrieve it from the
> >   device tree.
> > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing
> >   to retrieve it from gpio_regmap.gpio_chip, as suggested above.
> > - In max7360_set_gpos_count() to validate the coherency between
> >   requested gpios and keypad columns and set the correct configuration
> >   on the chip:
> >   - I can also retrieve the value from gpio_regmap.gpio_chip, but that
> >     means the check is made after the call to
> >     devm_gpio_regmap_register().
> >   - Or I will still need to retrieve it using device_property_read_u32()
> >     here.
> >
> > How do you feel about this solution?
> 
> Actually there is an additional issue: today, relying on gpiolib to
> parse the "ngpios" property does not work with gpio-regmap.
> 
> The gpiochip_get_ngpios() function in gpiolib is called in
> gpiochip_add_data_with_key(), but when using gpio_regmap_register(),
> we first ensure ngpio is set correctly before calling anything.
> 
> Yet I believe this check can safely be removed, allowing the magic in
> gpiolib happen as expected.

Not really. I'm about to send a series to address this issue.
Please, test.

...

P.S.
Maybe it's time to send a new version based on this discussion even
if not finished / working, so we can see the exact issues we still have
and target them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Thu Feb 13, 2025 at 8:47 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 13, 2025 at 02:45:31PM +0100, Mathieu Dubois-Briand wrote:
> > On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote:
> > > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> > > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > > > > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > > > > > +		return -ENODEV;
> > > > > > > > > +	}
> > > > > > > >
> > > > > > > > This is not needed, it is already done in GPIOLIB core.
> > > > > > > 
> > > > > > > I believe this is still needed:
> > > > > > > - For gpos, we need the gpio count to correctly set the partition
> > > > > > >   between gpo and keypad columns in max7360_set_gpos_count().
> > > > > >
> > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> > > > > >
> > > > > > > - For gpios, we need the gpio count to setup the IRQs.
> > > > > >
> > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > > > > > and other init callbacks?
> > > > > 
> > > > > No, I believe I have to register the IRQ before registering the GPIO, so
> > > > > I can get the IRQ domain.
> > > > > 
> > > > > Right now I have something like:
> > > > > 
> > > > > irq_chip->num_irqs = ngpios;
> > > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> > > > > irq, flags, 0, irq_chip, &irq_chip_data);
> > > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> > > > > devm_gpio_regmap_register(dev, &gpio_config);
> > > > > 
> > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while
> > > > > using gpio-regmap, this structure is masked behind the opaque
> > > > > gpio_regmap structure. So I believe there is no easy way to retrieve its
> > > > > value.
>
> Would it be needed in your driver ->probe() after all? (See also below)
>

No necessarily in the probe with the changes previously described, but I
will need it in other functions. So either I get it in the probe and
store it, or I will need to retrieve it by other means.

> > > > > This part of the code changed a lot, maybe it would be easier if I push
> > > > > a new version of the series and we continue the discussion there?
> > > >
> > > > So, what seems need to be added is some flag to GPIO regmap configuration
> > > > data structure and a code that is called after gpiochip_add_data() in
> > > > gpio_regmap_register() to create a domain. This will solve the above issue
> > > > and helps other drivers to get rid of potential duplication of
> > > > devm_regmap_add_irq_chip_fwnode() calls.
> > > >
> > > > Have you researched this path?
> > >
> > > OK, so looking at the code, I believe it would need to:
> > > - Add some flag in gpio_regmap_config structure, so
> > >   gpio_regmap_register() creates a new IRQ domain.
>
> Easy.
>
> > > - Add a function allowing to retrieve this domain out of the gpio_regmap
> > >   structure.
>
> Easy, as there is an API available for regmaps, so it looks like one-liner.
>
> > > - Allow to pass a domain in the regmap_irq_chip structure, so
> > >   regmap_add_irq_chip_fwnode() use this domain instead of calling
> > >   regmap_irq_create_domain().
>
> You need this because of...? (Please, remind me what the obstacle is there
> that requires this to be done)
>

OK, maybe I misunderstood you idea. Or maybe I misunderstood something
about IRQ domains.

So what I understood is, in the probe, we first call
gpio_regmap_register(), that will create a new IRQ domain, and we call
regmap_add_irq_chip_fwnode() in second. But this second call will again
create an IRQ domain, so we would end-up with a second one. We have to
prevent this second creation and make it use the one we created first.

Did I miss something?

> > > - Make sure this domain is still populated with the IRQ data: number of
> > >   IRQs, IRQ base but also a pointer on the regmap_irq_chip_data
> > >   structure in .host_data. I believe this will be a bit tricky.
>
> Hmm... But wouldn't gpio-regmap internals have all this information available?
>

I don't think so. It will not know the IRQ base nor the
regmap_irq_chip_data as it is not created at this point.

> > > - Add a function allowing to retrieve ngpio out of the
> > >   gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and
> > >   other places of the driver.
>
> I'm not sure where it can be needed.
>

Let's discuss this on the next version, but yes, I do need to know ngpio
in the driver.

> > > I'm sorry, but I feel like this is a lot of changes to solve this point.
> > > I've been thinking about it, and I can suggest a different solution.
> > >
> > > For gpios, I will remove the ngpios property of the device tree and use
> > > a fixed value:
> > > - For the today version of the chip, this is always 8.
> > > - I a chip variant or a similar chip ever arise later with a different
> > >   number of gpios, the fixed value can be set according to the
> > >   "compatible" value.
> > > - This removes any issue with the IRQ setup.
> > >
> > > For gpos, we have to keep ngpios, as it depends of the implementation on
> > > the board. That means ngpios will be used:
> > > - For the gpio chip configuration: we let gpiolib retrieve it from the
> > >   device tree.
> > > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing
> > >   to retrieve it from gpio_regmap.gpio_chip, as suggested above.
> > > - In max7360_set_gpos_count() to validate the coherency between
> > >   requested gpios and keypad columns and set the correct configuration
> > >   on the chip:
> > >   - I can also retrieve the value from gpio_regmap.gpio_chip, but that
> > >     means the check is made after the call to
> > >     devm_gpio_regmap_register().
> > >   - Or I will still need to retrieve it using device_property_read_u32()
> > >     here.
> > >
> > > How do you feel about this solution?
> > 
> > Actually there is an additional issue: today, relying on gpiolib to
> > parse the "ngpios" property does not work with gpio-regmap.
> > 
> > The gpiochip_get_ngpios() function in gpiolib is called in
> > gpiochip_add_data_with_key(), but when using gpio_regmap_register(),
> > we first ensure ngpio is set correctly before calling anything.
> > 
> > Yet I believe this check can safely be removed, allowing the magic in
> > gpiolib happen as expected.
>
> Not really. I'm about to send a series to address this issue.
> Please, test.

I will test it today.

>
> ...
>
> P.S.
> Maybe it's time to send a new version based on this discussion even
> if not finished / working, so we can see the exact issues we still have
> and target them.

Sure, I will send a new version, probably today.

Thanks again for your help.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 5/7] input: keyboard: Add support for MAX7360 keypad

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 keypad controller, providing
support for up to 64 keys, with a matrix of 8 columns and 8 rows.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/input/keyboard/Kconfig          |  12 ++
 drivers/input/keyboard/Makefile         |   1 +
 drivers/input/keyboard/max7360-keypad.c | 284 ++++++++++++++++++++++++++++++++
 3 files changed, 297 insertions(+)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 721ab69e84ac..bba029f65cfa 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -421,6 +421,18 @@ config KEYBOARD_MAX7359
 	  To compile this driver as a module, choose M here: the
 	  module will be called max7359_keypad.
 
+config KEYBOARD_MAX7360
+	tristate "Maxim MAX7360 Key Switch Controller"
+	select INPUT_MATRIXKMAP
+	depends on I2C
+	depends on MFD_MAX7360
+	help
+	  If you say yes here you get support for the keypad controller on the
+	  Maxim MAX7360 I/O Expander.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called max7360_keypad.
+
 config KEYBOARD_MPR121
 	tristate "Freescale MPR121 Touchkey"
 	depends on I2C
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 1e0721c30709..b49d32d4003d 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_KEYBOARD_LPC32XX)		+= lpc32xx-keys.o
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
 obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
+obj-$(CONFIG_KEYBOARD_MAX7360)		+= max7360-keypad.o
 obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
 obj-$(CONFIG_KEYBOARD_MT6779)		+= mt6779-keypad.o
 obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
diff --git a/drivers/input/keyboard/max7360-keypad.c b/drivers/input/keyboard/max7360-keypad.c
new file mode 100644
index 000000000000..f42a0663a6ab
--- /dev/null
+++ b/drivers/input/keyboard/max7360-keypad.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct max7360_keypad {
+	struct input_dev *input;
+	unsigned int rows;
+	unsigned int cols;
+	unsigned int debounce_ms;
+	int irq;
+	struct regmap *regmap;
+	unsigned short keycodes[MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS];
+};
+
+static irqreturn_t max7360_keypad_irq(int irq, void *data)
+{
+	struct max7360_keypad *max7360_keypad = data;
+	unsigned int val;
+	unsigned int row, col;
+	unsigned int release;
+	unsigned int code;
+	int error;
+
+	do {
+		error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
+				    &val);
+		if (error) {
+			dev_err(&max7360_keypad->input->dev,
+				"Failed to read max7360 FIFO");
+			return IRQ_NONE;
+		}
+
+		/* FIFO overflow: ignore it and get next event. */
+		if (val == MAX7360_FIFO_OVERFLOW)
+			dev_warn(&max7360_keypad->input->dev,
+				 "max7360 FIFO overflow");
+	} while (val == MAX7360_FIFO_OVERFLOW);
+
+	if (val == MAX7360_FIFO_EMPTY) {
+		dev_dbg(&max7360_keypad->input->dev,
+			"Got a spurious interrupt");
+
+		return IRQ_NONE;
+	}
+
+	row = FIELD_GET(MAX7360_FIFO_ROW, val);
+	col = FIELD_GET(MAX7360_FIFO_COL, val);
+	release = val & MAX7360_FIFO_RELEASE;
+
+	code = MATRIX_SCAN_CODE(row, col, MAX7360_ROW_SHIFT);
+
+	dev_dbg(&max7360_keypad->input->dev,
+		"key[%d:%d] %s\n", row, col, release ? "release" : "press");
+
+	input_event(max7360_keypad->input, EV_MSC, MSC_SCAN, code);
+	input_report_key(max7360_keypad->input, max7360_keypad->keycodes[code],
+			 !release);
+	input_sync(max7360_keypad->input);
+
+	return IRQ_HANDLED;
+}
+
+static int max7360_keypad_open(struct input_dev *pdev)
+{
+	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
+	int error;
+
+	/*
+	 * Somebody is using the device: get out of sleep.
+	 */
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
+				  MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
+	if (error) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration\n");
+		return error;
+	}
+
+	return 0;
+}
+
+static void max7360_keypad_close(struct input_dev *pdev)
+{
+	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
+	int error;
+
+	/*
+	 * Nobody is using the device anymore: go to sleep.
+	 */
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
+				  MAX7360_CFG_SLEEP, ~MAX7360_CFG_SLEEP);
+	if (error)
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration\n");
+}
+
+static int max7360_keypad_hw_init(struct max7360_keypad *max7360_keypad)
+{
+	unsigned int val;
+	int error;
+
+	val = max7360_keypad->debounce_ms - MAX7360_DEBOUNCE_MIN;
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_DEBOUNCE,
+				  MAX7360_DEBOUNCE,
+				  FIELD_PREP(MAX7360_DEBOUNCE, val));
+	if (error) {
+		return dev_err_probe(&max7360_keypad->input->dev, error,
+			"Failed to write max7360 debounce configuration\n");
+	}
+
+	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
+				  MAX7360_INTERRUPT_TIME_MASK,
+				  FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
+	if (error) {
+		return dev_err_probe(&max7360_keypad->input->dev, error,
+			"Failed to write max7360 keypad interrupt configuration\n");
+	}
+
+	return 0;
+}
+
+static int max7360_keypad_parse_dt(struct platform_device *pdev,
+				   struct max7360_keypad *max7360_keypad,
+				   bool *autorepeat)
+{
+	int error;
+
+	error = matrix_keypad_parse_properties(pdev->dev.parent,
+					       &max7360_keypad->rows,
+					       &max7360_keypad->cols);
+	if (error)
+		return error;
+
+	if (!max7360_keypad->rows || !max7360_keypad->cols ||
+	    max7360_keypad->rows > MAX7360_MAX_KEY_ROWS ||
+	    max7360_keypad->cols > MAX7360_MAX_KEY_COLS) {
+		dev_err(&pdev->dev,
+			"Invalid number of columns or rows (%ux%u)\n",
+			max7360_keypad->cols, max7360_keypad->rows);
+		return -EINVAL;
+	}
+
+	*autorepeat = device_property_read_bool(pdev->dev.parent, "autorepeat");
+
+	max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN;
+	error = device_property_read_u32(pdev->dev.parent,
+					 "keypad-debounce-delay-ms",
+					 &max7360_keypad->debounce_ms);
+	if (error == -EINVAL) {
+		dev_info(&pdev->dev, "Using default keypad-debounce-delay-ms: %u\n",
+			 max7360_keypad->debounce_ms);
+	} else if (error < 0) {
+		dev_err(&pdev->dev,
+			"Failed to read keypad-debounce-delay-ms property\n");
+		return error;
+	} else if (max7360_keypad->debounce_ms < MAX7360_DEBOUNCE_MIN ||
+		   max7360_keypad->debounce_ms > MAX7360_DEBOUNCE_MAX) {
+		dev_err(&pdev->dev,
+			"Invalid keypad-debounce-delay-ms: %u, should be between %u and %u.\n",
+			max7360_keypad->debounce_ms, MAX7360_DEBOUNCE_MIN,
+			MAX7360_DEBOUNCE_MAX);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int max7360_keypad_probe(struct platform_device *pdev)
+{
+	struct max7360_keypad *max7360_keypad;
+	struct input_dev *input;
+	bool autorepeat;
+	int error;
+	int irq;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
+
+	irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent),
+				      "intk");
+	if (irq < 0)
+		return irq;
+
+	max7360_keypad = devm_kzalloc(&pdev->dev, sizeof(*max7360_keypad),
+				      GFP_KERNEL);
+	if (!max7360_keypad)
+		return -ENOMEM;
+
+	max7360_keypad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_keypad->regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "Could not get parent regmap\n");
+
+	error = max7360_keypad_parse_dt(pdev, max7360_keypad, &autorepeat);
+	if (error)
+		return error;
+
+	input = devm_input_allocate_device(pdev->dev.parent);
+	if (!input)
+		return -ENOMEM;
+
+	max7360_keypad->input = input;
+
+	input->id.bustype = BUS_I2C;
+	input->name = pdev->name;
+	input->open = max7360_keypad_open;
+	input->close = max7360_keypad_close;
+
+	error = matrix_keypad_build_keymap(NULL, NULL,
+					   MAX7360_MAX_KEY_ROWS,
+					   MAX7360_MAX_KEY_COLS,
+					   max7360_keypad->keycodes, input);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to build keymap\n");
+
+	input_set_capability(input, EV_MSC, MSC_SCAN);
+	if (autorepeat)
+		__set_bit(EV_REP, input->evbit);
+
+	input_set_drvdata(input, max7360_keypad);
+
+	error = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					  max7360_keypad_irq,
+					  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					  "max7360-keypad", max7360_keypad);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to register interrupt\n");
+
+	error = input_register_device(input);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Could not register input device\n");
+
+	platform_set_drvdata(pdev, max7360_keypad);
+
+	error = max7360_keypad_hw_init(max7360_keypad);
+	if (error)
+		return dev_err_probe(&pdev->dev, error,
+				     "Failed to initialize max7360 keypad\n");
+
+	device_init_wakeup(&pdev->dev, true);
+	error = dev_pm_set_wake_irq(&pdev->dev, irq);
+	if (error)
+		dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n",
+			 error);
+
+	return 0;
+}
+
+static void max7360_keypad_remove(struct platform_device *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+}
+
+static struct platform_driver max7360_keypad_driver = {
+	.driver = {
+		.name	= MAX7360_DRVNAME_KEYPAD,
+	},
+	.probe		= max7360_keypad_probe,
+	.remove		= max7360_keypad_remove,
+};
+module_platform_driver(max7360_keypad_driver);
+
+MODULE_DESCRIPTION("MAX7360 Keypad driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_ALIAS("platform:" MAX7360_DRVNAME_KEYPAD);
+MODULE_LICENSE("GPL");

-- 
2.39.5

[PATCH v3 6/7] input: misc: Add support for MAX7360 rotary

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 rotary encoder controller,
supporting a single rotary switch.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/input/misc/Kconfig          |  11 +++
 drivers/input/misc/Makefile         |   1 +
 drivers/input/misc/max7360-rotary.c | 183 ++++++++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6a852c76331b..8430aaf08c04 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -230,6 +230,17 @@ config INPUT_M68K_BEEP
 	tristate "M68k Beeper support"
 	depends on M68K
 
+config INPUT_MAX7360_ROTARY
+	tristate "Maxim MAX7360 Rotary Encoder"
+	depends on I2C
+	depends on MFD_MAX7360
+	help
+	  If you say yes here you get support for the rotary encoder on the
+	  Maxim MAX7360 I/O Expander.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called max7360_rotary.
+
 config INPUT_MAX77650_ONKEY
 	tristate "Maxim MAX77650 ONKEY support"
 	depends on MFD_MAX77650
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4f7f736831ba..0ed447543e43 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_INPUT_IQS7222)		+= iqs7222.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
 obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
 obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
+obj-$(CONFIG_INPUT_MAX7360_ROTARY)	+= max7360-rotary.o
 obj-$(CONFIG_INPUT_MAX77650_ONKEY)	+= max77650-onkey.o
 obj-$(CONFIG_INPUT_MAX77693_HAPTIC)	+= max77693-haptic.o
 obj-$(CONFIG_INPUT_MAX8925_ONKEY)	+= max8925_onkey.o
diff --git a/drivers/input/misc/max7360-rotary.c b/drivers/input/misc/max7360-rotary.c
new file mode 100644
index 000000000000..e5bab58038f8
--- /dev/null
+++ b/drivers/input/misc/max7360-rotary.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct max7360_rotary {
+	u32 axis;
+	struct input_dev *input;
+	unsigned int debounce_ms;
+	struct regmap *regmap;
+};
+
+static irqreturn_t max7360_rotary_irq(int irq, void *data)
+{
+	struct max7360_rotary *max7360_rotary = data;
+	int val;
+	int ret;
+
+	ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val);
+	if (ret < 0) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to read rotary counter\n");
+		return IRQ_NONE;
+	}
+
+	if (val == 0) {
+		dev_dbg(&max7360_rotary->input->dev,
+			"Got a spurious interrupt\n");
+
+		return IRQ_NONE;
+	}
+
+	input_report_rel(max7360_rotary->input, max7360_rotary->axis,
+			 (int8_t)val);
+	input_sync(max7360_rotary->input);
+
+	return IRQ_HANDLED;
+}
+
+static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary)
+{
+	int val;
+	int ret;
+
+	ret = regmap_write_bits(max7360_rotary->regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_RTR_EN,
+				MAX7360_GPIO_CFG_RTR_EN);
+	if (ret) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to enable max7360 rotary encoder\n");
+		return ret;
+	}
+
+	val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) |
+		FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY;
+	ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val);
+	if (ret) {
+		dev_err(&max7360_rotary->input->dev,
+			"Failed to set max7360 rotary encoder configuration\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_rotary_probe(struct platform_device *pdev)
+{
+	struct max7360_rotary *max7360_rotary;
+	struct input_dev *input;
+	int irq;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
+
+	ret = max7360_port_pin_request(pdev->dev.parent, MAX7360_PORT_RTR_PIN,
+				       true);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not request rotary pin\n");
+
+	irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent),
+				      "inti");
+	if (irq < 0)
+		return irq;
+
+	max7360_rotary = devm_kzalloc(&pdev->dev, sizeof(*max7360_rotary),
+				      GFP_KERNEL);
+	if (!max7360_rotary)
+		return -ENOMEM;
+
+	max7360_rotary->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_rotary->regmap)
+		dev_err_probe(&pdev->dev, -ENODEV,
+			      "Could not get parent regmap\n");
+
+	device_property_read_u32(pdev->dev.parent, "linux,axis",
+				 &max7360_rotary->axis);
+	device_property_read_u32(pdev->dev.parent, "rotary-debounce-delay-ms",
+				 &max7360_rotary->debounce_ms);
+	if (max7360_rotary->debounce_ms > MAX7360_ROT_DEBOUNCE_MAX)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Invalid debounce timing: %u\n",
+				     max7360_rotary->debounce_ms);
+
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input)
+		return -ENOMEM;
+
+	max7360_rotary->input = input;
+
+	input->id.bustype = BUS_I2C;
+	input->name = pdev->name;
+	input->dev.parent = &pdev->dev;
+
+	input_set_capability(input, EV_REL, max7360_rotary->axis);
+	input_set_drvdata(input, max7360_rotary);
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					max7360_rotary_irq,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
+					"max7360-rotary", max7360_rotary);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to register interrupt\n");
+
+	ret = input_register_device(input);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not register input device\n");
+
+	platform_set_drvdata(pdev, max7360_rotary);
+
+	device_init_wakeup(&pdev->dev, true);
+	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
+	if (ret)
+		dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n", ret);
+
+	ret = max7360_rotary_hw_init(max7360_rotary);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to initialize max7360 rotary\n");
+
+	return 0;
+}
+
+static void max7360_rotary_remove(struct platform_device *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+
+	/*
+	 * Free the MAX7360_PORT_RTR_PIN pin, so it can be requested later by
+	 * this driver, the MAX7360 GPIO driver or the MAX7360 PWM driver.
+	 */
+	max7360_port_pin_request(pdev->dev.parent, MAX7360_PORT_RTR_PIN, false);
+}
+
+static struct platform_driver max7360_rotary_driver = {
+	.driver = {
+		.name	= MAX7360_DRVNAME_ROTARY,
+	},
+	.probe		= max7360_rotary_probe,
+	.remove		= max7360_rotary_remove,
+};
+module_platform_driver(max7360_rotary_driver);
+
+MODULE_DESCRIPTION("MAX7360 Rotary driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_ALIAS("platform:" MAX7360_DRVNAME_ROTARY);
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v3 6/7] input: misc: Add support for MAX7360 rotary

[Mathieu Dubois-Briand]

On Mon Jan 13, 2025 at 1:42 PM CET, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 rotary encoder controller,
> supporting a single rotary switch.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
...
> +static irqreturn_t max7360_rotary_irq(int irq, void *data)
> +{
> +	struct max7360_rotary *max7360_rotary = data;
> +	int val;
> +	int ret;
> +
> +	ret = regmap_read(max7360_rotary->regmap, MAX7360_REG_RTR_CNT, &val);
> +	if (ret < 0) {
> +		dev_err(&max7360_rotary->input->dev,
> +			"Failed to read rotary counter\n");
> +		return IRQ_NONE;
> +	}
> +
> +	if (val == 0) {
> +		dev_dbg(&max7360_rotary->input->dev,
> +			"Got a spurious interrupt\n");
> +
> +		return IRQ_NONE;
> +	}
> +
> +	input_report_rel(max7360_rotary->input, max7360_rotary->axis,
> +			 (int8_t)val);
> +	input_sync(max7360_rotary->input);
> +

I have a question about the type of events reported here.

I used rotary_encoder.c as a reference, so I'm reporting some EV_REL
events on a given axis, such as REL_X.

On the other hand, I know there is an out-of-tree version of this
driver that is instead reporting key events, such as KEY_DOWN or KEY_UP.
I also know there are existing applications that do rely on this
behaviour.

So my question is, what is the best kind of events to report here ?
Is there any guideline that do apply here? Should I better try to mimic
the behaviour of the existing out-of-tree driver, or should I mimic the
behaviour of rotary_encoder.c, so we have a similar behaviour for all
in-kernel rotary encoder drivers?

> +	return IRQ_HANDLED;
> +}
> +

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 7/7] MAINTAINERS: Add entry on MAX7360 driver

[Mathieu Dubois-Briand]

Add myself as maintainer of Maxim MAX7360 driver and device-tree bindings.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index baf0eeb9a355..18a7b7cf0b60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14132,6 +14132,18 @@ L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	drivers/iio/temperature/max30208.c
 
+MAXIM MAX7360 KEYPAD LED MFD DRIVER
+M:	Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
+F:	Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
+F:	drivers/gpio/gpio-max7360.c
+F:	drivers/input/keyboard/max7360-keypad.c
+F:	drivers/input/misc/max7360-rotary.c
+F:	drivers/mfd/max7360.c
+F:	drivers/pwm/pwm-max7360.c
+F:	include/linux/mfd/max7360.h
+
 MAXIM MAX77650 PMIC MFD DRIVER
 M:	Bartosz Golaszewski <brgl@bgdev.pl>
 L:	linux-kernel@vger.kernel.org

-- 
2.39.5