[PATCH v5 00/11] Add support for MAX7360

[PATCH v5 00/11] 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 v5:
- Add pinctrl driver to replace the previous use of request()/free()
  callbacks for PORT pins.
- Remove ngpios property from GPIO device tree bindings.
- Use GPIO valid_mask to mark unusable keypad columns GPOs, instead of
  changing ngpios.
- Drop patches adding support for request()/free() callbacks in GPIO
  regmap and gpio_regmap_get_ngpio().
- Allow gpio_regmap_register() to create the associated regmap IRQ.
- Various fixes in MFD, PWM, GPIO and KEYPAD drivers.
- Link to v4: https://lore.kernel.org/r/20250214-mdb-max7360-support-v4-0-8a35c6dbb966@bootlin.com

Changes in v4:
- Modified the GPIO driver to use gpio-regmap and regmap-irq.
- Add support for request()/free() callbacks in gpio-regmap.
- Add support for status_is_level in regmap-irq.
- Switched the PWM driver to waveform callbacks.
- Various small fixes in MFD, PWM, GPIO drivers and dt bindings.
- Rebased on v6.14-rc2.
- Link to v3: https://lore.kernel.org/r/20250113-mdb-max7360-support-v3-0-9519b4acb0b1@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 (9):
      dt-bindings: mfd: gpio: Add MAX7360
      pinctrl: Add MAX7360 pinctrl driver
      regmap: irq: Add support for chips without separate IRQ status
      gpio: regmap: Allow to allocate regmap-irq device
      gpio: regmap: Allow to provide init_valid_mask callback
      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          |  83 +++++++
 .../devicetree/bindings/mfd/maxim,max7360.yaml     | 170 +++++++++++++
 MAINTAINERS                                        |  13 +
 drivers/base/regmap/regmap-irq.c                   |  97 +++++---
 drivers/gpio/Kconfig                               |  12 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-max7360.c                        | 246 +++++++++++++++++++
 drivers/gpio/gpio-regmap.c                         |  26 +-
 drivers/input/keyboard/Kconfig                     |  12 +
 drivers/input/keyboard/Makefile                    |   1 +
 drivers/input/keyboard/max7360-keypad.c            | 264 +++++++++++++++++++++
 drivers/input/misc/Kconfig                         |  11 +
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/max7360-rotary.c                | 161 +++++++++++++
 drivers/mfd/Kconfig                                |  14 ++
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/max7360.c                              | 185 +++++++++++++++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-max7360.c                  | 195 +++++++++++++++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-max7360.c                          | 194 +++++++++++++++
 include/linux/gpio/regmap.h                        |  22 ++
 include/linux/mfd/max7360.h                        | 112 +++++++++
 include/linux/regmap.h                             |   3 +
 26 files changed, 1813 insertions(+), 35 deletions(-)
---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20241219-mdb-max7360-support-223a8ce45ba3

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

[PATCH v5 01/11] 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          |  83 ++++++++++
 .../devicetree/bindings/mfd/maxim,max7360.yaml     | 170 +++++++++++++++++++++
 2 files changed, 253 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..21d603d9504c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
@@ -0,0 +1,83 @@
+# 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.
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-gpio
+      - maxim,max7360-gpo
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  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.
+    maximum: 0xff
+
+required:
+  - compatible
+  - gpio-controller
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max7360-gpio
+        ngpios: false
+    then:
+      required:
+        - interrupt-controller
+    else:
+      properties:
+        interrupt-controller: false
+        maxim,constant-current-disable: false
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+      compatible = "maxim,max7360-gpio";
+
+      gpio-controller;
+      #gpio-cells = <2>;
+      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..d3c09531dc5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
@@ -0,0 +1,170 @@
+# 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
+
+  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. GPIOs
+      numbers used for this gpio-controller node do correspond to the column
+      numbers: values 0 and 1 are never valid, values from 2 to 7 might be valid
+      depending on the value of the keypad,num-column property.
+
+patternProperties:
+  '-pins$':
+    type: object
+    description:
+      Pinctrl node's client devices use subnodes for desired pin configuration.
+      Client device subnodes use below standard properties.
+    $ref: /schemas/pinctrl/pincfg-node.yaml
+
+    properties:
+      pins:
+        description:
+          List of gpio pins affected by the properties specified in this
+          subnode.
+        items:
+          pattern: '^PORT[0-7]|ROTARY$'
+        minItems: 1
+        maxItems: 8
+
+      function:
+        description:
+          Specify the alternative function to be configured for the specified
+          pins.
+        enum: [gpio, pwm, rotary]
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - linux,keymap
+  - linux,axis
+  - "#pwm-cells"
+  - gpio
+  - gpo
+
+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>;
+          maxim,constant-current-disable = <0x06>;
+
+          interrupt-controller;
+          #interrupt-cells = <0x2>;
+        };
+
+        max7360_gpo: gpo {
+          compatible = "maxim,max7360-gpo";
+
+          gpio-controller;
+          #gpio-cells = <2>;
+        };
+
+        backlight_pins: backlight-pins {
+          pins = "PORT2";
+          function = "pwm";
+        };
+      };
+    };

-- 
2.39.5

Re: [PATCH v5 01/11] dt-bindings: mfd: gpio: Add MAX7360

[Rob Herring]

On Tue, Mar 18, 2025 at 05:26:17PM +0100, Mathieu Dubois-Briand wrote:
> 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          |  83 ++++++++++
>  .../devicetree/bindings/mfd/maxim,max7360.yaml     | 170 +++++++++++++++++++++
>  2 files changed, 253 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..21d603d9504c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
> @@ -0,0 +1,83 @@
> +# 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.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-gpio
> +      - maxim,max7360-gpo
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  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.
> +    maximum: 0xff
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - maxim,max7360-gpio
> +        ngpios: false
> +    then:
> +      required:
> +        - interrupt-controller
> +    else:
> +      properties:
> +        interrupt-controller: false
> +        maxim,constant-current-disable: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio {
> +      compatible = "maxim,max7360-gpio";
> +
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      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..d3c09531dc5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
> @@ -0,0 +1,170 @@
> +# 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:

The existing debounce-delay-ms or poll-interval properties don't work 
for you?

> +    description: Keypad debounce delay in ms
> +    minimum: 9
> +    maximum: 40
> +    default: 9
> +
> +  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.

You should have a $ref to rotary-encoder.yaml too. None of the other 
properties in it are needed? 

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

Are these paragraphs? If so, add a blank line between paragraphs. If 
not, re-wrap the lines.

> +      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. GPIOs
> +      numbers used for this gpio-controller node do correspond to the column
> +      numbers: values 0 and 1 are never valid, values from 2 to 7 might be valid
> +      depending on the value of the keypad,num-column property.
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    description:
> +      Pinctrl node's client devices use subnodes for desired pin configuration.
> +      Client device subnodes use below standard properties.
> +    $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +    properties:
> +      pins:
> +        description:
> +          List of gpio pins affected by the properties specified in this
> +          subnode.
> +        items:
> +          pattern: '^PORT[0-7]|ROTARY$'

Don't you need ()?:

^(PORT[0-7]|ROTARY)$'

> +        minItems: 1
> +        maxItems: 8
> +
> +      function:
> +        description:
> +          Specify the alternative function to be configured for the specified
> +          pins.
> +        enum: [gpio, pwm, rotary]
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - linux,keymap
> +  - linux,axis
> +  - "#pwm-cells"
> +  - gpio
> +  - gpo
> +
> +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>;
> +          maxim,constant-current-disable = <0x06>;
> +
> +          interrupt-controller;
> +          #interrupt-cells = <0x2>;
> +        };
> +
> +        max7360_gpo: gpo {
> +          compatible = "maxim,max7360-gpo";
> +
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +        };
> +
> +        backlight_pins: backlight-pins {
> +          pins = "PORT2";
> +          function = "pwm";
> +        };
> +      };
> +    };
> 
> -- 
> 2.39.5
> 

Re: [PATCH v5 01/11] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

On Tue Mar 18, 2025 at 6:39 PM CET, Rob Herring wrote:
> On Tue, Mar 18, 2025 at 05:26:17PM +0100, Mathieu Dubois-Briand wrote:
> > 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          |  83 ++++++++++
> >  .../devicetree/bindings/mfd/maxim,max7360.yaml     | 170 +++++++++++++++++++++
> >  2 files changed, 253 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..21d603d9504c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

...

> > +
> > +  keypad-debounce-delay-ms:
>
> The existing debounce-delay-ms or poll-interval properties don't work 
> for you?
>

The issue is this node also describes the rotary encoder (just below),
so I feel using only debounce-delay-ms is a bit misleading.

> > +    description: Keypad debounce delay in ms
> > +    minimum: 9
> > +    maximum: 40
> > +    default: 9
> > +
> > +  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.
>
> You should have a $ref to rotary-encoder.yaml too. None of the other 
> properties in it are needed? 

Makes sense, thanks!

And no, I believe this is the only property we need.

>
> > +
> > +  "#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.
>
> Are these paragraphs? If so, add a blank line between paragraphs. If 
> not, re-wrap the lines.
>

OK

> > +      The MAX7360 has 8 column lines and 6 of them can be used as GPOs. GPIOs
> > +      numbers used for this gpio-controller node do correspond to the column
> > +      numbers: values 0 and 1 are never valid, values from 2 to 7 might be valid
> > +      depending on the value of the keypad,num-column property.
> > +
> > +patternProperties:
> > +  '-pins$':
> > +    type: object
> > +    description:
> > +      Pinctrl node's client devices use subnodes for desired pin configuration.
> > +      Client device subnodes use below standard properties.
> > +    $ref: /schemas/pinctrl/pincfg-node.yaml
> > +
> > +    properties:
> > +      pins:
> > +        description:
> > +          List of gpio pins affected by the properties specified in this
> > +          subnode.
> > +        items:
> > +          pattern: '^PORT[0-7]|ROTARY$'
>
> Don't you need ()?:
>
> ^(PORT[0-7]|ROTARY)$'
>

Yes!

Thanks for your review.
Mathieu

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

Re: [PATCH v5 01/11] dt-bindings: mfd: gpio: Add MAX7360

[Mathieu Dubois-Briand]

On Tue Mar 18, 2025 at 5:26 PM CET, Mathieu Dubois-Briand wrote:
> 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>
> ---

Hi,

Following discussion we had under the PWM patch of this series, we might
need to refactor a bit the device tree binding architecture, adding two
new subnodes, one for pinctrl and one for PWM.

This will need create two new compatible values with associated bindings
and modify a bit the properties of the maxim,max7360.yaml binding.

Here is the example modified to reflect what I have in mind.

> ...
>
> diff --git a/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
> new file mode 100644
> index 000000000000..d3c09531dc5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/maxim,max7360.yaml
> @@ -0,0 +1,170 @@
>
> ...
>
> +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 */
> +

+ max7360_pwm: pwm {
+         compatible = "maxim,max7360-pwm";

> +        #pwm-cells = <3>;

+ };

> +
> +        max7360_gpio: gpio {
> +          compatible = "maxim,max7360-gpio";
> +
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          maxim,constant-current-disable = <0x06>;
> +
> +          interrupt-controller;
> +          #interrupt-cells = <0x2>;
> +        };
> +
> +        max7360_gpo: gpo {
> +          compatible = "maxim,max7360-gpo";
> +
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +        };

+ pinctrl {
+         compatible = "maxim,max7360-pinctrl";

> +
> +        backlight_pins: backlight-pins {
> +          pins = "PORT2";
> +          function = "pwm";
> +        };

+ };

> +      };
> +    };

This would allow to assign a device tree node to both the pinctrl and
the PWM devices in the kernel?

Is there any comment regarding this proposal? Without any specific
comment, I will send a new version with these changes in a few days.

Best regards,
Mathieu

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

[PATCH v5 02/11] 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         |  14 ++++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/max7360.c       | 185 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max7360.h | 112 +++++++++++++++++++++++++++
 4 files changed, 312 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6b0682af6e32..ef02a1c4322c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2439,5 +2439,19 @@ config MFD_UPBOARD_FPGA
 	  To compile this driver as a module, choose M here: the module will be
 	  called upboard-fpga.
 
+config MFD_MAX7360
+	tristate "Maxim MAX7360 I2C IO Expander"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  Say yes here to add support for Maxim MAX7360 device, embedding
+	  keypad, rotary encoder, PWM and GPIO features.
+
+	  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 9220eaf7cf12..db2bd232c150 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..9f489a798f94
--- /dev/null
+++ b/drivers/mfd/max7360.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX7360 Core Driver
+ *
+ * Copyright 2025 Bootlin
+ *
+ * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.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/regmap.h>
+#include <linux/types.h>
+
+static const struct mfd_cell max7360_cells[] = {
+	{
+		.name           = "max7360-pinctrl",
+	},
+	{
+		.name           = "max7360-pwm",
+	},
+	{
+		.name           = "max7360-gpo",
+		.of_compatible	= "maxim,max7360-gpo",
+	},
+	{
+		.name           = "max7360-gpio",
+		.of_compatible	= "maxim,max7360-gpio",
+	},
+	{
+		.name           = "max7360-keypad",
+	},
+	{
+		.name           = "max7360-rotary",
+	},
+};
+
+static const struct regmap_range max7360_volatile_ranges[] = {
+	{
+		.range_min = MAX7360_REG_KEYFIFO,
+		.range_max = MAX7360_REG_KEYFIFO,
+	}, {
+		.range_min = MAX7360_REG_I2C_TIMEOUT,
+		.range_max = MAX7360_REG_RTR_CNT,
+	},
+};
+
+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 = MAX7360_REG_PWMCFG(MAX7360_PORT_PWM_COUNT - 1),
+	.volatile_table = &max7360_volatile_table,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int max7360_mask_irqs(struct regmap *regmap)
+{
+	struct device *dev = regmap_get_device(regmap);
+	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 (unsigned int i = 0; i < MAX7360_PORT_PWM_COUNT; i++) {
+		ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+					MAX7360_PORT_CFG_INTERRUPT_MASK,
+					MAX7360_PORT_CFG_INTERRUPT_MASK);
+		if (ret) {
+			dev_err(dev, "Failed to write max7360 port configuration");
+			return ret;
+		}
+	}
+
+	/* Read GPIO in register, to ACK any pending IRQ. */
+	ret = regmap_read(regmap, MAX7360_REG_GPIOIN, &val);
+	if (ret)
+		dev_err(dev, "Failed to read gpio values: %d\n", ret);
+
+	return ret;
+}
+
+static int max7360_reset(struct regmap *regmap)
+{
+	struct device *dev = regmap_get_device(regmap);
+	int ret;
+
+	ret = regmap_write(regmap, MAX7360_REG_GPIOCFG,
+			   MAX7360_GPIO_CFG_GPIO_RST);
+	if (ret) {
+		dev_err(dev, "Failed to reset GPIO configuration: %x\n", ret);
+		return ret;
+	}
+
+	ret = regcache_drop_region(regmap, MAX7360_REG_GPIOCFG,
+				   MAX7360_REG_GPIO_LAST);
+	if (ret) {
+		dev_err(dev, "Failed to drop regmap cache: %x\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(regmap, MAX7360_REG_SLEEP, 0);
+	if (ret) {
+		dev_err(dev, "Failed to reset autosleep configuration: %x\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(regmap, MAX7360_REG_DEBOUNCE, 0);
+	if (ret) {
+		dev_err(dev, "Failed to reset GPO port count: %x\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	int ret;
+
+	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");
+
+	i2c_set_clientdata(client, regmap);
+
+	ret = max7360_reset(regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to reset device\n");
+
+	/* Get the device out of shutdown mode. */
+	ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG,
+				MAX7360_GPIO_CFG_GPIO_EN,
+				MAX7360_GPIO_CFG_GPIO_EN);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable GPIO and PWM module\n");
+
+	ret = max7360_mask_irqs(regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
+
+	ret =  devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				    max7360_cells, ARRAY_SIZE(max7360_cells),
+				    NULL, 0, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "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 I2C IO Expander 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..c5a90fa5b76b
--- /dev/null
+++ b/include/linux/mfd/max7360.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __LINUX_MFD_MAX7360_H
+#define __LINUX_MFD_MAX7360_H
+
+#include <linux/bits.h>
+#include <linux/types.h>
+
+struct device;
+
+#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 GPIO registers
+ *
+ * All these registers are reset together when writing bit 3 of
+ * MAX7360_REG_GPIOCFG.
+ */
+#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_I2C_TIMEOUT		0x48
+#define MAX7360_REG_GPIOIN		0x49
+#define MAX7360_REG_RTR_CNT		0x4A
+#define MAX7360_REG_PWMBASE		0x50
+#define MAX7360_REG_PWMCFGBASE		0x58
+
+#define MAX7360_REG_GPIO_LAST		0x5F
+
+#define MAX7360_REG_PWM(x)		(MAX7360_REG_PWMBASE + (x))
+#define MAX7360_REG_PWMCFG(x)		(MAX7360_REG_PWMCFGBASE + (x))
+
+/*
+ * 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_PORT_CFG_COMMON_PWM		BIT(5)
+
+/*
+ * Autosleep register values
+ */
+#define MAX7360_AUTOSLEEP_8192MS	0x01
+#define MAX7360_AUTOSLEEP_4096MS	0x02
+#define MAX7360_AUTOSLEEP_2048MS	0x03
+#define MAX7360_AUTOSLEEP_1024MS	0x04
+#define MAX7360_AUTOSLEEP_512MS		0x05
+#define MAX7360_AUTOSLEEP_256MS		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
+
+#endif

-- 
2.39.5

Re: [PATCH v5 02/11] mfd: Add max7360 support

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:18PM +0100, 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.

...

+ array_size.h

> +#include <linux/bits.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

Since it won;t make v6.15-rc1 anyway the above can be better specified as

device/devres.h
dev_printk.h

as device.h is monstrous.

+ err.h

> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max7360.h>

+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>

...

> +	ret = regmap_write(regmap, MAX7360_REG_GPIOCFG,
> +			   MAX7360_GPIO_CFG_GPIO_RST);

I would suggest to leave it as a single line. In this and similar cases
when it is ~83 characters, it's still fine (and even for strict 80 there is
a documented exception)

...

> +		return ret;
> +	}
> +
> +	return 0;

Just

	return ret;

?

> +	ret = max7360_mask_irqs(regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");

Hmm... As far as I can read this masks GPIO interrups. Does it do anything
else? If it's covered by the GPIO/pin control drivers, one want probably to
see that to be done there in the respective callback (init_hw_irq or alike,
I don't remember the name by heart).

...

> +#ifndef __LINUX_MFD_MAX7360_H
> +#define __LINUX_MFD_MAX7360_H
> +
> +#include <linux/bits.h>

> +#include <linux/types.h>

Do you need types.h here? I don't see for what...

> +struct device;

Neither this. Perhaps it's for the following changes? Then add when required,
not now.

> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/11] mfd: Add max7360 support

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 12:10 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:18PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +	ret = max7360_mask_irqs(regmap);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
>
> Hmm... As far as I can read this masks GPIO interrups. Does it do anything
> else? If it's covered by the GPIO/pin control drivers, one want probably to
> see that to be done there in the respective callback (init_hw_irq or alike,
> I don't remember the name by heart).
>

Hum, I'm not sure I can do that.

So the "inti" interrupt line is shared across the GPIO and the rotary
encoder functionalities.

On reset, GPIO interrupts are not masked. This means, if we do the
masking in the GPIO driver and the GPIO driver is not loaded but the
rotary encoder driver is, the rotary encoder driver might get a lot of
spurious interrupts.

So I believe it makes sense to mask the interrupts here, setting the
chip in a sane configuration, whatever child drivers are present.

Any thought about that?

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

Re: [PATCH v5 02/11] mfd: Add max7360 support

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 05:26:12PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 12:10 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:18PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +	ret = max7360_mask_irqs(regmap);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Could not mask interrupts\n");
> >
> > Hmm... As far as I can read this masks GPIO interrups. Does it do anything
> > else? If it's covered by the GPIO/pin control drivers, one want probably to
> > see that to be done there in the respective callback (init_hw_irq or alike,
> > I don't remember the name by heart).
> 
> Hum, I'm not sure I can do that.
> 
> So the "inti" interrupt line is shared across the GPIO and the rotary
> encoder functionalities.
> 
> On reset, GPIO interrupts are not masked. This means, if we do the
> masking in the GPIO driver and the GPIO driver is not loaded but the
> rotary encoder driver is, the rotary encoder driver might get a lot of
> spurious interrupts.
> 
> So I believe it makes sense to mask the interrupts here, setting the
> chip in a sane configuration, whatever child drivers are present.
> 
> Any thought about that?

Okay, this makes sense. I forgot if you have any comment in the code
(probably not if I asked the question), but in any case the above can
be added on top of the function explaining this.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 03/11] pinctrl: Add MAX7360 pinctrl driver

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
can be used either for GPIO, PWM or rotary encoder functionalities.

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

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 95a8e2b9a614..cf178ef44d5d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -334,6 +334,17 @@ config PINCTRL_LPC18XX
 	help
 	  Pinctrl driver for NXP LPC18xx/43xx System Control Unit (SCU).
 
+config PINCTRL_MAX7360
+	tristate "MAX7360 Pincontrol support"
+	depends on MFD_MAX7360
+	select PINMUX
+	select GENERIC_PINCONF
+	help
+	  Say Y here to enable Pin control support for Maxim MAX7360 keypad
+	  controller.
+	  This keypad controller has 8 GPIO pins that work as GPIO as well as
+	  PWM or rotary encoder alternate modes.
+
 config PINCTRL_MAX77620
 	tristate "MAX77620/MAX20024 Pincontrol support"
 	depends on MFD_MAX77620 && OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index fba1c56624c0..f46209b7c930 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
 obj-$(CONFIG_PINCTRL_LOONGSON2) += pinctrl-loongson2.o
 obj-$(CONFIG_PINCTRL_XWAY)	+= pinctrl-xway.o
 obj-$(CONFIG_PINCTRL_LPC18XX)	+= pinctrl-lpc18xx.o
+obj-$(CONFIG_PINCTRL_MAX7360)	+= pinctrl-max7360.o
 obj-$(CONFIG_PINCTRL_MAX77620)	+= pinctrl-max77620.o
 obj-$(CONFIG_PINCTRL_MCP23S08_I2C)	+= pinctrl-mcp23s08_i2c.o
 obj-$(CONFIG_PINCTRL_MCP23S08_SPI)	+= pinctrl-mcp23s08_spi.o
diff --git a/drivers/pinctrl/pinctrl-max7360.c b/drivers/pinctrl/pinctrl-max7360.c
new file mode 100644
index 000000000000..5647a758ec8e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-max7360.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+struct max7360_pinctrl {
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_desc pinctrl_desc;
+};
+
+static const struct pinctrl_pin_desc max7360_pins[] = {
+	PINCTRL_PIN(0, "PORT0"),
+	PINCTRL_PIN(1, "PORT1"),
+	PINCTRL_PIN(2, "PORT2"),
+	PINCTRL_PIN(3, "PORT3"),
+	PINCTRL_PIN(4, "PORT4"),
+	PINCTRL_PIN(5, "PORT5"),
+	PINCTRL_PIN(6, "PORT6"),
+	PINCTRL_PIN(7, "PORT7"),
+};
+
+static const unsigned int port0_pins[] = {0};
+static const unsigned int port1_pins[] = {1};
+static const unsigned int port2_pins[] = {2};
+static const unsigned int port3_pins[] = {3};
+static const unsigned int port4_pins[] = {4};
+static const unsigned int port5_pins[] = {5};
+static const unsigned int port6_pins[] = {6};
+static const unsigned int port7_pins[] = {7};
+static const unsigned int rotary_pins[] = {6, 7};
+
+static const struct pingroup max7360_groups[] = {
+	PINCTRL_PINGROUP("PORT0", port0_pins, ARRAY_SIZE(port0_pins)),
+	PINCTRL_PINGROUP("PORT1", port1_pins, ARRAY_SIZE(port1_pins)),
+	PINCTRL_PINGROUP("PORT2", port2_pins, ARRAY_SIZE(port2_pins)),
+	PINCTRL_PINGROUP("PORT3", port3_pins, ARRAY_SIZE(port3_pins)),
+	PINCTRL_PINGROUP("PORT4", port4_pins, ARRAY_SIZE(port4_pins)),
+	PINCTRL_PINGROUP("PORT5", port5_pins, ARRAY_SIZE(port5_pins)),
+	PINCTRL_PINGROUP("PORT6", port6_pins, ARRAY_SIZE(port6_pins)),
+	PINCTRL_PINGROUP("PORT7", port7_pins, ARRAY_SIZE(port7_pins)),
+	PINCTRL_PINGROUP("ROTARY", rotary_pins, ARRAY_SIZE(rotary_pins))
+};
+
+static int max7360_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(max7360_groups);
+}
+
+static const char *max7360_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						  unsigned int group)
+{
+	return max7360_groups[group].name;
+}
+
+static int max7360_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					  unsigned int group,
+					  const unsigned int **pins,
+					  unsigned int *num_pins)
+{
+	*pins = max7360_groups[group].pins;
+	*num_pins = max7360_groups[group].npins;
+	return 0;
+}
+
+static const struct pinctrl_ops max7360_pinctrl_ops = {
+	.get_groups_count = max7360_pinctrl_get_groups_count,
+	.get_group_name = max7360_pinctrl_get_group_name,
+	.get_group_pins = max7360_pinctrl_get_group_pins,
+#ifdef CONFIG_OF
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinconf_generic_dt_free_map,
+#endif
+};
+
+static const char * const simple_groups[] = {"PORT0", "PORT1", "PORT2", "PORT3",
+					     "PORT4", "PORT5", "PORT6", "PORT7"};
+static const char * const rotary_groups[] = {"ROTARY"};
+
+#define MAX7360_PINCTRL_FN_ROTARY	2
+static const struct pinfunction max7360_functions[] = {
+	PINCTRL_PINFUNCTION("gpio", simple_groups, ARRAY_SIZE(simple_groups)),
+	PINCTRL_PINFUNCTION("pwm", simple_groups, ARRAY_SIZE(simple_groups)),
+	[MAX7360_PINCTRL_FN_ROTARY] = PINCTRL_PINFUNCTION("rotary", rotary_groups,
+							  ARRAY_SIZE(rotary_groups)),
+};
+
+static int max7360_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(max7360_functions);
+}
+
+static const char *max7360_get_function_name(struct pinctrl_dev *pctldev, unsigned int selector)
+{
+	return max7360_functions[selector].name;
+}
+
+static int max7360_get_function_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+				       const char * const **groups,
+				       unsigned int * const num_groups)
+{
+	*groups = max7360_functions[selector].groups;
+	*num_groups = max7360_functions[selector].ngroups;
+
+	return 0;
+}
+
+static int max7360_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			   unsigned int group)
+{
+	struct regmap *regmap;
+	int ret = 0;
+	int val;
+
+	/*
+	 * GPIO and PWM functions are the same: we only need to handle the
+	 * rotary encoder function, on pins 6 and 7.
+	 */
+	if (max7360_groups[group].pins[0] >= 6) {
+		if (selector == MAX7360_PINCTRL_FN_ROTARY)
+			val = MAX7360_GPIO_CFG_RTR_EN;
+		else
+			val = 0;
+
+		regmap = dev_get_regmap(pctldev->dev, NULL);
+		ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_RTR_EN, val);
+	}
+
+	return ret;
+}
+
+static const struct pinmux_ops max7360_pmxops = {
+	.get_functions_count = max7360_get_functions_count,
+	.get_function_name = max7360_get_function_name,
+	.get_function_groups = max7360_get_function_groups,
+	.set_mux = max7360_set_mux,
+	.strict = true,
+};
+
+static int max7360_pinctrl_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	struct pinctrl_desc *pd;
+	struct max7360_pinctrl *chip;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap)
+		dev_err_probe(&pdev->dev, -ENODEV, "Could not get parent regmap\n");
+
+	pd = &chip->pinctrl_desc;
+
+	pd->pctlops = &max7360_pinctrl_ops;
+	pd->pmxops = &max7360_pmxops;
+	pd->name = dev_name(&pdev->dev);
+	pd->pins = max7360_pins;
+	pd->npins = MAX7360_MAX_GPIO;
+	pd->owner = THIS_MODULE;
+
+	chip->pctldev = devm_pinctrl_register(pdev->dev.parent, pd, chip);
+	if (IS_ERR(chip->pctldev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(chip->pctldev),
+			"can't register controller\n");
+
+	return 0;
+}
+
+static struct platform_driver max7360_pinctrl_driver = {
+	.driver = {
+		.name	= "max7360-pinctrl",
+	},
+	.probe		= max7360_pinctrl_probe,
+};
+module_platform_driver(max7360_pinctrl_driver);
+
+MODULE_DESCRIPTION("MAX7360 pinctrl driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v5 03/11] pinctrl: Add MAX7360 pinctrl driver

[Linus Walleij]

Hi Mathieu,

thanks for your patch!

On Tue, Mar 18, 2025 at 5:26 PM Mathieu Dubois-Briand
<mathieu.dubois-briand@bootlin.com> wrote:

> Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
> can be used either for GPIO, PWM or rotary encoder functionalities.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>

Overall it's a clean and simple pin control driver, so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 03/11] pinctrl: Add MAX7360 pinctrl driver

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:19PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
> can be used either for GPIO, PWM or rotary encoder functionalities.

...

> +	help
> +	  Say Y here to enable Pin control support for Maxim MAX7360 keypad

s/Pin/pin/

> +	  controller.
> +	  This keypad controller has 8 GPIO pins that work as GPIO as well as

"...that may work as GPIO, or PWM, or..."

> +	  PWM or rotary encoder alternate modes.

...

+ array_size.h
+ dev_printk.h
+ device/devres.h // currently only in Linux Next
+ err.h

> +#include <linux/init.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>

> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>

We usually move this group of inclusions...

> +#include <linux/regmap.h>
> +#include <linux/slab.h>

...to somewhere here.

> +#include "core.h"
> +#include "pinmux.h"

...

> +static const struct pingroup max7360_groups[] = {
> +	PINCTRL_PINGROUP("PORT0", port0_pins, ARRAY_SIZE(port0_pins)),
> +	PINCTRL_PINGROUP("PORT1", port1_pins, ARRAY_SIZE(port1_pins)),
> +	PINCTRL_PINGROUP("PORT2", port2_pins, ARRAY_SIZE(port2_pins)),
> +	PINCTRL_PINGROUP("PORT3", port3_pins, ARRAY_SIZE(port3_pins)),
> +	PINCTRL_PINGROUP("PORT4", port4_pins, ARRAY_SIZE(port4_pins)),
> +	PINCTRL_PINGROUP("PORT5", port5_pins, ARRAY_SIZE(port5_pins)),
> +	PINCTRL_PINGROUP("PORT6", port6_pins, ARRAY_SIZE(port6_pins)),
> +	PINCTRL_PINGROUP("PORT7", port7_pins, ARRAY_SIZE(port7_pins)),
> +	PINCTRL_PINGROUP("ROTARY", rotary_pins, ARRAY_SIZE(rotary_pins))

Leave trailing comma. Helps in the future in case of expansion.

> +};

...

> +static const char * const simple_groups[] = {"PORT0", "PORT1", "PORT2", "PORT3",
> +					     "PORT4", "PORT5", "PORT6", "PORT7"};

It's better to read when split as

static const char * const simple_groups[] = {
	"PORT0", "PORT1", "PORT2", "PORT3",
	"PORT4", "PORT5", "PORT6", "PORT7",
};

(also note the trailing comma).

…

> +static const char * const rotary_groups[] = {"ROTARY"};

Add spaces inside {}.

...

> +#define MAX7360_PINCTRL_FN_ROTARY	2
> +static const struct pinfunction max7360_functions[] = {
> +	PINCTRL_PINFUNCTION("gpio", simple_groups, ARRAY_SIZE(simple_groups)),
> +	PINCTRL_PINFUNCTION("pwm", simple_groups, ARRAY_SIZE(simple_groups)),
> +	[MAX7360_PINCTRL_FN_ROTARY] = PINCTRL_PINFUNCTION("rotary", rotary_groups,
> +							  ARRAY_SIZE(rotary_groups)),

Please make them all look the same, if indexed, than add indices to all.

> +};

...

> +static int max7360_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			   unsigned int group)
> +{
> +	struct regmap *regmap;

> +	int ret = 0;

Variable is not needed, just return directly.

> +	int val;
> +
> +	/*
> +	 * GPIO and PWM functions are the same: we only need to handle the
> +	 * rotary encoder function, on pins 6 and 7.
> +	 */
> +	if (max7360_groups[group].pins[0] >= 6) {
> +		if (selector == MAX7360_PINCTRL_FN_ROTARY)
> +			val = MAX7360_GPIO_CFG_RTR_EN;
> +		else
> +			val = 0;
> +
> +		regmap = dev_get_regmap(pctldev->dev, NULL);
> +		ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_RTR_EN, val);
> +	}
> +
> +	return ret;
> +}

...

> +static int max7360_pinctrl_probe(struct platform_device *pdev)
> +{

With

	struct device *dev = &pdev->dev;

the below will look better.

> +	struct regmap *regmap;
> +	struct pinctrl_desc *pd;
> +	struct max7360_pinctrl *chip;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!regmap)
> +		dev_err_probe(&pdev->dev, -ENODEV, "Could not get parent regmap\n");

Make it first check, in such a case you don't even need to allocate memory for
peanuts.

> +	pd = &chip->pinctrl_desc;
> +
> +	pd->pctlops = &max7360_pinctrl_ops;
> +	pd->pmxops = &max7360_pmxops;
> +	pd->name = dev_name(&pdev->dev);
> +	pd->pins = max7360_pins;
> +	pd->npins = MAX7360_MAX_GPIO;
> +	pd->owner = THIS_MODULE;
> +
> +	chip->pctldev = devm_pinctrl_register(pdev->dev.parent, pd, chip);
> +	if (IS_ERR(chip->pctldev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(chip->pctldev),
> +			"can't register controller\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 04/11] 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>
Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@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 | 194 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..930cc7c61b82 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
+	select PINCTRL_MAX7360
+	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..059b39df791d
--- /dev/null
+++ b/drivers/pwm/pwm-max7360.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@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/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/math64.h>
+#include <linux/mfd/max7360.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#define MAX7360_NUM_PWMS			8
+#define MAX7360_PWM_MAX_RES			255
+#define MAX7360_PWM_PERIOD_NS			(2 * NSEC_PER_MSEC)
+
+struct max7360_pwm_waveform {
+	u8 duty_steps;
+	bool enabled;
+};
+
+static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct regmap *regmap;
+	struct device *dev;
+	int ret;
+
+	regmap = pwmchip_get_drvdata(chip);
+	dev = regmap_get_device(regmap);
+
+	ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
+				MAX7360_PORT_CFG_COMMON_PWM, 0);
+	if (ret)
+		return ret;
+
+	return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm));
+}
+
+static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct regmap *regmap;
+	struct device *dev;
+
+	regmap = pwmchip_get_drvdata(chip);
+	dev = regmap_get_device(regmap);
+}
+
+static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const struct pwm_waveform *wf,
+					   void *_wfhw)
+{
+	struct max7360_pwm_waveform *wfhw = _wfhw;
+	u64 duty_steps;
+
+	/*
+	 * Ignore user provided values for period_length_ns and duty_offset_ns:
+	 * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of 0.
+	 */
+	duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
+					 MAX7360_PWM_PERIOD_NS);
+
+	wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
+	wfhw->enabled = (wf->duty_length_ns != 0);
+
+	return 0;
+}
+
+static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+					     const void *_wfhw, struct pwm_waveform *wf)
+{
+	const struct max7360_pwm_waveform *wfhw = _wfhw;
+
+	wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0;
+	wf->duty_offset_ns = 0;
+	wf->duty_length_ns = DIV64_U64_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
+						MAX7360_PWM_MAX_RES);
+
+	return 0;
+}
+
+static int max7360_pwm_write_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      const void *_wfhw)
+{
+	const struct max7360_pwm_waveform *wfhw = _wfhw;
+	struct regmap *regmap;
+	unsigned int val;
+	int ret;
+
+	regmap = pwmchip_get_drvdata(chip);
+	val = (wfhw->enabled) ? BIT(pwm->hwpwm) : 0;
+	ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val);
+	if (ret)
+		return ret;
+
+	if (wfhw->duty_steps)
+		return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
+
+	return 0;
+}
+
+static int max7360_pwm_read_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     void *_wfhw)
+{
+	struct max7360_pwm_waveform *wfhw = _wfhw;
+	struct regmap *regmap;
+	unsigned int val;
+	int ret;
+
+	regmap = pwmchip_get_drvdata(chip);
+
+	ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val);
+	if (ret)
+		return ret;
+
+	if (val & BIT(pwm->hwpwm)) {
+		wfhw->enabled = true;
+		ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val);
+		wfhw->duty_steps = val;
+	} else {
+		wfhw->enabled = false;
+		wfhw->duty_steps = 0;
+	}
+
+	return ret;
+}
+
+static const struct pwm_ops max7360_pwm_ops = {
+	.request = max7360_pwm_request,
+	.free = max7360_pwm_free,
+	.round_waveform_tohw = max7360_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = max7360_pwm_round_waveform_fromhw,
+	.read_waveform = max7360_pwm_read_waveform,
+	.write_waveform = max7360_pwm_write_waveform,
+};
+
+static int max7360_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pwm_chip *chip;
+	struct regmap *regmap;
+	int ret;
+
+	if (!dev->parent)
+		return dev_err_probe(dev, -ENODEV, "no parent device\n");
+
+	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	chip->ops = &max7360_pwm_ops;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
+
+	pwmchip_set_drvdata(chip, regmap);
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver max7360_pwm_driver = {
+	.driver = {
+		.name = "max7360-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_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:20PM +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.

...

> +#include <linux/bits.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/minmax.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>

...

> +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct regmap *regmap;
> +	struct device *dev;
> +
> +	regmap = pwmchip_get_drvdata(chip);
> +	dev = regmap_get_device(regmap);

Huh?!

> +}

...

> +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const struct pwm_waveform *wf,
> +					   void *_wfhw)

I would expect other way around, i.e. naming with leading underscore(s) to be
private / local. Ditto for all similar cases.

...

> +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	const struct max7360_pwm_waveform *wfhw = _wfhw;
> +	struct regmap *regmap;
> +	unsigned int val;
> +	int ret;
> +
> +	regmap = pwmchip_get_drvdata(chip);
> +	val = (wfhw->enabled) ? BIT(pwm->hwpwm) : 0;

Redundant parentheses.

> +	ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val);
> +	if (ret)
> +		return ret;
> +
> +	if (wfhw->duty_steps)
> +		return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
> +
> +	return 0;
> +}

...

> +static int max7360_pwm_read_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     void *_wfhw)
> +{
> +	struct max7360_pwm_waveform *wfhw = _wfhw;
> +	struct regmap *regmap;
> +	unsigned int val;
> +	int ret;
> +
> +	regmap = pwmchip_get_drvdata(chip);
> +
> +	ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & BIT(pwm->hwpwm)) {
> +		wfhw->enabled = true;

Also can be (but up to you)

	wfhw->enabled = val & BIT(pwm->hwpwm);
	if (wfhw->enabled) {

And also see below. Perhaps it is not a good suggestion after all.

> +		ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val);
> +		wfhw->duty_steps = val;

Set to a garbage in case of error, why?

> +	} else {
> +		wfhw->enabled = false;
> +		wfhw->duty_steps = 0;
> +	}
> +
> +	return ret;
> +}

...

> +static int max7360_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pwm_chip *chip;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	if (!dev->parent)
> +		return dev_err_probe(dev, -ENODEV, "no parent device\n");

Why? Code most likely will fail on the regmap retrieval. Just do that first.

> +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);

This is quite worrying. The devm_ to parent makes a lot of assumptions that may
not be realised. If you really need this, it has to have a very good comment
explaining why and object lifetimes.

> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	chip->ops = &max7360_pwm_ops;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
> +
> +	pwmchip_set_drvdata(chip, regmap);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

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

On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > +					   struct pwm_device *pwm,
> > +					   const struct pwm_waveform *wf,
> > +					   void *_wfhw)
> 
> I would expect other way around, i.e. naming with leading underscore(s) to be
> private / local. Ditto for all similar cases.

I guess that one of the other waveform drivers is the source of that. I
chose to name the void pointer with the underscore because I consider
that the strange one that has the void* type for technical reasons.
That's obviously subjective, but I'm happy with that choice.

> > +static int max7360_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pwm_chip *chip;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	if (!dev->parent)
> > +		return dev_err_probe(dev, -ENODEV, "no parent device\n");
> 
> Why? Code most likely will fail on the regmap retrieval. Just do that first.
> 
> > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> 
> This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> not be realised. If you really need this, it has to have a very good comment
> explaining why and object lifetimes.

Pretty sure this is broken. This results for example in the device link
being created on the parent. So if the pwm devices goes away a consumer
might not notice (at least in the usual way). I guess this was done to
ensure that #pwm-cells is parsed from the right dt node? If so, that
needs a different adaption. That will probably involve calling
device_set_of_node_from_dev().

Best regards
Uwe

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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:

...

> > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > 
> > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > not be realised. If you really need this, it has to have a very good comment
> > explaining why and object lifetimes.
> 
> Pretty sure this is broken. This results for example in the device link
> being created on the parent. So if the pwm devices goes away a consumer
> might not notice (at least in the usual way). I guess this was done to
> ensure that #pwm-cells is parsed from the right dt node? If so, that
> needs a different adaption. That will probably involve calling
> device_set_of_node_from_dev().

It's an MFD based driver, and MFD core cares about propagating fwnode by
default. I believe it should just work if we drop that '->parent' part.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:
>
> ...
>
> > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > 
> > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > not be realised. If you really need this, it has to have a very good comment
> > > explaining why and object lifetimes.
> > 
> > Pretty sure this is broken. This results for example in the device link
> > being created on the parent. So if the pwm devices goes away a consumer
> > might not notice (at least in the usual way). I guess this was done to
> > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > needs a different adaption. That will probably involve calling
> > device_set_of_node_from_dev().
>
> It's an MFD based driver, and MFD core cares about propagating fwnode by
> default. I believe it should just work if we drop that '->parent' part.

Are you sure about that?

On my side it does not work if I just drop the '->parent', this is why I
ended whit this (bad) pattern.

Now it does work if I do call device_set_of_node_from_dev() manually, so
it's definitely better. But I believe the MFD core is not propagating
OF data, and I did not find where it would do that in the code. Yet it
does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
do something bad in our MFD driver?


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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:

...

> > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > 
> > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > not be realised. If you really need this, it has to have a very good comment
> > > > explaining why and object lifetimes.
> > > 
> > > Pretty sure this is broken. This results for example in the device link
> > > being created on the parent. So if the pwm devices goes away a consumer
> > > might not notice (at least in the usual way). I guess this was done to
> > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > needs a different adaption. That will probably involve calling
> > > device_set_of_node_from_dev().
> >
> > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > default. I believe it should just work if we drop that '->parent' part.
> 
> Are you sure about that?

Yes and no. If your DT looks like (pseudo code as I don't know
DTS syntax by heart):

	device: {
		parent-property = value;
		child0:
			...
		child1:
			...
	}

the parent-property value is automatically accessible via fwnode API,
but I don't know what will happen to the cases when each of the children
has its own compatible string. This might be your case, but again,
I'm not an expert in DT.

> On my side it does not work if I just drop the '->parent', this is why I
> ended whit this (bad) pattern.

> Now it does work if I do call device_set_of_node_from_dev() manually,

AFAICT, this is wrong API to be called in the children. Are you talking about
parent code?

> so it's definitely better. But I believe the MFD core is not propagating
> OF data, and I did not find where it would do that in the code. Yet it
> does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> do something bad in our MFD driver?

...or MFD needs something to have... Dunno.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:
>
> ...
>
> > > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > > 
> > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > > not be realised. If you really need this, it has to have a very good comment
> > > > > explaining why and object lifetimes.
> > > > 
> > > > Pretty sure this is broken. This results for example in the device link
> > > > being created on the parent. So if the pwm devices goes away a consumer
> > > > might not notice (at least in the usual way). I guess this was done to
> > > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > > needs a different adaption. That will probably involve calling
> > > > device_set_of_node_from_dev().
> > >
> > > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > > default. I believe it should just work if we drop that '->parent' part.
> > 
> > Are you sure about that?
>
> Yes and no. If your DT looks like (pseudo code as I don't know
> DTS syntax by heart):
>
> 	device: {
> 		parent-property = value;
> 		child0:
> 			...
> 		child1:
> 			...
> 	}
>
> the parent-property value is automatically accessible via fwnode API,
> but I don't know what will happen to the cases when each of the children
> has its own compatible string. This might be your case, but again,
> I'm not an expert in DT.
>

On my side:
- Some MFD child do have a child node in the device tree, with an
  associated compatible value. No problem for these, they do get correct
  of_node/fwnode values pointing on the child device tree node.
- Some MFD child do not have any node in the device tree, and for these,
  they have to use properties from the parent (MFD) device tree node.
  And here we do have some problems.

> > On my side it does not work if I just drop the '->parent', this is why I
> > ended whit this (bad) pattern.
>
> > Now it does work if I do call device_set_of_node_from_dev() manually,
>
> AFAICT, this is wrong API to be called in the children. Are you talking about
> parent code?
>

I believe I cannot do it in the parent code, as I would need to do it
after the call to devm_mfd_add_devices(), and so it might happen after
the probe. I still tried to see how it behaved, and it looks like PWM
core really did not expect to get an of_node assigned to the device
after adding the PWM device.

So either I can do something in MFD core or in sub devices probe(), or I
need to come with a different way to do things.

> > so it's definitely better. But I believe the MFD core is not propagating
> > OF data, and I did not find where it would do that in the code. Yet it
> > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> > do something bad in our MFD driver?
>
> ...or MFD needs something to have... Dunno.

I have something working with a very simple change in mfd-core.c, but
I'm really not confident it won't break anything else. I wish I could
get some insights from an MFD expert.

@@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id,
                if (!pdev->dev.of_node)
                        pr_warn("%s: Failed to locate of_node [id: %d]\n",
                                cell->name, platform_id);
+       } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) {
+               device_set_of_node_from_dev(&pdev->dev, parent);
        }


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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 03:44:28PM +0100, Mathieu Dubois-Briand wrote:
> On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:

...

> > > > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > > > 
> > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > > > not be realised. If you really need this, it has to have a very good comment
> > > > > > explaining why and object lifetimes.
> > > > > 
> > > > > Pretty sure this is broken. This results for example in the device link
> > > > > being created on the parent. So if the pwm devices goes away a consumer
> > > > > might not notice (at least in the usual way). I guess this was done to
> > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > > > needs a different adaption. That will probably involve calling
> > > > > device_set_of_node_from_dev().
> > > >
> > > > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > > > default. I believe it should just work if we drop that '->parent' part.
> > > 
> > > Are you sure about that?
> >
> > Yes and no. If your DT looks like (pseudo code as I don't know
> > DTS syntax by heart):
> >
> > 	device: {
> > 		parent-property = value;
> > 		child0:
> > 			...
> > 		child1:
> > 			...
> > 	}
> >
> > the parent-property value is automatically accessible via fwnode API,
> > but I don't know what will happen to the cases when each of the children
> > has its own compatible string. This might be your case, but again,
> > I'm not an expert in DT.
> >
> 
> On my side:
> - Some MFD child do have a child node in the device tree, with an
>   associated compatible value. No problem for these, they do get correct
>   of_node/fwnode values pointing on the child device tree node.
> - Some MFD child do not have any node in the device tree, and for these,
>   they have to use properties from the parent (MFD) device tree node.
>   And here we do have some problems.
> 
> > > On my side it does not work if I just drop the '->parent', this is why I
> > > ended whit this (bad) pattern.
> >
> > > Now it does work if I do call device_set_of_node_from_dev() manually,
> >
> > AFAICT, this is wrong API to be called in the children. Are you talking about
> > parent code?
> >
> 
> I believe I cannot do it in the parent code, as I would need to do it
> after the call to devm_mfd_add_devices(), and so it might happen after
> the probe. I still tried to see how it behaved, and it looks like PWM
> core really did not expect to get an of_node assigned to the device
> after adding the PWM device.
> 
> So either I can do something in MFD core or in sub devices probe(), or I
> need to come with a different way to do things.
> 
> > > so it's definitely better. But I believe the MFD core is not propagating
> > > OF data, and I did not find where it would do that in the code. Yet it
> > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> > > do something bad in our MFD driver?
> >
> > ...or MFD needs something to have... Dunno.
> 
> I have something working with a very simple change in mfd-core.c, but
> I'm really not confident it won't break anything else. I wish I could
> get some insights from an MFD expert.
> 
> @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id,
>                 if (!pdev->dev.of_node)
>                         pr_warn("%s: Failed to locate of_node [id: %d]\n",
>                                 cell->name, platform_id);
> +       } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) {
> +               device_set_of_node_from_dev(&pdev->dev, parent);

The use of this API is inappropriate here AFAICT. It drops the parent refcount
and on the second call to it you will have a warning from refcount library.

It should be as simple as device_set_node().

>         }

With that, the conditional becomes

	} else if (is_of_node(fwnode)) {
		device_set_node(&pdev->dev, fwnode);
	}

where fwnode is something like

	struct fwnode_handle *fwnode = dev_fwnode(parent);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

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

On Wed, Mar 26, 2025 at 05:49:07PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 26, 2025 at 03:44:28PM +0100, Mathieu Dubois-Briand wrote:
> > On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote:
> > > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> > > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> 
> ...
> 
> > > > > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > > > > 
> > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > > > > not be realised. If you really need this, it has to have a very good comment
> > > > > > > explaining why and object lifetimes.
> > > > > > 
> > > > > > Pretty sure this is broken. This results for example in the device link
> > > > > > being created on the parent. So if the pwm devices goes away a consumer
> > > > > > might not notice (at least in the usual way). I guess this was done to
> > > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > > > > needs a different adaption. That will probably involve calling
> > > > > > device_set_of_node_from_dev().
> > > > >
> > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > > > > default. I believe it should just work if we drop that '->parent' part.
> > > > 
> > > > Are you sure about that?
> > >
> > > Yes and no. If your DT looks like (pseudo code as I don't know
> > > DTS syntax by heart):
> > >
> > > 	device: {
> > > 		parent-property = value;
> > > 		child0:
> > > 			...
> > > 		child1:
> > > 			...
> > > 	}
> > >
> > > the parent-property value is automatically accessible via fwnode API,
> > > but I don't know what will happen to the cases when each of the children
> > > has its own compatible string. This might be your case, but again,
> > > I'm not an expert in DT.
> > >
> > 
> > On my side:
> > - Some MFD child do have a child node in the device tree, with an
> >   associated compatible value. No problem for these, they do get correct
> >   of_node/fwnode values pointing on the child device tree node.
> > - Some MFD child do not have any node in the device tree, and for these,
> >   they have to use properties from the parent (MFD) device tree node.
> >   And here we do have some problems.
> > 
> > > > On my side it does not work if I just drop the '->parent', this is why I
> > > > ended whit this (bad) pattern.
> > >
> > > > Now it does work if I do call device_set_of_node_from_dev() manually,
> > >
> > > AFAICT, this is wrong API to be called in the children. Are you talking about
> > > parent code?
> > >
> > 
> > I believe I cannot do it in the parent code, as I would need to do it
> > after the call to devm_mfd_add_devices(), and so it might happen after
> > the probe. I still tried to see how it behaved, and it looks like PWM
> > core really did not expect to get an of_node assigned to the device
> > after adding the PWM device.
> > 
> > So either I can do something in MFD core or in sub devices probe(), or I
> > need to come with a different way to do things.
> > 
> > > > so it's definitely better. But I believe the MFD core is not propagating
> > > > OF data, and I did not find where it would do that in the code. Yet it
> > > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> > > > do something bad in our MFD driver?
> > >
> > > ...or MFD needs something to have... Dunno.
> > 
> > I have something working with a very simple change in mfd-core.c, but
> > I'm really not confident it won't break anything else. I wish I could
> > get some insights from an MFD expert.
> > 
> > @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id,
> >                 if (!pdev->dev.of_node)
> >                         pr_warn("%s: Failed to locate of_node [id: %d]\n",
> >                                 cell->name, platform_id);
> > +       } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) {
> > +               device_set_of_node_from_dev(&pdev->dev, parent);
> 
> The use of this API is inappropriate here AFAICT. It drops the parent refcount
> and on the second call to it you will have a warning from refcount library.

device_set_of_node_from_dev() does:

	of_node_put(pdev->dev->of_node);
	pdev->dev->of_node = of_node_get(parent->of_node);
	pdev->dev->of_node_reused = true;

Note that pdev isn't the platform device associated with the parent
device but the just allocated one representing the subdevice so
pdev->dev->of_node is NULL and the parent refcount isn't dropped.

But I'm unsure if this is the right place to call it or if
device_set_node() is indeed enough (also I wonder if
device_set_of_node_from_dev() should care for fwnode). I'll keep that
question for someone else.

Best regards
Uwe

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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:46:41PM +0100, Uwe Kleine-König wrote:
> On Wed, Mar 26, 2025 at 05:49:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 26, 2025 at 03:44:28PM +0100, Mathieu Dubois-Briand wrote:
> > > On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> > > > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > > > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:

...

> > > > > > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > > > > > 
> > > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > > > > > not be realised. If you really need this, it has to have a very good comment
> > > > > > > > explaining why and object lifetimes.
> > > > > > > 
> > > > > > > Pretty sure this is broken. This results for example in the device link
> > > > > > > being created on the parent. So if the pwm devices goes away a consumer
> > > > > > > might not notice (at least in the usual way). I guess this was done to
> > > > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > > > > > needs a different adaption. That will probably involve calling
> > > > > > > device_set_of_node_from_dev().
> > > > > >
> > > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > > > > > default. I believe it should just work if we drop that '->parent' part.
> > > > > 
> > > > > Are you sure about that?
> > > >
> > > > Yes and no. If your DT looks like (pseudo code as I don't know
> > > > DTS syntax by heart):
> > > >
> > > > 	device: {
> > > > 		parent-property = value;
> > > > 		child0:
> > > > 			...
> > > > 		child1:
> > > > 			...
> > > > 	}
> > > >
> > > > the parent-property value is automatically accessible via fwnode API,
> > > > but I don't know what will happen to the cases when each of the children
> > > > has its own compatible string. This might be your case, but again,
> > > > I'm not an expert in DT.
> > > >
> > > 
> > > On my side:
> > > - Some MFD child do have a child node in the device tree, with an
> > >   associated compatible value. No problem for these, they do get correct
> > >   of_node/fwnode values pointing on the child device tree node.
> > > - Some MFD child do not have any node in the device tree, and for these,
> > >   they have to use properties from the parent (MFD) device tree node.
> > >   And here we do have some problems.
> > > 
> > > > > On my side it does not work if I just drop the '->parent', this is why I
> > > > > ended whit this (bad) pattern.
> > > >
> > > > > Now it does work if I do call device_set_of_node_from_dev() manually,
> > > >
> > > > AFAICT, this is wrong API to be called in the children. Are you talking about
> > > > parent code?
> > > >
> > > 
> > > I believe I cannot do it in the parent code, as I would need to do it
> > > after the call to devm_mfd_add_devices(), and so it might happen after
> > > the probe. I still tried to see how it behaved, and it looks like PWM
> > > core really did not expect to get an of_node assigned to the device
> > > after adding the PWM device.
> > > 
> > > So either I can do something in MFD core or in sub devices probe(), or I
> > > need to come with a different way to do things.
> > > 
> > > > > so it's definitely better. But I believe the MFD core is not propagating
> > > > > OF data, and I did not find where it would do that in the code. Yet it
> > > > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> > > > > do something bad in our MFD driver?
> > > >
> > > > ...or MFD needs something to have... Dunno.
> > > 
> > > I have something working with a very simple change in mfd-core.c, but
> > > I'm really not confident it won't break anything else. I wish I could
> > > get some insights from an MFD expert.
> > > 
> > > @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id,
> > >                 if (!pdev->dev.of_node)
> > >                         pr_warn("%s: Failed to locate of_node [id: %d]\n",
> > >                                 cell->name, platform_id);
> > > +       } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) {
> > > +               device_set_of_node_from_dev(&pdev->dev, parent);
> > 
> > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > and on the second call to it you will have a warning from refcount library.
> 
> device_set_of_node_from_dev() does:
> 
> 	of_node_put(pdev->dev->of_node);
> 	pdev->dev->of_node = of_node_get(parent->of_node);
> 	pdev->dev->of_node_reused = true;
> 
> Note that pdev isn't the platform device associated with the parent
> device but the just allocated one representing the subdevice so
> pdev->dev->of_node is NULL and the parent refcount isn't dropped.

Ah, I stand corrected, thanks! Okay, so what it does basically, it drops
a reference for the child, and propagates the parent's node at the same time
bumping its reference count. Sounds legit, but this should be done equally for
DT, ACPI and software node cases.

> But I'm unsure if this is the right place to call it or if
> device_set_node() is indeed enough

This all about node reference counting. Whatever the correct choice for DT.
Anyway this sounds right to do for any of the system, but also note, that
this API is good when device node is not backed by the struct device,
otherwise, the struct device reference count covers the device node AFAIU.

TL;DR: What are the object lifetimes for struct device and struct device_node
(struct fwnode_handle)?

> (also I wonder if
> device_set_of_node_from_dev() should care for fwnode).
> I'll keep that question for someone else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:
> The use of this API is inappropriate here AFAICT. It drops the parent refcount
> and on the second call to it you will have a warning from refcount library.
>
> It should be as simple as device_set_node().
>
> >         }
>
> With that, the conditional becomes
>
> 	} else if (is_of_node(fwnode)) {
> 		device_set_node(&pdev->dev, fwnode);
> 	}
>
> where fwnode is something like
>
> 	struct fwnode_handle *fwnode = dev_fwnode(parent);

Hi,

I tried to use device_set_node(), but then I got some other issue: as we
now have several devices with the same firmware node, they all share the
same properties. In particular, if we do use pinctrl- properties to
apply some pinmmuxing, all devices will try to apply this pinmuxing and
of course all but one will fail.

And this makes me think again about the whole thing, maybe copying the
fwnode or of_node from the parent is not the way to go.

So today we rely on the parent node for four drivers:
- keypad and rotary, just to ease a bit the parsing of some properties,
  such as the keymap with matrix_keypad_build_keymap(). I can easily do
  it another way.
- PWM and pinctrl drivers, are a bit more complicated, as in both case
  the device tree node associated with the device is used internally. In
  one case to find the correct PWM device for PWM clients listed in the
  device tree, in the other case to find the pinctrl device when
  applying pinctrl described in the device tree.

So maybe I have to find a better way for have this association. One way
would be to modify the device tree bindings to add a PWM and a pinctrl
node, with their own compatible, so they are associated to the
corresponding device. But maybe there is a better way to do it.


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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:

> > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > and on the second call to it you will have a warning from refcount library.
> >
> > It should be as simple as device_set_node().
> >
> > >         }
> >
> > With that, the conditional becomes
> >
> > 	} else if (is_of_node(fwnode)) {
> > 		device_set_node(&pdev->dev, fwnode);
> > 	}
> >
> > where fwnode is something like
> >
> > 	struct fwnode_handle *fwnode = dev_fwnode(parent);
> 
> I tried to use device_set_node(), but then I got some other issue: as we
> now have several devices with the same firmware node, they all share the
> same properties. In particular, if we do use pinctrl- properties to
> apply some pinmmuxing, all devices will try to apply this pinmuxing and
> of course all but one will fail.
> 
> And this makes me think again about the whole thing, maybe copying the
> fwnode or of_node from the parent is not the way to go.
> 
> So today we rely on the parent node for four drivers:
> - keypad and rotary, just to ease a bit the parsing of some properties,
>   such as the keymap with matrix_keypad_build_keymap(). I can easily do
>   it another way.
> - PWM and pinctrl drivers, are a bit more complicated, as in both case
>   the device tree node associated with the device is used internally. In
>   one case to find the correct PWM device for PWM clients listed in the
>   device tree, in the other case to find the pinctrl device when
>   applying pinctrl described in the device tree.
> 
> So maybe I have to find a better way for have this association. One way
> would be to modify the device tree bindings to add a PWM and a pinctrl
> node, with their own compatible, so they are associated to the
> corresponding device. But maybe there is a better way to do it.

Okay, so the main question now, why do the device share their properties
to begin with? It can be done via fwnode graph or similar APIs (in case
it is _really_ needed).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote:
> On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote:
> > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:
>
> > > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > > and on the second call to it you will have a warning from refcount library.
> > >
> > > It should be as simple as device_set_node().
> > >
> > > >         }
> > >
> > > With that, the conditional becomes
> > >
> > > 	} else if (is_of_node(fwnode)) {
> > > 		device_set_node(&pdev->dev, fwnode);
> > > 	}
> > >
> > > where fwnode is something like
> > >
> > > 	struct fwnode_handle *fwnode = dev_fwnode(parent);
> > 
> > I tried to use device_set_node(), but then I got some other issue: as we
> > now have several devices with the same firmware node, they all share the
> > same properties. In particular, if we do use pinctrl- properties to
> > apply some pinmmuxing, all devices will try to apply this pinmuxing and
> > of course all but one will fail.
> > 
> > And this makes me think again about the whole thing, maybe copying the
> > fwnode or of_node from the parent is not the way to go.
> > 
> > So today we rely on the parent node for four drivers:
> > - keypad and rotary, just to ease a bit the parsing of some properties,
> >   such as the keymap with matrix_keypad_build_keymap(). I can easily do
> >   it another way.
> > - PWM and pinctrl drivers, are a bit more complicated, as in both case
> >   the device tree node associated with the device is used internally. In
> >   one case to find the correct PWM device for PWM clients listed in the
> >   device tree, in the other case to find the pinctrl device when
> >   applying pinctrl described in the device tree.
> > 
> > So maybe I have to find a better way for have this association. One way
> > would be to modify the device tree bindings to add a PWM and a pinctrl
> > node, with their own compatible, so they are associated to the
> > corresponding device. But maybe there is a better way to do it.
>
> Okay, so the main question now, why do the device share their properties
> to begin with? It can be done via fwnode graph or similar APIs (in case
> it is _really_ needed).

I wouldn't say the properties are shared: we have a single node in the
device tree as this is just one device. But as we create several
(software) devices in the MFD driver, we now have several devices linked
with a single device tree node.

One solution would be to create more subnodes in the device tree, one
for pinctrl and one for PWM, but this feels a bit like describing our
software implementation in the device tree instead of describing the
hardware.

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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Fri, Mar 28, 2025 at 09:13:12AM +0100, Mathieu Dubois-Briand wrote:
> On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote:
> > On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote:
> > > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:

...

> > > > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > > > and on the second call to it you will have a warning from refcount library.
> > > >
> > > > It should be as simple as device_set_node().
> > > >
> > > > >         }
> > > >
> > > > With that, the conditional becomes
> > > >
> > > > 	} else if (is_of_node(fwnode)) {
> > > > 		device_set_node(&pdev->dev, fwnode);
> > > > 	}
> > > >
> > > > where fwnode is something like
> > > >
> > > > 	struct fwnode_handle *fwnode = dev_fwnode(parent);
> > > 
> > > I tried to use device_set_node(), but then I got some other issue: as we
> > > now have several devices with the same firmware node, they all share the
> > > same properties. In particular, if we do use pinctrl- properties to
> > > apply some pinmmuxing, all devices will try to apply this pinmuxing and
> > > of course all but one will fail.
> > > 
> > > And this makes me think again about the whole thing, maybe copying the
> > > fwnode or of_node from the parent is not the way to go.
> > > 
> > > So today we rely on the parent node for four drivers:
> > > - keypad and rotary, just to ease a bit the parsing of some properties,
> > >   such as the keymap with matrix_keypad_build_keymap(). I can easily do
> > >   it another way.
> > > - PWM and pinctrl drivers, are a bit more complicated, as in both case
> > >   the device tree node associated with the device is used internally. In
> > >   one case to find the correct PWM device for PWM clients listed in the
> > >   device tree, in the other case to find the pinctrl device when
> > >   applying pinctrl described in the device tree.
> > > 
> > > So maybe I have to find a better way for have this association. One way
> > > would be to modify the device tree bindings to add a PWM and a pinctrl
> > > node, with their own compatible, so they are associated to the
> > > corresponding device. But maybe there is a better way to do it.
> >
> > Okay, so the main question now, why do the device share their properties
> > to begin with? It can be done via fwnode graph or similar APIs (in case
> > it is _really_ needed).
> 
> I wouldn't say the properties are shared: we have a single node in the
> device tree as this is just one device. But as we create several
> (software) devices in the MFD driver, we now have several devices linked
> with a single device tree node.
> 
> One solution would be to create more subnodes in the device tree, one
> for pinctrl and one for PWM, but this feels a bit like describing our
> software implementation in the device tree instead of describing the
> hardware.

I see. From my point of view the above is the correct approach, but
you need to ask DT experts, I'm not one of them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:20PM +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.
>
> ...
>
>
> > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > +					   struct pwm_device *pwm,
> > +					   const struct pwm_waveform *wf,
> > +					   void *_wfhw)
>
> I would expect other way around, i.e. naming with leading underscore(s) to be
> private / local. Ditto for all similar cases.

I get the point, but the 2 existing drivers based on pwm_ops structure
name it that way: drivers/pwm/pwm-axi-pwmgen.c and
drivers/pwm/pwm-stm32.c.

Also, the parameter is mostly unusable as-is, as it is a void*, so I
believe it also makes sense to have no underscore for the correctly
casted one, that we will be using in the function body (wfhw).

>
> ...
>
> > +static int max7360_pwm_read_waveform(struct pwm_chip *chip,
> > +				     struct pwm_device *pwm,
> > +				     void *_wfhw)
> > +{
> > +	struct max7360_pwm_waveform *wfhw = _wfhw;
> > +	struct regmap *regmap;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	regmap = pwmchip_get_drvdata(chip);
> > +
> > +	ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val & BIT(pwm->hwpwm)) {
> > +		wfhw->enabled = true;
>
> Also can be (but up to you)
>
> 	wfhw->enabled = val & BIT(pwm->hwpwm);
> 	if (wfhw->enabled) {
>
> And also see below. Perhaps it is not a good suggestion after all.
>
> > +		ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val);
> > +		wfhw->duty_steps = val;
>
> Set to a garbage in case of error, why?
>

Ok, I'm fixing the whole block of code.

> > +	} else {
> > +		wfhw->enabled = false;
> > +		wfhw->duty_steps = 0;
> > +	}
> > +
> > +	return ret;
> > +}
>
> ...
>
> > +static int max7360_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct pwm_chip *chip;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	if (!dev->parent)
> > +		return dev_err_probe(dev, -ENODEV, "no parent device\n");
>
> Why? Code most likely will fail on the regmap retrieval. Just do that first.
>
> > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
>
> This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> not be realised. If you really need this, it has to have a very good comment
> explaining why and object lifetimes.
>

Thanks, I'm fixing this in this driver and similar code in keypad,
rotary and pinctrl. More details in the child mail.

Thanks for your review!
Mathieu

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

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 03:29:18PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote:

...

> > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > > +					   struct pwm_device *pwm,
> > > +					   const struct pwm_waveform *wf,
> > > +					   void *_wfhw)
> >
> > I would expect other way around, i.e. naming with leading underscore(s) to be
> > private / local. Ditto for all similar cases.
> 
> I get the point, but the 2 existing drivers based on pwm_ops structure
> name it that way: drivers/pwm/pwm-axi-pwmgen.c and
> drivers/pwm/pwm-stm32.c.
> 
> Also, the parameter is mostly unusable as-is, as it is a void*, so I
> believe it also makes sense to have no underscore for the correctly
> casted one, that we will be using in the function body (wfhw).

It's all up to PWM maintainers, but I find this style a bit weird, sorry.
I only saw this in the macros, where it's kinda okay. In functions it's
something that needs an additional thinking and understanding the semantics
of the underscore.

...

> > > +static int max7360_pwm_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct pwm_chip *chip;
> > > +	struct regmap *regmap;
> > > +	int ret;
> > > +
> > > +	if (!dev->parent)
> > > +		return dev_err_probe(dev, -ENODEV, "no parent device\n");
> >
> > Why? Code most likely will fail on the regmap retrieval. Just do that first.
> >
> > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> >
> > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > not be realised. If you really need this, it has to have a very good comment
> > explaining why and object lifetimes.
> 
> Thanks, I'm fixing this in this driver and similar code in keypad,
> rotary and pinctrl. More details in the child mail.

Sure, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-4-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192044.ICDBVYzc-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192044.ICDBVYzc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503192044.ICDBVYzc-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pwm/pwm-max7360.c: In function 'max7360_pwm_request':
>> drivers/pwm/pwm-max7360.c:41:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable]
     struct device *dev;
                    ^~~
   drivers/pwm/pwm-max7360.c: In function 'max7360_pwm_free':
   drivers/pwm/pwm-max7360.c:58:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable]
     struct device *dev;
                    ^~~


vim +/dev +41 drivers/pwm/pwm-max7360.c

    37	
    38	static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
    39	{
    40		struct regmap *regmap;
  > 41		struct device *dev;
    42		int ret;
    43	
    44		regmap = pwmchip_get_drvdata(chip);
    45		dev = regmap_get_device(regmap);
    46	
    47		ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
    48					MAX7360_PORT_CFG_COMMON_PWM, 0);
    49		if (ret)
    50			return ret;
    51	
    52		return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm));
    53	}
    54	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-4-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support
config: nios2-kismet-CONFIG_PINCTRL_MAX7360-CONFIG_PWM_MAX7360-0-0 (https://download.01.org/0day-ci/archive/20250320/202503201022.7smCPVZj-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250320/202503201022.7smCPVZj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503201022.7smCPVZj-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PINCTRL_MAX7360 when selected by PWM_MAX7360
   WARNING: unmet direct dependencies detected for PINCTRL_MAX7360
     Depends on [n]: PINCTRL [=n] && MFD_MAX7360 [=y]
     Selected by [y]:
     - PWM_MAX7360 [=y] && PWM [=y] && MFD_MAX7360 [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v5 05/11] regmap: irq: Add support for chips without separate IRQ status

[Mathieu Dubois-Briand]

Some GPIO chips allow to rise an IRQ on GPIO level changes but do not
provide an IRQ status for each separate line: only the current gpio
level can be retrieved.

Add support for these chips, emulating IRQ status by comparing GPIO
levels with the levels during the previous interrupt.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/base/regmap/regmap-irq.c | 97 +++++++++++++++++++++++++++-------------
 include/linux/regmap.h           |  3 ++
 2 files changed, 69 insertions(+), 31 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 0bcd81389a29..0e53b64d028d 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -33,6 +33,7 @@ struct regmap_irq_chip_data {
 	void *status_reg_buf;
 	unsigned int *main_status_buf;
 	unsigned int *status_buf;
+	unsigned int *prev_status_buf;
 	unsigned int *mask_buf;
 	unsigned int *mask_buf_def;
 	unsigned int *wake_buf;
@@ -332,27 +333,13 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 	return ret;
 }
 
-static irqreturn_t regmap_irq_thread(int irq, void *d)
+static int read_irq_data(struct regmap_irq_chip_data *data)
 {
-	struct regmap_irq_chip_data *data = d;
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	int ret, i;
-	bool handled = false;
 	u32 reg;
 
-	if (chip->handle_pre_irq)
-		chip->handle_pre_irq(chip->irq_drv_data);
-
-	if (chip->runtime_pm) {
-		ret = pm_runtime_get_sync(map->dev);
-		if (ret < 0) {
-			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
-				ret);
-			goto exit;
-		}
-	}
-
 	/*
 	 * Read only registers with active IRQs if the chip has 'main status
 	 * register'. Else read in the statuses, using a single bulk read if
@@ -379,10 +366,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 			reg = data->get_irq_reg(data, chip->main_status, i);
 			ret = regmap_read(map, reg, &data->main_status_buf[i]);
 			if (ret) {
-				dev_err(map->dev,
-					"Failed to read IRQ status %d\n",
-					ret);
-				goto exit;
+				dev_err(map->dev, "Failed to read IRQ status %d\n", ret);
+				return ret;
 			}
 		}
 
@@ -398,10 +383,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				ret = read_sub_irq_data(data, b);
 
 				if (ret != 0) {
-					dev_err(map->dev,
-						"Failed to read IRQ status %d\n",
-						ret);
-					goto exit;
+					dev_err(map->dev, "Failed to read IRQ status %d\n", ret);
+					return ret;
 				}
 			}
 
@@ -418,9 +401,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				       data->status_reg_buf,
 				       chip->num_regs);
 		if (ret != 0) {
-			dev_err(map->dev, "Failed to read IRQ status: %d\n",
-				ret);
-			goto exit;
+			dev_err(map->dev, "Failed to read IRQ status: %d\n", ret);
+			return ret;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -436,7 +418,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -447,10 +429,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 			ret = regmap_read(map, reg, &data->status_buf[i]);
 
 			if (ret != 0) {
-				dev_err(map->dev,
-					"Failed to read IRQ status: %d\n",
-					ret);
-				goto exit;
+				dev_err(map->dev, "Failed to read IRQ status: %d\n", ret);
+				return ret;
 			}
 		}
 	}
@@ -459,6 +439,42 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		for (i = 0; i < data->chip->num_regs; i++)
 			data->status_buf[i] = ~data->status_buf[i];
 
+	return 0;
+}
+
+static irqreturn_t regmap_irq_thread(int irq, void *d)
+{
+	struct regmap_irq_chip_data *data = d;
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	int ret, i;
+	bool handled = false;
+	u32 reg;
+
+	if (chip->handle_pre_irq)
+		chip->handle_pre_irq(chip->irq_drv_data);
+
+	if (chip->runtime_pm) {
+		ret = pm_runtime_get_sync(map->dev);
+		if (ret < 0) {
+			dev_err(map->dev, "IRQ thread failed to resume: %d\n", ret);
+			goto exit;
+		}
+	}
+
+	ret = read_irq_data(data);
+	if (ret < 0)
+		goto exit;
+
+	if (chip->status_is_level) {
+		for (i = 0; i < data->chip->num_regs; i++) {
+			unsigned int val = data->status_buf[i];
+
+			data->status_buf[i] ^= data->prev_status_buf[i];
+			data->prev_status_buf[i] = val;
+		}
+	}
+
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
 	 * there is no race between handling and acknowledging the
@@ -705,6 +721,13 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	if (!d->status_buf)
 		goto err_alloc;
 
+	if (chip->status_is_level) {
+		d->prev_status_buf = kcalloc(chip->num_regs, sizeof(*d->prev_status_buf),
+					     GFP_KERNEL);
+		if (!d->prev_status_buf)
+			goto err_alloc;
+	}
+
 	d->mask_buf = kcalloc(chip->num_regs, sizeof(*d->mask_buf),
 			      GFP_KERNEL);
 	if (!d->mask_buf)
@@ -881,6 +904,16 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
+	/* Store current levels */
+	if (chip->status_is_level) {
+		ret = read_irq_data(d);
+		if (ret < 0)
+			goto err_alloc;
+
+		memcpy(d->prev_status_buf, d->status_buf,
+		       d->chip->num_regs * sizeof(d->prev_status_buf[0]));
+	}
+
 	ret = regmap_irq_create_domain(fwnode, irq_base, chip, d);
 	if (ret)
 		goto err_alloc;
@@ -907,6 +940,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	kfree(d->mask_buf_def);
 	kfree(d->mask_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	kfree(d->status_reg_buf);
 	if (d->config_buf) {
 		for (i = 0; i < chip->num_config_bases; i++)
@@ -983,6 +1017,7 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 	kfree(d->mask_buf);
 	kfree(d->status_reg_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	if (d->config_buf) {
 		for (i = 0; i < d->chip->num_config_bases; i++)
 			kfree(d->config_buf[i]);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3a96d068915f..159527e97f00 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1640,6 +1640,8 @@ struct regmap_irq_chip_data;
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
  * @status_invert: Inverted status register: cleared bits are active interrupts.
+ * @status_is_level: Status register is actuall signal level: Xor status
+ *		     register with previous value to get active interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_in_mask: Use the mask registers for controlling irq type. Use this if
  *		  the hardware provides separate bits for rising/falling edge
@@ -1703,6 +1705,7 @@ struct regmap_irq_chip {
 	unsigned int ack_invert:1;
 	unsigned int clear_ack:1;
 	unsigned int status_invert:1;
+	unsigned int status_is_level:1;
 	unsigned int wake_invert:1;
 	unsigned int type_in_mask:1;
 	unsigned int clear_on_unmask:1;

-- 
2.39.5

Re: [PATCH v5 05/11] regmap: irq: Add support for chips without separate IRQ status

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:21PM +0100, Mathieu Dubois-Briand wrote:
> Some GPIO chips allow to rise an IRQ on GPIO level changes but do not
> provide an IRQ status for each separate line: only the current gpio
> level can be retrieved.
> 
> Add support for these chips, emulating IRQ status by comparing GPIO
> levels with the levels during the previous interrupt.


Some nit-picks below, but either way
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

>  			default:
>  				BUG();
> -				goto exit;
> +				return ret;

Hmm... BUG() implies unreachable, perhaps just a precursor patch to drop this
goto completely?

...

> +	/* Store current levels */
> +	if (chip->status_is_level) {
> +		ret = read_irq_data(d);
> +		if (ret < 0)
> +			goto err_alloc;
> +
> +		memcpy(d->prev_status_buf, d->status_buf,
> +		       d->chip->num_regs * sizeof(d->prev_status_buf[0]));

Perhaps array_size()?

> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 05/11] regmap: irq: Add support for chips without separate IRQ status

[Mathieu Dubois-Briand]

On Tue Mar 18, 2025 at 5:39 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:21PM +0100, Mathieu Dubois-Briand wrote:
> > Some GPIO chips allow to rise an IRQ on GPIO level changes but do not
> > provide an IRQ status for each separate line: only the current gpio
> > level can be retrieved.
> > 
> > Add support for these chips, emulating IRQ status by comparing GPIO
> > levels with the levels during the previous interrupt.
>
>
> Some nit-picks below, but either way
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> ...
>
> >  			default:
> >  				BUG();
> > -				goto exit;
> > +				return ret;
>
> Hmm... BUG() implies unreachable, perhaps just a precursor patch to drop this
> goto completely?
>

Ok, I will add a separate patch to remove the goto.

> ...
>
> > +	/* Store current levels */
> > +	if (chip->status_is_level) {
> > +		ret = read_irq_data(d);
> > +		if (ret < 0)
> > +			goto err_alloc;
> > +
> > +		memcpy(d->prev_status_buf, d->status_buf,
> > +		       d->chip->num_regs * sizeof(d->prev_status_buf[0]));
>
> Perhaps array_size()?
>

OK

> > +	}

Thanks for your review, and thanks for the tag.
Mathieu

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

Re: [PATCH v5 05/11] regmap: irq: Add support for chips without separate IRQ status

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 09:45:28AM +0100, Mathieu Dubois-Briand wrote:
> On Tue Mar 18, 2025 at 5:39 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:21PM +0100, Mathieu Dubois-Briand wrote:

...

> > >  			default:
> > >  				BUG();
> > > -				goto exit;
> > > +				return ret;
> >
> > Hmm... BUG() implies unreachable, perhaps just a precursor patch to drop this
> > goto completely?
> 
> Ok, I will add a separate patch to remove the goto.

I just browsed the code for the similar and there are handful that do this.
At least one commit (from 2011) refers to GCC 4.3.3 that complains, but our
minimum requirement AFAIR is 5.1 nowadays. In any case it's up to you. I am
totally fine if you leave this as is.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Mathieu Dubois-Briand]

GPIO controller often have support for IRQ: allow to easily allocate
both gpio-regmap and regmap-irq in one operation.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
 include/linux/gpio/regmap.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 05f8781b5204..61d5f48b445d 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
  */
 struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
 {
+	struct irq_domain *irq_domain;
 	struct gpio_regmap *gpio;
 	struct gpio_chip *chip;
 	int ret;
@@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (ret < 0)
 		goto err_free_gpio;
 
-	if (config->irq_domain) {
-		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
+	irq_domain = config->irq_domain;
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	if (config->regmap_irq_chip) {
+		struct regmap_irq_chip_data *irq_chip_data;
+
+		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
+						      config->regmap, config->regmap_irq_irqno,
+						      config->regmap_irq_flags, 0,
+						      config->regmap_irq_chip, &irq_chip_data);
+		if (ret)
+			goto err_free_gpio;
+
+		irq_domain = regmap_irq_get_domain(irq_chip_data);
+		if (config->regmap_irq_chip_data)
+			*config->regmap_irq_chip_data = irq_chip_data;
+	}
+#endif
+
+	if (irq_domain) {
+		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
 		if (ret)
 			goto err_remove_gpiochip;
 	}
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index a9f7b7faf57b..55df2527b982 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -40,6 +40,14 @@ struct regmap;
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
+ * @regmap_irq_chip:	(Optional) Pointer on an regmap_irq_chip structure. If
+ *			set, a regmap-irq device will be created and the IRQ
+ *			domain will be set accordingly.
+ * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data
+ *                      structure pointer. If set, it will be populated with a
+ *                      pointer on allocated regmap_irq data.
+ * @regmap_irq_irqno	(Optional) The IRQ the device uses to signal interrupts.
+ * @regmap_irq_flags	(Optional) The IRQF_ flags to use for the interrupt.
  *
  * The ->reg_mask_xlate translates a given base address and GPIO offset to
  * register and mask pair. The base address is one of the given register
@@ -78,6 +86,13 @@ struct gpio_regmap_config {
 	int ngpio_per_reg;
 	struct irq_domain *irq_domain;
 
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	struct regmap_irq_chip *regmap_irq_chip;
+	struct regmap_irq_chip_data **regmap_irq_chip_data;
+	int regmap_irq_irqno;
+	unsigned long regmap_irq_flags;
+#endif
+
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);

-- 
2.39.5

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:
> GPIO controller often have support for IRQ: allow to easily allocate
> both gpio-regmap and regmap-irq in one operation.

...

> -	if (config->irq_domain) {
> -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);

> +	irq_domain = config->irq_domain;

Better to move it into #else, so we avoid double assignment (see below).

> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	if (config->regmap_irq_chip) {
> +		struct regmap_irq_chip_data *irq_chip_data;
> +
> +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> +						      config->regmap, config->regmap_irq_irqno,
> +						      config->regmap_irq_flags, 0,
> +						      config->regmap_irq_chip, &irq_chip_data);
> +		if (ret)
> +			goto err_free_gpio;
> +
> +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> +		if (config->regmap_irq_chip_data)
> +			*config->regmap_irq_chip_data = irq_chip_data;

Hmm... I was under impression that we don't need this to be returned.
Do we have any user of it right now? If not, no need to export until
it is needed.

> +	}

	} else

> +#endif

	irq_domain = config->irq_domain;

> +
> +	if (irq_domain) {
> +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
>  		if (ret)
>  			goto err_remove_gpiochip;
>  	}

...

> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct regmap_irq_chip_data **regmap_irq_chip_data;

But why double pointer?

> +	int regmap_irq_irqno;
> +	unsigned long regmap_irq_flags;
> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Mathieu Dubois-Briand]

On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:
> > GPIO controller often have support for IRQ: allow to easily allocate
> > both gpio-regmap and regmap-irq in one operation.
>
> ...
>
> > -	if (config->irq_domain) {
> > -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
>
> > +	irq_domain = config->irq_domain;
>
> Better to move it into #else, so we avoid double assignment (see below).
>

OK

> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > +	if (config->regmap_irq_chip) {
> > +		struct regmap_irq_chip_data *irq_chip_data;
> > +
> > +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> > +						      config->regmap, config->regmap_irq_irqno,
> > +						      config->regmap_irq_flags, 0,
> > +						      config->regmap_irq_chip, &irq_chip_data);
> > +		if (ret)
> > +			goto err_free_gpio;
> > +
> > +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> > +		if (config->regmap_irq_chip_data)
> > +			*config->regmap_irq_chip_data = irq_chip_data;
>
> Hmm... I was under impression that we don't need this to be returned.
> Do we have any user of it right now? If not, no need to export until
> it is needed.
>

Right, I will remove it.

> > +	}
>
> 	} else
>
> > +#endif
>
> 	irq_domain = config->irq_domain;
>
> > +
> > +	if (irq_domain) {
> > +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
> >  		if (ret)
> >  			goto err_remove_gpiochip;
> >  	}
>
> ...
>
> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > +	struct regmap_irq_chip *regmap_irq_chip;
> > +	struct regmap_irq_chip_data **regmap_irq_chip_data;
>
> But why double pointer?
>

I believe this has to be a double pointer, as it is going to be assigned
a pointer value: data buffer is allocated inside of
devm_regmap_add_irq_chip_fwnode().

But as you said, it's better to remove it and add it later if there is
an use case.

> > +	int regmap_irq_irqno;
> > +	unsigned long regmap_irq_flags;
> > +#endif

Thanks for your review.


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

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 08:55:57AM +0100, Mathieu Dubois-Briand wrote:
> On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > > +	struct regmap_irq_chip *regmap_irq_chip;
> > > +	struct regmap_irq_chip_data **regmap_irq_chip_data;
> >
> > But why double pointer?
> 
> I believe this has to be a double pointer, as it is going to be assigned
> a pointer value: data buffer is allocated inside of
> devm_regmap_add_irq_chip_fwnode().

Yes, but it doesn't need to be a double one in the data structrure, right?

> But as you said, it's better to remove it and add it later if there is
> an use case.

This would be even better for now, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Michael Walle]

Hi,

> GPIO controller often have support for IRQ: allow to easily allocate
> both gpio-regmap and regmap-irq in one operation.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
>  include/linux/gpio/regmap.h | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 05f8781b5204..61d5f48b445d 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
>   */
>  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
>  {
> +	struct irq_domain *irq_domain;
>  	struct gpio_regmap *gpio;
>  	struct gpio_chip *chip;
>  	int ret;
> @@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>  	if (ret < 0)
>  		goto err_free_gpio;
>  
> -	if (config->irq_domain) {
> -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
> +	irq_domain = config->irq_domain;
> +#ifdef CONFIG_GPIOLIB_IRQCHIP

Why do we need this ifdef?

> +	if (config->regmap_irq_chip) {
> +		struct regmap_irq_chip_data *irq_chip_data;
> +
> +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> +						      config->regmap, config->regmap_irq_irqno,
> +						      config->regmap_irq_flags, 0,
> +						      config->regmap_irq_chip, &irq_chip_data);
> +		if (ret)
> +			goto err_free_gpio;
> +
> +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> +		if (config->regmap_irq_chip_data)
> +			*config->regmap_irq_chip_data = irq_chip_data;

I'm not a fan of misusing the config to return any data. Can we have
a normal gpio_regmap_get_...()? Usually, the config is on the stack
of the caller, what if you need to get irq_chip_data afterwards?
Then your caller has to save it somewhere.

Also, what is the advantage of this? Your caller doesn't have to
call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
have to cram all its parameters in the gpio_regmap config. I'd like
to keep that small and simple (but still extensible!). IMHO just
setting the irq_domain is enough to achieve that.

-michael

> +	}
> +#endif
> +
> +	if (irq_domain) {
> +		ret = gpiochip_irqchip_add_domain(chip, irq_domain);
>  		if (ret)
>  			goto err_remove_gpiochip;
>  	}
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index a9f7b7faf57b..55df2527b982 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -40,6 +40,14 @@ struct regmap;
>   * @drvdata:		(Optional) Pointer to driver specific data which is
>   *			not used by gpio-remap but is provided "as is" to the
>   *			driver callback(s).
> + * @regmap_irq_chip:	(Optional) Pointer on an regmap_irq_chip structure. If
> + *			set, a regmap-irq device will be created and the IRQ
> + *			domain will be set accordingly.
> + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data
> + *                      structure pointer. If set, it will be populated with a
> + *                      pointer on allocated regmap_irq data.
> + * @regmap_irq_irqno	(Optional) The IRQ the device uses to signal interrupts.
> + * @regmap_irq_flags	(Optional) The IRQF_ flags to use for the interrupt.
>   *
>   * The ->reg_mask_xlate translates a given base address and GPIO offset to
>   * register and mask pair. The base address is one of the given register
> @@ -78,6 +86,13 @@ struct gpio_regmap_config {
>  	int ngpio_per_reg;
>  	struct irq_domain *irq_domain;
>  
> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	struct regmap_irq_chip *regmap_irq_chip;
> +	struct regmap_irq_chip_data **regmap_irq_chip_data;
> +	int regmap_irq_irqno;
> +	unsigned long regmap_irq_flags;
> +#endif
> +
>  	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
>  			      unsigned int offset, unsigned int *reg,
>  			      unsigned int *mask);

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 8:15 AM CET, Michael Walle wrote:
> Hi,
>
> > GPIO controller often have support for IRQ: allow to easily allocate
> > both gpio-regmap and regmap-irq in one operation.
> >
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > ---
> >  drivers/gpio/gpio-regmap.c  | 23 +++++++++++++++++++++--
> >  include/linux/gpio/regmap.h | 15 +++++++++++++++
> >  2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> > index 05f8781b5204..61d5f48b445d 100644
> > --- a/drivers/gpio/gpio-regmap.c
> > +++ b/drivers/gpio/gpio-regmap.c
> > @@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
> >   */
> >  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
> >  {
> > +	struct irq_domain *irq_domain;
> >  	struct gpio_regmap *gpio;
> >  	struct gpio_chip *chip;
> >  	int ret;
> > @@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
> >  	if (ret < 0)
> >  		goto err_free_gpio;
> >  
> > -	if (config->irq_domain) {
> > -		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
> > +	irq_domain = config->irq_domain;
> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
>
> Why do we need this ifdef?
>

Hum yes, on second thought we probably need to depend on
CONFIG_REGMAP_IRQ here.

> > +	if (config->regmap_irq_chip) {
> > +		struct regmap_irq_chip_data *irq_chip_data;
> > +
> > +		ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent),
> > +						      config->regmap, config->regmap_irq_irqno,
> > +						      config->regmap_irq_flags, 0,
> > +						      config->regmap_irq_chip, &irq_chip_data);
> > +		if (ret)
> > +			goto err_free_gpio;
> > +
> > +		irq_domain = regmap_irq_get_domain(irq_chip_data);
> > +		if (config->regmap_irq_chip_data)
> > +			*config->regmap_irq_chip_data = irq_chip_data;
>
> I'm not a fan of misusing the config to return any data. Can we have
> a normal gpio_regmap_get_...()? Usually, the config is on the stack
> of the caller, what if you need to get irq_chip_data afterwards?
> Then your caller has to save it somewhere.
>

Yes, makes sense. As suggested by Andy Shevchenko, I will remove this
parameter as there is no user today: a way to retrieve it can be added
later if needed.

> Also, what is the advantage of this? Your caller doesn't have to
> call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> have to cram all its parameters in the gpio_regmap config. I'd like
> to keep that small and simple (but still extensible!). IMHO just
> setting the irq_domain is enough to achieve that.

This was a request from Andy on my previous series.

>
> -michael
>

Thanks for your review.
Mathieu

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

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 09:35:15AM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 8:15 AM CET, Michael Walle wrote:

...

> > Also, what is the advantage of this? Your caller doesn't have to
> > call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> > have to cram all its parameters in the gpio_regmap config. I'd like
> > to keep that small and simple (but still extensible!). IMHO just
> > setting the irq_domain is enough to achieve that.
> 
> This was a request from Andy on my previous series.

The benefit is deduplication of a lot of code. You may consider it the same as
GPIO library does with IRQ chip. This is just the same on a different level.

Besides the driver in this series, I would think of other GPIO drivers that
are not (yet) converted to regmap (partially because of this is being absent)
or existing drivers, if any, that may utilise it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Michael Walle]

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

Hi,

> > > Also, what is the advantage of this? Your caller doesn't have to
> > > call devm_regmap_add_irq_chip_fwnode(), but on the flip side you
> > > have to cram all its parameters in the gpio_regmap config. I'd like
> > > to keep that small and simple (but still extensible!). IMHO just
> > > setting the irq_domain is enough to achieve that.
> > 
> > This was a request from Andy on my previous series.
>
> The benefit is deduplication of a lot of code. You may consider it the same as
> GPIO library does with IRQ chip. This is just the same on a different level.

I'd say "a lot of code" is slightly exaggerated :-) I was hesitant
because it sounded like a one-off for the regmap_irq support. There
could theoretically be other irq_domain providers (I think).

I just had a quick look at all the gpio_regmap drivers and they all
use regmap_irq. So maybe it's fair to say that one could be directly
supported within gpio_regmap.

> Besides the driver in this series, I would think of other GPIO drivers that
> are not (yet) converted to regmap (partially because of this is being absent)
> or existing drivers, if any, that may utilise it.

Yes probably all of the existing ones :)

-michael

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

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Michael Walle]

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

Hi,

> > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> >
> > Why do we need this ifdef?
> >
>
> Hum yes, on second thought we probably need to depend on
> CONFIG_REGMAP_IRQ here.

But then, you'd also require the regmap_irq support for chips that
don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
missing a stub version.

-michael

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

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Mathieu Dubois-Briand]

On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote:
> > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > >
> > > Why do we need this ifdef?
> > >
> >
> > Hum yes, on second thought we probably need to depend on
> > CONFIG_REGMAP_IRQ here.
>
> But then, you'd also require the regmap_irq support for chips that
> don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
> missing a stub version.
>

Sorry, maybe my previous message was not clear, when I said "depend",
what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of
"#ifdef CONFIG_GPIOLIB_IRQCHIP"

If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is
built, so we do have both devm_regmap_add_irq_chip_fwnode() and
regmap_irq_get_domain(). So this code block should compile and link
correctly.

I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I
believe this is fine.

Or am I missing something?


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

Re: [PATCH v5 06/11] gpio: regmap: Allow to allocate regmap-irq device

[Michael Walle]

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

On Wed Mar 26, 2025 at 12:00 PM CET, Mathieu Dubois-Briand wrote:
> On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote:
> > > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > > >
> > > > Why do we need this ifdef?
> > > >
> > >
> > > Hum yes, on second thought we probably need to depend on
> > > CONFIG_REGMAP_IRQ here.
> >
> > But then, you'd also require the regmap_irq support for chips that
> > don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be
> > missing a stub version.
> >
>
> Sorry, maybe my previous message was not clear, when I said "depend",
> what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of
> "#ifdef CONFIG_GPIOLIB_IRQCHIP"
>
> If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is
> built, so we do have both devm_regmap_add_irq_chip_fwnode() and
> regmap_irq_get_domain(). So this code block should compile and link
> correctly.

Yes.

> I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I
> believe this is fine.
>
> Or am I missing something?

I'd like to avoid the ifdef macros if possible. Thus you'd need
stubs for devm_regmap_add_irq_chip_fwnode() and
regmap_irq_get_domain() if CONFIG_REGMAP_IRQ is not defined.

Not sure if broonie agrees though (?).

-michael

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

[PATCH v5 07/11] gpio: regmap: Allow to provide init_valid_mask callback

[Mathieu Dubois-Briand]

Allows to populate the gpio_regmap_config structure with
init_valid_mask() callback to set on the final gpio_chip structure.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/gpio/gpio-regmap.c  | 3 +--
 include/linux/gpio/regmap.h | 7 +++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 61d5f48b445d..4c66a90bbbf1 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -262,9 +262,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	chip->names = config->names;
 	chip->label = config->label ?: dev_name(config->parent);
 	chip->can_sleep = regmap_might_sleep(config->regmap);
+	chip->init_valid_mask = config->init_valid_mask;
 
-	chip->request = gpiochip_generic_request;
-	chip->free = gpiochip_generic_free;
 	chip->get = gpio_regmap_get;
 	if (gpio->reg_set_base && gpio->reg_clr_base)
 		chip->set = gpio_regmap_set_with_clear;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 55df2527b982..c14bf65d22e9 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -6,6 +6,7 @@
 struct device;
 struct fwnode_handle;
 struct gpio_regmap;
+struct gpio_chip;
 struct irq_domain;
 struct regmap;
 
@@ -40,6 +41,8 @@ struct regmap;
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
+ * @init_valid_mask:	(Optional) Routine to initialize @valid_mask, to be used
+ *			if not all GPIOs are valid.
  * @regmap_irq_chip:	(Optional) Pointer on an regmap_irq_chip structure. If
  *			set, a regmap-irq device will be created and the IRQ
  *			domain will be set accordingly.
@@ -97,6 +100,10 @@ struct gpio_regmap_config {
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
 
+	int (*init_valid_mask)(struct gpio_chip *gc,
+			       unsigned long *valid_mask,
+			       unsigned int ngpios);
+
 	void *drvdata;
 };
 

-- 
2.39.5

Re: [PATCH v5 07/11] gpio: regmap: Allow to provide init_valid_mask callback

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:23PM +0100, Mathieu Dubois-Briand wrote:
> Allows to populate the gpio_regmap_config structure with
> init_valid_mask() callback to set on the final gpio_chip structure.

...

> -	chip->request = gpiochip_generic_request;
> -	chip->free = gpiochip_generic_free;

Stray change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 07/11] gpio: regmap: Allow to provide init_valid_mask callback

[Mathieu Dubois-Briand]

On Tue Mar 18, 2025 at 5:53 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:23PM +0100, Mathieu Dubois-Briand wrote:
> > Allows to populate the gpio_regmap_config structure with
> > init_valid_mask() callback to set on the final gpio_chip structure.
>
> ...
>
> > -	chip->request = gpiochip_generic_request;
> > -	chip->free = gpiochip_generic_free;
>
> Stray change.

Oops, reminder of a dropped patch, thanks for noticing this.

Best regards,
Mathieu

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

Re: [PATCH v5 07/11] gpio: regmap: Allow to provide init_valid_mask callback

[Michael Walle]

Hi,

> Allows to populate the gpio_regmap_config structure with
> init_valid_mask() callback to set on the final gpio_chip structure.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---

> -	chip->request = gpiochip_generic_request;
> -	chip->free = gpiochip_generic_free;

With this removed:

Reviewed-by: Michael Walle <mwalle@kernel.org>

Re: [PATCH v5 07/11] gpio: regmap: Allow to provide init_valid_mask callback

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 8:02 AM CET, Michael Walle wrote:
> Hi,
>
> > Allows to populate the gpio_regmap_config structure with
> > init_valid_mask() callback to set on the final gpio_chip structure.
> >
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> > ---
>
> > -	chip->request = gpiochip_generic_request;
> > -	chip->free = gpiochip_generic_free;
>
> With this removed:
>
> Reviewed-by: Michael Walle <mwalle@kernel.org>

Yes, reminder of a dropped patch.

Thanks for your review and the tag.
Mathieu

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

[PATCH v5 08/11] 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        |  12 +++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-max7360.c | 246 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 98b4d1633b25..4ff68ec6a990 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1445,6 +1445,18 @@ config GPIO_MADERA
 	help
 	  Support for GPIOs on Cirrus Logic Madera class codecs.
 
+config GPIO_MAX7360
+	tristate "MAX7360 GPIO support"
+	depends on MFD_MAX7360
+	select GPIO_REGMAP
+	select PINCTRL_MAX7360
+	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..4acf0a9dbaba
--- /dev/null
+++ b/drivers/gpio/gpio-max7360.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/bitmap.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX7360_GPIO_PORT	1
+#define MAX7360_GPIO_COL	2
+
+static int max7360_get_available_gpos(struct device *dev, unsigned int *available_gpios)
+{
+	u32 columns;
+	int ret;
+
+	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read columns count\n");
+		return ret;
+	}
+
+	*available_gpios = min(MAX7360_MAX_GPO, MAX7360_MAX_KEY_COLS - columns);
+
+	return 0;
+}
+
+static int max7360_gpo_init_valid_mask(struct gpio_chip *gc,
+				       unsigned long *valid_mask,
+				       unsigned int ngpios)
+{
+	unsigned int available_gpios;
+	int ret;
+
+	ret = max7360_get_available_gpos(gc->parent, &available_gpios);
+	if (ret)
+		return ret;
+
+	bitmap_clear(valid_mask, 0, MAX7360_MAX_KEY_COLS - ngpios);
+
+	return 0;
+}
+
+static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
+{
+	/*
+	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
+	 * general purpose output or a mix of both.
+	 * By default, all pins are used as keypad, here we update this
+	 * configuration to allow to use some of them as GPIOs.
+	 */
+	unsigned int available_gpios;
+	unsigned int val;
+	int ret;
+
+	ret = max7360_get_available_gpos(dev, &available_gpios);
+	if (ret)
+		return ret;
+
+	/*
+	 * Configure which GPIOs will be used for keypad.
+	 * 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, available_gpios);
+	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
+	if (ret) {
+		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_gpio_reg_mask_xlate(struct gpio_regmap *gpio,
+				       unsigned int base, unsigned int offset,
+				       unsigned int *reg, unsigned int *mask)
+{
+	if (base == MAX7360_REG_PWMBASE) {
+		/*
+		 * GPIO output is using PWM duty cycle registers: one register
+		 * per line, with value being either 0 or 255.
+		 */
+		*reg = base + offset;
+		*mask = 0xFF;
+	} else {
+		*reg = base;
+		*mask = BIT(offset);
+	}
+
+	return 0;
+}
+
+static const struct regmap_irq max7360_regmap_irqs[MAX7360_MAX_GPIO] = {
+	REGMAP_IRQ_REG(0, 0, BIT(0)),
+	REGMAP_IRQ_REG(1, 0, BIT(1)),
+	REGMAP_IRQ_REG(2, 0, BIT(2)),
+	REGMAP_IRQ_REG(3, 0, BIT(3)),
+	REGMAP_IRQ_REG(4, 0, BIT(4)),
+	REGMAP_IRQ_REG(5, 0, BIT(5)),
+	REGMAP_IRQ_REG(6, 0, BIT(6)),
+	REGMAP_IRQ_REG(7, 0, BIT(7)),
+};
+
+static int max7360_handle_mask_sync(const int index,
+				    const unsigned int mask_buf_def,
+				    const unsigned int mask_buf,
+				    void *const irq_drv_data)
+{
+	struct regmap *regmap = irq_drv_data;
+	unsigned int val;
+
+	for (unsigned int i = 0; i < MAX7360_MAX_GPIO; ++i) {
+		val = (mask_buf & BIT(i)) ? MAX7360_PORT_CFG_INTERRUPT_MASK : 0;
+		regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+				  MAX7360_PORT_CFG_INTERRUPT_MASK, val);
+	}
+
+	return 0;
+}
+
+static int max7360_gpio_probe(struct platform_device *pdev)
+{
+	struct regmap_irq_chip *irq_chip;
+	struct gpio_regmap_config gpio_config = { };
+	struct device *dev = &pdev->dev;
+	unsigned long gpio_function;
+	struct regmap *regmap;
+	unsigned int outconf;
+	int ret;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
+
+	gpio_function = (uintptr_t)device_get_match_data(dev);
+
+	if (gpio_function == MAX7360_GPIO_PORT &&
+	    (device_property_read_bool(dev, "interrupt-controller"))) {
+		/*
+		 * Port GPIOs with interrupt-controller property: add IRQ
+		 * controller.
+		 */
+		gpio_config.regmap_irq_flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
+		gpio_config.regmap_irq_irqno = fwnode_irq_get_byname(dev_fwnode(dev->parent),
+								     "inti");
+		if (gpio_config.regmap_irq_irqno < 0)
+			return dev_err_probe(dev, gpio_config.regmap_irq_irqno,
+					     "Failed to get IRQ\n");
+
+		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
+		gpio_config.regmap_irq_chip = irq_chip;
+		if (!irq_chip)
+			return -ENOMEM;
+
+		irq_chip->name = dev_name(dev);
+		irq_chip->status_base = MAX7360_REG_GPIOIN;
+		irq_chip->num_regs = 1;
+		irq_chip->num_irqs = MAX7360_MAX_GPIO;
+		irq_chip->irqs = max7360_regmap_irqs;
+		irq_chip->handle_mask_sync = max7360_handle_mask_sync;
+		irq_chip->status_is_level = true;
+		irq_chip->irq_drv_data = regmap;
+
+		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
+			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
+		}
+	}
+
+	if (gpio_function == MAX7360_GPIO_PORT) {
+		/*
+		 * Port GPIOs: set output mode configuration (constant-current or not).
+		 * This property is optional.
+		 */
+		outconf = 0;
+		ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf);
+		if (ret && (ret != -EINVAL))
+			return dev_err_probe(dev, ret, "Failed to read %s device property\n",
+					     "maxim,constant-current-disable");
+
+		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
+	}
+
+	/* Add gpio device. */
+	gpio_config.parent = dev;
+	gpio_config.regmap = regmap;
+	if (gpio_function == MAX7360_GPIO_PORT) {
+		gpio_config.ngpio = MAX7360_MAX_GPIO;
+		gpio_config.reg_dat_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOIN);
+		gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PWMBASE);
+		gpio_config.reg_dir_out_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOCTRL);
+		gpio_config.ngpio_per_reg = MAX7360_MAX_GPIO;
+		gpio_config.reg_mask_xlate = max7360_gpio_reg_mask_xlate;
+	} else {
+		ret = max7360_set_gpos_count(dev, regmap);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n");
+
+		gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PORTS);
+		gpio_config.ngpio = MAX7360_MAX_KEY_COLS;
+		gpio_config.init_valid_mask = max7360_gpo_init_valid_mask;
+	}
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
+}
+
+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-gpio",
+		.of_match_table = 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_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:24PM +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.

...

+ bitfield.h

> +#include <linux/bitmap.h>

+ err.h

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max7360.h>

+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>

> +static int max7360_get_available_gpos(struct device *dev, unsigned int *available_gpios)
> +{
> +	u32 columns;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
> +	if (ret < 0) {

' < 0' is redundant,

> +		dev_err(dev, "Failed to read columns count\n");
> +		return ret;
> +	}
> +
> +	*available_gpios = min(MAX7360_MAX_GPO, MAX7360_MAX_KEY_COLS - columns);
> +
> +	return 0;
> +}

...

> +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
> +{
> +	/*
> +	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
> +	 * general purpose output or a mix of both.
> +	 * By default, all pins are used as keypad, here we update this
> +	 * configuration to allow to use some of them as GPIOs.
> +	 */
> +	unsigned int available_gpios;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = max7360_get_available_gpos(dev, &available_gpios);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure which GPIOs will be used for keypad.
> +	 * 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, available_gpios);
> +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> +	if (ret) {
> +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");

> +		return ret;
> +	}
> +
> +	return 0;

Just

	return ret;

?

> +}

...

> +static int max7360_gpio_reg_mask_xlate(struct gpio_regmap *gpio,
> +				       unsigned int base, unsigned int offset,
> +				       unsigned int *reg, unsigned int *mask)
> +{
> +	if (base == MAX7360_REG_PWMBASE) {
> +		/*
> +		 * GPIO output is using PWM duty cycle registers: one register
> +		 * per line, with value being either 0 or 255.
> +		 */
> +		*reg = base + offset;
> +		*mask = 0xFF;

GENMASK() ?

> +	} else {
> +		*reg = base;
> +		*mask = BIT(offset);
> +	}
> +
> +	return 0;
> +}

...

> +static int max7360_handle_mask_sync(const int index,
> +				    const unsigned int mask_buf_def,
> +				    const unsigned int mask_buf,
> +				    void *const irq_drv_data)
> +{
> +	struct regmap *regmap = irq_drv_data;
> +	unsigned int val;
> +
> +	for (unsigned int i = 0; i < MAX7360_MAX_GPIO; ++i) {
> +		val = (mask_buf & BIT(i)) ? MAX7360_PORT_CFG_INTERRUPT_MASK : 0;
> +		regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> +				  MAX7360_PORT_CFG_INTERRUPT_MASK, val);

Wondering if regmap_assign_bits() can be used here.

But in any case, no error checks? It seems you do elsewhere, but this driver...

> +	}
> +
> +	return 0;
> +}

...

> +static int max7360_gpio_probe(struct platform_device *pdev)
> +{
> +	struct regmap_irq_chip *irq_chip;
> +	struct gpio_regmap_config gpio_config = { };
> +	struct device *dev = &pdev->dev;
> +	unsigned long gpio_function;
> +	struct regmap *regmap;
> +	unsigned int outconf;
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
> +
> +	gpio_function = (uintptr_t)device_get_match_data(dev);

> +

Redundant blank line.

> +	if (gpio_function == MAX7360_GPIO_PORT &&
> +	    (device_property_read_bool(dev, "interrupt-controller"))) {

Unneeded parentheses.

> +		/*
> +		 * Port GPIOs with interrupt-controller property: add IRQ
> +		 * controller.
> +		 */
> +		gpio_config.regmap_irq_flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;

But why is this being overridden? The DT or another firmware description has to
provide the correct settings, no?

> +		gpio_config.regmap_irq_irqno = fwnode_irq_get_byname(dev_fwnode(dev->parent),
> +								     "inti");

Better split is

		gpio_config.regmap_irq_irqno =
			fwnode_irq_get_byname(dev_fwnode(dev->parent), "inti");

You also can use the same trick elsewhere in the similar cases.

> +		if (gpio_config.regmap_irq_irqno < 0)
> +			return dev_err_probe(dev, gpio_config.regmap_irq_irqno,
> +					     "Failed to get IRQ\n");
> +
> +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> +		gpio_config.regmap_irq_chip = irq_chip;
> +		if (!irq_chip)
> +			return -ENOMEM;
> +
> +		irq_chip->name = dev_name(dev);
> +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> +		irq_chip->num_regs = 1;
> +		irq_chip->num_irqs = MAX7360_MAX_GPIO;
> +		irq_chip->irqs = max7360_regmap_irqs;
> +		irq_chip->handle_mask_sync = max7360_handle_mask_sync;
> +		irq_chip->status_is_level = true;

I would group this with status_base above. Easier to read and I think they are
kinda related.

> +		irq_chip->irq_drv_data = regmap;
> +
> +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);

No error checks?

> +		}

> +	}
> +

Probably a comment why it's not 'else if' here?

> +	if (gpio_function == MAX7360_GPIO_PORT) {
> +		/*
> +		 * Port GPIOs: set output mode configuration (constant-current or not).
> +		 * This property is optional.
> +		 */
> +		outconf = 0;
> +		ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf);

> +		if (ret && (ret != -EINVAL))
> +			return dev_err_probe(dev, ret, "Failed to read %s device property\n",
> +					     "maxim,constant-current-disable");

This part is fragile, error codes are not _so_ stable inside the kernel,
and this may add an unneeded churn in case of pedantic cleanup.

Personally I would drop any messages and avoid failing the probe as to me it
does not sound like a critical issue.

> +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> +	}
> +
> +	/* Add gpio device. */
> +	gpio_config.parent = dev;
> +	gpio_config.regmap = regmap;
> +	if (gpio_function == MAX7360_GPIO_PORT) {
> +		gpio_config.ngpio = MAX7360_MAX_GPIO;
> +		gpio_config.reg_dat_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOIN);
> +		gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PWMBASE);
> +		gpio_config.reg_dir_out_base = GPIO_REGMAP_ADDR(MAX7360_REG_GPIOCTRL);
> +		gpio_config.ngpio_per_reg = MAX7360_MAX_GPIO;
> +		gpio_config.reg_mask_xlate = max7360_gpio_reg_mask_xlate;
> +	} else {
> +		ret = max7360_set_gpos_count(dev, regmap);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to set GPOS pin count\n");
> +
> +		gpio_config.reg_set_base = GPIO_REGMAP_ADDR(MAX7360_REG_PORTS);
> +		gpio_config.ngpio = MAX7360_MAX_KEY_COLS;
> +		gpio_config.init_valid_mask = max7360_gpo_init_valid_mask;
> +	}
> +
> +	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_config));
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 12:50 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:24PM +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.
>
> ...
>
> > +		/*
> > +		 * Port GPIOs with interrupt-controller property: add IRQ
> > +		 * controller.
> > +		 */
> > +		gpio_config.regmap_irq_flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
>
> But why is this being overridden? The DT or another firmware description has to
> provide the correct settings, no?
>

Ok, thinking about it, yes, IRQF_TRIGGER_LOW shoud come from firmware
description. But IRQF_ONESHOT and IRQF_SHARED should still come from
here, no?


I'm OK with all other points.

Thanks for your review!
Mathieu

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

Re: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 03:46:20PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 12:50 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:24PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +		/*
> > > +		 * Port GPIOs with interrupt-controller property: add IRQ
> > > +		 * controller.
> > > +		 */
> > > +		gpio_config.regmap_irq_flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> >
> > But why is this being overridden? The DT or another firmware description has to
> > provide the correct settings, no?
> 
> Ok, thinking about it, yes, IRQF_TRIGGER_LOW shoud come from firmware
> description. But IRQF_ONESHOT and IRQF_SHARED should still come from
> here, no?

This is my view as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build errors:

[auto build test ERROR on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-8-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192257.KfRkL589-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192257.KfRkL589-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503192257.KfRkL589-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-max7360.c: In function 'max7360_set_gpos_count':
>> drivers/gpio/gpio-max7360.c:78:8: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     val = FIELD_PREP(MAX7360_PORTS, available_gpios);
           ^~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_PREP +78 drivers/gpio/gpio-max7360.c

    55	
    56	static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
    57	{
    58		/*
    59		 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
    60		 * general purpose output or a mix of both.
    61		 * By default, all pins are used as keypad, here we update this
    62		 * configuration to allow to use some of them as GPIOs.
    63		 */
    64		unsigned int available_gpios;
    65		unsigned int val;
    66		int ret;
    67	
    68		ret = max7360_get_available_gpos(dev, &available_gpios);
    69		if (ret)
    70			return ret;
    71	
    72		/*
    73		 * Configure which GPIOs will be used for keypad.
    74		 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
    75		 * timings and gpos/keypad columns repartition. Only the later is
    76		 * modified here.
    77		 */
  > 78		val = FIELD_PREP(MAX7360_PORTS, available_gpios);
    79		ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
    80		if (ret) {
    81			dev_err(dev, "Failed to write max7360 columns/gpos configuration");
    82			return ret;
    83		}
    84	
    85		return 0;
    86	}
    87	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-8-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 08/11] gpio: max7360: Add MAX7360 gpio support
config: nios2-kismet-CONFIG_PINCTRL_MAX7360-CONFIG_GPIO_MAX7360-0-0 (https://download.01.org/0day-ci/archive/20250320/202503200617.h8re2FlY-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250320/202503200617.h8re2FlY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503200617.h8re2FlY-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PINCTRL_MAX7360 when selected by GPIO_MAX7360
   WARNING: unmet direct dependencies detected for PINCTRL_MAX7360
     Depends on [n]: PINCTRL [=n] && MFD_MAX7360 [=y]
     Selected by [y]:
     - GPIO_MAX7360 [=y] && GPIOLIB [=y] && MFD_MAX7360 [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v5 09/11] 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 | 264 ++++++++++++++++++++++++++++++++
 3 files changed, 277 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..bea4621c0622
--- /dev/null
+++ b/drivers/input/keyboard/max7360-keypad.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 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/property.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 ret;
+
+	do {
+		ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
+		if (ret) {
+			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 ret;
+
+	/*
+	 * Somebody is using the device: get out of sleep.
+	 */
+	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
+				MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
+	if (ret) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void max7360_keypad_close(struct input_dev *pdev)
+{
+	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
+	int ret;
+
+	/*
+	 * Nobody is using the device anymore: go to sleep.
+	 */
+	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0);
+	if (ret)
+		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 ret;
+
+	val = max7360_keypad->debounce_ms - MAX7360_DEBOUNCE_MIN;
+	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_DEBOUNCE,
+				MAX7360_DEBOUNCE,
+				FIELD_PREP(MAX7360_DEBOUNCE, val));
+	if (ret) {
+		return dev_err_probe(&max7360_keypad->input->dev, ret,
+			"Failed to write max7360 debounce configuration\n");
+	}
+
+	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
+				MAX7360_INTERRUPT_TIME_MASK,
+				FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
+	if (ret) {
+		return dev_err_probe(&max7360_keypad->input->dev, ret,
+			"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 ret;
+
+	ret = matrix_keypad_parse_properties(pdev->dev.parent, &max7360_keypad->rows,
+					     &max7360_keypad->cols);
+	if (ret)
+		return ret;
+
+	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;
+	ret = device_property_read_u32(pdev->dev.parent, "keypad-debounce-delay-ms",
+				       &max7360_keypad->debounce_ms);
+	if (ret == -EINVAL) {
+		dev_info(&pdev->dev, "Using default keypad-debounce-delay-ms: %u\n",
+			 max7360_keypad->debounce_ms);
+	} else if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Failed to read keypad-debounce-delay-ms property\n");
+		return ret;
+	} 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 ret;
+	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");
+
+	ret = max7360_keypad_parse_dt(pdev, max7360_keypad, &autorepeat);
+	if (ret)
+		return ret;
+
+	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;
+
+	ret = matrix_keypad_build_keymap(NULL, NULL, MAX7360_MAX_KEY_ROWS, MAX7360_MAX_KEY_COLS,
+					 max7360_keypad->keycodes, input);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "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);
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					"max7360-keypad", max7360_keypad);
+	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_keypad);
+
+	ret = max7360_keypad_hw_init(max7360_keypad);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to initialize max7360 keypad\n");
+
+	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);
+
+	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-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_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v5 09/11] input: keyboard: Add support for MAX7360 keypad

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:25PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 keypad controller, providing
> support for up to 64 keys, with a matrix of 8 columns and 8 rows.

...

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

One paragraph is wrapped way too late or too early, can you make them approx.
the same in terms of a line width?

...

+ bitfield.h
+ bitops.h
+ dev_printk.h
+ device/devres.h
+ err.h

> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max7360.h>

+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>

> +#include <linux/slab.h>

IS it used? I think it's device/devres.h that covers it.


...

> +static int max7360_keypad_open(struct input_dev *pdev)
> +{
> +	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
> +	int ret;
> +
> +	/*
> +	 * Somebody is using the device: get out of sleep.
> +	 */
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
> +				MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
> +	if (ret) {
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to write max7360 configuration\n");

> +		return ret;
> +	}
> +
> +	return 0;

Just

	return ret;

?

> +}

...

> +	/*
> +	 * Nobody is using the device anymore: go to sleep.
> +	 */

The comment message can take only a line.

> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0);
> +	if (ret)
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to write max7360 configuration\n");
> +}

...

> +static int max7360_keypad_parse_dt(struct platform_device *pdev,

s/dt/fw

> +				   struct max7360_keypad *max7360_keypad,
> +				   bool *autorepeat)
> +{

	struct device *dev = &pdev>dev;

but why not supply struct device to begin with? How is the platform part used here?

> +	int ret;
> +
> +	ret = matrix_keypad_parse_properties(pdev->dev.parent, &max7360_keypad->rows,
> +					     &max7360_keypad->cols);
> +	if (ret)
> +		return ret;
> +
> +	if (!max7360_keypad->rows || !max7360_keypad->cols ||
> +	    max7360_keypad->rows > MAX7360_MAX_KEY_ROWS ||
> +	    max7360_keypad->cols > MAX7360_MAX_KEY_COLS) {

See also below comment.

> +		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;
> +	ret = device_property_read_u32(pdev->dev.parent, "keypad-debounce-delay-ms",
> +				       &max7360_keypad->debounce_ms);
> +	if (ret == -EINVAL) {
> +		dev_info(&pdev->dev, "Using default keypad-debounce-delay-ms: %u\n",
> +			 max7360_keypad->debounce_ms);
> +	} else if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Failed to read keypad-debounce-delay-ms property\n");
> +		return ret;

> +	} else if (max7360_keypad->debounce_ms < MAX7360_DEBOUNCE_MIN ||

Redundant 'else'.

> +		   max7360_keypad->debounce_ms > MAX7360_DEBOUNCE_MAX) {

Maybe in_range()? But up to you, it takes start:len and not start:end.

> +		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 device *dev = &pdev>dev;

> +	struct input_dev *input;
> +	bool autorepeat;
> +	int ret;
> +	int irq;

> +	if (!pdev->dev.parent)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");

Just do like in the rest, i.e. local variable for regmap and its validness will
be the one that indicates the wrong enumeration path.

> +	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");
> +
> +	ret = max7360_keypad_parse_dt(pdev, max7360_keypad, &autorepeat);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +
> +	ret = matrix_keypad_build_keymap(NULL, NULL, MAX7360_MAX_KEY_ROWS, MAX7360_MAX_KEY_COLS,
> +					 max7360_keypad->keycodes, input);
> +	if (ret)

> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to build keymap\n");

One line.

> +
> +	input_set_capability(input, EV_MSC, MSC_SCAN);
> +	if (autorepeat)
> +		__set_bit(EV_REP, input->evbit);
> +
> +	input_set_drvdata(input, max7360_keypad);
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq,
> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,

What's wrong with the interrupt flags provided by firmware description?

> +					"max7360-keypad", max7360_keypad);
> +	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_keypad);

Is it used?

> +	ret = max7360_keypad_hw_init(max7360_keypad);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to initialize max7360 keypad\n");
> +
> +	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);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/11] input: keyboard: Add support for MAX7360 keypad

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 1:02 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:25PM +0100, Mathieu Dubois-Briand wrote:
> > Add driver for Maxim Integrated MAX7360 keypad controller, providing
> > support for up to 64 keys, with a matrix of 8 columns and 8 rows.
>
> ...
>
> > +	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.
>
> One paragraph is wrapped way too late or too early, can you make them approx.
> the same in terms of a line width?
>

Thanks. I had a look at all other Kconfigs, I believe they are all fixed
now.

> > +
> > +	input_set_capability(input, EV_MSC, MSC_SCAN);
> > +	if (autorepeat)
> > +		__set_bit(EV_REP, input->evbit);
> > +
> > +	input_set_drvdata(input, max7360_keypad);
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq,
> > +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>
> What's wrong with the interrupt flags provided by firmware description?
>

So same question as for the GPIO driver: IRQF_TRIGGER_LOW from the
firmware, but IRQF_ONESHOT from the driver? Or should everything come
from the firmware?

Thanks again for your review.
Mathieu

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

Re: [PATCH v5 09/11] input: keyboard: Add support for MAX7360 keypad

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 03:57:01PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 1:02 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:25PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq,
> > > +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> >
> > What's wrong with the interrupt flags provided by firmware description?
> 
> So same question as for the GPIO driver: IRQF_TRIGGER_LOW from the
> firmware, but IRQF_ONESHOT from the driver? Or should everything come
> from the firmware?

The same answer, yes, the Linux stuff (e.g., ONESHOT) should be given
explicitly here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 09/11] input: keyboard: Add support for MAX7360 keypad

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build errors:

[auto build test ERROR on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-9-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 09/11] input: keyboard: Add support for MAX7360 keypad
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192258.ULXxG0T4-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192258.ULXxG0T4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503192258.ULXxG0T4-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/input/keyboard/max7360-keypad.c: In function 'max7360_keypad_irq':
>> drivers/input/keyboard/max7360-keypad.c:57:8: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
     row = FIELD_GET(MAX7360_FIFO_ROW, val);
           ^~~~~~~~~
   drivers/input/keyboard/max7360-keypad.c: In function 'max7360_keypad_hw_init':
>> drivers/input/keyboard/max7360-keypad.c:114:5: error: implicit declaration of function 'FIELD_PREP'; did you mean 'EV_REP'? [-Werror=implicit-function-declaration]
        FIELD_PREP(MAX7360_DEBOUNCE, val));
        ^~~~~~~~~~
        EV_REP
   cc1: some warnings being treated as errors


vim +/FIELD_GET +57 drivers/input/keyboard/max7360-keypad.c

    29	
    30	static irqreturn_t max7360_keypad_irq(int irq, void *data)
    31	{
    32		struct max7360_keypad *max7360_keypad = data;
    33		unsigned int val;
    34		unsigned int row, col;
    35		unsigned int release;
    36		unsigned int code;
    37		int ret;
    38	
    39		do {
    40			ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
    41			if (ret) {
    42				dev_err(&max7360_keypad->input->dev, "Failed to read max7360 FIFO");
    43				return IRQ_NONE;
    44			}
    45	
    46			/* FIFO overflow: ignore it and get next event. */
    47			if (val == MAX7360_FIFO_OVERFLOW)
    48				dev_warn(&max7360_keypad->input->dev, "max7360 FIFO overflow");
    49		} while (val == MAX7360_FIFO_OVERFLOW);
    50	
    51		if (val == MAX7360_FIFO_EMPTY) {
    52			dev_dbg(&max7360_keypad->input->dev, "Got a spurious interrupt");
    53	
    54			return IRQ_NONE;
    55		}
    56	
  > 57		row = FIELD_GET(MAX7360_FIFO_ROW, val);
    58		col = FIELD_GET(MAX7360_FIFO_COL, val);
    59		release = val & MAX7360_FIFO_RELEASE;
    60	
    61		code = MATRIX_SCAN_CODE(row, col, MAX7360_ROW_SHIFT);
    62	
    63		dev_dbg(&max7360_keypad->input->dev, "key[%d:%d] %s\n", row, col,
    64			release ? "release" : "press");
    65	
    66		input_event(max7360_keypad->input, EV_MSC, MSC_SCAN, code);
    67		input_report_key(max7360_keypad->input, max7360_keypad->keycodes[code], !release);
    68		input_sync(max7360_keypad->input);
    69	
    70		return IRQ_HANDLED;
    71	}
    72	
    73	static int max7360_keypad_open(struct input_dev *pdev)
    74	{
    75		struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
    76		int ret;
    77	
    78		/*
    79		 * Somebody is using the device: get out of sleep.
    80		 */
    81		ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
    82					MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
    83		if (ret) {
    84			dev_err(&max7360_keypad->input->dev,
    85				"Failed to write max7360 configuration\n");
    86			return ret;
    87		}
    88	
    89		return 0;
    90	}
    91	
    92	static void max7360_keypad_close(struct input_dev *pdev)
    93	{
    94		struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
    95		int ret;
    96	
    97		/*
    98		 * Nobody is using the device anymore: go to sleep.
    99		 */
   100		ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0);
   101		if (ret)
   102			dev_err(&max7360_keypad->input->dev,
   103				"Failed to write max7360 configuration\n");
   104	}
   105	
   106	static int max7360_keypad_hw_init(struct max7360_keypad *max7360_keypad)
   107	{
   108		unsigned int val;
   109		int ret;
   110	
   111		val = max7360_keypad->debounce_ms - MAX7360_DEBOUNCE_MIN;
   112		ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_DEBOUNCE,
   113					MAX7360_DEBOUNCE,
 > 114					FIELD_PREP(MAX7360_DEBOUNCE, val));
   115		if (ret) {
   116			return dev_err_probe(&max7360_keypad->input->dev, ret,
   117				"Failed to write max7360 debounce configuration\n");
   118		}
   119	
   120		ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
   121					MAX7360_INTERRUPT_TIME_MASK,
   122					FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
   123		if (ret) {
   124			return dev_err_probe(&max7360_keypad->input->dev, ret,
   125				"Failed to write max7360 keypad interrupt configuration\n");
   126		}
   127	
   128		return 0;
   129	}
   130	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v5 10/11] 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 | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 13d135257e06..77b07e053265 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 MFD_MAX7360
+	select PINCTRL_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 6d91804d0a6f..c454fba3a3ae 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..3046ef64dd56
--- /dev/null
+++ b/drivers/input/misc/max7360-rotary.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 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/property.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;
+
+	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");
+
+	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);
+}
+
+static struct platform_driver max7360_rotary_driver = {
+	.driver = {
+		.name	= "max7360-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_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 rotary encoder controller,
> supporting a single rotary switch.

...

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

Seems all your patches have unaligned line widths in the help texts.

...

+ bitfield.h
+ device/devres.h
+ dev_printk.h

> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/property.h>

> +#include <linux/of.h>

Why?

> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>

> +#include <linux/slab.h>

Not needed.

+ types.h

...

> +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);

This is strange:
1) why casting to begin with?
2) why to C type and not kernel (s8) type?

> +	input_sync(max7360_rotary->input);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary)
> +{
> +	int val;
> +	int 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;

Just

	return ret;

?

> +}

...

> +static int max7360_rotary_probe(struct platform_device *pdev)
> +{
> +	struct max7360_rotary *max7360_rotary;

	struct device *dev = &pdev->dev;

> +	struct input_dev *input;
> +	int irq;
> +	int ret;

> +	if (!pdev->dev.parent)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");

Just check for regmap.

> +	irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent),
> +				      "inti");

One line (and instead of that dance with container_of(), just use
fwnode_irq_get_byname().

> +	if (irq < 0)
> +		return irq;
> +
> +	max7360_rotary = devm_kzalloc(&pdev->dev, sizeof(*max7360_rotary),
> +				      GFP_KERNEL);

One line.

> +	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");

One line.

> +	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);

No, instead of parent, make sure that fwnode is propagated to the children (and
tadaam, it's done in MFD already).

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

What's wrong with the firmware description of the interrupt?

> +					"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");

One line.

> +	platform_set_drvdata(pdev, max7360_rotary);

Is it used?

> +	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");

In the other driver the HW initialisation and wake setup were swapped,
can you choose one way? Otherwise both cases need a comment to explain
the order of calls.

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary

[Mathieu Dubois-Briand]

On Wed Mar 19, 2025 at 1:11 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote:
> > Add driver for Maxim Integrated MAX7360 rotary encoder controller,
> > supporting a single rotary switch.
>
> ...
>
> > +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);
>
> This is strange:
> 1) why casting to begin with?
> 2) why to C type and not kernel (s8) type?
>

I believe the cast is needed, as, while the value read with
regmap_read() is stored in an int, the underlying value is indeed a
signed 8 bits integer.

Without cast negative values will not be correct: -1 (0xFF) -> will be
interpreted as 255 (0x000000FF).

Ok for s8.

Thanks for your review
Mathieu

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

Re: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary

[Andy Shevchenko]

On Tue, Mar 25, 2025 at 04:56:20PM +0100, Mathieu Dubois-Briand wrote:
> On Wed Mar 19, 2025 at 1:11 PM CET, Andy Shevchenko wrote:
> > On Tue, Mar 18, 2025 at 05:26:26PM +0100, Mathieu Dubois-Briand wrote:

...

> > > +	int val;

Btw, this has to be unsigned to match the API.

> > > +	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;
> > > +	}

> > > +	input_report_rel(max7360_rotary->input, max7360_rotary->axis,
> > > +			 (int8_t)val);
> >
> > This is strange:
> > 1) why casting to begin with?
> > 2) why to C type and not kernel (s8) type?
> 
> I believe the cast is needed, as, while the value read with
> regmap_read() is stored in an int, the underlying value is indeed a
> signed 8 bits integer.
> 
> Without cast negative values will not be correct: -1 (0xFF) -> will be
> interpreted as 255 (0x000000FF).

With the above fix it makes sense, but it's not clear for the reader still.
What you want is most likely to call sign_extend32().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build errors:

[auto build test ERROR on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-10-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192326.KOCnEkdj-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192326.KOCnEkdj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503192326.KOCnEkdj-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/input/misc/max7360-rotary.c: In function 'max7360_rotary_hw_init':
>> drivers/input/misc/max7360-rotary.c:58:8: error: implicit declaration of function 'FIELD_PREP'; did you mean 'EV_REP'? [-Werror=implicit-function-declaration]
     val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) |
           ^~~~~~~~~~
           EV_REP
   cc1: some warnings being treated as errors


vim +58 drivers/input/misc/max7360-rotary.c

    52	
    53	static int max7360_rotary_hw_init(struct max7360_rotary *max7360_rotary)
    54	{
    55		int val;
    56		int ret;
    57	
  > 58		val = FIELD_PREP(MAX7360_ROT_DEBOUNCE, max7360_rotary->debounce_ms) |
    59			FIELD_PREP(MAX7360_ROT_INTCNT, 1) | MAX7360_ROT_INTCNT_DLY;
    60		ret = regmap_write(max7360_rotary->regmap, MAX7360_REG_RTRCFG, val);
    61		if (ret) {
    62			dev_err(&max7360_rotary->input->dev,
    63				"Failed to set max7360 rotary encoder configuration\n");
    64			return ret;
    65		}
    66	
    67		return 0;
    68	}
    69	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base:   a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link:    https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-10-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 10/11] input: misc: Add support for MAX7360 rotary
config: nios2-kismet-CONFIG_PINCTRL_MAX7360-CONFIG_INPUT_MAX7360_ROTARY-0-0 (https://download.01.org/0day-ci/archive/20250320/202503200825.rmtamLkh-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250320/202503200825.rmtamLkh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503200825.rmtamLkh-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PINCTRL_MAX7360 when selected by INPUT_MAX7360_ROTARY
   WARNING: unmet direct dependencies detected for PINCTRL_MAX7360
     Depends on [n]: PINCTRL [=n] && MFD_MAX7360 [=y]
     Selected by [y]:
     - INPUT_MAX7360_ROTARY [=y] && INPUT [=y] && INPUT_MISC [=y] && MFD_MAX7360 [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v5 11/11] 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 25c86f47353d..2b67d356a4a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14259,6 +14259,19 @@ 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/pinctrl/pinctrl-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

Re: [PATCH v5 00/11] Add support for MAX7360

[Andy Shevchenko]

On Tue, Mar 18, 2025 at 05:26:16PM +0100, Mathieu Dubois-Briand wrote:
> 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.

Thanks!

Skeleton more or less looks at it's stable phase, there are tons of the style
and small amendments that may be made. I would expect one or two at most new
versions of this series.

-- 
With Best Regards,
Andy Shevchenko