[PATCH 0/8] Add support for MAX7360 multifunction device

[PATCH 0/8] Add support for MAX7360 multifunction device

[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>
---
Kamel Bouhara (2):
      mfd: Add max7360 support
      pwm: max7360: Add MAX7360 PWM support

Mathieu Dubois-Briand (6):
      dt-bindings: Add MAX7360 MFD device
      dt-bindings: Add MAX7360 subdevices
      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

 .../devicetree/bindings/gpio/max7360-gpio.yaml     |  96 +++++
 .../devicetree/bindings/input/max7360-keypad.yaml  |  67 +++
 .../devicetree/bindings/input/max7360-rotary.yaml  |  52 +++
 Documentation/devicetree/bindings/mfd/max7360.yaml |  56 +++
 .../devicetree/bindings/pwm/max7360-pwm.yaml       |  35 ++
 MAINTAINERS                                        |  12 +
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-max7360.c                        | 453 +++++++++++++++++++++
 drivers/input/keyboard/Kconfig                     |  12 +
 drivers/input/keyboard/Makefile                    |   1 +
 drivers/input/keyboard/max7360-keypad.c            | 297 ++++++++++++++
 drivers/input/misc/Kconfig                         |  11 +
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/max7360-rotary.c                | 194 +++++++++
 drivers/mfd/Kconfig                                |  12 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/max7360.c                              | 302 ++++++++++++++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-max7360.c                          | 173 ++++++++
 include/linux/mfd/max7360.h                        | 103 +++++
 22 files changed, 1902 insertions(+)
---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241219-mdb-max7360-support-223a8ce45ba3

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

[PATCH 1/8] dt-bindings: Add MAX7360 MFD device

[Mathieu Dubois-Briand]

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

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 Documentation/devicetree/bindings/mfd/max7360.yaml | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max7360.yaml b/Documentation/devicetree/bindings/mfd/max7360.yaml
new file mode 100644
index 000000000000..49dd437fd313
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max7360.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/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 MFD device, with following functions:
+    - keypad controller
+    - rotary controller
+    - GPIO and GPO controller
+    - PWM controller
+
+  https://www.analog.com/en/products/max7360.html
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: The interrupt line the device is connected to.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      max7360@38 {
+        compatible = "maxim,max7360";
+        reg = <0x38>;
+
+        interrupt-parent = <&gpio1>;
+        interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
+      };
+    };

-- 
2.39.5

Re: [PATCH 1/8] dt-bindings: Add MAX7360 MFD device

[Krzysztof Kozlowski]

On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> Add device tree bindings for Maxim Integrated MAX7360 MFD device with
> support for keypad, rotary, gpios and pwm functionalities.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Subject/commit msg: drop MFD and explain what the hardware is. MFD is
Linuxism.

> 
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  Documentation/devicetree/bindings/mfd/max7360.yaml | 56 ++++++++++++++++++++++

Use compatible as filename.
>  1 file changed, 56 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max7360.yaml b/Documentation/devicetree/bindings/mfd/max7360.yaml
> new file mode 100644
> index 000000000000..49dd437fd313
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/max7360.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/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 MFD device, with following functions:
> +    - keypad controller
> +    - rotary controller
> +    - GPIO and GPO controller
> +    - PWM controller
> +
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.

Drop description,

> +    maxItems: 1

I don't think this was tested at all. It is heavily incomplete,
considering this is sort of MFD device.

Or you split patches in odd way. Look how other PMIC-style things are
upstreamed.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      max7360@38 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        compatible = "maxim,max7360";
> +        reg = <0x38>;
> +
Best regards,
Krzysztof

[PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Mathieu Dubois-Briand]

Add device tree bindings for Maxim Integrated MAX7360 MFD functions:
keypad, rotary, gpios and pwm.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 .../devicetree/bindings/gpio/max7360-gpio.yaml     | 96 ++++++++++++++++++++++
 .../devicetree/bindings/input/max7360-keypad.yaml  | 67 +++++++++++++++
 .../devicetree/bindings/input/max7360-rotary.yaml  | 52 ++++++++++++
 .../devicetree/bindings/pwm/max7360-pwm.yaml       | 35 ++++++++
 4 files changed, 250 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
new file mode 100644
index 000000000000..3c006dc0380b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/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 MFD
+  https://www.analog.com/en/products/max7360.html
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-gpio
+      - maxim,max7360-gpo
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  ngpios:
+    minimum: 0
+    maximum: 8
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    description: The interrupt line the device is connected to.
+    maxItems: 1
+
+  constant-current-disable:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Bit field, each bit disables constant-current output of the
+                 associated GPIO.
+
+
+required:
+  - compatible
+  - gpio-controller
+  - ngpios
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - maxim,max7360-gpio
+then:
+  required:
+    - interrupt-controller
+    - interrupts
+else:
+  properties:
+    interrupt-controller: false
+    interrupts: false
+    constant-current-disable: false
+
+    ngpios:
+      maximum: 6
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    max7360_gpo: max7360_gpo {
+            compatible = "maxim,max7360-gpo";
+            gpio-controller;
+            #gpio-cells = <0x2>;
+            ngpios = <4>;
+    };
+
+    max7360_gpio: max7360_gpio {
+            compatible = "maxim,max7360-gpio";
+
+            gpio-controller;
+            #gpio-cells = <0x2>;
+            ngpios = <8>;
+            constant-current-disable = <0x06>;
+
+            interrupt-controller;
+            #interrupt-cells = <0x2>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
+    };
diff --git a/Documentation/devicetree/bindings/input/max7360-keypad.yaml b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
new file mode 100644
index 000000000000..8bc3c841465b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/max7360-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7360 Keypad Controller
+
+maintainers:
+  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+
+description: |
+  Maxim MAX7360 Keypad Controller, in MAX7360 MFD
+  https://www.analog.com/en/products/max7360.html
+
+allOf:
+  - $ref: matrix-keymap.yaml#
+  - $ref: input.yaml#
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-keypad
+
+  interrupts:
+    description: The interrupt line the device is connected to.
+    maxItems: 1
+
+  debounce-delay-ms:
+    description: Debounce delay in ms
+    minimum: 9
+    maximum: 40
+    default: 9
+
+  linux,input-no-autorepeat:
+    description: If present, the keys will not autorepeat when pressed
+
+required:
+  - compatible
+  - interrupts
+  - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/input/input.h>
+
+    max7360_keypad {
+      compatible = "maxim,max7360-keypad";
+
+      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)
+        >;
+
+      interrupt-parent = <&gpio1>;
+      interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
+
+      debounce-delay-ms = <10>;
+      linux,input-no-autorepeat;
+    };
diff --git a/Documentation/devicetree/bindings/input/max7360-rotary.yaml b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
new file mode 100644
index 000000000000..19afa8344249
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/max7360-rotary.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7360 Rotary Encoder
+
+maintainers:
+  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+
+description: |
+  Maxim MAX7360 Rotary Encoder, in MAX7360 MFD
+  https://www.analog.com/en/products/max7360.html
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-rotary
+
+  interrupts:
+    description: The interrupt line the device is connected to.
+    maxItems: 1
+
+  debounce-delay-ms:
+    description: Debounce delay in ms
+    minimum: 0
+    maximum: 15
+    default: 0
+
+  linux,axis:
+    description: The input subsystem axis to map to this rotary encoder.
+
+required:
+  - compatible
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    max7360_rotary: max7360_rotary {
+      compatible = "maxim,max7360-rotary";
+
+      debounce-delay-ms = <2>;
+      linux,axis = <0>; /* REL_X */
+
+      interrupt-parent = <&gpio1>;
+      interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
+    };
diff --git a/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml b/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml
new file mode 100644
index 000000000000..68d48969e542
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/max7360-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX7360 PWM controller
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+description: |
+  Maxim MAX7360 PWM controller, in MAX7360 MFD
+  https://www.analog.com/en/products/max7360.html
+
+properties:
+  compatible:
+    enum:
+      - maxim,max7360-pwm
+
+  "#pwm-cells":
+    const: 2
+
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    max7360_pwm: max7360_pwm {
+      compatible = "maxim,max7360-pwm";
+      #pwm-cells = <2>;
+    };

-- 
2.39.5

Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Uwe Kleine-König]

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

Hello,

On Thu, Dec 19, 2024 at 05:21:19PM +0100, Mathieu Dubois-Briand wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml b/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml
> new file mode 100644
> index 000000000000..68d48969e542
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/max7360-pwm.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/max7360-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 PWM controller
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 PWM controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-pwm
> +
> +  "#pwm-cells":
> +    const: 2

Please make this 3.

> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    max7360_pwm: max7360_pwm {
> +      compatible = "maxim,max7360-pwm";
> +      #pwm-cells = <2>;
> +    };

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

Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Krzysztof Kozlowski]

On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> ---
>  .../devicetree/bindings/gpio/max7360-gpio.yaml     | 96 ++++++++++++++++++++++
>  .../devicetree/bindings/input/max7360-keypad.yaml  | 67 +++++++++++++++
>  .../devicetree/bindings/input/max7360-rotary.yaml  | 52 ++++++++++++
>  .../devicetree/bindings/pwm/max7360-pwm.yaml       | 35 ++++++++
>  4 files changed, 250 insertions(+)


I don't understand how this patchset was split. MFD binding cannot be
empty and cannot be before child devices.

All filenames are wrong here: use compatibles.


> 
> diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> new file mode 100644
> index 000000000000..3c006dc0380b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/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 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-gpio
> +      - maxim,max7360-gpo

Why? What are the differences?

> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  ngpios:
> +    minimum: 0
> +    maximum: 8

Why this is flexible?

> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.

Drop

> +    maxItems: 1
> +
> +  constant-current-disable:

You always need vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Bit field, each bit disables constant-current output of the
> +                 associated GPIO.

Oddly aligned.

Missing constraints.

> +
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - ngpios
> +
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - maxim,max7360-gpio
> +then:
> +  required:
> +    - interrupt-controller
> +    - interrupts
> +else:
> +  properties:
> +    interrupt-controller: false
> +    interrupts: false
> +    constant-current-disable: false
> +
> +    ngpios:
> +      maximum: 6
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    max7360_gpo: max7360_gpo {

Plaese follow DTS coding style.

Keep only one, complete example.

> +            compatible = "maxim,max7360-gpo";
> +            gpio-controller;
> +            #gpio-cells = <0x2>;
> +            ngpios = <4>;

Odd indentation. Your MFD patch had very different one.

> +    };
> +
> +    max7360_gpio: max7360_gpio {
> +            compatible = "maxim,max7360-gpio";
> +
> +            gpio-controller;
> +            #gpio-cells = <0x2>;
> +            ngpios = <8>;
> +            constant-current-disable = <0x06>;
> +
> +            interrupt-controller;
> +            #interrupt-cells = <0x2>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +    };
> diff --git a/Documentation/devicetree/bindings/input/max7360-keypad.yaml b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> new file mode 100644
> index 000000000000..8bc3c841465b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Keypad Controller
> +
> +maintainers:
> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 Keypad Controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +allOf:
> +  - $ref: matrix-keymap.yaml#
> +  - $ref: input.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-keypad
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.


Really? Each separate device has its own interrupt line? How is it
possible if diagram here:
https://www.analog.com/en/products/max7360.html

has only one interrupt?

Fold the binding into the parent node.


> +    maxItems: 1
> +
> +  debounce-delay-ms:
> +    description: Debounce delay in ms
> +    minimum: 9
> +    maximum: 40
> +    default: 9
> +
> +  linux,input-no-autorepeat:
> +    description: If present, the keys will not autorepeat when pressed
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/input/input.h>
> +
> +    max7360_keypad {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Please read and follow DTS coding style.

> +      compatible = "maxim,max7360-keypad";
> +
> +      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)
> +        >;
> +
> +      interrupt-parent = <&gpio1>;
> +      interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +
> +      debounce-delay-ms = <10>;
> +      linux,input-no-autorepeat;
> +    };
> diff --git a/Documentation/devicetree/bindings/input/max7360-rotary.yaml b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> new file mode 100644
> index 000000000000..19afa8344249
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-rotary.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Rotary Encoder
> +
> +maintainers:
> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 Rotary Encoder, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-rotary
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.
> +    maxItems: 1
> +
> +  debounce-delay-ms:
> +    description: Debounce delay in ms
> +    minimum: 0
> +    maximum: 15
> +    default: 0
> +
> +  linux,axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +

Fold into parent node.

> +description: |
> +  Maxim MAX7360 PWM controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-pwm
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +


Fold into parent.



Best regards,
Krzysztof

Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Mathieu Dubois-Briand]

On Sat Dec 21, 2024 at 9:34 PM CET, Krzysztof Kozlowski wrote:
> On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> > ---
> >  .../devicetree/bindings/gpio/max7360-gpio.yaml     | 96 ++++++++++++++++++++++
> >  .../devicetree/bindings/input/max7360-keypad.yaml  | 67 +++++++++++++++
> >  .../devicetree/bindings/input/max7360-rotary.yaml  | 52 ++++++++++++
> >  .../devicetree/bindings/pwm/max7360-pwm.yaml       | 35 ++++++++
> >  4 files changed, 250 insertions(+)
>
>
> I don't understand how this patchset was split. MFD binding cannot be
> empty and cannot be before child devices.
>

Ok, my bad. So I believe squashing both dt-bindings commit should fix
this.

> > diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> > new file mode 100644
> > index 000000000000..3c006dc0380b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> > @@ -0,0 +1,96 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/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 MFD
> > +  https://www.analog.com/en/products/max7360.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max7360-gpio
> > +      - maxim,max7360-gpo
>
> Why? What are the differences?
>

Ok, so maybe my approach here is completely wrong. I'm not sure what
would be the best way to describe the device here, if you have any
suggestion I would be happy to use it. Let me try to summarize the GPIO
setup of the chip.

First we have two series of GPIOs on the chips, which I tend to think
about as two separate "banks". Thus two separate subnodes of the max7360
node.

- On one side we have what I refer to as GPIOs, here with
  maxim,max7360-gpio:
  - PORT0 to PORT7 pins of the chip.
  - Shared with PWM and rotary encoder functionalities. Functionality
    selection can be made independently for each pin. This selection is
    not described here. Runtime will have to ensure the same pin is not
    used by two drivers at the same time. E.g. we cannot have at the
    same time GPIO4 and PWM4.
  - Supports input and interrupts.
  - Outputs may be configured as constant current.
  - 8 GPIOS supported, so ngpios maximum is 8. Thinking about it now, we
    should probably also set minimum to 8, I don't see any reason to
    have ngpios set to something less.

On the other side, we have what I refer to as GPOs, here with
maxim,max7360-gpo compatible:
  - COL2 to COL7 pins of the chip.
  - Shared with the keypad functionality. Selections is made by
    partitioning the pins: first pins for keypad columns, last pins for
    GPOs. Partition is described here by ngpios and on keypad node by
    keypad,num-columns. Runtime will have to ensure values are coherent
    and configure the chip accordingly.
  - Only support outputs.
  - No support for constant current mode.
  - Supports 0 to 6 GPOs, so ngpios maximum is 6.

> > +
> > +  gpio-controller: true
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +
> > +  ngpios:
> > +    minimum: 0
> > +    maximum: 8
>
> Why this is flexible?
>

I believe this makes sense, as this keypad/gpos partition really changes
the actual number of GPIOS. Yet we could argue that this is just runtime
configuration. Tell me what you think about it, if you think this should
be a fixed value, I will find a way.

>
> Best regards,
> Krzysztof

Thanks a lot for your review. I am preparing a new version of this
series that should address all of your other comments.

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

Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Krzysztof Kozlowski]

On 23/12/2024 16:20, Mathieu Dubois-Briand wrote:
> On Sat Dec 21, 2024 at 9:34 PM CET, Krzysztof Kozlowski wrote:
>> On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
>>> ---
>>>  .../devicetree/bindings/gpio/max7360-gpio.yaml     | 96 ++++++++++++++++++++++
>>>  .../devicetree/bindings/input/max7360-keypad.yaml  | 67 +++++++++++++++
>>>  .../devicetree/bindings/input/max7360-rotary.yaml  | 52 ++++++++++++
>>>  .../devicetree/bindings/pwm/max7360-pwm.yaml       | 35 ++++++++
>>>  4 files changed, 250 insertions(+)
>>
>>
>> I don't understand how this patchset was split. MFD binding cannot be
>> empty and cannot be before child devices.
>>
> 
> Ok, my bad. So I believe squashing both dt-bindings commit should fix
> this.
> 
>>> diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..3c006dc0380b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
>>> @@ -0,0 +1,96 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/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 MFD
>>> +  https://www.analog.com/en/products/max7360.html
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - maxim,max7360-gpio
>>> +      - maxim,max7360-gpo
>>
>> Why? What are the differences?
>>
> 
> Ok, so maybe my approach here is completely wrong. I'm not sure what
> would be the best way to describe the device here, if you have any
> suggestion I would be happy to use it. Let me try to summarize the GPIO
> setup of the chip.
> 
> First we have two series of GPIOs on the chips, which I tend to think
> about as two separate "banks". Thus two separate subnodes of the max7360
> node.

First, splitting MFD device into multiple children is pretty often wrong
approach because it tries to mimic Linux driver design.

Such split in DT makes sense if these are really separate blocks, e.g.
separate I2C addresses, re-usable on different designs.

In this case Functional Block Diagram shows separate blocks, but still
the same I2C block. This can be one device. This can be also two devices
if that's easier to represent in DT.

But in any case binding description should explain this.

> 
> - On one side we have what I refer to as GPIOs, here with
>   maxim,max7360-gpio:
>   - PORT0 to PORT7 pins of the chip.
>   - Shared with PWM and rotary encoder functionalities. Functionality
>     selection can be made independently for each pin. This selection is
>     not described here. Runtime will have to ensure the same pin is not
>     used by two drivers at the same time. E.g. we cannot have at the
>     same time GPIO4 and PWM4.
>   - Supports input and interrupts.
>   - Outputs may be configured as constant current.
>   - 8 GPIOS supported, so ngpios maximum is 8. Thinking about it now, we
>     should probably also set minimum to 8, I don't see any reason to
>     have ngpios set to something less.
> 
> On the other side, we have what I refer to as GPOs, here with
> maxim,max7360-gpo compatible:
>   - COL2 to COL7 pins of the chip.
>   - Shared with the keypad functionality. Selections is made by
>     partitioning the pins: first pins for keypad columns, last pins for
>     GPOs. Partition is described here by ngpios and on keypad node by
>     keypad,num-columns. Runtime will have to ensure values are coherent
>     and configure the chip accordingly.
>   - Only support outputs.
>   - No support for constant current mode.
>   - Supports 0 to 6 GPOs, so ngpios maximum is 6.
> 
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  "#gpio-cells":
>>> +    const: 2
>>> +
>>> +  ngpios:
>>> +    minimum: 0
>>> +    maximum: 8
>>
>> Why this is flexible?
>>
> 
> I believe this makes sense, as this keypad/gpos partition really changes
> the actual number of GPIOS. Yet we could argue that this is just runtime
> configuration. Tell me what you think about it, if you think this should
> be a fixed value, I will find a way.

Depends whether this is actual runtime configuration. If you configure
keypad in DT, then the pins go away from GPIOs (especially considering
that board might have these pins really connected to keypad). Anyway,
explain this briefly in binding description.

> 
Best regards,
Krzysztof

Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Mathieu Dubois-Briand]

On Tue Dec 24, 2024 at 10:00 AM CET, Krzysztof Kozlowski wrote:
> On 23/12/2024 16:20, Mathieu Dubois-Briand wrote:
> > On Sat Dec 21, 2024 at 9:34 PM CET, Krzysztof Kozlowski wrote:
> >> On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> >>> ---
> >>> diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..3c006dc0380b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> >>> @@ -0,0 +1,96 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/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 MFD
> >>> +  https://www.analog.com/en/products/max7360.html
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - maxim,max7360-gpio
> >>> +      - maxim,max7360-gpo
> >>
> >> Why? What are the differences?
> >>
> > 
> > Ok, so maybe my approach here is completely wrong. I'm not sure what
> > would be the best way to describe the device here, if you have any
> > suggestion I would be happy to use it. Let me try to summarize the GPIO
> > setup of the chip.
> > 
> > First we have two series of GPIOs on the chips, which I tend to think
> > about as two separate "banks". Thus two separate subnodes of the max7360
> > node.
>
> First, splitting MFD device into multiple children is pretty often wrong
> approach because it tries to mimic Linux driver design.
>
> Such split in DT makes sense if these are really separate blocks, e.g.
> separate I2C addresses, re-usable on different designs.
>
> In this case Functional Block Diagram shows separate blocks, but still
> the same I2C block. This can be one device. This can be also two devices
> if that's easier to represent in DT.

Ok, I get it. So I could try to remove the children, but I'm not really
sure about the way to go:
- About the two series of GPIOs, how should I represent them? In a
  continuous way, like 0-7 is gpios and 8+ is gpos? Or maybe setting
  #gpio-cells to 3 and using the added cell to select between gpios and
  gpos ?
- About the interrupt-controller: today we have a children where all
  gpios have a corresponding interrupt and another one without any
  interrupt. If I remove the children, we will have a mix of both. I
  don't think there is anything preventing to do this, but is this ok?

So I'm keeping the two children for now, but I'm open to the possibility
of removing them.

>
> But in any case binding description should explain this.
>

Ok, I will add some documentation.

> > 
> > - On one side we have what I refer to as GPIOs, here with
> >   maxim,max7360-gpio:
> >   - PORT0 to PORT7 pins of the chip.
> >   - Shared with PWM and rotary encoder functionalities. Functionality
> >     selection can be made independently for each pin. This selection is
> >     not described here. Runtime will have to ensure the same pin is not
> >     used by two drivers at the same time. E.g. we cannot have at the
> >     same time GPIO4 and PWM4.
> >   - Supports input and interrupts.
> >   - Outputs may be configured as constant current.
> >   - 8 GPIOS supported, so ngpios maximum is 8. Thinking about it now, we
> >     should probably also set minimum to 8, I don't see any reason to
> >     have ngpios set to something less.
> > 
> > On the other side, we have what I refer to as GPOs, here with
> > maxim,max7360-gpo compatible:
> >   - COL2 to COL7 pins of the chip.
> >   - Shared with the keypad functionality. Selections is made by
> >     partitioning the pins: first pins for keypad columns, last pins for
> >     GPOs. Partition is described here by ngpios and on keypad node by
> >     keypad,num-columns. Runtime will have to ensure values are coherent
> >     and configure the chip accordingly.
> >   - Only support outputs.
> >   - No support for constant current mode.
> >   - Supports 0 to 6 GPOs, so ngpios maximum is 6.
> > 
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  "#gpio-cells":
> >>> +    const: 2
> >>> +
> >>> +  ngpios:
> >>> +    minimum: 0
> >>> +    maximum: 8
> >>
> >> Why this is flexible?
> >>
> > 
> > I believe this makes sense, as this keypad/gpos partition really changes
> > the actual number of GPIOS. Yet we could argue that this is just runtime
> > configuration. Tell me what you think about it, if you think this should
> > be a fixed value, I will find a way.
>
> Depends whether this is actual runtime configuration. If you configure
> keypad in DT, then the pins go away from GPIOs (especially considering
> that board might have these pins really connected to keypad). Anyway,
> explain this briefly in binding description.

Keypad is configured in DT and yes, the pins partition is a consequence
of the hardware implementation on the board. So on second thought I
believe this is cannot be a runtime configuration and should be
described in the DT.

I will add some documentation about it.

>
> > 
> Best regards,
> Krzysztof

Thanks again for your review.
Mathieu

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

[PATCH 3/8] mfd: Add max7360 support

[mathieu.dubois-briand]

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

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

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

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

-- 
2.39.5

Re: [PATCH 3/8] mfd: Add max7360 support

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-Add-MAX7360-MFD-device/20241220-002541
base:   78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
patch link:    https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-3-8e8317584121%40bootlin.com
patch subject: [PATCH 3/8] mfd: Add max7360 support
config: openrisc-randconfig-r122-20241220 (https://download.01.org/0day-ci/archive/20241221/202412210613.xoQvMKlk-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20241221/202412210613.xoQvMKlk-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/202412210613.xoQvMKlk-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/mfd/max7360.c:52:27: sparse: sparse: symbol 'max7360_volatile_ranges' was not declared. Should it be static?

vim +/max7360_volatile_ranges +52 drivers/mfd/max7360.c

    51	
  > 52	const struct regmap_range max7360_volatile_ranges[] = {
    53		{
    54			.range_min = MAX7360_REG_KEYFIFO,
    55			.range_max = MAX7360_REG_KEYFIFO,
    56		}, {
    57			.range_min = 0x48,
    58			.range_max = 0x4a,
    59		},
    60	};
    61	

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

[PATCH 4/8] pwm: max7360: Add MAX7360 PWM support

[mathieu.dubois-briand]

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

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

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

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..399dc3f76e92 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -745,4 +745,15 @@ config PWM_XILINX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-xilinx.
 
+config PWM_MAX7360
+	tristate "MAX7360 PWMs"
+	depends on MFD_MAX7360
+	depends on OF_GPIO
+	help
+	  PWM driver for Maxim Integrated MAX7360 multifunction device, with
+	  support for up to 8 PWM outputs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-max7360.
+
 endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..ae8908ffc892 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MAX7360)	+= pwm-max7360.o
 obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
new file mode 100644
index 000000000000..b1cde3e86864
--- /dev/null
+++ b/drivers/pwm/pwm-max7360.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
+ */
+#include <linux/math.h>
+#include <linux/mfd/max7360.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define MAX7360_NUM_PWMS			8
+#define MAX7360_PWM_MAX_RES			256
+#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
+#define MAX7360_PWM_COMMON_PWN			BIT(5)
+#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
+#define MAX7360_PWM_PORT(n)			BIT(n)
+
+struct max7360_pwm {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+static inline struct max7360_pwm *to_max7360_pwm(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+	int ret;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = max7360_port_pin_request(max7360_pwm->dev->parent, pwm->hwpwm,
+				       true);
+	if (ret) {
+		dev_err(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_pwm->regmap,
+				MAX7360_REG_PWMCFG + pwm->hwpwm,
+				MAX7360_PWM_COMMON_PWN,
+				0);
+	if (ret) {
+		dev_err(&chip->dev, "failed to write pwm-%d cfg register, error %d\n",
+			pwm->hwpwm, ret);
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
+				MAX7360_PWM_PORT(pwm->hwpwm),
+				MAX7360_PWM_PORT(pwm->hwpwm));
+	if (ret) {
+		dev_err(&chip->dev, "failed to write pwm-%d ports register, error %d\n",
+			pwm->hwpwm, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct max7360_pwm *max7360_pwm;
+	int ret;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
+				0);
+	if (ret)
+		dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n",
+			pwm->hwpwm, ret);
+
+	max7360_port_pin_request(max7360_pwm->dev->parent, pwm->hwpwm,
+				 false);
+}
+
+static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct max7360_pwm *max7360_pwm;
+	u64 duty_steps = state->duty_cycle * MAX7360_PWM_MAX_RES;
+	int ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
+				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
+	if (ret) {
+		dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n",
+			pwm->hwpwm, ret);
+		return ret;
+	}
+
+	do_div(duty_steps, MAX7360_PWM_PERIOD_NS);
+
+	ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
+			   duty_steps >= 255 ? 255 : duty_steps);
+	if (ret) {
+		dev_err(&chip->dev,
+			"failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
+			duty_steps, pwm->hwpwm, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops max7360_pwm_ops = {
+	.request = max7360_pwm_request,
+	.free = max7360_pwm_free,
+	.apply = max7360_pwm_apply,
+};
+
+static int max7360_pwm_probe(struct platform_device *pdev)
+{
+	struct max7360_pwm *max7360_pwm;
+	struct pwm_chip *chip;
+
+	if (!pdev->dev.parent) {
+		dev_err(&pdev->dev, "no parent device\n");
+		return -ENODEV;
+	}
+
+	chip = devm_pwmchip_alloc(&pdev->dev, MAX7360_NUM_PWMS,
+				  sizeof(*max7360_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	chip->ops = &max7360_pwm_ops;
+
+	max7360_pwm = to_max7360_pwm(chip);
+	max7360_pwm->dev = &pdev->dev;
+
+	max7360_pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_pwm->regmap) {
+		dev_err(&pdev->dev, "could not get parent regmap\n");
+		return -ENODEV;
+	}
+
+	return devm_pwmchip_add(&pdev->dev, chip);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id max7360_pwm_of_match[] = {
+	{ .compatible = "maxim,max7360-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max7360_pwm_of_match);
+#endif
+
+static struct platform_driver max7360_pwm_driver = {
+	.driver = {
+		.name = "max7360-pwm",
+		.of_match_table = of_match_ptr(max7360_pwm_of_match),
+	},
+	.probe = max7360_pwm_probe,
+};
+module_platform_driver(max7360_pwm_driver);
+
+MODULE_DESCRIPTION("MAX7360 PWM driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_ALIAS("platform:max7360-pwm");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH 4/8] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

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

Hello,

On Thu, Dec 19, 2024 at 05:21:21PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
> 8 independent PWM outputs.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
>  drivers/pwm/Kconfig       |  11 +++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-max7360.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 185 insertions(+)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..399dc3f76e92 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -745,4 +745,15 @@ config PWM_XILINX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-xilinx.
>  
> +config PWM_MAX7360
> +	tristate "MAX7360 PWMs"
> +	depends on MFD_MAX7360
> +	depends on OF_GPIO
> +	help
> +	  PWM driver for Maxim Integrated MAX7360 multifunction device, with
> +	  support for up to 8 PWM outputs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-max7360.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..ae8908ffc892 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
>  obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
>  obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
>  obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MAX7360)	+= pwm-max7360.o
>  obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..b1cde3e86864
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *
> + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com>
> + */
> +#include <linux/math.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define MAX7360_NUM_PWMS			8
> +#define MAX7360_PWM_MAX_RES			256
> +#define MAX7360_PWM_PERIOD_NS			2000000 /* 500 Hz */
> +#define MAX7360_PWM_COMMON_PWN			BIT(5)
> +#define MAX7360_PWM_CTRL_ENABLE(n)		BIT(n)
> +#define MAX7360_PWM_PORT(n)			BIT(n)
> +
> +struct max7360_pwm {
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct max7360_pwm *to_max7360_pwm(struct pwm_chip *chip)
> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	int ret;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = max7360_port_pin_request(max7360_pwm->dev->parent, pwm->hwpwm,
> +				       true);
> +	if (ret) {
> +		dev_err(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);

Please no error messages after probe.

> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_pwm->regmap,
> +				MAX7360_REG_PWMCFG + pwm->hwpwm,
> +				MAX7360_PWM_COMMON_PWN,
> +				0);
> +	if (ret) {
> +		dev_err(&chip->dev, "failed to write pwm-%d cfg register, error %d\n",
> +			pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
> +				MAX7360_PWM_PORT(pwm->hwpwm),
> +				MAX7360_PWM_PORT(pwm->hwpwm));
> +	if (ret) {
> +		dev_err(&chip->dev, "failed to write pwm-%d ports register, error %d\n",
> +			pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	int ret;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> +				0);
> +	if (ret)
> +		dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n",
> +			pwm->hwpwm, ret);
> +
> +	max7360_port_pin_request(max7360_pwm->dev->parent, pwm->hwpwm,
> +				 false);
> +}
> +
> +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	u64 duty_steps = state->duty_cycle * MAX7360_PWM_MAX_RES;

This might overflow.

> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> +	if (ret) {
> +		dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n",
> +			pwm->hwpwm, ret);
> +		return ret;
> +	}
> +
> +	do_div(duty_steps, MAX7360_PWM_PERIOD_NS);
> +
> +	ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> +			   duty_steps >= 255 ? 255 : duty_steps);
> +	if (ret) {
> +		dev_err(&chip->dev,
> +			"failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
> +			duty_steps, pwm->hwpwm, ret);
> +		return ret;
> +	}

Huh, state->period isn't used at all. That is wrong for sure.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops max7360_pwm_ops = {
> +	.request = max7360_pwm_request,
> +	.free = max7360_pwm_free,
> +	.apply = max7360_pwm_apply,

Please implement .get_state() (or the new waveform callbacks)

> +};
> +
> +static int max7360_pwm_probe(struct platform_device *pdev)
> +{
> +	struct max7360_pwm *max7360_pwm;
> +	struct pwm_chip *chip;
> +
> +	if (!pdev->dev.parent) {
> +		dev_err(&pdev->dev, "no parent device\n");
> +		return -ENODEV;

return dev_err_probe(...);

> +	}
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, MAX7360_NUM_PWMS,
> +				  sizeof(*max7360_pwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	chip->ops = &max7360_pwm_ops;
> +
> +	max7360_pwm = to_max7360_pwm(chip);
> +	max7360_pwm->dev = &pdev->dev;

This is only ever used to get max7360_pwm->dev->parent; so better store
the parent directly in max7360_pwm.

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

error message please

> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id max7360_pwm_of_match[] = {
> +	{ .compatible = "maxim,max7360-pwm", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max7360_pwm_of_match);
> +#endif

Please drop the CONFIG_OF guard.

> +
> +static struct platform_driver max7360_pwm_driver = {
> +	.driver = {
> +		.name = "max7360-pwm",
> +		.of_match_table = of_match_ptr(max7360_pwm_of_match),
> +	},
> +	.probe = max7360_pwm_probe,
> +};
> +module_platform_driver(max7360_pwm_driver);
> +
> +MODULE_DESCRIPTION("MAX7360 PWM driver");
> +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
> +MODULE_ALIAS("platform:max7360-pwm");
> +MODULE_LICENSE("GPL");

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

Re: [PATCH 4/8] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Thu Dec 19, 2024 at 10:53 PM CET, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Dec 19, 2024 at 05:21:21PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > 
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	max7360_pwm = to_max7360_pwm(chip);
> > +	ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> > +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> > +				MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> > +	if (ret) {
> > +		dev_err(&chip->dev, "failed to enable pwm-%d , error %d\n",
> > +			pwm->hwpwm, ret);
> > +		return ret;
> > +	}
> > +
> > +	do_div(duty_steps, MAX7360_PWM_PERIOD_NS);
> > +
> > +	ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> > +			   duty_steps >= 255 ? 255 : duty_steps);
> > +	if (ret) {
> > +		dev_err(&chip->dev,
> > +			"failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
> > +			duty_steps, pwm->hwpwm, ret);
> > +		return ret;
> > +	}
>
> Huh, state->period isn't used at all. That is wrong for sure.
>

Yes this was definitely missing. Period is fixed by the chip, so I will
make sure the requested one is valid or return -EINVAL.

Thanks a lot for your review, I am preparing a new version of this
series that should address all your comments.

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

Re: [PATCH 4/8] pwm: max7360: Add MAX7360 PWM support

[Uwe Kleine-König]

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

Hello again,

On Thu, Dec 19, 2024 at 05:21:21PM +0100, mathieu.dubois-briand@bootlin.com wrote:
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..b1cde3e86864
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *

I forgot: Please also add a paragraph here describing the relevant
features here in the format that several other drivers use. Such that

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

emits it. Relevant are at least:

 - Only supports normal polarity

 - How does it behave on disable? (Constant low? Freeze at the level
   where it just happens to be? High-Z?)

 - How does it behave on reconfiguration? (counter reset? Mixed output
   possible?

Best regards
Uwe

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

Re: [PATCH 4/8] pwm: max7360: Add MAX7360 PWM support

[Krzysztof Kozlowski]

On 19/12/2024 17:21, 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.
> 


...

> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id max7360_pwm_of_match[] = {
> +	{ .compatible = "maxim,max7360-pwm", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max7360_pwm_of_match);
> +#endif
> +
> +static struct platform_driver max7360_pwm_driver = {
> +	.driver = {
> +		.name = "max7360-pwm",
> +		.of_match_table = of_match_ptr(max7360_pwm_of_match),

Better to drop of_match_ptr and #ifdef earlier, so ACPI could use it.

> +	},
> +	.probe = max7360_pwm_probe,
> +};
> +module_platform_driver(max7360_pwm_driver);
> +
> +MODULE_DESCRIPTION("MAX7360 PWM driver");
> +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
> +MODULE_ALIAS("platform:max7360-pwm");


You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

Maybe you need it because OF table will get dropped, then it would be
fine. But in current code it's unnecessary and copy-pasta from other
drivers.


Best regards,
Krzysztof

[PATCH 5/8] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

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

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

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

-- 
2.39.5

Re: [PATCH 5/8] gpio: max7360: Add MAX7360 gpio support

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-Add-MAX7360-MFD-device/20241220-002541
base:   78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
patch link:    https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-5-8e8317584121%40bootlin.com
patch subject: [PATCH 5/8] gpio: max7360: Add MAX7360 gpio support
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20241220/202412202337.8jOygMaK-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241220/202412202337.8jOygMaK-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/202412202337.8jOygMaK-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpio/gpio-max7360.c:57:41: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
      57 |                         "failed to set value %d on gpio-%d", val, pin);
         |                                                              ^~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                        ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                                     ^~~~~~~~~~~
   drivers/gpio/gpio-max7360.c:42:18: note: initialize the variable 'val' to silence this warning
      42 |         unsigned int val;
         |                         ^
         |                          = 0
>> drivers/gpio/gpio-max7360.c:370:32: warning: cast to smaller integer type 'int' from 'const void *' [-Wvoid-pointer-to-int-cast]
     370 |         max7360_gpio->gpio_function = (int)device_get_match_data(&pdev->dev);
         |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 warnings generated.


vim +370 drivers/gpio/gpio-max7360.c

   333	
   334	static int max7360_gpio_probe(struct platform_device *pdev)
   335	{
   336		struct max7360_gpio *max7360_gpio;
   337		unsigned int ngpios;
   338		unsigned int outconf;
   339		struct gpio_irq_chip *girq;
   340		unsigned long flags;
   341		int irq;
   342		int ret;
   343	
   344		if (!pdev->dev.parent) {
   345			dev_err(&pdev->dev, "no parent device\n");
   346			return -ENODEV;
   347		}
   348	
   349		max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(struct max7360_gpio),
   350					    GFP_KERNEL);
   351		if (!max7360_gpio)
   352			return -ENOMEM;
   353	
   354		if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
   355			dev_err(&pdev->dev, "Missing ngpios OF property\n");
   356			return -ENODEV;
   357		}
   358	
   359		max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
   360		if (!max7360_gpio->regmap) {
   361			dev_err(&pdev->dev, "could not get parent regmap\n");
   362			return -ENODEV;
   363		}
   364	
   365		max7360_gpio->dev = &pdev->dev;
   366		max7360_gpio->chip = max7360_gpio_chip;
   367		max7360_gpio->chip.ngpio = ngpios;
   368		max7360_gpio->chip.parent = &pdev->dev;
   369		max7360_gpio->chip.base = -1;
 > 370		max7360_gpio->gpio_function = (int)device_get_match_data(&pdev->dev);
   371	
   372		dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio);
   373	
   374		if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
   375			/* Port GPIOs: set output mode configuration (constant-current
   376			 * or not).
   377			 * This property is optional.
   378			 */
   379			outconf = 0;
   380			ret = of_property_read_u32(pdev->dev.of_node,
   381						   "constant-current-disable", &outconf);
   382			if (ret && (ret != -EINVAL)) {
   383				dev_err(&pdev->dev,
   384					"Failed to read constant-current-disable OF property\n");
   385				return -ENODEV;
   386			}
   387	
   388		    regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf);
   389		}
   390	
   391		if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT &&
   392		    of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
   393			/* Port GPIOs: declare IRQ chip, if configuration was provided.
   394			 */
   395			irq = platform_get_irq(pdev, 0);
   396			if (irq < 0)
   397				return dev_err_probe(&pdev->dev, irq,
   398						     "Failed to get IRQ");
   399	
   400			flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
   401			ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
   402							max7360_gpio_irq, flags,
   403							"max7360-gpio", max7360_gpio);
   404			if (ret)
   405				return dev_err_probe(&pdev->dev, ret,
   406						     "Failed to register interrupt: %d\n",
   407						     ret);
   408	
   409			girq = &max7360_gpio->chip.irq;
   410			gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip);
   411			girq->parent_handler = NULL;
   412			girq->num_parents = 0;
   413			girq->parents = NULL;
   414			girq->threaded = true;
   415			girq->default_type = IRQ_TYPE_NONE;
   416			girq->handler = handle_simple_irq;
   417		}
   418	
   419		ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio);
   420		if (ret) {
   421			dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
   422			return ret;
   423		}
   424	
   425		return 0;
   426	}
   427	

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

Re: [PATCH 5/8] gpio: max7360: Add MAX7360 gpio support

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-Add-MAX7360-MFD-device/20241220-002541
base:   78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
patch link:    https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-5-8e8317584121%40bootlin.com
patch subject: [PATCH 5/8] gpio: max7360: Add MAX7360 gpio support
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20241221/202412210008.Saks0Eu4-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241221/202412210008.Saks0Eu4-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/202412210008.Saks0Eu4-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpio/gpio-max7360.c: In function 'max7360_gpio_probe':
>> drivers/gpio/gpio-max7360.c:370:39: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     370 |         max7360_gpio->gpio_function = (int)device_get_match_data(&pdev->dev);
         |                                       ^


vim +370 drivers/gpio/gpio-max7360.c

   333	
   334	static int max7360_gpio_probe(struct platform_device *pdev)
   335	{
   336		struct max7360_gpio *max7360_gpio;
   337		unsigned int ngpios;
   338		unsigned int outconf;
   339		struct gpio_irq_chip *girq;
   340		unsigned long flags;
   341		int irq;
   342		int ret;
   343	
   344		if (!pdev->dev.parent) {
   345			dev_err(&pdev->dev, "no parent device\n");
   346			return -ENODEV;
   347		}
   348	
   349		max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(struct max7360_gpio),
   350					    GFP_KERNEL);
   351		if (!max7360_gpio)
   352			return -ENOMEM;
   353	
   354		if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
   355			dev_err(&pdev->dev, "Missing ngpios OF property\n");
   356			return -ENODEV;
   357		}
   358	
   359		max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
   360		if (!max7360_gpio->regmap) {
   361			dev_err(&pdev->dev, "could not get parent regmap\n");
   362			return -ENODEV;
   363		}
   364	
   365		max7360_gpio->dev = &pdev->dev;
   366		max7360_gpio->chip = max7360_gpio_chip;
   367		max7360_gpio->chip.ngpio = ngpios;
   368		max7360_gpio->chip.parent = &pdev->dev;
   369		max7360_gpio->chip.base = -1;
 > 370		max7360_gpio->gpio_function = (int)device_get_match_data(&pdev->dev);
   371	
   372		dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio);
   373	
   374		if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
   375			/* Port GPIOs: set output mode configuration (constant-current
   376			 * or not).
   377			 * This property is optional.
   378			 */
   379			outconf = 0;
   380			ret = of_property_read_u32(pdev->dev.of_node,
   381						   "constant-current-disable", &outconf);
   382			if (ret && (ret != -EINVAL)) {
   383				dev_err(&pdev->dev,
   384					"Failed to read constant-current-disable OF property\n");
   385				return -ENODEV;
   386			}
   387	
   388		    regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf);
   389		}
   390	
   391		if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT &&
   392		    of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
   393			/* Port GPIOs: declare IRQ chip, if configuration was provided.
   394			 */
   395			irq = platform_get_irq(pdev, 0);
   396			if (irq < 0)
   397				return dev_err_probe(&pdev->dev, irq,
   398						     "Failed to get IRQ");
   399	
   400			flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
   401			ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
   402							max7360_gpio_irq, flags,
   403							"max7360-gpio", max7360_gpio);
   404			if (ret)
   405				return dev_err_probe(&pdev->dev, ret,
   406						     "Failed to register interrupt: %d\n",
   407						     ret);
   408	
   409			girq = &max7360_gpio->chip.irq;
   410			gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip);
   411			girq->parent_handler = NULL;
   412			girq->num_parents = 0;
   413			girq->parents = NULL;
   414			girq->threaded = true;
   415			girq->default_type = IRQ_TYPE_NONE;
   416			girq->handler = handle_simple_irq;
   417		}
   418	
   419		ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio);
   420		if (ret) {
   421			dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
   422			return ret;
   423		}
   424	
   425		return 0;
   426	}
   427	

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

[PATCH 6/8] 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 | 297 ++++++++++++++++++++++++++++++++
 3 files changed, 310 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..fbc51c89dba1
--- /dev/null
+++ b/drivers/input/keyboard/max7360-keypad.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Bootlin
+ *
+ * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
+ */
+
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/max7360.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+struct max7360_keypad {
+	struct input_dev *input;
+	unsigned int rows;
+	unsigned int cols;
+	unsigned int debounce_ms;
+	int irq;
+	bool no_autorepeat;
+	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;
+
+	ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
+	if (!ret && val == MAX7360_FIFO_OVERFLOW) {
+		/* FIFO overflow: ignore it and get next event. */
+		dev_err(&max7360_keypad->input->dev, "max7360 FIFO overflow");
+		ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
+				  &val);
+	}
+	if (ret) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to read max7360 FIFO");
+		return ret;
+	}
+
+	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");
+		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, ~MAX7360_CFG_SLEEP);
+	if (ret) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 configuration");
+	}
+}
+
+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) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 debounce configuration");
+		return ret;
+	}
+
+	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
+				MAX7360_INTERRUPT_TIME_MASK,
+				FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
+	if (ret) {
+		dev_err(&max7360_keypad->input->dev,
+			"Failed to write max7360 keypad interrupt configuration");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max7360_keypad_parse_dt(struct platform_device *pdev,
+				   struct max7360_keypad *max7360_keypad)
+{
+	bool no_autorepeat;
+	int ret;
+
+	ret = matrix_keypad_parse_properties(&pdev->dev, &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;
+	}
+
+	max7360_keypad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!max7360_keypad->regmap) {
+		dev_err(&pdev->dev, "Could not get parent regmap\n");
+		return -ENODEV;
+	}
+
+	no_autorepeat = of_property_read_bool(pdev->dev.of_node,
+					      "linux,input-no-autorepeat");
+	max7360_keypad->no_autorepeat = no_autorepeat;
+
+	max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN;
+	ret = of_property_read_u32(pdev->dev.of_node, "debounce-delay-ms",
+				   &max7360_keypad->debounce_ms);
+	if (ret == -EINVAL) {
+		dev_info(&pdev->dev, "Using default debounce-delay-ms: %u\n",
+			 max7360_keypad->debounce_ms);
+	} else if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to read 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 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;
+	unsigned long flags;
+	int irq;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	max7360_keypad = devm_kzalloc(&pdev->dev, sizeof(*max7360_keypad),
+				      GFP_KERNEL);
+	if (!max7360_keypad)
+		return -ENOMEM;
+
+	ret = max7360_keypad_parse_dt(pdev, max7360_keypad);
+	if (ret)
+		return ret;
+
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input)
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "Failed to allocate input device\n");
+
+	max7360_keypad->input = input;
+
+	input->id.bustype = BUS_I2C;
+	input->name = pdev->name;
+	input->dev.parent = &pdev->dev;
+
+	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 (!max7360_keypad->no_autorepeat)
+		__set_bit(EV_REP, input->evbit);
+
+	input_set_drvdata(input, max7360_keypad);
+
+	flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					max7360_keypad_irq, flags,
+					"max7360-keypad", max7360_keypad);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to register interrupt: %d\n", ret);
+
+	ret = input_register_device(input);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Could not register input device: %d\n",
+				     ret);
+
+	platform_set_drvdata(pdev, max7360_keypad);
+
+	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_keypad_hw_init(max7360_keypad);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to initialize max7360 keypad\n");
+
+	return 0;
+}
+
+static void max7360_keypad_remove(struct platform_device *pdev)
+{
+	dev_pm_clear_wake_irq(&pdev->dev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id max7360_keypad_of_match[] = {
+	{ .compatible = "maxim,max7360-keypad", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max7360_keypad_of_match);
+#endif
+
+static struct platform_driver max7360_keypad_driver = {
+	.driver = {
+		.name	= "max7360-keypad",
+		.of_match_table = of_match_ptr(max7360_keypad_of_match),
+	},
+	.probe		= max7360_keypad_probe,
+	.remove		= max7360_keypad_remove,
+};
+module_platform_driver(max7360_keypad_driver);
+
+MODULE_DESCRIPTION("MAX7360 Keypad driver");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_ALIAS("platform:max7360-keypad");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH 6/8] input: keyboard: Add support for MAX7360 keypad

[Dmitry Torokhov]

Hi Mathieu,

On Thu, Dec 19, 2024 at 05:21:23PM +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.
> 
> 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 | 297 ++++++++++++++++++++++++++++++++
>  3 files changed, 310 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..fbc51c89dba1
> --- /dev/null
> +++ b/drivers/input/keyboard/max7360-keypad.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *
> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +struct max7360_keypad {
> +	struct input_dev *input;
> +	unsigned int rows;
> +	unsigned int cols;
> +	unsigned int debounce_ms;
> +	int irq;
> +	bool no_autorepeat;
> +	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;
> +
> +	ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
> +	if (!ret && val == MAX7360_FIFO_OVERFLOW) {
> +		/* FIFO overflow: ignore it and get next event. */
> +		dev_err(&max7360_keypad->input->dev, "max7360 FIFO overflow");
> +		ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO,
> +				  &val);
> +	}

Maybe

	do {
		error = regmap_read(...);
		if (error) {
			dev_err(...);
			return IRQ_HANDLED;
		}
	} while (val == MAX7360_FIFO_OVERFLOW);

> +	if (ret) {
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to read max7360 FIFO");
> +		return ret;

Wrong value for irqreturn_t.

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

	int error;

> +
> +	/*
> +	 * 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");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max7360_keypad_close(struct input_dev *pdev)
> +{
> +	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
> +	int ret;

	int error;

> +
> +	/*
> +	 * Nobody is using the device anymore: go to 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");
> +	}

No need for braces here.

> +}
> +
> +static int max7360_keypad_hw_init(struct max7360_keypad *max7360_keypad)
> +{
> +	unsigned int val;
> +	int ret;

	int error;

> +
> +	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) {
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to write max7360 debounce configuration");
> +		return ret;
> +	}
> +
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
> +				MAX7360_INTERRUPT_TIME_MASK,
> +				FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
> +	if (ret) {
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to write max7360 keypad interrupt configuration");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_keypad_parse_dt(struct platform_device *pdev,
> +				   struct max7360_keypad *max7360_keypad)
> +{
> +	bool no_autorepeat;
> +	int ret;
> +
> +	ret = matrix_keypad_parse_properties(&pdev->dev, &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;
> +	}
> +
> +	max7360_keypad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!max7360_keypad->regmap) {
> +		dev_err(&pdev->dev, "Could not get parent regmap\n");
> +		return -ENODEV;
> +	}

What this has to do with parsing DT? 

> +
> +	no_autorepeat = of_property_read_bool(pdev->dev.of_node,
> +					      "linux,input-no-autorepeat");

This is not a standard property, please using positive "autorepeat".

> +	max7360_keypad->no_autorepeat = no_autorepeat;

I do not think you need to store this in driver data, just check and
immediately set EV_REP bit.

> +
> +	max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN;
> +	ret = of_property_read_u32(pdev->dev.of_node, "debounce-delay-ms",
> +				   &max7360_keypad->debounce_ms);

Please use generic device properties:

	error = device_property_read_u32(...);

> +	if (ret == -EINVAL) {
> +		dev_info(&pdev->dev, "Using default debounce-delay-ms: %u\n",
> +			 max7360_keypad->debounce_ms);
> +	} else if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to read 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 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;
> +	unsigned long flags;
> +	int irq;
> +	int ret;

	int error;

> +
> +	if (!pdev->dev.parent)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	max7360_keypad = devm_kzalloc(&pdev->dev, sizeof(*max7360_keypad),
> +				      GFP_KERNEL);
> +	if (!max7360_keypad)
> +		return -ENOMEM;
> +
> +	ret = max7360_keypad_parse_dt(pdev, max7360_keypad);
> +	if (ret)
> +		return ret;
> +
> +	input = devm_input_allocate_device(&pdev->dev);
> +	if (!input)
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +				     "Failed to allocate input device\n");
> +
> +	max7360_keypad->input = input;
> +
> +	input->id.bustype = BUS_I2C;
> +	input->name = pdev->name;
> +	input->dev.parent = &pdev->dev;

This is done by devm_input_allocate_device(), no need to repeat.

> +
> +	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 (!max7360_keypad->no_autorepeat)
> +		__set_bit(EV_REP, input->evbit);
> +
> +	input_set_drvdata(input, max7360_keypad);
> +
> +	flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;

Shared? Why? And why do you need a temp variable?

> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					max7360_keypad_irq, flags,
> +					"max7360-keypad", max7360_keypad);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to register interrupt: %d\n", ret);
> +
> +	ret = input_register_device(input);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Could not register input device: %d\n",
> +				     ret);
> +
> +	platform_set_drvdata(pdev, max7360_keypad);
> +
> +	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_keypad_hw_init(max7360_keypad);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to initialize max7360 keypad\n");

You need to clear wake irq here. Or move setting it to after HW init.

> +
> +	return 0;
> +}
> +
> +static void max7360_keypad_remove(struct platform_device *pdev)
> +{
> +	dev_pm_clear_wake_irq(&pdev->dev);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id max7360_keypad_of_match[] = {
> +	{ .compatible = "maxim,max7360-keypad", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max7360_keypad_of_match);
> +#endif
> +
> +static struct platform_driver max7360_keypad_driver = {
> +	.driver = {
> +		.name	= "max7360-keypad",
> +		.of_match_table = of_match_ptr(max7360_keypad_of_match),
> +	},
> +	.probe		= max7360_keypad_probe,
> +	.remove		= max7360_keypad_remove,
> +};
> +module_platform_driver(max7360_keypad_driver);
> +
> +MODULE_DESCRIPTION("MAX7360 Keypad driver");
> +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
> +MODULE_ALIAS("platform:max7360-keypad");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.39.5
> 

Thanks.

-- 
Dmitry

Re: [PATCH 6/8] input: keyboard: Add support for MAX7360 keypad

[Mathieu Dubois-Briand]

On Thu Dec 19, 2024 at 8:35 PM CET, Dmitry Torokhov wrote:
> Hi Mathieu,
>
> On Thu, Dec 19, 2024 at 05:21:23PM +0100, Mathieu Dubois-Briand wrote:
> > +
> > +	input_set_capability(input, EV_MSC, MSC_SCAN);
> > +	if (!max7360_keypad->no_autorepeat)
> > +		__set_bit(EV_REP, input->evbit);
> > +
> > +	input_set_drvdata(input, max7360_keypad);
> > +
> > +	flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
>
> Shared? Why? And why do you need a temp variable?
>

Ok, this is probably wrong.

I have a board using the MAX7360, were both interrupt lines of the
chipset (INTI, interrupt for GPIO and rotary encoder and INTK, interrupt
for the keypad) have been both wired to the same pin on the CPU side. So
on this board, the interrupt line is indeed shared between the three
corresponding drivers.

Yet this is probably very specific to my board, as I have seen no data
about MAXIM suggesting this design. So having IRQF_SHARED is probably
more a hack than a valid implementation. I will drop it in my next
series.

>
> Thanks.

Thanks a lot for you review, I am preparing a new version of this series
that should address all your comments.


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

Re: [PATCH 6/8] input: keyboard: Add support for MAX7360 keypad

[kernel test robot]

Hi Mathieu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-Add-MAX7360-MFD-device/20241220-002541
base:   78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
patch link:    https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-6-8e8317584121%40bootlin.com
patch subject: [PATCH 6/8] input: keyboard: Add support for MAX7360 keypad
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20241221/202412210024.DavK6KEl-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241221/202412210024.DavK6KEl-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/202412210024.DavK6KEl-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/input/keyboard/max7360-keypad.c:105:24: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551487 to 4294967167 [-Wconstant-conversion]
     104 |         ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
         |               ~~~~~~~~~~~~~~~~~
     105 |                                 MAX7360_CFG_SLEEP, ~MAX7360_CFG_SLEEP);
         |                                                    ^~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +105 drivers/input/keyboard/max7360-keypad.c

    95	
    96	static void max7360_keypad_close(struct input_dev *pdev)
    97	{
    98		struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
    99		int ret;
   100	
   101		/*
   102		 * Nobody is using the device anymore: go to sleep.
   103		 */
   104		ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
 > 105					MAX7360_CFG_SLEEP, ~MAX7360_CFG_SLEEP);
   106		if (ret) {
   107			dev_err(&max7360_keypad->input->dev,
   108				"Failed to write max7360 configuration");
   109		}
   110	}
   111	

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

[PATCH 7/8] 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 | 194 ++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)

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

-- 
2.39.5

[PATCH 8/8] MAINTAINERS: Add entry on MAX7360 driver

[Mathieu Dubois-Briand]

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

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

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

-- 
2.39.5