[PATCH 0/5] ADP5585 GPIO expander, PWM and keypad controller support

[PATCH 0/5] ADP5585 GPIO expander, PWM and keypad controller support

[Laurent Pinchart]

Hello,

This patch series introduces support for the Analog Devices ADP5585, a
GPIO expander, PWM and keyboard controller. It models the chip as an MFD
device, and includes DT bindings (2/5), an MFD driver (3/5) and drivers
for the GPIO (4/5) and PWM (5/5) functions.

Support for the keypad controller is left out, as I have no means to
test it at the moment. The chip also includes a tiny reset controller,
as well as a 3-bit input programmable logic block, which I haven't tried
to support (and also have no means to test).

The driver is based on an initial version from the NXP BSP kernel, then
extensively and nearly completely rewritten, with added DT bindings. I
have nonetheless retained original authorship. Clark, Haibo, if you
would prefer not being credited and/or listed as authors, please let me
know.

Clark Wang (1):
  pwm: adp5585: Add Analog Devices ADP5585 support

Haibo Chen (2):
  mfd: adp5585: Add Analog Devices ADP5585 core support
  gpio: adp5585: Add Analog Devices ADP5585 support

Laurent Pinchart (2):
  dt-bindings: trivial-devices: Drop adi,adp5585 and adi,adp5585-02
  dt-bindings: Add bindings for the Analog Devices ADP5585

 .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 +++
 .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 +++++++++
 .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 +++
 .../devicetree/bindings/trivial-devices.yaml  |   4 -
 MAINTAINERS                                   |  11 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-adp5585.c                   | 232 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/adp5585.c                         | 207 ++++++++++++++++
 drivers/pwm/Kconfig                           |   7 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-adp5585.c                     | 230 +++++++++++++++++
 include/linux/mfd/adp5585.h                   | 120 +++++++++
 15 files changed, 1016 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
 create mode 100644 drivers/gpio/gpio-adp5585.c
 create mode 100644 drivers/mfd/adp5585.c
 create mode 100644 drivers/pwm/pwm-adp5585.c
 create mode 100644 include/linux/mfd/adp5585.h


base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
-- 
Regards,

Laurent Pinchart

[PATCH 1/5] dt-bindings: trivial-devices: Drop adi,adp5585 and adi,adp5585-02

[Laurent Pinchart]

The Analog Devices ADP5585 is a multi-function device that requires
non-trivial DT bindings. To prepare for proper support of the device,
drop the related compatible strings from trivial-devices.yaml. They were
added by mistake, without any user in the mainline kernel, neither in
device tree sources nor in drivers.

Fixes: e5dddbedfe09 ("dt-bindings: add ADP5585/ADP5589 entries to trivial-devices")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index e07be7bf8395..59102d46b98e 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -39,10 +39,6 @@ properties:
             # AD5110 - Nonvolatile Digital Potentiometer
           - adi,ad5110
             # Analog Devices ADP5585 Keypad Decoder and I/O Expansion
-          - adi,adp5585
-            # Analog Devices ADP5585 Keypad Decoder and I/O Expansion with support for Row5
-          - adi,adp5585-02
-            # Analog Devices ADP5589 Keypad Decoder and I/O Expansion
           - adi,adp5589
             # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
           - adi,lt7182s
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] dt-bindings: trivial-devices: Drop adi,adp5585 and adi,adp5585-02

[Krzysztof Kozlowski]

On 20/05/2024 21:59, Laurent Pinchart wrote:
> The Analog Devices ADP5585 is a multi-function device that requires
> non-trivial DT bindings. To prepare for proper support of the device,
> drop the related compatible strings from trivial-devices.yaml. They were
> added by mistake, without any user in the mainline kernel, neither in
> device tree sources nor in drivers.
> 
> Fixes: e5dddbedfe09 ("dt-bindings: add ADP5585/ADP5589 entries to trivial-devices")

I don't see a bug there. Just because there are no users, it is not yet
a bug.

This should be squashed with next patch so you keep compatibles documented.



Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: trivial-devices: Drop adi,adp5585 and adi,adp5585-02

[Laurent Pinchart]

On Tue, May 21, 2024 at 09:02:19PM +0200, Krzysztof Kozlowski wrote:
> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > The Analog Devices ADP5585 is a multi-function device that requires
> > non-trivial DT bindings. To prepare for proper support of the device,
> > drop the related compatible strings from trivial-devices.yaml. They were
> > added by mistake, without any user in the mainline kernel, neither in
> > device tree sources nor in drivers.
> > 
> > Fixes: e5dddbedfe09 ("dt-bindings: add ADP5585/ADP5589 entries to trivial-devices")
> 
> I don't see a bug there. Just because there are no users, it is not yet
> a bug.

The bug (as I see it) is that compatible strings were added to the list
of trivial devices when then patch series that did so did not intend
those compatible strings to be used by anything.

> This should be squashed with next patch so you keep compatibles documented.

I'm fine with that.

-- 
Regards,

Laurent Pinchart

[PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

The ADP5585 is a 10/11 input/output port expander with a built in keypad
matrix decoder, programmable logic, reset generator, and PWM generator.
These bindings model the device as an MFD, and support the GPIO expander
and PWM functions.

These bindings support the GPIO and PWM functions.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
I've limited the bindings to GPIO and PWM as I lack hardware to design,
implement and test the rest of the features the chip supports.
---
 .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
 .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
 .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
 MAINTAINERS                                   |   7 ++
 4 files changed, 195 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml

diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
new file mode 100644
index 000000000000..210e4d53e764
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5585 GPIO Expander
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |
+  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
+  node of the parent MFD device. See
+  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
+  well as an example.
+
+properties:
+  compatible:
+    const: adi,adp5585-gpio
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-reserved-ranges: true
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+...
diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
new file mode 100644
index 000000000000..217c038b2842
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |
+  The ADP5585 is a 10/11 input/output port expander with a built in keypad
+  matrix decoder, programmable logic, reset generator, and PWM generator.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - adi,adp5585-00  # Default
+          - adi,adp5585-01  # 11 GPIOs
+          - adi,adp5585-02  # No pull-up resistors by default on special pins
+          - adi,adp5585-03  # Alternate I2C address
+          - adi,adp5585-04  # Pull-down resistors on all pins by default
+      - const: adi,adp5585
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+  gpio:
+    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
+
+  pwm:
+    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
+
+required:
+  - compatible
+  - reg
+  - gpio
+  - pwm
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,adp5585-01
+    then:
+      properties:
+        gpio:
+          properties:
+            gpio-reserved-ranges: false
+    else:
+      properties:
+        gpio:
+          properties:
+            gpio-reserved-ranges:
+              items:
+                - const: 5
+                - const: 1
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mfd@34 {
+            compatible = "adi,adp5585-00", "adi,adp5585";
+            reg = <0x34>;
+
+            gpio {
+                compatible = "adi,adp5585-gpio";
+                gpio-controller;
+                #gpio-cells = <2>;
+                gpio-reserved-ranges = <5 1>;
+            };
+
+            pwm {
+                compatible = "adi,adp5585-pwm";
+                #pwm-cells = <3>;
+            };
+        };
+    };
+
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mfd@34 {
+            compatible = "adi,adp5585-01", "adi,adp5585";
+            reg = <0x34>;
+
+            vdd-supply = <&reg_3v3>;
+
+            gpio {
+                compatible = "adi,adp5585-gpio";
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            pwm {
+                compatible = "adi,adp5585-pwm";
+                #pwm-cells = <3>;
+            };
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
new file mode 100644
index 000000000000..351a9d5da566
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/adi,adp5585-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5585 PWM Generator
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |
+  The Analog Devices ADP5585 generates a PWM output with configurable frequency
+  and duty cycle represented by a "pwm" child node of the parent MFD device.
+  See Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further
+  details as well as an example.
+
+allOf:
+  - $ref: /schemas/pwm/pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,adp5585-pwm
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - "#pwm-cells"
+
+additionalProperties: false
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 28e20975c26f..afecdb82e783 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -497,6 +497,13 @@ F:	drivers/leds/leds-adp5520.c
 F:	drivers/mfd/adp5520.c
 F:	drivers/video/backlight/adp5520_bl.c
 
+ADP5585 GPIO EXPANDER, PWM AND KEYPAD CONTROLLER DRIVER
+M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+L:	linux-gpio@vger.kernel.org
+L:	linux-pwm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
+
 ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587)
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Krzysztof Kozlowski]

On 20/05/2024 21:59, Laurent Pinchart wrote:
> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> matrix decoder, programmable logic, reset generator, and PWM generator.
> These bindings model the device as an MFD, and support the GPIO expander
> and PWM functions.
> 
> These bindings support the GPIO and PWM functions.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> implement and test the rest of the features the chip supports.
> ---
>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
>  MAINTAINERS                                   |   7 ++
>  4 files changed, 195 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> new file mode 100644
> index 000000000000..210e4d53e764
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADP5585 GPIO Expander
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> +  node of the parent MFD device. See
> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> +  well as an example.
> +
> +properties:
> +  compatible:
> +    const: adi,adp5585-gpio
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  gpio-reserved-ranges: true

There are no resources here, so new compatible is not really warranted.
Squash the node into parent.

> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +...
> diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> new file mode 100644
> index 000000000000..217c038b2842
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The ADP5585 is a 10/11 input/output port expander with a built in keypad
> +  matrix decoder, programmable logic, reset generator, and PWM generator.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - adi,adp5585-00  # Default
> +          - adi,adp5585-01  # 11 GPIOs
> +          - adi,adp5585-02  # No pull-up resistors by default on special pins
> +          - adi,adp5585-03  # Alternate I2C address
> +          - adi,adp5585-04  # Pull-down resistors on all pins by default
> +      - const: adi,adp5585
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  gpio:
> +    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
> +
> +  pwm:
> +    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio
> +  - pwm
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,adp5585-01
> +    then:
> +      properties:
> +        gpio:
> +          properties:
> +            gpio-reserved-ranges: false

This also points to fact your child node is pointless. It does not stand
on its own...

> +    else:
> +      properties:
> +        gpio:
> +          properties:
> +            gpio-reserved-ranges:
> +              items:
> +                - const: 5
> +                - const: 1
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mfd@34 {
> +            compatible = "adi,adp5585-00", "adi,adp5585";
> +            reg = <0x34>;
> +
> +            gpio {
> +                compatible = "adi,adp5585-gpio";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                gpio-reserved-ranges = <5 1>;
> +            };
> +
> +            pwm {
> +                compatible = "adi,adp5585-pwm";
> +                #pwm-cells = <3>;
> +            };
> +        };
> +    };
> +
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mfd@34 {
> +            compatible = "adi,adp5585-01", "adi,adp5585";
> +            reg = <0x34>;
> +
> +            vdd-supply = <&reg_3v3>;
> +
> +            gpio {
> +                compatible = "adi,adp5585-gpio";
> +                gpio-controller;
> +                #gpio-cells = <2>;

Different by one property? So just keep one example, unless there are
more differences.

> +            };
> +
> +            pwm {
> +                compatible = "adi,adp5585-pwm";
> +                #pwm-cells = <3>;
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> new file mode 100644
> index 000000000000..351a9d5da566
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/adi,adp5585-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADP5585 PWM Generator
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The Analog Devices ADP5585 generates a PWM output with configurable frequency
> +  and duty cycle represented by a "pwm" child node of the parent MFD device.
> +  See Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further
> +  details as well as an example.
> +
> +allOf:
> +  - $ref: /schemas/pwm/pwm.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adp5585-pwm
> +
> +  "#pwm-cells":
> +    const: 3

Also no resources, so this can be part of the parent node.



Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

Hi Krzysztof,

On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > matrix decoder, programmable logic, reset generator, and PWM generator.
> > These bindings model the device as an MFD, and support the GPIO expander
> > and PWM functions.
> > 
> > These bindings support the GPIO and PWM functions.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > I've limited the bindings to GPIO and PWM as I lack hardware to design,
> > implement and test the rest of the features the chip supports.
> > ---
> >  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> >  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> >  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> >  MAINTAINERS                                   |   7 ++
> >  4 files changed, 195 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > new file mode 100644
> > index 000000000000..210e4d53e764
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > @@ -0,0 +1,36 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADP5585 GPIO Expander
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> > +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> > +  node of the parent MFD device. See
> > +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> > +  well as an example.
> > +
> > +properties:
> > +  compatible:
> > +    const: adi,adp5585-gpio
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  gpio-reserved-ranges: true
> 
> There are no resources here, so new compatible is not really warranted.
> Squash the node into parent.

Child nodes seem (to me) to be the standard way to model functions in
MFD devices. Looking at mfd_add_device(), for OF-based systems, the
function iterates over child nodes. I don't mind going a different
routes, could you indicate what you have in mind, perhaps pointing to an
existing driver as an example ?

> > +
> > +required:
> > +  - compatible
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +
> > +additionalProperties: false
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > new file mode 100644
> > index 000000000000..217c038b2842
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > +  matrix decoder, programmable logic, reset generator, and PWM generator.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - adi,adp5585-00  # Default
> > +          - adi,adp5585-01  # 11 GPIOs
> > +          - adi,adp5585-02  # No pull-up resistors by default on special pins
> > +          - adi,adp5585-03  # Alternate I2C address
> > +          - adi,adp5585-04  # Pull-down resistors on all pins by default
> > +      - const: adi,adp5585
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  gpio:
> > +    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
> > +
> > +  pwm:
> > +    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - gpio
> > +  - pwm
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: adi,adp5585-01
> > +    then:
> > +      properties:
> > +        gpio:
> > +          properties:
> > +            gpio-reserved-ranges: false
> 
> This also points to fact your child node is pointless. It does not stand
> on its own...

That doesn't make the child pointless just for that reason. There are
numerous examples of child nodes that don't stand on their own.

> > +    else:
> > +      properties:
> > +        gpio:
> > +          properties:
> > +            gpio-reserved-ranges:
> > +              items:
> > +                - const: 5
> > +                - const: 1
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        mfd@34 {
> > +            compatible = "adi,adp5585-00", "adi,adp5585";
> > +            reg = <0x34>;
> > +
> > +            gpio {
> > +                compatible = "adi,adp5585-gpio";
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> > +                gpio-reserved-ranges = <5 1>;
> > +            };
> > +
> > +            pwm {
> > +                compatible = "adi,adp5585-pwm";
> > +                #pwm-cells = <3>;
> > +            };
> > +        };
> > +    };
> > +
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        mfd@34 {
> > +            compatible = "adi,adp5585-01", "adi,adp5585";
> > +            reg = <0x34>;
> > +
> > +            vdd-supply = <&reg_3v3>;
> > +
> > +            gpio {
> > +                compatible = "adi,adp5585-gpio";
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> 
> Different by one property? So just keep one example, unless there are
> more differences.

I found the two examples useful during development of the binding to
test the gpio-reserved-ranges rule (I got it wrong in the first place,
and the dt schema validator told me), but I'm fine dropping one of the
two.

> > +            };
> > +
> > +            pwm {
> > +                compatible = "adi,adp5585-pwm";
> > +                #pwm-cells = <3>;
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > diff --git a/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > new file mode 100644
> > index 000000000000..351a9d5da566
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > @@ -0,0 +1,35 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/adi,adp5585-pwm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADP5585 PWM Generator
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  The Analog Devices ADP5585 generates a PWM output with configurable frequency
> > +  and duty cycle represented by a "pwm" child node of the parent MFD device.
> > +  See Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further
> > +  details as well as an example.
> > +
> > +allOf:
> > +  - $ref: /schemas/pwm/pwm.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adp5585-pwm
> > +
> > +  "#pwm-cells":
> > +    const: 3
> 
> Also no resources, so this can be part of the parent node.

I'll sure follow the same design for the GPIO and PWM functions :-)
Let's answer the above question first.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Krzysztof Kozlowski]

On 21/05/2024 21:43, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
>> On 20/05/2024 21:59, Laurent Pinchart wrote:
>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
>>> matrix decoder, programmable logic, reset generator, and PWM generator.
>>> These bindings model the device as an MFD, and support the GPIO expander
>>> and PWM functions.
>>>
>>> These bindings support the GPIO and PWM functions.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
>>> implement and test the rest of the features the chip supports.
>>> ---
>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
>>>  MAINTAINERS                                   |   7 ++
>>>  4 files changed, 195 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..210e4d53e764
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>> @@ -0,0 +1,36 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Analog Devices ADP5585 GPIO Expander
>>> +
>>> +maintainers:
>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> +
>>> +description: |
>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
>>> +  node of the parent MFD device. See
>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
>>> +  well as an example.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: adi,adp5585-gpio
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  '#gpio-cells':
>>> +    const: 2
>>> +
>>> +  gpio-reserved-ranges: true
>>
>> There are no resources here, so new compatible is not really warranted.
>> Squash the node into parent.
> 
> Child nodes seem (to me) to be the standard way to model functions in
> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> function iterates over child nodes. I don't mind going a different

Only to assign of node, which could be skipped as well.

> routes, could you indicate what you have in mind, perhaps pointing to an
> existing driver as an example ?

Most of them? OK, let's take the last added driver in MFD directory:
cirrus,cs42l43
It has three children and only two nodes, because only these two devices
actually need/use/benefit the subnodes.


> 
>>> +
>>> +required:
>>> +  - compatible
>>> +  - gpio-controller
>>> +  - "#gpio-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +...
>>> diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>>> new file mode 100644
>>> index 000000000000..217c038b2842
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>>> @@ -0,0 +1,117 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion
>>> +
>>> +maintainers:
>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +  The ADP5585 is a 10/11 input/output port expander with a built in keypad
>>> +  matrix decoder, programmable logic, reset generator, and PWM generator.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - adi,adp5585-00  # Default
>>> +          - adi,adp5585-01  # 11 GPIOs
>>> +          - adi,adp5585-02  # No pull-up resistors by default on special pins
>>> +          - adi,adp5585-03  # Alternate I2C address
>>> +          - adi,adp5585-04  # Pull-down resistors on all pins by default
>>> +      - const: adi,adp5585
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  vdd-supply: true
>>> +
>>> +  gpio:
>>> +    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
>>> +
>>> +  pwm:
>>> +    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - gpio
>>> +  - pwm
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: adi,adp5585-01
>>> +    then:
>>> +      properties:
>>> +        gpio:
>>> +          properties:
>>> +            gpio-reserved-ranges: false
>>
>> This also points to fact your child node is pointless. It does not stand
>> on its own...
> 
> That doesn't make the child pointless just for that reason. There are
> numerous examples of child nodes that don't stand on their own.

No, your if-then must be in the schema defining it. This is just
unmaintianable code. It proves that child's compatible means nothing. If
you cannot use child's compatible to make any meaningful choices, then
it is useless.


Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Krzysztof Kozlowski]

On 22/05/2024 08:57, Krzysztof Kozlowski wrote:
> On 21/05/2024 21:43, Laurent Pinchart wrote:
>> Hi Krzysztof,
>>
>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
>>>> These bindings model the device as an MFD, and support the GPIO expander
>>>> and PWM functions.
>>>>
>>>> These bindings support the GPIO and PWM functions.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> ---
>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
>>>> implement and test the rest of the features the chip supports.
>>>> ---
>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
>>>>  MAINTAINERS                                   |   7 ++
>>>>  4 files changed, 195 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>> new file mode 100644
>>>> index 000000000000..210e4d53e764
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>> @@ -0,0 +1,36 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Analog Devices ADP5585 GPIO Expander
>>>> +
>>>> +maintainers:
>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> +
>>>> +description: |
>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
>>>> +  node of the parent MFD device. See
>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
>>>> +  well as an example.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: adi,adp5585-gpio
>>>> +
>>>> +  gpio-controller: true
>>>> +
>>>> +  '#gpio-cells':
>>>> +    const: 2
>>>> +
>>>> +  gpio-reserved-ranges: true
>>>
>>> There are no resources here, so new compatible is not really warranted.
>>> Squash the node into parent.
>>
>> Child nodes seem (to me) to be the standard way to model functions in
>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
>> function iterates over child nodes. I don't mind going a different
> 
> Only to assign of node, which could be skipped as well.
> 
>> routes, could you indicate what you have in mind, perhaps pointing to an
>> existing driver as an example ?
> 
> Most of them? OK, let's take the last added driver in MFD directory:
> cirrus,cs42l43
> It has three children and only two nodes, because only these two devices
> actually need/use/benefit the subnodes.

BTW, if these devices have their own I2C addresses or are reusable
blocks, then of course their existence as child nodes is warranted.

Similarly if these devices had additional subnodes, like pinctrl or SPI
controller.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 21:43, Laurent Pinchart wrote:
> > Hi Krzysztof,
> > 
> > On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> >> On 20/05/2024 21:59, Laurent Pinchart wrote:
> >>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> >>> matrix decoder, programmable logic, reset generator, and PWM generator.
> >>> These bindings model the device as an MFD, and support the GPIO expander
> >>> and PWM functions.
> >>>
> >>> These bindings support the GPIO and PWM functions.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> >>> implement and test the rest of the features the chip supports.
> >>> ---
> >>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> >>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> >>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> >>>  MAINTAINERS                                   |   7 ++
> >>>  4 files changed, 195 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..210e4d53e764
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>> @@ -0,0 +1,36 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Analog Devices ADP5585 GPIO Expander
> >>> +
> >>> +maintainers:
> >>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> +
> >>> +description: |
> >>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> >>> +  node of the parent MFD device. See
> >>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> >>> +  well as an example.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: adi,adp5585-gpio
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  '#gpio-cells':
> >>> +    const: 2
> >>> +
> >>> +  gpio-reserved-ranges: true
> >>
> >> There are no resources here, so new compatible is not really warranted.
> >> Squash the node into parent.
> > 
> > Child nodes seem (to me) to be the standard way to model functions in
> > MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > function iterates over child nodes. I don't mind going a different
> 
> Only to assign of node, which could be skipped as well.

It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
work. That doesn't have to be done in mfd_add_device() though, it can
also be done manually by the driver. Looking at the example you gave,
cs42l43_pin_probe() handles that assignment. I would have considered
that a bit of a hack, but if that's your preferred approach, I'm fine
with it. Could you confirm you're OK with that ?

> > routes, could you indicate what you have in mind, perhaps pointing to an
> > existing driver as an example ?
> 
> Most of them? OK, let's take the last added driver in MFD directory:
> cirrus,cs42l43
> It has three children and only two nodes, because only these two devices
> actually need/use/benefit the subnodes.

Still trying to understand what bothers you here, is it the child nodes,
or the fact that they have a compatible string and are documented in a
separate binding ? Looking at the cirrus,cs42l43 bindings and the
corresponding drivers, the pinctrl child node serves the purpose of
grouping properties related to the pinctrl function, and allows
referencing pinctrl entries from other DT nodes. All those properties
could have been placed in the parent node. Are you fine with the
adi,adp5585 having gpio and pwm child nodes, as long as they don't have
compatible strings, and are documented in a single binding ?

> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - gpio-controller
> >>> +  - "#gpio-cells"
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +...
> >>> diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>> new file mode 100644
> >>> index 000000000000..217c038b2842
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>> @@ -0,0 +1,117 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Analog Devices ADP5585 Keypad Decoder and I/O Expansion
> >>> +
> >>> +maintainers:
> >>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> +
> >>> +description: |
> >>
> >> Do not need '|' unless you need to preserve formatting.
> >>
> >>> +  The ADP5585 is a 10/11 input/output port expander with a built in keypad
> >>> +  matrix decoder, programmable logic, reset generator, and PWM generator.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - adi,adp5585-00  # Default
> >>> +          - adi,adp5585-01  # 11 GPIOs
> >>> +          - adi,adp5585-02  # No pull-up resistors by default on special pins
> >>> +          - adi,adp5585-03  # Alternate I2C address
> >>> +          - adi,adp5585-04  # Pull-down resistors on all pins by default
> >>> +      - const: adi,adp5585
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  vdd-supply: true
> >>> +
> >>> +  gpio:
> >>> +    $ref: /schemas/gpio/adi,adp5585-gpio.yaml
> >>> +
> >>> +  pwm:
> >>> +    $ref: /schemas/pwm/adi,adp5585-pwm.yaml
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - gpio
> >>> +  - pwm
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: adi,adp5585-01
> >>> +    then:
> >>> +      properties:
> >>> +        gpio:
> >>> +          properties:
> >>> +            gpio-reserved-ranges: false
> >>
> >> This also points to fact your child node is pointless. It does not stand
> >> on its own...
> > 
> > That doesn't make the child pointless just for that reason. There are
> > numerous examples of child nodes that don't stand on their own.
> 
> No, your if-then must be in the schema defining it. This is just
> unmaintianable code. It proves that child's compatible means nothing. If
> you cannot use child's compatible to make any meaningful choices, then
> it is useless.

The compatible string may not be very useful. The child nodes have a
use.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Krzysztof Kozlowski]

On 22/05/2024 09:22, Laurent Pinchart wrote:
> On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
>> On 21/05/2024 21:43, Laurent Pinchart wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
>>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
>>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
>>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
>>>>> These bindings model the device as an MFD, and support the GPIO expander
>>>>> and PWM functions.
>>>>>
>>>>> These bindings support the GPIO and PWM functions.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> ---
>>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
>>>>> implement and test the rest of the features the chip supports.
>>>>> ---
>>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
>>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
>>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
>>>>>  MAINTAINERS                                   |   7 ++
>>>>>  4 files changed, 195 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
>>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..210e4d53e764
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
>>>>> @@ -0,0 +1,36 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Analog Devices ADP5585 GPIO Expander
>>>>> +
>>>>> +maintainers:
>>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> +
>>>>> +description: |
>>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
>>>>> +  node of the parent MFD device. See
>>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
>>>>> +  well as an example.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: adi,adp5585-gpio
>>>>> +
>>>>> +  gpio-controller: true
>>>>> +
>>>>> +  '#gpio-cells':
>>>>> +    const: 2
>>>>> +
>>>>> +  gpio-reserved-ranges: true
>>>>
>>>> There are no resources here, so new compatible is not really warranted.
>>>> Squash the node into parent.
>>>
>>> Child nodes seem (to me) to be the standard way to model functions in
>>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
>>> function iterates over child nodes. I don't mind going a different
>>
>> Only to assign of node, which could be skipped as well.
> 
> It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> work. That doesn't have to be done in mfd_add_device() though, it can
> also be done manually by the driver. Looking at the example you gave,
> cs42l43_pin_probe() handles that assignment. I would have considered
> that a bit of a hack, but if that's your preferred approach, I'm fine
> with it. Could you confirm you're OK with that ?

I am fine with the drivers doing that. It's not a hack, for all
sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
assignment.

> 
>>> routes, could you indicate what you have in mind, perhaps pointing to an
>>> existing driver as an example ?
>>
>> Most of them? OK, let's take the last added driver in MFD directory:
>> cirrus,cs42l43
>> It has three children and only two nodes, because only these two devices
>> actually need/use/benefit the subnodes.
> 
> Still trying to understand what bothers you here, is it the child nodes,
> or the fact that they have a compatible string and are documented in a
> separate binding ? Looking at the cirrus,cs42l43 bindings and the

What bothers me (and as expressed in many reviews by us) is representing
driver structure directly in DT. People model DT based how their Linux
drivers are represented. I don't care about driver stuff here, but DT/DTS.

> corresponding drivers, the pinctrl child node serves the purpose of
> grouping properties related to the pinctrl function, and allows
> referencing pinctrl entries from other DT nodes. All those properties

If you have sub-subnodes, it warrants for me such child. Why? Because it
makes DTS easier to read.

> could have been placed in the parent node. Are you fine with the
> adi,adp5585 having gpio and pwm child nodes, as long as they don't have
> compatible strings, and are documented in a single binding ?

As well not, because then you have even less reasons to have them as
separate nodes. With compatible, one could at least try to argue that
sub-devices are re-usable across families.

>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - gpio
>>>>> +  - pwm
>>>>> +
>>>>> +allOf:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: adi,adp5585-01
>>>>> +    then:
>>>>> +      properties:
>>>>> +        gpio:
>>>>> +          properties:
>>>>> +            gpio-reserved-ranges: false
>>>>
>>>> This also points to fact your child node is pointless. It does not stand
>>>> on its own...
>>>
>>> That doesn't make the child pointless just for that reason. There are
>>> numerous examples of child nodes that don't stand on their own.
>>
>> No, your if-then must be in the schema defining it. This is just
>> unmaintianable code. It proves that child's compatible means nothing. If
>> you cannot use child's compatible to make any meaningful choices, then
>> it is useless.
> 
> The compatible string may not be very useful. The child nodes have a
> use.

What is their use? Grouping few properties? As mentioned above -
grouping subnodes like pinctrl does, is argument on its own for code
readability. Grouping few properties, which in many other devices are in
top-node (see last 100 reviews of new drivers doing exactly the same),
is not that argument.

OTOH, my first, main argument was:

They do not have any resources on their own. Otherwise please point me -
which property represents their resource, like clock, reset, gpio,
suppy, IO address?

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

Hi Krzysztof,

(There's a question for the GPIO and PWM maintainers below)

On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> On 22/05/2024 09:22, Laurent Pinchart wrote:
> > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> >>>>> These bindings model the device as an MFD, and support the GPIO expander
> >>>>> and PWM functions.
> >>>>>
> >>>>> These bindings support the GPIO and PWM functions.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> >>>>> implement and test the rest of the features the chip supports.
> >>>>> ---
> >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> >>>>>  MAINTAINERS                                   |   7 ++
> >>>>>  4 files changed, 195 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..210e4d53e764
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>> @@ -0,0 +1,36 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Analog Devices ADP5585 GPIO Expander
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> +
> >>>>> +description: |
> >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> >>>>> +  node of the parent MFD device. See
> >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> >>>>> +  well as an example.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: adi,adp5585-gpio
> >>>>> +
> >>>>> +  gpio-controller: true
> >>>>> +
> >>>>> +  '#gpio-cells':
> >>>>> +    const: 2
> >>>>> +
> >>>>> +  gpio-reserved-ranges: true
> >>>>
> >>>> There are no resources here, so new compatible is not really warranted.
> >>>> Squash the node into parent.
> >>>
> >>> Child nodes seem (to me) to be the standard way to model functions in
> >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> >>> function iterates over child nodes. I don't mind going a different
> >>
> >> Only to assign of node, which could be skipped as well.
> > 
> > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > work. That doesn't have to be done in mfd_add_device() though, it can
> > also be done manually by the driver. Looking at the example you gave,
> > cs42l43_pin_probe() handles that assignment. I would have considered
> > that a bit of a hack, but if that's your preferred approach, I'm fine
> > with it. Could you confirm you're OK with that ?
> 
> I am fine with the drivers doing that. It's not a hack, for all
> sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> assignment.

I gave this a try, and here's what I came up with to drop the compatible
string. Please ignore for a moment the fact that the child nodes are
still there, that's an orthogonal question which I can address
separately. What I would like is feedback on how the OF nodes are
handled.

diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
index 9696a4cdcfc1..8480ceef05ce 100644
--- a/drivers/gpio/gpio-adp5585.c
+++ b/drivers/gpio/gpio-adp5585.c
@@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
 	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
 	struct adp5585_gpio_dev *adp5585_gpio;
 	struct device *dev = &pdev->dev;
+	struct device_node *node;
 	struct gpio_chip *gc;
 	int ret;

@@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)

 	mutex_init(&adp5585_gpio->lock);

+	node = of_get_child_by_name(dev->parent->of_node, "gpio");
+	if (!node)
+		return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
+
+	dev->of_node = node;
+	dev->fwnode = &node->fwnode;
+
 	gc = &adp5585_gpio->gpio_chip;
 	gc->parent = dev;
 	gc->direction_input = adp5585_gpio_direction_input;
@@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
 	ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
 				     adp5585_gpio);
 	if (ret) {
+		of_node_put(dev->of_node);
+		dev->of_node = NULL;
+		dev->fwnode = NULL;
 		mutex_destroy(&adp5585_gpio->lock);
 		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
 	}
@@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
 {
 	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);

+	of_node_put(pdev->dev.of_node);
+	pdev->dev.of_node = NULL;
+	pdev->dev.fwnode = NULL;
+
 	mutex_destroy(&adp5585_gpio->lock);
 }

diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
index e39a6ea5f794..3b190567ea0b 100644
--- a/drivers/pwm/pwm-adp5585.c
+++ b/drivers/pwm/pwm-adp5585.c
@@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
 static int adp5585_pwm_probe(struct platform_device *pdev)
 {
 	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct device_node *node;
 	struct pwm_chip *chip;
 	int ret;

@@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);

+	node = of_get_child_by_name(dev->parent->of_node, "pwm");
+	if (!node)
+		return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
+
+	dev->of_node = node;
+	dev->fwnode = &node->fwnode;
+
 	pwmchip_set_drvdata(chip, adp5585->regmap);
 	chip->ops = &adp5585_pwm_ops;

 	ret = devm_pwmchip_add(&pdev->dev, chip);
-	if (ret)
+	if (ret) {
+		of_node_put(dev->of_node);
+		dev->of_node = NULL;
+		dev->fwnode = NULL;
 		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}

 	return 0;
 }

+static void adp5585_pwm_remove(struct platform_device *pdev)
+{
+	of_node_put(pdev->dev.of_node);
+	pdev->dev.of_node = NULL;
+	pdev->dev.fwnode = NULL;
+}
+
 static struct platform_driver adp5585_pwm_driver = {
 	.driver	= {
 		.name = "adp5585-pwm",

Is this acceptable ? I'm a bit concerned about poking the internals of
struct device directly from drivers.

I have also refrained from setting fnode->dev to point back to the
device as fone by cs42l43_pin_probe(), as a comment in struct
fwnode_handle indicates that the dev field is for device links only and
shouldn't be touched by anything else. I'm not sure if I should set it.

> >>> routes, could you indicate what you have in mind, perhaps pointing to an
> >>> existing driver as an example ?
> >>
> >> Most of them? OK, let's take the last added driver in MFD directory:
> >> cirrus,cs42l43
> >> It has three children and only two nodes, because only these two devices
> >> actually need/use/benefit the subnodes.
> > 
> > Still trying to understand what bothers you here, is it the child nodes,
> > or the fact that they have a compatible string and are documented in a
> > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> 
> What bothers me (and as expressed in many reviews by us) is representing
> driver structure directly in DT. People model DT based how their Linux
> drivers are represented. I don't care about driver stuff here, but DT/DTS.

DT models the hardware as seen from a software point of view. It
shouldn't reflect the structure of Linux drivers, but it has to be
usable by drivers.

> > corresponding drivers, the pinctrl child node serves the purpose of
> > grouping properties related to the pinctrl function, and allows
> > referencing pinctrl entries from other DT nodes. All those properties
> 
> If you have sub-subnodes, it warrants for me such child. Why? Because it
> makes DTS easier to read.
> 
> > could have been placed in the parent node. Are you fine with the
> > adi,adp5585 having gpio and pwm child nodes, as long as they don't have
> > compatible strings, and are documented in a single binding ?
> 
> As well not, because then you have even less reasons to have them as
> separate nodes. With compatible, one could at least try to argue that
> sub-devices are re-usable across families.

I'll reuse your argument, I think the child nodes make the DTS easier to
read :-)

> >>>>> +required:
> >>>>> +  - compatible
> >>>>> +  - reg
> >>>>> +  - gpio
> >>>>> +  - pwm
> >>>>> +
> >>>>> +allOf:
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            const: adi,adp5585-01
> >>>>> +    then:
> >>>>> +      properties:
> >>>>> +        gpio:
> >>>>> +          properties:
> >>>>> +            gpio-reserved-ranges: false
> >>>>
> >>>> This also points to fact your child node is pointless. It does not stand
> >>>> on its own...
> >>>
> >>> That doesn't make the child pointless just for that reason. There are
> >>> numerous examples of child nodes that don't stand on their own.
> >>
> >> No, your if-then must be in the schema defining it. This is just
> >> unmaintianable code. It proves that child's compatible means nothing. If
> >> you cannot use child's compatible to make any meaningful choices, then
> >> it is useless.
> > 
> > The compatible string may not be very useful. The child nodes have a
> > use.
> 
> What is their use? Grouping few properties? As mentioned above -
> grouping subnodes like pinctrl does, is argument on its own for code
> readability. Grouping few properties, which in many other devices are in
> top-node (see last 100 reviews of new drivers doing exactly the same),
> is not that argument.
> 
> OTOH, my first, main argument was:
> 
> They do not have any resources on their own. Otherwise please point me -
> which property represents their resource, like clock, reset, gpio,
> suppy, IO address?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Rob Herring]

On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> (There's a question for the GPIO and PWM maintainers below)
> 
> On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> > On 22/05/2024 09:22, Laurent Pinchart wrote:
> > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> > >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> > >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> > >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> > >>>>> These bindings model the device as an MFD, and support the GPIO expander
> > >>>>> and PWM functions.
> > >>>>>
> > >>>>> These bindings support the GPIO and PWM functions.
> > >>>>>
> > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>> ---
> > >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> > >>>>> implement and test the rest of the features the chip supports.
> > >>>>> ---
> > >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> > >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> > >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> > >>>>>  MAINTAINERS                                   |   7 ++
> > >>>>>  4 files changed, 195 insertions(+)
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > >>>>> new file mode 100644
> > >>>>> index 000000000000..210e4d53e764
> > >>>>> --- /dev/null
> > >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > >>>>> @@ -0,0 +1,36 @@
> > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>>>> +%YAML 1.2
> > >>>>> +---
> > >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>>> +
> > >>>>> +title: Analog Devices ADP5585 GPIO Expander
> > >>>>> +
> > >>>>> +maintainers:
> > >>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>> +
> > >>>>> +description: |
> > >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> > >>>>> +  node of the parent MFD device. See
> > >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> > >>>>> +  well as an example.
> > >>>>> +
> > >>>>> +properties:
> > >>>>> +  compatible:
> > >>>>> +    const: adi,adp5585-gpio
> > >>>>> +
> > >>>>> +  gpio-controller: true
> > >>>>> +
> > >>>>> +  '#gpio-cells':
> > >>>>> +    const: 2
> > >>>>> +
> > >>>>> +  gpio-reserved-ranges: true
> > >>>>
> > >>>> There are no resources here, so new compatible is not really warranted.
> > >>>> Squash the node into parent.
> > >>>
> > >>> Child nodes seem (to me) to be the standard way to model functions in
> > >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > >>> function iterates over child nodes. I don't mind going a different
> > >>
> > >> Only to assign of node, which could be skipped as well.
> > > 
> > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > > work. That doesn't have to be done in mfd_add_device() though, it can
> > > also be done manually by the driver. Looking at the example you gave,
> > > cs42l43_pin_probe() handles that assignment. I would have considered
> > > that a bit of a hack, but if that's your preferred approach, I'm fine
> > > with it. Could you confirm you're OK with that ?
> > 
> > I am fine with the drivers doing that. It's not a hack, for all
> > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> > assignment.
> 
> I gave this a try, and here's what I came up with to drop the compatible
> string. Please ignore for a moment the fact that the child nodes are
> still there, that's an orthogonal question which I can address
> separately. What I would like is feedback on how the OF nodes are
> handled.
> 
> diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
> index 9696a4cdcfc1..8480ceef05ce 100644
> --- a/drivers/gpio/gpio-adp5585.c
> +++ b/drivers/gpio/gpio-adp5585.c
> @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
>  	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
>  	struct adp5585_gpio_dev *adp5585_gpio;
>  	struct device *dev = &pdev->dev;
> +	struct device_node *node;
>  	struct gpio_chip *gc;
>  	int ret;
> 
> @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> 
>  	mutex_init(&adp5585_gpio->lock);
> 
> +	node = of_get_child_by_name(dev->parent->of_node, "gpio");
> +	if (!node)
> +		return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
> +
> +	dev->of_node = node;
> +	dev->fwnode = &node->fwnode;

Use device_set_of_node_from_dev().

> +
>  	gc = &adp5585_gpio->gpio_chip;
>  	gc->parent = dev;
>  	gc->direction_input = adp5585_gpio_direction_input;
> @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
>  	ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
>  				     adp5585_gpio);
>  	if (ret) {
> +		of_node_put(dev->of_node);
> +		dev->of_node = NULL;
> +		dev->fwnode = NULL;
>  		mutex_destroy(&adp5585_gpio->lock);
>  		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
>  	}
> @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
>  {
>  	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
> 
> +	of_node_put(pdev->dev.of_node);
> +	pdev->dev.of_node = NULL;
> +	pdev->dev.fwnode = NULL;
> +
>  	mutex_destroy(&adp5585_gpio->lock);
>  }
> 
> diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> index e39a6ea5f794..3b190567ea0b 100644
> --- a/drivers/pwm/pwm-adp5585.c
> +++ b/drivers/pwm/pwm-adp5585.c
> @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
>  static int adp5585_pwm_probe(struct platform_device *pdev)
>  {
>  	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node;
>  	struct pwm_chip *chip;
>  	int ret;
> 
> @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
> 
> +	node = of_get_child_by_name(dev->parent->of_node, "pwm");
> +	if (!node)
> +		return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
> +
> +	dev->of_node = node;
> +	dev->fwnode = &node->fwnode;
> +
>  	pwmchip_set_drvdata(chip, adp5585->regmap);
>  	chip->ops = &adp5585_pwm_ops;
> 
>  	ret = devm_pwmchip_add(&pdev->dev, chip);
> -	if (ret)
> +	if (ret) {
> +		of_node_put(dev->of_node);
> +		dev->of_node = NULL;
> +		dev->fwnode = NULL;
>  		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +	}
> 
>  	return 0;
>  }
> 
> +static void adp5585_pwm_remove(struct platform_device *pdev)
> +{
> +	of_node_put(pdev->dev.of_node);

Wouldn't the driver core do this already? It's not going to know how or 
when of_node was set, so should be doing a put regardless.

> +	pdev->dev.of_node = NULL;
> +	pdev->dev.fwnode = NULL;
> +}
> +
>  static struct platform_driver adp5585_pwm_driver = {
>  	.driver	= {
>  		.name = "adp5585-pwm",
> 
> Is this acceptable ? I'm a bit concerned about poking the internals of
> struct device directly from drivers.
> 
> I have also refrained from setting fnode->dev to point back to the
> device as fone by cs42l43_pin_probe(), as a comment in struct
> fwnode_handle indicates that the dev field is for device links only and
> shouldn't be touched by anything else. I'm not sure if I should set it.

I think no, but best for Saravana to comment.

> 
> > >>> routes, could you indicate what you have in mind, perhaps pointing to an
> > >>> existing driver as an example ?
> > >>
> > >> Most of them? OK, let's take the last added driver in MFD directory:
> > >> cirrus,cs42l43
> > >> It has three children and only two nodes, because only these two devices
> > >> actually need/use/benefit the subnodes.
> > > 
> > > Still trying to understand what bothers you here, is it the child nodes,
> > > or the fact that they have a compatible string and are documented in a
> > > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> > 
> > What bothers me (and as expressed in many reviews by us) is representing
> > driver structure directly in DT. People model DT based how their Linux
> > drivers are represented. I don't care about driver stuff here, but DT/DTS.
> 
> DT models the hardware as seen from a software point of view. 

True, but it's for all software's PoV, not some specific s/w.

> It
> shouldn't reflect the structure of Linux drivers, but it has to be
> usable by drivers.

Either way is usable.

Rob

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

Hi Rob,

On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote:
> On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote:
> > Hi Krzysztof,
> > 
> > (There's a question for the GPIO and PWM maintainers below)
> > 
> > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> > > On 22/05/2024 09:22, Laurent Pinchart wrote:
> > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> > > >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> > > >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> > > >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > > >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > > >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> > > >>>>> These bindings model the device as an MFD, and support the GPIO expander
> > > >>>>> and PWM functions.
> > > >>>>>
> > > >>>>> These bindings support the GPIO and PWM functions.
> > > >>>>>
> > > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>>> ---
> > > >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> > > >>>>> implement and test the rest of the features the chip supports.
> > > >>>>> ---
> > > >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> > > >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> > > >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> > > >>>>>  MAINTAINERS                                   |   7 ++
> > > >>>>>  4 files changed, 195 insertions(+)
> > > >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > > >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > > >>>>>
> > > >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > >>>>> new file mode 100644
> > > >>>>> index 000000000000..210e4d53e764
> > > >>>>> --- /dev/null
> > > >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > >>>>> @@ -0,0 +1,36 @@
> > > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > >>>>> +%YAML 1.2
> > > >>>>> +---
> > > >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> > > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > >>>>> +
> > > >>>>> +title: Analog Devices ADP5585 GPIO Expander
> > > >>>>> +
> > > >>>>> +maintainers:
> > > >>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>>> +
> > > >>>>> +description: |
> > > >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> > > >>>>> +  node of the parent MFD device. See
> > > >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> > > >>>>> +  well as an example.
> > > >>>>> +
> > > >>>>> +properties:
> > > >>>>> +  compatible:
> > > >>>>> +    const: adi,adp5585-gpio
> > > >>>>> +
> > > >>>>> +  gpio-controller: true
> > > >>>>> +
> > > >>>>> +  '#gpio-cells':
> > > >>>>> +    const: 2
> > > >>>>> +
> > > >>>>> +  gpio-reserved-ranges: true
> > > >>>>
> > > >>>> There are no resources here, so new compatible is not really warranted.
> > > >>>> Squash the node into parent.
> > > >>>
> > > >>> Child nodes seem (to me) to be the standard way to model functions in
> > > >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > > >>> function iterates over child nodes. I don't mind going a different
> > > >>
> > > >> Only to assign of node, which could be skipped as well.
> > > > 
> > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > > > work. That doesn't have to be done in mfd_add_device() though, it can
> > > > also be done manually by the driver. Looking at the example you gave,
> > > > cs42l43_pin_probe() handles that assignment. I would have considered
> > > > that a bit of a hack, but if that's your preferred approach, I'm fine
> > > > with it. Could you confirm you're OK with that ?
> > > 
> > > I am fine with the drivers doing that. It's not a hack, for all
> > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> > > assignment.
> > 
> > I gave this a try, and here's what I came up with to drop the compatible
> > string. Please ignore for a moment the fact that the child nodes are
> > still there, that's an orthogonal question which I can address
> > separately. What I would like is feedback on how the OF nodes are
> > handled.
> > 
> > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
> > index 9696a4cdcfc1..8480ceef05ce 100644
> > --- a/drivers/gpio/gpio-adp5585.c
> > +++ b/drivers/gpio/gpio-adp5585.c
> > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> >  	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> >  	struct adp5585_gpio_dev *adp5585_gpio;
> >  	struct device *dev = &pdev->dev;
> > +	struct device_node *node;
> >  	struct gpio_chip *gc;
> >  	int ret;
> > 
> > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > 
> >  	mutex_init(&adp5585_gpio->lock);
> > 
> > +	node = of_get_child_by_name(dev->parent->of_node, "gpio");
> > +	if (!node)
> > +		return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
> > +
> > +	dev->of_node = node;
> > +	dev->fwnode = &node->fwnode;
> 
> Use device_set_of_node_from_dev().

That only works without child nodes in DT. Here I'm assigning the gpio
child node, not the node of the parent device.

> > +
> >  	gc = &adp5585_gpio->gpio_chip;
> >  	gc->parent = dev;
> >  	gc->direction_input = adp5585_gpio_direction_input;
> > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> >  	ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
> >  				     adp5585_gpio);
> >  	if (ret) {
> > +		of_node_put(dev->of_node);
> > +		dev->of_node = NULL;
> > +		dev->fwnode = NULL;
> >  		mutex_destroy(&adp5585_gpio->lock);
> >  		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
> >  	}
> > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
> >  {
> >  	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
> > 
> > +	of_node_put(pdev->dev.of_node);
> > +	pdev->dev.of_node = NULL;
> > +	pdev->dev.fwnode = NULL;
> > +
> >  	mutex_destroy(&adp5585_gpio->lock);
> >  }
> > 
> > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > index e39a6ea5f794..3b190567ea0b 100644
> > --- a/drivers/pwm/pwm-adp5585.c
> > +++ b/drivers/pwm/pwm-adp5585.c
> > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
> >  static int adp5585_pwm_probe(struct platform_device *pdev)
> >  {
> >  	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node;
> >  	struct pwm_chip *chip;
> >  	int ret;
> > 
> > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
> >  	if (IS_ERR(chip))
> >  		return PTR_ERR(chip);
> > 
> > +	node = of_get_child_by_name(dev->parent->of_node, "pwm");
> > +	if (!node)
> > +		return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
> > +
> > +	dev->of_node = node;
> > +	dev->fwnode = &node->fwnode;
> > +
> >  	pwmchip_set_drvdata(chip, adp5585->regmap);
> >  	chip->ops = &adp5585_pwm_ops;
> > 
> >  	ret = devm_pwmchip_add(&pdev->dev, chip);
> > -	if (ret)
> > +	if (ret) {
> > +		of_node_put(dev->of_node);
> > +		dev->of_node = NULL;
> > +		dev->fwnode = NULL;
> >  		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +	}
> > 
> >  	return 0;
> >  }
> > 
> > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > +{
> > +	of_node_put(pdev->dev.of_node);
> 
> Wouldn't the driver core do this already? It's not going to know how or 
> when of_node was set, so should be doing a put regardless.

It does, but only when the struct device is being destroyed. Unbinding
and rebinding would leak a reference. Using
device_set_of_node_from_dev() solves that problem, but doesn't work with
child nodes.

I'm going to send a v2 that squashes everything in a single DT node,
which allows usage of device_set_of_node_from_dev(). Let's see if it
will be more palatable.

> > +	pdev->dev.of_node = NULL;
> > +	pdev->dev.fwnode = NULL;
> > +}
> > +
> >  static struct platform_driver adp5585_pwm_driver = {
> >  	.driver	= {
> >  		.name = "adp5585-pwm",
> > 
> > Is this acceptable ? I'm a bit concerned about poking the internals of
> > struct device directly from drivers.
> > 
> > I have also refrained from setting fnode->dev to point back to the
> > device as fone by cs42l43_pin_probe(), as a comment in struct
> > fwnode_handle indicates that the dev field is for device links only and
> > shouldn't be touched by anything else. I'm not sure if I should set it.
> 
> I think no, but best for Saravana to comment.
> 
> > > >>> routes, could you indicate what you have in mind, perhaps pointing to an
> > > >>> existing driver as an example ?
> > > >>
> > > >> Most of them? OK, let's take the last added driver in MFD directory:
> > > >> cirrus,cs42l43
> > > >> It has three children and only two nodes, because only these two devices
> > > >> actually need/use/benefit the subnodes.
> > > > 
> > > > Still trying to understand what bothers you here, is it the child nodes,
> > > > or the fact that they have a compatible string and are documented in a
> > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> > > 
> > > What bothers me (and as expressed in many reviews by us) is representing
> > > driver structure directly in DT. People model DT based how their Linux
> > > drivers are represented. I don't care about driver stuff here, but DT/DTS.
> > 
> > DT models the hardware as seen from a software point of view. 
> 
> True, but it's for all software's PoV, not some specific s/w.

Agreed.

> > It
> > shouldn't reflect the structure of Linux drivers, but it has to be
> > usable by drivers.
> 
> Either way is usable.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Saravana Kannan]

On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote:
> > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote:
> > > Hi Krzysztof,
> > >
> > > (There's a question for the GPIO and PWM maintainers below)
> > >
> > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> > > > On 22/05/2024 09:22, Laurent Pinchart wrote:
> > > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> > > > >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> > > > >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> > > > >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > > > >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > > > >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> > > > >>>>> These bindings model the device as an MFD, and support the GPIO expander
> > > > >>>>> and PWM functions.
> > > > >>>>>
> > > > >>>>> These bindings support the GPIO and PWM functions.
> > > > >>>>>
> > > > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >>>>> ---
> > > > >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> > > > >>>>> implement and test the rest of the features the chip supports.
> > > > >>>>> ---
> > > > >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> > > > >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> > > > >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> > > > >>>>>  MAINTAINERS                                   |   7 ++
> > > > >>>>>  4 files changed, 195 insertions(+)
> > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > > > >>>>>
> > > > >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > >>>>> new file mode 100644
> > > > >>>>> index 000000000000..210e4d53e764
> > > > >>>>> --- /dev/null
> > > > >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > >>>>> @@ -0,0 +1,36 @@
> > > > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > >>>>> +%YAML 1.2
> > > > >>>>> +---
> > > > >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> > > > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > >>>>> +
> > > > >>>>> +title: Analog Devices ADP5585 GPIO Expander
> > > > >>>>> +
> > > > >>>>> +maintainers:
> > > > >>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >>>>> +
> > > > >>>>> +description: |
> > > > >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> > > > >>>>> +  node of the parent MFD device. See
> > > > >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> > > > >>>>> +  well as an example.
> > > > >>>>> +
> > > > >>>>> +properties:
> > > > >>>>> +  compatible:
> > > > >>>>> +    const: adi,adp5585-gpio
> > > > >>>>> +
> > > > >>>>> +  gpio-controller: true
> > > > >>>>> +
> > > > >>>>> +  '#gpio-cells':
> > > > >>>>> +    const: 2
> > > > >>>>> +
> > > > >>>>> +  gpio-reserved-ranges: true
> > > > >>>>
> > > > >>>> There are no resources here, so new compatible is not really warranted.
> > > > >>>> Squash the node into parent.
> > > > >>>
> > > > >>> Child nodes seem (to me) to be the standard way to model functions in
> > > > >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > > > >>> function iterates over child nodes. I don't mind going a different
> > > > >>
> > > > >> Only to assign of node, which could be skipped as well.
> > > > >
> > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > > > > work. That doesn't have to be done in mfd_add_device() though, it can
> > > > > also be done manually by the driver. Looking at the example you gave,
> > > > > cs42l43_pin_probe() handles that assignment. I would have considered
> > > > > that a bit of a hack, but if that's your preferred approach, I'm fine
> > > > > with it. Could you confirm you're OK with that ?
> > > >
> > > > I am fine with the drivers doing that. It's not a hack, for all
> > > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> > > > assignment.
> > >
> > > I gave this a try, and here's what I came up with to drop the compatible
> > > string. Please ignore for a moment the fact that the child nodes are
> > > still there, that's an orthogonal question which I can address
> > > separately. What I would like is feedback on how the OF nodes are
> > > handled.
> > >
> > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
> > > index 9696a4cdcfc1..8480ceef05ce 100644
> > > --- a/drivers/gpio/gpio-adp5585.c
> > > +++ b/drivers/gpio/gpio-adp5585.c
> > > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > >     struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > >     struct adp5585_gpio_dev *adp5585_gpio;
> > >     struct device *dev = &pdev->dev;
> > > +   struct device_node *node;
> > >     struct gpio_chip *gc;
> > >     int ret;
> > >
> > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > >
> > >     mutex_init(&adp5585_gpio->lock);
> > >
> > > +   node = of_get_child_by_name(dev->parent->of_node, "gpio");
> > > +   if (!node)
> > > +           return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
> > > +
> > > +   dev->of_node = node;
> > > +   dev->fwnode = &node->fwnode;
> >
> > Use device_set_of_node_from_dev().
>
> That only works without child nodes in DT. Here I'm assigning the gpio
> child node, not the node of the parent device.
>
> > > +
> > >     gc = &adp5585_gpio->gpio_chip;
> > >     gc->parent = dev;
> > >     gc->direction_input = adp5585_gpio_direction_input;
> > > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > >     ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
> > >                                  adp5585_gpio);
> > >     if (ret) {
> > > +           of_node_put(dev->of_node);
> > > +           dev->of_node = NULL;
> > > +           dev->fwnode = NULL;
> > >             mutex_destroy(&adp5585_gpio->lock);
> > >             return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
> > >     }
> > > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
> > >  {
> > >     struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
> > >
> > > +   of_node_put(pdev->dev.of_node);
> > > +   pdev->dev.of_node = NULL;
> > > +   pdev->dev.fwnode = NULL;
> > > +
> > >     mutex_destroy(&adp5585_gpio->lock);
> > >  }
> > >
> > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > > index e39a6ea5f794..3b190567ea0b 100644
> > > --- a/drivers/pwm/pwm-adp5585.c
> > > +++ b/drivers/pwm/pwm-adp5585.c
> > > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
> > >  static int adp5585_pwm_probe(struct platform_device *pdev)
> > >  {
> > >     struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > > +   struct device *dev = &pdev->dev;
> > > +   struct device_node *node;
> > >     struct pwm_chip *chip;
> > >     int ret;
> > >
> > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
> > >     if (IS_ERR(chip))
> > >             return PTR_ERR(chip);
> > >
> > > +   node = of_get_child_by_name(dev->parent->of_node, "pwm");
> > > +   if (!node)
> > > +           return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
> > > +
> > > +   dev->of_node = node;
> > > +   dev->fwnode = &node->fwnode;
> > > +
> > >     pwmchip_set_drvdata(chip, adp5585->regmap);
> > >     chip->ops = &adp5585_pwm_ops;
> > >
> > >     ret = devm_pwmchip_add(&pdev->dev, chip);
> > > -   if (ret)
> > > +   if (ret) {
> > > +           of_node_put(dev->of_node);
> > > +           dev->of_node = NULL;
> > > +           dev->fwnode = NULL;
> > >             return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > +   }
> > >
> > >     return 0;
> > >  }
> > >
> > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +   of_node_put(pdev->dev.of_node);
> >
> > Wouldn't the driver core do this already? It's not going to know how or
> > when of_node was set, so should be doing a put regardless.
>
> It does, but only when the struct device is being destroyed. Unbinding
> and rebinding would leak a reference. Using
> device_set_of_node_from_dev() solves that problem, but doesn't work with
> child nodes.
>
> I'm going to send a v2 that squashes everything in a single DT node,
> which allows usage of device_set_of_node_from_dev(). Let's see if it
> will be more palatable.
>
> > > +   pdev->dev.of_node = NULL;
> > > +   pdev->dev.fwnode = NULL;
> > > +}
> > > +
> > >  static struct platform_driver adp5585_pwm_driver = {
> > >     .driver = {
> > >             .name = "adp5585-pwm",
> > >
> > > Is this acceptable ? I'm a bit concerned about poking the internals of
> > > struct device directly from drivers.
> > >
> > > I have also refrained from setting fnode->dev to point back to the
> > > device as fone by cs42l43_pin_probe(), as a comment in struct
> > > fwnode_handle indicates that the dev field is for device links only and
> > > shouldn't be touched by anything else. I'm not sure if I should set it.
> >
> > I think no, but best for Saravana to comment.

Don't set fwnode->dev. But I'd actually go even further and say don't
set dev->fwnode to NULL. If it ever needs to be set to NULL the driver
core will take care of it. And when it does that is when it'll set
fwnode->dev to NULL too. So, you setting dev->fwnode to NULL is
actually not good once you add the device.

-Saravana

> >
> > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an
> > > > >>> existing driver as an example ?
> > > > >>
> > > > >> Most of them? OK, let's take the last added driver in MFD directory:
> > > > >> cirrus,cs42l43
> > > > >> It has three children and only two nodes, because only these two devices
> > > > >> actually need/use/benefit the subnodes.
> > > > >
> > > > > Still trying to understand what bothers you here, is it the child nodes,
> > > > > or the fact that they have a compatible string and are documented in a
> > > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> > > >
> > > > What bothers me (and as expressed in many reviews by us) is representing
> > > > driver structure directly in DT. People model DT based how their Linux
> > > > drivers are represented. I don't care about driver stuff here, but DT/DTS.
> > >
> > > DT models the hardware as seen from a software point of view.
> >
> > True, but it's for all software's PoV, not some specific s/w.
>
> Agreed.
>
> > > It
> > > shouldn't reflect the structure of Linux drivers, but it has to be
> > > usable by drivers.
> >
> > Either way is usable.
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

On Wed, May 29, 2024 at 12:02:39AM +0200, Saravana Kannan wrote:
> On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart wrote:
> > On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote:
> > > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote:
> > > > Hi Krzysztof,
> > > >
> > > > (There's a question for the GPIO and PWM maintainers below)
> > > >
> > > > On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 22/05/2024 09:22, Laurent Pinchart wrote:
> > > > > > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> > > > > >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> > > > > >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> > > > > >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> > > > > >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > > > > >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> > > > > >>>>> These bindings model the device as an MFD, and support the GPIO expander
> > > > > >>>>> and PWM functions.
> > > > > >>>>>
> > > > > >>>>> These bindings support the GPIO and PWM functions.
> > > > > >>>>>
> > > > > >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > >>>>> ---
> > > > > >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> > > > > >>>>> implement and test the rest of the features the chip supports.
> > > > > >>>>> ---
> > > > > >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> > > > > >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> > > > > >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> > > > > >>>>>  MAINTAINERS                                   |   7 ++
> > > > > >>>>>  4 files changed, 195 insertions(+)
> > > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > > > > >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> > > > > >>>>>
> > > > > >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > > >>>>> new file mode 100644
> > > > > >>>>> index 000000000000..210e4d53e764
> > > > > >>>>> --- /dev/null
> > > > > >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> > > > > >>>>> @@ -0,0 +1,36 @@
> > > > > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > >>>>> +%YAML 1.2
> > > > > >>>>> +---
> > > > > >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> > > > > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > >>>>> +
> > > > > >>>>> +title: Analog Devices ADP5585 GPIO Expander
> > > > > >>>>> +
> > > > > >>>>> +maintainers:
> > > > > >>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > >>>>> +
> > > > > >>>>> +description: |
> > > > > >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> > > > > >>>>> +  node of the parent MFD device. See
> > > > > >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> > > > > >>>>> +  well as an example.
> > > > > >>>>> +
> > > > > >>>>> +properties:
> > > > > >>>>> +  compatible:
> > > > > >>>>> +    const: adi,adp5585-gpio
> > > > > >>>>> +
> > > > > >>>>> +  gpio-controller: true
> > > > > >>>>> +
> > > > > >>>>> +  '#gpio-cells':
> > > > > >>>>> +    const: 2
> > > > > >>>>> +
> > > > > >>>>> +  gpio-reserved-ranges: true
> > > > > >>>>
> > > > > >>>> There are no resources here, so new compatible is not really warranted.
> > > > > >>>> Squash the node into parent.
> > > > > >>>
> > > > > >>> Child nodes seem (to me) to be the standard way to model functions in
> > > > > >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> > > > > >>> function iterates over child nodes. I don't mind going a different
> > > > > >>
> > > > > >> Only to assign of node, which could be skipped as well.
> > > > > >
> > > > > > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > > > > > work. That doesn't have to be done in mfd_add_device() though, it can
> > > > > > also be done manually by the driver. Looking at the example you gave,
> > > > > > cs42l43_pin_probe() handles that assignment. I would have considered
> > > > > > that a bit of a hack, but if that's your preferred approach, I'm fine
> > > > > > with it. Could you confirm you're OK with that ?
> > > > >
> > > > > I am fine with the drivers doing that. It's not a hack, for all
> > > > > sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> > > > > assignment.
> > > >
> > > > I gave this a try, and here's what I came up with to drop the compatible
> > > > string. Please ignore for a moment the fact that the child nodes are
> > > > still there, that's an orthogonal question which I can address
> > > > separately. What I would like is feedback on how the OF nodes are
> > > > handled.
> > > >
> > > > diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
> > > > index 9696a4cdcfc1..8480ceef05ce 100644
> > > > --- a/drivers/gpio/gpio-adp5585.c
> > > > +++ b/drivers/gpio/gpio-adp5585.c
> > > > @@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > > >     struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > > >     struct adp5585_gpio_dev *adp5585_gpio;
> > > >     struct device *dev = &pdev->dev;
> > > > +   struct device_node *node;
> > > >     struct gpio_chip *gc;
> > > >     int ret;
> > > >
> > > > @@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > > >
> > > >     mutex_init(&adp5585_gpio->lock);
> > > >
> > > > +   node = of_get_child_by_name(dev->parent->of_node, "gpio");
> > > > +   if (!node)
> > > > +           return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
> > > > +
> > > > +   dev->of_node = node;
> > > > +   dev->fwnode = &node->fwnode;
> > >
> > > Use device_set_of_node_from_dev().
> >
> > That only works without child nodes in DT. Here I'm assigning the gpio
> > child node, not the node of the parent device.
> >
> > > > +
> > > >     gc = &adp5585_gpio->gpio_chip;
> > > >     gc->parent = dev;
> > > >     gc->direction_input = adp5585_gpio_direction_input;
> > > > @@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
> > > >     ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
> > > >                                  adp5585_gpio);
> > > >     if (ret) {
> > > > +           of_node_put(dev->of_node);
> > > > +           dev->of_node = NULL;
> > > > +           dev->fwnode = NULL;
> > > >             mutex_destroy(&adp5585_gpio->lock);
> > > >             return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
> > > >     }
> > > > @@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
> > > >  {
> > > >     struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
> > > >
> > > > +   of_node_put(pdev->dev.of_node);
> > > > +   pdev->dev.of_node = NULL;
> > > > +   pdev->dev.fwnode = NULL;
> > > > +
> > > >     mutex_destroy(&adp5585_gpio->lock);
> > > >  }
> > > >
> > > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > > > index e39a6ea5f794..3b190567ea0b 100644
> > > > --- a/drivers/pwm/pwm-adp5585.c
> > > > +++ b/drivers/pwm/pwm-adp5585.c
> > > > @@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
> > > >  static int adp5585_pwm_probe(struct platform_device *pdev)
> > > >  {
> > > >     struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > > > +   struct device *dev = &pdev->dev;
> > > > +   struct device_node *node;
> > > >     struct pwm_chip *chip;
> > > >     int ret;
> > > >
> > > > @@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
> > > >     if (IS_ERR(chip))
> > > >             return PTR_ERR(chip);
> > > >
> > > > +   node = of_get_child_by_name(dev->parent->of_node, "pwm");
> > > > +   if (!node)
> > > > +           return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
> > > > +
> > > > +   dev->of_node = node;
> > > > +   dev->fwnode = &node->fwnode;
> > > > +
> > > >     pwmchip_set_drvdata(chip, adp5585->regmap);
> > > >     chip->ops = &adp5585_pwm_ops;
> > > >
> > > >     ret = devm_pwmchip_add(&pdev->dev, chip);
> > > > -   if (ret)
> > > > +   if (ret) {
> > > > +           of_node_put(dev->of_node);
> > > > +           dev->of_node = NULL;
> > > > +           dev->fwnode = NULL;
> > > >             return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > > +   }
> > > >
> > > >     return 0;
> > > >  }
> > > >
> > > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +   of_node_put(pdev->dev.of_node);
> > >
> > > Wouldn't the driver core do this already? It's not going to know how or
> > > when of_node was set, so should be doing a put regardless.
> >
> > It does, but only when the struct device is being destroyed. Unbinding
> > and rebinding would leak a reference. Using
> > device_set_of_node_from_dev() solves that problem, but doesn't work with
> > child nodes.
> >
> > I'm going to send a v2 that squashes everything in a single DT node,
> > which allows usage of device_set_of_node_from_dev(). Let's see if it
> > will be more palatable.
> >
> > > > +   pdev->dev.of_node = NULL;
> > > > +   pdev->dev.fwnode = NULL;
> > > > +}
> > > > +
> > > >  static struct platform_driver adp5585_pwm_driver = {
> > > >     .driver = {
> > > >             .name = "adp5585-pwm",
> > > >
> > > > Is this acceptable ? I'm a bit concerned about poking the internals of
> > > > struct device directly from drivers.
> > > >
> > > > I have also refrained from setting fnode->dev to point back to the
> > > > device as fone by cs42l43_pin_probe(), as a comment in struct
> > > > fwnode_handle indicates that the dev field is for device links only and
> > > > shouldn't be touched by anything else. I'm not sure if I should set it.
> > >
> > > I think no, but best for Saravana to comment.
> 
> Don't set fwnode->dev. But I'd actually go even further and say don't
> set dev->fwnode to NULL. If it ever needs to be set to NULL the driver
> core will take care of it. And when it does that is when it'll set
> fwnode->dev to NULL too. So, you setting dev->fwnode to NULL is
> actually not good once you add the device.

As far as I understand, the driver core will set dev->fwnode and
dev->of_node to NULL when the device is destroyed. As dev->of_node is
set in the probe function, an unbind-rebind cycle would result in a
reference leak if I don't put the dev->of_node reference in the
.remove() handler. And if I do so, I don't want to leave dev->of_node
and dev->fwnode pointing to nodes whose reference has been released, as
they could then possibly disappear (in theory at least).

In any case, I've changed the DT bindings in v2, and the child devices
now reuse the OF node of the parent MFD, instead of using subnodes. This
allows using device_set_of_node_from_dev(), which seems to do fix this
whole issue.

> > > > > >>> routes, could you indicate what you have in mind, perhaps pointing to an
> > > > > >>> existing driver as an example ?
> > > > > >>
> > > > > >> Most of them? OK, let's take the last added driver in MFD directory:
> > > > > >> cirrus,cs42l43
> > > > > >> It has three children and only two nodes, because only these two devices
> > > > > >> actually need/use/benefit the subnodes.
> > > > > >
> > > > > > Still trying to understand what bothers you here, is it the child nodes,
> > > > > > or the fact that they have a compatible string and are documented in a
> > > > > > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> > > > >
> > > > > What bothers me (and as expressed in many reviews by us) is representing
> > > > > driver structure directly in DT. People model DT based how their Linux
> > > > > drivers are represented. I don't care about driver stuff here, but DT/DTS.
> > > >
> > > > DT models the hardware as seen from a software point of view.
> > >
> > > True, but it's for all software's PoV, not some specific s/w.
> >
> > Agreed.
> >
> > > > It
> > > > shouldn't reflect the structure of Linux drivers, but it has to be
> > > > usable by drivers.
> > >
> > > Either way is usable.

-- 
Regards,

Laurent Pinchart

[PATCH 3/5] mfd: adp5585: Add Analog Devices ADP5585 core support

[Laurent Pinchart]

From: Haibo Chen <haibo.chen@nxp.com>

The ADP5585 is a 10/11 input/output port expander with a built in keypad
matrix decoder, programmable logic, reset generator, and PWM generator.
This driver supports the chip by modelling it as an MFD device, with two
child devices for the GPIO and PWM functions.

The driver is derived from an initial implementation from NXP, available
in commit 8059835bee19 ("MLK-25917-1 mfd: adp5585: add ADI adp5585 core
support") in their BSP kernel tree. It has been extensively rewritten.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes compared to the NXP original version:

- Add MAINTAINERS entry
- Fix compatible strings for child devices
- Fix header guards
- Use lowercase hex constants
- White space fixes
- Use module_i2c_driver()
- Switch to regmap
- Drop I2C device ID table
- Drop ADP5585_REG_MASK
- Support R5 GPIO pin
- Drop dev field from adp5585_dev structure
- Check device ID at probe time
- Fix register field names
- Update copyright
- Update license to GPL-2.0-only
- Implement suspend/resume
---
 MAINTAINERS                 |   2 +
 drivers/mfd/Kconfig         |  11 ++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/adp5585.c       | 207 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/adp5585.h | 120 +++++++++++++++++++++
 5 files changed, 341 insertions(+)
 create mode 100644 drivers/mfd/adp5585.c
 create mode 100644 include/linux/mfd/adp5585.h

diff --git a/MAINTAINERS b/MAINTAINERS
index afecdb82e783..7150f091b69b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -503,6 +503,8 @@ L:	linux-gpio@vger.kernel.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
+F:	drivers/mfd/adp5585.c
+F:	include/linux/mfd/adp5585.h
 
 ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587)
 M:	Michael Hennerich <michael.hennerich@analog.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..3423eac0877a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -20,6 +20,17 @@ config MFD_CS5535
 	  This is the core driver for CS5535/CS5536 MFD functions.  This is
 	  necessary for using the board's GPIO and MFGPT functionality.
 
+config MFD_ADP5585
+	tristate "Analog Devices ADP5585 MFD driver"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C && OF
+	help
+	  Say yes here to add support for the Analog Devices ADP5585 GPIO
+	  expander, PWM and keypad controller. This includes the I2C driver and
+	  the core APIs _only_, you have to select individual components like
+	  the GPIO and PWM functions under the corresponding menus.
+
 config MFD_ALTERA_A10SR
 	bool "Altera Arria10 DevKit System Resource chip"
 	depends on ARCH_INTEL_SOCFPGA && SPI_MASTER=y && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..37f36a019a68 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -188,6 +188,7 @@ obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
 obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
 obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
 obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
+obj-$(CONFIG_MFD_ADP5585)	+= adp5585.o
 obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
 obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
new file mode 100644
index 000000000000..75957f9b67c2
--- /dev/null
+++ b/drivers/mfd/adp5585.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ADP5585 I/O expander, PWM controller and keypad controller
+ *
+ * Copyright 2022 NXP
+ * Copyright 2024 Ideas on Board Oy
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell adp5585_devs[] = {
+	{
+		.name = "adp5585-gpio",
+		.of_compatible = "adi,adp5585-gpio",
+	},
+	{
+		.name = "adp5585-pwm",
+		.of_compatible = "adi,adp5585-pwm",
+	},
+};
+
+static const struct regmap_range adp5585_volatile_ranges[] = {
+	regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B),
+};
+
+static const struct regmap_access_table adp5585_volatile_regs = {
+	.yes_ranges = adp5585_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges),
+};
+
+static const u8 adp5585_regmap_defaults_00[ADP5585_MAX_REG + 1] = {
+	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00,
+};
+
+static const u8 adp5585_regmap_defaults_02[ADP5585_MAX_REG + 1] = {
+	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc3,
+	/* 0x18 */ 0x03, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00,
+};
+
+static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = {
+	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55,
+	/* 0x18 */ 0x05, 0x55, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00,
+};
+
+enum adp5585_regmap_type {
+	ADP5585_REGMAP_00,
+	ADP5585_REGMAP_02,
+	ADP5585_REGMAP_04,
+};
+
+static const struct regmap_config adp5585_regmap_configs[] = {
+	[ADP5585_REGMAP_00] = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = ADP5585_MAX_REG,
+		.volatile_table = &adp5585_volatile_regs,
+		.cache_type = REGCACHE_MAPLE,
+		.reg_defaults_raw = adp5585_regmap_defaults_00,
+		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_00),
+	},
+	[ADP5585_REGMAP_02] = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = ADP5585_MAX_REG,
+		.volatile_table = &adp5585_volatile_regs,
+		.cache_type = REGCACHE_MAPLE,
+		.reg_defaults_raw = adp5585_regmap_defaults_02,
+		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_02),
+	},
+	[ADP5585_REGMAP_04] = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.max_register = ADP5585_MAX_REG,
+		.volatile_table = &adp5585_volatile_regs,
+		.cache_type = REGCACHE_MAPLE,
+		.reg_defaults_raw = adp5585_regmap_defaults_04,
+		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04),
+	},
+};
+
+
+static int adp5585_i2c_probe(struct i2c_client *i2c)
+{
+	const struct regmap_config *regmap_config;
+	struct adp5585_dev *adp5585;
+	unsigned int id;
+	int ret;
+
+	adp5585 = devm_kzalloc(&i2c->dev, sizeof(struct adp5585_dev),
+			       GFP_KERNEL);
+	if (adp5585 == NULL)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, adp5585);
+
+	regmap_config = of_device_get_match_data(&i2c->dev);
+	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
+	if (IS_ERR(adp5585->regmap))
+		return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap),
+				     "Failed to initialize register map\n");
+
+	/* Verify the device ID. */
+	ret = regmap_read(adp5585->regmap, ADP5585_ID, &id);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to read device ID\n");
+
+	if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
+		return dev_err_probe(&i2c->dev, -ENODEV,
+				     "Invalid device ID 0x%02x\n", id);
+
+	dev_dbg(&i2c->dev, "device ID 0x%02x\n", id);
+
+	/* Add MFD devices. */
+	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
+				   adp5585_devs, ARRAY_SIZE(adp5585_devs),
+				   NULL, 0, NULL);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to add MFD devices\n");
+
+	return 0;
+}
+
+static int adp5585_suspend(struct device *dev)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
+
+	regcache_cache_only(adp5585->regmap, true);
+
+	return 0;
+}
+
+static int adp5585_resume(struct device *dev)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
+
+	regcache_cache_only(adp5585->regmap, false);
+	regcache_mark_dirty(adp5585->regmap);
+
+	return regcache_sync(adp5585->regmap);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend, adp5585_resume);
+
+static const struct of_device_id adp5585_of_match[] = {
+	{
+		.compatible = "adi,adp5585-00",
+		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
+	}, {
+		.compatible = "adi,adp5585-01",
+		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
+	}, {
+		.compatible = "adi,adp5585-02",
+		.data = &adp5585_regmap_configs[ADP5585_REGMAP_02],
+	}, {
+		.compatible = "adi,adp5585-03",
+		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
+	}, {
+		.compatible = "adi,adp5585-04",
+		.data = &adp5585_regmap_configs[ADP5585_REGMAP_04],
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_of_match);
+
+static struct i2c_driver adp5585_i2c_driver = {
+	.driver = {
+		.name = "adp5585",
+		.of_match_table = of_match_ptr(adp5585_of_match),
+		.pm = pm_sleep_ptr(&adp5585_pm),
+	},
+	.probe = adp5585_i2c_probe,
+};
+module_i2c_driver(adp5585_i2c_driver);
+
+MODULE_DESCRIPTION("ADP5585 core driver");
+MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h
new file mode 100644
index 000000000000..3f178f30fac6
--- /dev/null
+++ b/include/linux/mfd/adp5585.h
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Analog Devices ADP5585 I/O expander, PWM controller and keypad controller
+ *
+ * Copyright 2022 NXP
+ * Copyright 2024 Ideas on Board Oy
+ */
+
+#ifndef __LINUX_MFD_ADP5585_H_
+#define __LINUX_MFD_ADP5585_H_
+
+#include <linux/bits.h>
+
+#define ADP5585_ID			0x00
+#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
+#define		ADP5585_MAN_ID_VALUE		0x02
+#define ADP5585_INT_STATUS		0x01
+#define ADP5585_STATUS			0x02
+#define ADP5585_FIFO_1			0x03
+#define ADP5585_FIFO_2			0x04
+#define ADP5585_FIFO_3			0x05
+#define ADP5585_FIFO_4			0x06
+#define ADP5585_FIFO_5			0x07
+#define ADP5585_FIFO_6			0x08
+#define ADP5585_FIFO_7			0x09
+#define ADP5585_FIFO_8			0x0a
+#define ADP5585_FIFO_9			0x0b
+#define ADP5585_FIFO_10			0x0c
+#define ADP5585_FIFO_11			0x0d
+#define ADP5585_FIFO_12			0x0e
+#define ADP5585_FIFO_13			0x0f
+#define ADP5585_FIFO_14			0x10
+#define ADP5585_FIFO_15			0x11
+#define ADP5585_FIFO_16			0x12
+#define ADP5585_GPI_INT_STAT_A		0x13
+#define ADP5585_GPI_INT_STAT_B		0x14
+#define ADP5585_GPI_STATUS_A		0x15
+#define ADP5585_GPI_STATUS_B		0x16
+#define ADP5585_RPULL_CONFIG_A		0x17
+#define ADP5585_RPULL_CONFIG_B		0x18
+#define ADP5585_RPULL_CONFIG_C		0x19
+#define ADP5585_RPULL_CONFIG_D		0x1a
+#define		ADP5585_Rx_PULL_CFG(n, v)	((v) << ((n) * 2))
+#define		ADP5585_Rx_PULL_CFG_PU_300K	(0)
+#define		ADP5585_Rx_PULL_CFG_PD_300K	(1)
+#define		ADP5585_Rx_PULL_CFG_PU_100K	(2)
+#define		ADP5585_Rx_PULL_CFG_DISABLE	(3)
+#define		ADP5585_Rx_PULL_CFG_MASK	(3)
+#define ADP5585_GPI_INT_LEVEL_A		0x1b
+#define ADP5585_GPI_INT_LEVEL_B		0x1c
+#define ADP5585_GPI_EVENT_EN_A		0x1d
+#define ADP5585_GPI_EVENT_EN_B		0x1e
+#define ADP5585_GPI_INTERRUPT_EN_A	0x1f
+#define ADP5585_GPI_INTERRUPT_EN_B	0x20
+#define ADP5585_DEBOUNCE_DIS_A		0x21
+#define ADP5585_DEBOUNCE_DIS_B		0x22
+#define ADP5585_GPO_DATA_OUT_A		0x23
+#define ADP5585_GPO_DATA_OUT_B		0x24
+#define ADP5585_GPO_OUT_MODE_A		0x25
+#define ADP5585_GPO_OUT_MODE_B		0x26
+#define ADP5585_GPIO_DIRECTION_A	0x27
+#define ADP5585_GPIO_DIRECTION_B	0x28
+#define ADP5585_RESET1_EVENT_A		0x29
+#define ADP5585_RESET1_EVENT_B		0x2a
+#define ADP5585_RESET1_EVENT_C		0x2b
+#define ADP5585_RESET2_EVENT_A		0x2c
+#define ADP5585_RESET2_EVENT_B		0x2d
+#define ADP5585_RESET_CFG		0x2e
+#define ADP5585_PWM_OFFT_LOW		0x2f
+#define ADP5585_PWM_OFFT_HIGH		0x30
+#define ADP5585_PWM_ONT_LOW		0x31
+#define ADP5585_PWM_ONT_HIGH		0x32
+#define ADP5585_PWM_CFG			0x33
+#define		ADP5585_PWM_IN_AND		BIT(2)
+#define		ADP5585_PWM_MODE		BIT(1)
+#define		ADP5585_PWM_EN			BIT(0)
+#define ADP5585_LOGIC_CFG		0x34
+#define ADP5585_LOGIC_FF_CFG		0x35
+#define ADP5585_LOGIC_INT_EVENT_EN	0x36
+#define ADP5585_POLL_PTIME_CFG		0x37
+#define ADP5585_PIN_CONFIG_A		0x38
+#define ADP5585_PIN_CONFIG_B		0x39
+#define ADP5585_PIN_CONFIG_C		0x3a
+#define		ADP5585_PULL_SELECT		BIT(7)
+#define		ADP5585_C4_EXTEND_CFG_GPIO11	(0U << 6)
+#define		ADP5585_C4_EXTEND_CFG_RESET2	(1U << 6)
+#define		ADP5585_C4_EXTEND_CFG_MASK	(1U << 6)
+#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
+#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
+#define		ADP5585_R4_EXTEND_CFG_MASK	(1U << 5)
+#define		ADP5585_R3_EXTEND_CFG_GPIO4	(0U << 2)
+#define		ADP5585_R3_EXTEND_CFG_LC	(1U << 2)
+#define		ADP5585_R3_EXTEND_CFG_PWM_OUT	(2U << 2)
+#define		ADP5585_R3_EXTEND_CFG_MASK	(3U << 2)
+#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
+#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
+#define		ADP5585_R0_EXTEND_CFG_MASK	(1U << 0)
+#define ADP5585_GENERAL_CFG		0x3b
+#define		ADP5585_OSC_EN			BIT(7)
+#define		ADP5585_OSC_FREQ_50KHZ		(0U << 5)
+#define		ADP5585_OSC_FREQ_100KHZ		(1U << 5)
+#define		ADP5585_OSC_FREQ_200KHZ		(2U << 5)
+#define		ADP5585_OSC_FREQ_500KHZ		(3U << 5)
+#define		ADP5585_OSC_FREQ_MASK		(3U << 5)
+#define		ADP5585_INT_CFG			BIT(1)
+#define		ADP5585_RST_CFG			BIT(0)
+#define ADP5585_INT_EN			0x3c
+
+#define ADP5585_MAX_REG			ADP5585_INT_EN
+
+#define ADP5585_BANK(n)			((n) >= 6 ? 1 : 0)
+#define ADP5585_BIT(n)			((n) >= 6 ? BIT((n) - 6) : BIT(n))
+
+struct regmap;
+
+struct adp5585_dev {
+	struct regmap *regmap;
+};
+
+#endif
-- 
Regards,

Laurent Pinchart

RE: [PATCH 3/5] mfd: adp5585: Add Analog Devices ADP5585 core support

[Bough Chen]

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 2024年5月21日 4:00
> To: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-gpio@vger.kernel.org; linux-pwm@vger.kernel.org
> Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>; Bartosz
> Golaszewski <brgl@bgdev.pl>; Conor Dooley <conor+dt@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Lee Jones <lee@kernel.org>; Linus Walleij
> <linus.walleij@linaro.org>; Rob Herring <robh@kernel.org>; Uwe Kleine-König
> <ukleinek@kernel.org>; Bough Chen <haibo.chen@nxp.com>
> Subject: [PATCH 3/5] mfd: adp5585: Add Analog Devices ADP5585 core support
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> matrix decoder, programmable logic, reset generator, and PWM generator.
> This driver supports the chip by modelling it as an MFD device, with two child
> devices for the GPIO and PWM functions.
> 
> The driver is derived from an initial implementation from NXP, available in
> commit 8059835bee19 ("MLK-25917-1 mfd: adp5585: add ADI adp5585 core
> support") in their BSP kernel tree. It has been extensively rewritten.

Hi Laurent

Thanks for sending this to upstream.
Just one minor comment in the end of this patch.

> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes compared to the NXP original version:
> 
> - Add MAINTAINERS entry
> - Fix compatible strings for child devices
> - Fix header guards
> - Use lowercase hex constants
> - White space fixes
> - Use module_i2c_driver()
> - Switch to regmap
> - Drop I2C device ID table
> - Drop ADP5585_REG_MASK
> - Support R5 GPIO pin
> - Drop dev field from adp5585_dev structure
> - Check device ID at probe time
> - Fix register field names
> - Update copyright
> - Update license to GPL-2.0-only
> - Implement suspend/resume
> ---
>  MAINTAINERS                 |   2 +
>  drivers/mfd/Kconfig         |  11 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/adp5585.c       | 207
> ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/adp5585.h | 120 +++++++++++++++++++++
>  5 files changed, 341 insertions(+)
>  create mode 100644 drivers/mfd/adp5585.c  create mode 100644
> include/linux/mfd/adp5585.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afecdb82e783..7150f091b69b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -503,6 +503,8 @@ L:	linux-gpio@vger.kernel.org
>  L:	linux-pwm@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
> +F:	drivers/mfd/adp5585.c
> +F:	include/linux/mfd/adp5585.h
> 
>  ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587)
>  M:	Michael Hennerich <michael.hennerich@analog.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> 4b023ee229cf..3423eac0877a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -20,6 +20,17 @@ config MFD_CS5535
>  	  This is the core driver for CS5535/CS5536 MFD functions.  This is
>  	  necessary for using the board's GPIO and MFGPT functionality.
> 
> +config MFD_ADP5585
> +	tristate "Analog Devices ADP5585 MFD driver"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C && OF
> +	help
> +	  Say yes here to add support for the Analog Devices ADP5585 GPIO
> +	  expander, PWM and keypad controller. This includes the I2C driver and
> +	  the core APIs _only_, you have to select individual components like
> +	  the GPIO and PWM functions under the corresponding menus.
> +
>  config MFD_ALTERA_A10SR
>  	bool "Altera Arria10 DevKit System Resource chip"
>  	depends on ARCH_INTEL_SOCFPGA && SPI_MASTER=y && OF diff --git
> a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> c66f07edcd0e..37f36a019a68 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -188,6 +188,7 @@ obj-$(CONFIG_MFD_DB8500_PRCMU)	+=
> db8500-prcmu.o
>  obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
>  obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
>  obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
> +obj-$(CONFIG_MFD_ADP5585)	+= adp5585.o
>  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
>  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c new file mode
> 100644 index 000000000000..75957f9b67c2
> --- /dev/null
> +++ b/drivers/mfd/adp5585.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices ADP5585 I/O expander, PWM controller and keypad
> +controller
> + *
> + * Copyright 2022 NXP
> + * Copyright 2024 Ideas on Board Oy
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/regmap.h>
> +
> +static const struct mfd_cell adp5585_devs[] = {
> +	{
> +		.name = "adp5585-gpio",
> +		.of_compatible = "adi,adp5585-gpio",
> +	},
> +	{
> +		.name = "adp5585-pwm",
> +		.of_compatible = "adi,adp5585-pwm",
> +	},
> +};
> +
> +static const struct regmap_range adp5585_volatile_ranges[] = {
> +	regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), };
> +
> +static const struct regmap_access_table adp5585_volatile_regs = {
> +	.yes_ranges = adp5585_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges),
> +};
> +
> +static const u8 adp5585_regmap_defaults_00[ADP5585_MAX_REG + 1] = {
> +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> +
> +static const u8 adp5585_regmap_defaults_02[ADP5585_MAX_REG + 1] = {
> +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc3,
> +	/* 0x18 */ 0x03, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> +
> +static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = {
> +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55,
> +	/* 0x18 */ 0x05, 0x55, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> +
> +enum adp5585_regmap_type {
> +	ADP5585_REGMAP_00,
> +	ADP5585_REGMAP_02,
> +	ADP5585_REGMAP_04,
> +};
> +
> +static const struct regmap_config adp5585_regmap_configs[] = {
> +	[ADP5585_REGMAP_00] = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = ADP5585_MAX_REG,
> +		.volatile_table = &adp5585_volatile_regs,
> +		.cache_type = REGCACHE_MAPLE,
> +		.reg_defaults_raw = adp5585_regmap_defaults_00,
> +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_00),
> +	},
> +	[ADP5585_REGMAP_02] = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = ADP5585_MAX_REG,
> +		.volatile_table = &adp5585_volatile_regs,
> +		.cache_type = REGCACHE_MAPLE,
> +		.reg_defaults_raw = adp5585_regmap_defaults_02,
> +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_02),
> +	},
> +	[ADP5585_REGMAP_04] = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = ADP5585_MAX_REG,
> +		.volatile_table = &adp5585_volatile_regs,
> +		.cache_type = REGCACHE_MAPLE,
> +		.reg_defaults_raw = adp5585_regmap_defaults_04,
> +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04),
> +	},
> +};
> +
> +
> +static int adp5585_i2c_probe(struct i2c_client *i2c) {
> +	const struct regmap_config *regmap_config;
> +	struct adp5585_dev *adp5585;
> +	unsigned int id;
> +	int ret;
> +
> +	adp5585 = devm_kzalloc(&i2c->dev, sizeof(struct adp5585_dev),
> +			       GFP_KERNEL);
> +	if (adp5585 == NULL)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, adp5585);
> +
> +	regmap_config = of_device_get_match_data(&i2c->dev);
> +	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
> +	if (IS_ERR(adp5585->regmap))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap),
> +				     "Failed to initialize register map\n");
> +
> +	/* Verify the device ID. */
> +	ret = regmap_read(adp5585->regmap, ADP5585_ID, &id);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to read device ID\n");
> +
> +	if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
> +		return dev_err_probe(&i2c->dev, -ENODEV,
> +				     "Invalid device ID 0x%02x\n", id);
> +
> +	dev_dbg(&i2c->dev, "device ID 0x%02x\n", id);
> +
> +	/* Add MFD devices. */
> +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				   adp5585_devs, ARRAY_SIZE(adp5585_devs),
> +				   NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to add MFD devices\n");
> +
> +	return 0;
> +}
> +
> +static int adp5585_suspend(struct device *dev) {
> +	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(adp5585->regmap, true);
> +
> +	return 0;
> +}
> +
> +static int adp5585_resume(struct device *dev) {
> +	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(adp5585->regmap, false);
> +	regcache_mark_dirty(adp5585->regmap);
> +
> +	return regcache_sync(adp5585->regmap); }
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend,
> +adp5585_resume);
> +
> +static const struct of_device_id adp5585_of_match[] = {
> +	{
> +		.compatible = "adi,adp5585-00",
> +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +	}, {
> +		.compatible = "adi,adp5585-01",
> +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +	}, {
> +		.compatible = "adi,adp5585-02",
> +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_02],
> +	}, {
> +		.compatible = "adi,adp5585-03",
> +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> +	}, {
> +		.compatible = "adi,adp5585-04",
> +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_04],
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, adp5585_of_match);
> +
> +static struct i2c_driver adp5585_i2c_driver = {
> +	.driver = {
> +		.name = "adp5585",
> +		.of_match_table = of_match_ptr(adp5585_of_match),
> +		.pm = pm_sleep_ptr(&adp5585_pm),
> +	},
> +	.probe = adp5585_i2c_probe,
> +};
> +module_i2c_driver(adp5585_i2c_driver);
> +
> +MODULE_DESCRIPTION("ADP5585 core driver"); MODULE_AUTHOR("Haibo
> Chen
> +<haibo.chen@nxp.com>"); MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h new file
> mode 100644 index 000000000000..3f178f30fac6
> --- /dev/null
> +++ b/include/linux/mfd/adp5585.h
> @@ -0,0 +1,120 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Analog Devices ADP5585 I/O expander, PWM controller and keypad
> +controller
> + *
> + * Copyright 2022 NXP
> + * Copyright 2024 Ideas on Board Oy
> + */
> +
> +#ifndef __LINUX_MFD_ADP5585_H_
> +#define __LINUX_MFD_ADP5585_H_
> +
> +#include <linux/bits.h>
> +
> +#define ADP5585_ID			0x00
> +#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
> +#define		ADP5585_MAN_ID_VALUE		0x02
> +#define ADP5585_INT_STATUS		0x01
> +#define ADP5585_STATUS			0x02
> +#define ADP5585_FIFO_1			0x03
> +#define ADP5585_FIFO_2			0x04
> +#define ADP5585_FIFO_3			0x05
> +#define ADP5585_FIFO_4			0x06
> +#define ADP5585_FIFO_5			0x07
> +#define ADP5585_FIFO_6			0x08
> +#define ADP5585_FIFO_7			0x09
> +#define ADP5585_FIFO_8			0x0a
> +#define ADP5585_FIFO_9			0x0b
> +#define ADP5585_FIFO_10			0x0c
> +#define ADP5585_FIFO_11			0x0d
> +#define ADP5585_FIFO_12			0x0e
> +#define ADP5585_FIFO_13			0x0f
> +#define ADP5585_FIFO_14			0x10
> +#define ADP5585_FIFO_15			0x11
> +#define ADP5585_FIFO_16			0x12
> +#define ADP5585_GPI_INT_STAT_A		0x13
> +#define ADP5585_GPI_INT_STAT_B		0x14
> +#define ADP5585_GPI_STATUS_A		0x15
> +#define ADP5585_GPI_STATUS_B		0x16
> +#define ADP5585_RPULL_CONFIG_A		0x17
> +#define ADP5585_RPULL_CONFIG_B		0x18
> +#define ADP5585_RPULL_CONFIG_C		0x19
> +#define ADP5585_RPULL_CONFIG_D		0x1a
> +#define		ADP5585_Rx_PULL_CFG(n, v)	((v) << ((n) * 2))
> +#define		ADP5585_Rx_PULL_CFG_PU_300K	(0)
> +#define		ADP5585_Rx_PULL_CFG_PD_300K	(1)
> +#define		ADP5585_Rx_PULL_CFG_PU_100K	(2)
> +#define		ADP5585_Rx_PULL_CFG_DISABLE	(3)
> +#define		ADP5585_Rx_PULL_CFG_MASK	(3)
> +#define ADP5585_GPI_INT_LEVEL_A		0x1b
> +#define ADP5585_GPI_INT_LEVEL_B		0x1c
> +#define ADP5585_GPI_EVENT_EN_A		0x1d
> +#define ADP5585_GPI_EVENT_EN_B		0x1e
> +#define ADP5585_GPI_INTERRUPT_EN_A	0x1f
> +#define ADP5585_GPI_INTERRUPT_EN_B	0x20
> +#define ADP5585_DEBOUNCE_DIS_A		0x21
> +#define ADP5585_DEBOUNCE_DIS_B		0x22
> +#define ADP5585_GPO_DATA_OUT_A		0x23
> +#define ADP5585_GPO_DATA_OUT_B		0x24
> +#define ADP5585_GPO_OUT_MODE_A		0x25
> +#define ADP5585_GPO_OUT_MODE_B		0x26
> +#define ADP5585_GPIO_DIRECTION_A	0x27
> +#define ADP5585_GPIO_DIRECTION_B	0x28
> +#define ADP5585_RESET1_EVENT_A		0x29
> +#define ADP5585_RESET1_EVENT_B		0x2a
> +#define ADP5585_RESET1_EVENT_C		0x2b
> +#define ADP5585_RESET2_EVENT_A		0x2c
> +#define ADP5585_RESET2_EVENT_B		0x2d
> +#define ADP5585_RESET_CFG		0x2e
> +#define ADP5585_PWM_OFFT_LOW		0x2f
> +#define ADP5585_PWM_OFFT_HIGH		0x30
> +#define ADP5585_PWM_ONT_LOW		0x31
> +#define ADP5585_PWM_ONT_HIGH		0x32
> +#define ADP5585_PWM_CFG			0x33
> +#define		ADP5585_PWM_IN_AND		BIT(2)
> +#define		ADP5585_PWM_MODE		BIT(1)
> +#define		ADP5585_PWM_EN			BIT(0)
> +#define ADP5585_LOGIC_CFG		0x34
> +#define ADP5585_LOGIC_FF_CFG		0x35
> +#define ADP5585_LOGIC_INT_EVENT_EN	0x36
> +#define ADP5585_POLL_PTIME_CFG		0x37
> +#define ADP5585_PIN_CONFIG_A		0x38
> +#define ADP5585_PIN_CONFIG_B		0x39
> +#define ADP5585_PIN_CONFIG_C		0x3a
> +#define		ADP5585_PULL_SELECT		BIT(7)
> +#define		ADP5585_C4_EXTEND_CFG_GPIO11	(0U << 6)
> +#define		ADP5585_C4_EXTEND_CFG_RESET2	(1U << 6)
> +#define		ADP5585_C4_EXTEND_CFG_MASK	(1U << 6)
> +#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
> +#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
> +#define		ADP5585_R4_EXTEND_CFG_MASK	(1U << 5)
> +#define		ADP5585_R3_EXTEND_CFG_GPIO4	(0U << 2)
> +#define		ADP5585_R3_EXTEND_CFG_LC	(1U << 2)
> +#define		ADP5585_R3_EXTEND_CFG_PWM_OUT	(2U << 2)
> +#define		ADP5585_R3_EXTEND_CFG_MASK	(3U << 2)
> +#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
> +#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
> +#define		ADP5585_R0_EXTEND_CFG_MASK	(1U << 0)
> +#define ADP5585_GENERAL_CFG		0x3b
> +#define		ADP5585_OSC_EN			BIT(7)
> +#define		ADP5585_OSC_FREQ_50KHZ		(0U << 5)
> +#define		ADP5585_OSC_FREQ_100KHZ		(1U << 5)
> +#define		ADP5585_OSC_FREQ_200KHZ		(2U << 5)
> +#define		ADP5585_OSC_FREQ_500KHZ		(3U << 5)
> +#define		ADP5585_OSC_FREQ_MASK		(3U << 5)
> +#define		ADP5585_INT_CFG			BIT(1)
> +#define		ADP5585_RST_CFG			BIT(0)
> +#define ADP5585_INT_EN			0x3c
> +
> +#define ADP5585_MAX_REG			ADP5585_INT_EN
> +
> +#define ADP5585_BANK(n)			((n) >= 6 ? 1 : 0)
> +#define ADP5585_BIT(n)			((n) >= 6 ? BIT((n) - 6) : BIT(n))

Since you use 6 here for the BANK, maybe better to add comment here.
For adi,adp5585-00(default), need to add " gpio-reserved-ranges = <5 1>;" to reserve gpio5 (GPIO 6/Row 5 in datasheet).
People will meet issue when they try to use GPIO7~GPIO11 without config the reserved range, then will check the BANK here, add comment here will help.

Best Regards
Haibo Chen

> +
> +struct regmap;
> +
> +struct adp5585_dev {
> +	struct regmap *regmap;
> +};
> +
> +#endif
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 3/5] mfd: adp5585: Add Analog Devices ADP5585 core support

[Laurent Pinchart]

Hi Bough,

On Thu, May 23, 2024 at 08:29:32AM +0000, Bough Chen wrote:
> On Sent: 2024年5月21日 4:00, Laurent Pinchart wrote:
> > 
> > From: Haibo Chen <haibo.chen@nxp.com>
> > 
> > The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > matrix decoder, programmable logic, reset generator, and PWM generator.
> > This driver supports the chip by modelling it as an MFD device, with two child
> > devices for the GPIO and PWM functions.
> > 
> > The driver is derived from an initial implementation from NXP, available in
> > commit 8059835bee19 ("MLK-25917-1 mfd: adp5585: add ADI adp5585 core
> > support") in their BSP kernel tree. It has been extensively rewritten.
> 
> Hi Laurent
> 
> Thanks for sending this to upstream.
> Just one minor comment in the end of this patch.
> 
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes compared to the NXP original version:
> > 
> > - Add MAINTAINERS entry
> > - Fix compatible strings for child devices
> > - Fix header guards
> > - Use lowercase hex constants
> > - White space fixes
> > - Use module_i2c_driver()
> > - Switch to regmap
> > - Drop I2C device ID table
> > - Drop ADP5585_REG_MASK
> > - Support R5 GPIO pin
> > - Drop dev field from adp5585_dev structure
> > - Check device ID at probe time
> > - Fix register field names
> > - Update copyright
> > - Update license to GPL-2.0-only
> > - Implement suspend/resume
> > ---
> >  MAINTAINERS                 |   2 +
> >  drivers/mfd/Kconfig         |  11 ++
> >  drivers/mfd/Makefile        |   1 +
> >  drivers/mfd/adp5585.c       | 207
> > ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/adp5585.h | 120 +++++++++++++++++++++
> >  5 files changed, 341 insertions(+)
> >  create mode 100644 drivers/mfd/adp5585.c  create mode 100644
> > include/linux/mfd/adp5585.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index afecdb82e783..7150f091b69b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -503,6 +503,8 @@ L:	linux-gpio@vger.kernel.org
> >  L:	linux-pwm@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
> > +F:	drivers/mfd/adp5585.c
> > +F:	include/linux/mfd/adp5585.h
> > 
> >  ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587)
> >  M:	Michael Hennerich <michael.hennerich@analog.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 4b023ee229cf..3423eac0877a 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -20,6 +20,17 @@ config MFD_CS5535
> >  	  This is the core driver for CS5535/CS5536 MFD functions.  This is
> >  	  necessary for using the board's GPIO and MFGPT functionality.
> > 
> > +config MFD_ADP5585
> > +	tristate "Analog Devices ADP5585 MFD driver"
> > +	select MFD_CORE
> > +	select REGMAP_I2C
> > +	depends on I2C && OF
> > +	help
> > +	  Say yes here to add support for the Analog Devices ADP5585 GPIO
> > +	  expander, PWM and keypad controller. This includes the I2C driver and
> > +	  the core APIs _only_, you have to select individual components like
> > +	  the GPIO and PWM functions under the corresponding menus.
> > +
> >  config MFD_ALTERA_A10SR
> >  	bool "Altera Arria10 DevKit System Resource chip"
> >  	depends on ARCH_INTEL_SOCFPGA && SPI_MASTER=y && OF diff --git
> > a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c66f07edcd0e..37f36a019a68 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -188,6 +188,7 @@ obj-$(CONFIG_MFD_DB8500_PRCMU)	+=
> > db8500-prcmu.o
> >  obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
> >  obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
> >  obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
> > +obj-$(CONFIG_MFD_ADP5585)	+= adp5585.o
> >  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
> >  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
> >  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
> > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c new file mode
> > 100644 index 000000000000..75957f9b67c2
> > --- /dev/null
> > +++ b/drivers/mfd/adp5585.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices ADP5585 I/O expander, PWM controller and keypad
> > +controller
> > + *
> > + * Copyright 2022 NXP
> > + * Copyright 2024 Ideas on Board Oy
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/regmap.h>
> > +
> > +static const struct mfd_cell adp5585_devs[] = {
> > +	{
> > +		.name = "adp5585-gpio",
> > +		.of_compatible = "adi,adp5585-gpio",
> > +	},
> > +	{
> > +		.name = "adp5585-pwm",
> > +		.of_compatible = "adi,adp5585-pwm",
> > +	},
> > +};
> > +
> > +static const struct regmap_range adp5585_volatile_ranges[] = {
> > +	regmap_reg_range(ADP5585_ID, ADP5585_GPI_STATUS_B), };
> > +
> > +static const struct regmap_access_table adp5585_volatile_regs = {
> > +	.yes_ranges = adp5585_volatile_ranges,
> > +	.n_yes_ranges = ARRAY_SIZE(adp5585_volatile_ranges),
> > +};
> > +
> > +static const u8 adp5585_regmap_defaults_00[ADP5585_MAX_REG + 1] = {
> > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x18 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> > +
> > +static const u8 adp5585_regmap_defaults_02[ADP5585_MAX_REG + 1] = {
> > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc3,
> > +	/* 0x18 */ 0x03, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> > +
> > +static const u8 adp5585_regmap_defaults_04[ADP5585_MAX_REG + 1] = {
> > +	/* 0x00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x08 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x10 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55,
> > +	/* 0x18 */ 0x05, 0x55, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x20 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x28 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x30 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +	/* 0x38 */ 0x00, 0x00, 0x00, 0x00, 0x00, };
> > +
> > +enum adp5585_regmap_type {
> > +	ADP5585_REGMAP_00,
> > +	ADP5585_REGMAP_02,
> > +	ADP5585_REGMAP_04,
> > +};
> > +
> > +static const struct regmap_config adp5585_regmap_configs[] = {
> > +	[ADP5585_REGMAP_00] = {
> > +		.reg_bits = 8,
> > +		.val_bits = 8,
> > +		.max_register = ADP5585_MAX_REG,
> > +		.volatile_table = &adp5585_volatile_regs,
> > +		.cache_type = REGCACHE_MAPLE,
> > +		.reg_defaults_raw = adp5585_regmap_defaults_00,
> > +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_00),
> > +	},
> > +	[ADP5585_REGMAP_02] = {
> > +		.reg_bits = 8,
> > +		.val_bits = 8,
> > +		.max_register = ADP5585_MAX_REG,
> > +		.volatile_table = &adp5585_volatile_regs,
> > +		.cache_type = REGCACHE_MAPLE,
> > +		.reg_defaults_raw = adp5585_regmap_defaults_02,
> > +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_02),
> > +	},
> > +	[ADP5585_REGMAP_04] = {
> > +		.reg_bits = 8,
> > +		.val_bits = 8,
> > +		.max_register = ADP5585_MAX_REG,
> > +		.volatile_table = &adp5585_volatile_regs,
> > +		.cache_type = REGCACHE_MAPLE,
> > +		.reg_defaults_raw = adp5585_regmap_defaults_04,
> > +		.num_reg_defaults_raw = sizeof(adp5585_regmap_defaults_04),
> > +	},
> > +};
> > +
> > +
> > +static int adp5585_i2c_probe(struct i2c_client *i2c) {
> > +	const struct regmap_config *regmap_config;
> > +	struct adp5585_dev *adp5585;
> > +	unsigned int id;
> > +	int ret;
> > +
> > +	adp5585 = devm_kzalloc(&i2c->dev, sizeof(struct adp5585_dev),
> > +			       GFP_KERNEL);
> > +	if (adp5585 == NULL)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(i2c, adp5585);
> > +
> > +	regmap_config = of_device_get_match_data(&i2c->dev);
> > +	adp5585->regmap = devm_regmap_init_i2c(i2c, regmap_config);
> > +	if (IS_ERR(adp5585->regmap))
> > +		return dev_err_probe(&i2c->dev, PTR_ERR(adp5585->regmap),
> > +				     "Failed to initialize register map\n");
> > +
> > +	/* Verify the device ID. */
> > +	ret = regmap_read(adp5585->regmap, ADP5585_ID, &id);
> > +	if (ret)
> > +		return dev_err_probe(&i2c->dev, ret,
> > +				     "Failed to read device ID\n");
> > +
> > +	if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
> > +		return dev_err_probe(&i2c->dev, -ENODEV,
> > +				     "Invalid device ID 0x%02x\n", id);
> > +
> > +	dev_dbg(&i2c->dev, "device ID 0x%02x\n", id);
> > +
> > +	/* Add MFD devices. */
> > +	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > +				   adp5585_devs, ARRAY_SIZE(adp5585_devs),
> > +				   NULL, 0, NULL);
> > +	if (ret)
> > +		return dev_err_probe(&i2c->dev, ret,
> > +				     "Failed to add MFD devices\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int adp5585_suspend(struct device *dev) {
> > +	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
> > +
> > +	regcache_cache_only(adp5585->regmap, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int adp5585_resume(struct device *dev) {
> > +	struct adp5585_dev *adp5585 = dev_get_drvdata(dev);
> > +
> > +	regcache_cache_only(adp5585->regmap, false);
> > +	regcache_mark_dirty(adp5585->regmap);
> > +
> > +	return regcache_sync(adp5585->regmap); }
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(adp5585_pm, adp5585_suspend,
> > +adp5585_resume);
> > +
> > +static const struct of_device_id adp5585_of_match[] = {
> > +	{
> > +		.compatible = "adi,adp5585-00",
> > +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > +	}, {
> > +		.compatible = "adi,adp5585-01",
> > +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > +	}, {
> > +		.compatible = "adi,adp5585-02",
> > +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_02],
> > +	}, {
> > +		.compatible = "adi,adp5585-03",
> > +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_00],
> > +	}, {
> > +		.compatible = "adi,adp5585-04",
> > +		.data = &adp5585_regmap_configs[ADP5585_REGMAP_04],
> > +	},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_of_match);
> > +
> > +static struct i2c_driver adp5585_i2c_driver = {
> > +	.driver = {
> > +		.name = "adp5585",
> > +		.of_match_table = of_match_ptr(adp5585_of_match),
> > +		.pm = pm_sleep_ptr(&adp5585_pm),
> > +	},
> > +	.probe = adp5585_i2c_probe,
> > +};
> > +module_i2c_driver(adp5585_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("ADP5585 core driver"); MODULE_AUTHOR("Haibo
> > Chen
> > +<haibo.chen@nxp.com>"); MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h new file
> > mode 100644 index 000000000000..3f178f30fac6
> > --- /dev/null
> > +++ b/include/linux/mfd/adp5585.h
> > @@ -0,0 +1,120 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Analog Devices ADP5585 I/O expander, PWM controller and keypad
> > +controller
> > + *
> > + * Copyright 2022 NXP
> > + * Copyright 2024 Ideas on Board Oy
> > + */
> > +
> > +#ifndef __LINUX_MFD_ADP5585_H_
> > +#define __LINUX_MFD_ADP5585_H_
> > +
> > +#include <linux/bits.h>
> > +
> > +#define ADP5585_ID			0x00
> > +#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
> > +#define		ADP5585_MAN_ID_VALUE		0x02
> > +#define ADP5585_INT_STATUS		0x01
> > +#define ADP5585_STATUS			0x02
> > +#define ADP5585_FIFO_1			0x03
> > +#define ADP5585_FIFO_2			0x04
> > +#define ADP5585_FIFO_3			0x05
> > +#define ADP5585_FIFO_4			0x06
> > +#define ADP5585_FIFO_5			0x07
> > +#define ADP5585_FIFO_6			0x08
> > +#define ADP5585_FIFO_7			0x09
> > +#define ADP5585_FIFO_8			0x0a
> > +#define ADP5585_FIFO_9			0x0b
> > +#define ADP5585_FIFO_10			0x0c
> > +#define ADP5585_FIFO_11			0x0d
> > +#define ADP5585_FIFO_12			0x0e
> > +#define ADP5585_FIFO_13			0x0f
> > +#define ADP5585_FIFO_14			0x10
> > +#define ADP5585_FIFO_15			0x11
> > +#define ADP5585_FIFO_16			0x12
> > +#define ADP5585_GPI_INT_STAT_A		0x13
> > +#define ADP5585_GPI_INT_STAT_B		0x14
> > +#define ADP5585_GPI_STATUS_A		0x15
> > +#define ADP5585_GPI_STATUS_B		0x16
> > +#define ADP5585_RPULL_CONFIG_A		0x17
> > +#define ADP5585_RPULL_CONFIG_B		0x18
> > +#define ADP5585_RPULL_CONFIG_C		0x19
> > +#define ADP5585_RPULL_CONFIG_D		0x1a
> > +#define		ADP5585_Rx_PULL_CFG(n, v)	((v) << ((n) * 2))
> > +#define		ADP5585_Rx_PULL_CFG_PU_300K	(0)
> > +#define		ADP5585_Rx_PULL_CFG_PD_300K	(1)
> > +#define		ADP5585_Rx_PULL_CFG_PU_100K	(2)
> > +#define		ADP5585_Rx_PULL_CFG_DISABLE	(3)
> > +#define		ADP5585_Rx_PULL_CFG_MASK	(3)
> > +#define ADP5585_GPI_INT_LEVEL_A		0x1b
> > +#define ADP5585_GPI_INT_LEVEL_B		0x1c
> > +#define ADP5585_GPI_EVENT_EN_A		0x1d
> > +#define ADP5585_GPI_EVENT_EN_B		0x1e
> > +#define ADP5585_GPI_INTERRUPT_EN_A	0x1f
> > +#define ADP5585_GPI_INTERRUPT_EN_B	0x20
> > +#define ADP5585_DEBOUNCE_DIS_A		0x21
> > +#define ADP5585_DEBOUNCE_DIS_B		0x22
> > +#define ADP5585_GPO_DATA_OUT_A		0x23
> > +#define ADP5585_GPO_DATA_OUT_B		0x24
> > +#define ADP5585_GPO_OUT_MODE_A		0x25
> > +#define ADP5585_GPO_OUT_MODE_B		0x26
> > +#define ADP5585_GPIO_DIRECTION_A	0x27
> > +#define ADP5585_GPIO_DIRECTION_B	0x28
> > +#define ADP5585_RESET1_EVENT_A		0x29
> > +#define ADP5585_RESET1_EVENT_B		0x2a
> > +#define ADP5585_RESET1_EVENT_C		0x2b
> > +#define ADP5585_RESET2_EVENT_A		0x2c
> > +#define ADP5585_RESET2_EVENT_B		0x2d
> > +#define ADP5585_RESET_CFG		0x2e
> > +#define ADP5585_PWM_OFFT_LOW		0x2f
> > +#define ADP5585_PWM_OFFT_HIGH		0x30
> > +#define ADP5585_PWM_ONT_LOW		0x31
> > +#define ADP5585_PWM_ONT_HIGH		0x32
> > +#define ADP5585_PWM_CFG			0x33
> > +#define		ADP5585_PWM_IN_AND		BIT(2)
> > +#define		ADP5585_PWM_MODE		BIT(1)
> > +#define		ADP5585_PWM_EN			BIT(0)
> > +#define ADP5585_LOGIC_CFG		0x34
> > +#define ADP5585_LOGIC_FF_CFG		0x35
> > +#define ADP5585_LOGIC_INT_EVENT_EN	0x36
> > +#define ADP5585_POLL_PTIME_CFG		0x37
> > +#define ADP5585_PIN_CONFIG_A		0x38
> > +#define ADP5585_PIN_CONFIG_B		0x39
> > +#define ADP5585_PIN_CONFIG_C		0x3a
> > +#define		ADP5585_PULL_SELECT		BIT(7)
> > +#define		ADP5585_C4_EXTEND_CFG_GPIO11	(0U << 6)
> > +#define		ADP5585_C4_EXTEND_CFG_RESET2	(1U << 6)
> > +#define		ADP5585_C4_EXTEND_CFG_MASK	(1U << 6)
> > +#define		ADP5585_R4_EXTEND_CFG_GPIO5	(0U << 5)
> > +#define		ADP5585_R4_EXTEND_CFG_RESET1	(1U << 5)
> > +#define		ADP5585_R4_EXTEND_CFG_MASK	(1U << 5)
> > +#define		ADP5585_R3_EXTEND_CFG_GPIO4	(0U << 2)
> > +#define		ADP5585_R3_EXTEND_CFG_LC	(1U << 2)
> > +#define		ADP5585_R3_EXTEND_CFG_PWM_OUT	(2U << 2)
> > +#define		ADP5585_R3_EXTEND_CFG_MASK	(3U << 2)
> > +#define		ADP5585_R0_EXTEND_CFG_GPIO1	(0U << 0)
> > +#define		ADP5585_R0_EXTEND_CFG_LY	(1U << 0)
> > +#define		ADP5585_R0_EXTEND_CFG_MASK	(1U << 0)
> > +#define ADP5585_GENERAL_CFG		0x3b
> > +#define		ADP5585_OSC_EN			BIT(7)
> > +#define		ADP5585_OSC_FREQ_50KHZ		(0U << 5)
> > +#define		ADP5585_OSC_FREQ_100KHZ		(1U << 5)
> > +#define		ADP5585_OSC_FREQ_200KHZ		(2U << 5)
> > +#define		ADP5585_OSC_FREQ_500KHZ		(3U << 5)
> > +#define		ADP5585_OSC_FREQ_MASK		(3U << 5)
> > +#define		ADP5585_INT_CFG			BIT(1)
> > +#define		ADP5585_RST_CFG			BIT(0)
> > +#define ADP5585_INT_EN			0x3c
> > +
> > +#define ADP5585_MAX_REG			ADP5585_INT_EN
> > +
> > +#define ADP5585_BANK(n)			((n) >= 6 ? 1 : 0)
> > +#define ADP5585_BIT(n)			((n) >= 6 ? BIT((n) - 6) : BIT(n))
> 
> Since you use 6 here for the BANK, maybe better to add comment here.
> For adi,adp5585-00(default), need to add "gpio-reserved-ranges = <5 1>;"
> to reserve gpio5 (GPIO 6/Row 5 in datasheet).

That's right. The DT bindings document that. I decided to follow the
numbering scheme from the datasheet, and mark GPIO 6/R5 as reserved for
chips that don't support it. The alternative would shift all the upper
GPIOs by one, and I think that would be more confusing for the users.

> People will meet issue when they try to use GPIO7~GPIO11 without
> config the reserved range, then will check the BANK here, add comment
> here will help.

I'll add a comment in the next version.

> > +
> > +struct regmap;
> > +
> > +struct adp5585_dev {
> > +	struct regmap *regmap;
> > +};
> > +
> > +#endif

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] gpio: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

From: Haibo Chen <haibo.chen@nxp.com>

The ADP5585 is a 10/11 input/output port expander with a built in keypad
matrix decoder, programmable logic, reset generator, and PWM generator.
This driver supports the GPIO function using the platform device
registered by the core MFD driver.

The driver is derived from an initial implementation from NXP, available
in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add
adp5585-gpio support") in their BSP kernel tree. It has been extensively
rewritten.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes compared to the NXP original version

- Add MAINTAINERS entry
- Add prefix to compatible string
- Switch to regmap
- White space fixes
- Use sizeof(*variable)
- Initialize variables at declaration time
- Use mutex scope guards
- Cleanup header includes
- Support R5 GPIO pin
- Reorder functions
- Add bias support
- Return real pin value from .get()
- Add debounce support
- Add drive mode support
- Destroy mutex on remove
- Update copyright
- Update license to GPL-2.0-only
---
 MAINTAINERS                 |   1 +
 drivers/gpio/Kconfig        |   7 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-adp5585.c | 232 ++++++++++++++++++++++++++++++++++++
 4 files changed, 241 insertions(+)
 create mode 100644 drivers/gpio/gpio-adp5585.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7150f091b69b..5689fec270ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -503,6 +503,7 @@ L:	linux-gpio@vger.kernel.org
 L:	linux-pwm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
+F:	drivers/gpio/gpio-adp5585.c
 F:	drivers/mfd/adp5585.c
 F:	include/linux/mfd/adp5585.h
 
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b50d0b470849..79d004e31a47 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1206,6 +1206,13 @@ config GPIO_ADP5520
 	  This option enables support for on-chip GPIO found
 	  on Analog Devices ADP5520 PMICs.
 
+config GPIO_ADP5585
+	tristate "GPIO Support for ADP5585"
+	depends on MFD_ADP5585
+	help
+	  This option enables support for the GPIO function found in the Analog
+	  Devices ADP5585.
+
 config GPIO_ALTERA_A10SR
 	tristate "Altera Arria10 System Resource GPIO"
 	depends on MFD_ALTERA_A10SR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index fdd28c58d890..b44b6bc3a74f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_74X164)		+= gpio-74x164.o
 obj-$(CONFIG_GPIO_74XX_MMIO)		+= gpio-74xx-mmio.o
 obj-$(CONFIG_GPIO_ADNP)			+= gpio-adnp.o
 obj-$(CONFIG_GPIO_ADP5520)		+= gpio-adp5520.o
+obj-$(CONFIG_GPIO_ADP5585)		+= gpio-adp5585.o
 obj-$(CONFIG_GPIO_AGGREGATOR)		+= gpio-aggregator.o
 obj-$(CONFIG_GPIO_ALTERA_A10SR)		+= gpio-altera-a10sr.o
 obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
new file mode 100644
index 000000000000..30e538f8d1ac
--- /dev/null
+++ b/drivers/gpio/gpio-adp5585.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ADP5585 GPIO driver
+ *
+ * Copyright 2022 NXP
+ * Copyright 2024 Ideas on Board Oy
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define ADP5585_GPIO_MAX	11
+
+struct adp5585_gpio_dev {
+	struct gpio_chip gpio_chip;
+	struct regmap *regmap;
+	struct mutex lock;
+};
+
+static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	return regmap_update_bits(adp5585_gpio->regmap,
+				  ADP5585_GPIO_DIRECTION_A + bank,
+				  bit, 0);
+}
+
+static int adp5585_gpio_direction_output(struct gpio_chip *chip, unsigned int off, int val)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+	int ret;
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	ret = regmap_update_bits(adp5585_gpio->regmap,
+				 ADP5585_GPO_DATA_OUT_A + bank, bit,
+				 val ? bit : 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(adp5585_gpio->regmap,
+				  ADP5585_GPIO_DIRECTION_A + bank,
+				  bit, bit);
+}
+
+static int adp5585_gpio_get_value(struct gpio_chip *chip, unsigned int off)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+	unsigned int val;
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	regmap_read(adp5585_gpio->regmap, ADP5585_GPI_STATUS_A + bank, &val);
+
+	return !!(val & bit);
+}
+
+static void adp5585_gpio_set_value(struct gpio_chip *chip, unsigned int off, int val)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	regmap_update_bits(adp5585_gpio->regmap, ADP5585_GPO_DATA_OUT_A + bank,
+			   bit, val ? bit : 0);
+}
+
+static int adp5585_gpio_set_bias(struct adp5585_gpio_dev *adp5585_gpio,
+				 unsigned int off, unsigned int bias)
+{
+	unsigned int bit, reg, mask, val;
+
+	/*
+	 * The bias configuration fields are 2 bits wide and laid down in
+	 * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits
+	 * after R5.
+	 */
+	bit = off * 2 + (off > 5 ? 4 : 0);
+	reg = ADP5585_RPULL_CONFIG_A + bit / 8;
+	mask = ADP5585_Rx_PULL_CFG(bit % 8, ADP5585_Rx_PULL_CFG_MASK);
+	val = ADP5585_Rx_PULL_CFG(bit % 8, bias);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	return regmap_update_bits(adp5585_gpio->regmap, reg, mask, val);
+}
+
+static int adp5585_gpio_set_drive(struct adp5585_gpio_dev *adp5585_gpio,
+				  unsigned int off, enum pin_config_param drive)
+{
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	return regmap_update_bits(adp5585_gpio->regmap,
+				  ADP5585_GPO_OUT_MODE_A + bank, bit,
+				  drive == PIN_CONFIG_DRIVE_OPEN_DRAIN ? 1 : 0);
+}
+
+static int adp5585_gpio_set_debounce(struct adp5585_gpio_dev *adp5585_gpio,
+				     unsigned int off, unsigned int debounce)
+{
+	unsigned int bank = ADP5585_BANK(off);
+	unsigned int bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	return regmap_update_bits(adp5585_gpio->regmap,
+				  ADP5585_DEBOUNCE_DIS_A + bank, bit,
+				  debounce ? 0 : 1);
+}
+
+static int adp5585_gpio_set_config(struct gpio_chip *chip, unsigned int off,
+				   unsigned long config)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
+	enum pin_config_param param = pinconf_to_config_param(config);
+	u32 arg = pinconf_to_config_argument(config);
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		return adp5585_gpio_set_bias(adp5585_gpio, off,
+					     ADP5585_Rx_PULL_CFG_DISABLE);
+
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return adp5585_gpio_set_bias(adp5585_gpio, off, arg ?
+					     ADP5585_Rx_PULL_CFG_PD_300K :
+					     ADP5585_Rx_PULL_CFG_DISABLE);
+
+	case PIN_CONFIG_BIAS_PULL_UP:
+		return adp5585_gpio_set_bias(adp5585_gpio, off, arg ?
+					     ADP5585_Rx_PULL_CFG_PU_300K :
+					     ADP5585_Rx_PULL_CFG_DISABLE);
+
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return adp5585_gpio_set_drive(adp5585_gpio, off, param);
+
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		return adp5585_gpio_set_debounce(adp5585_gpio, off, arg);
+
+	default:
+		return -ENOTSUPP;
+	};
+}
+
+static int adp5585_gpio_probe(struct platform_device *pdev)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
+	struct adp5585_gpio_dev *adp5585_gpio;
+	struct device *dev = &pdev->dev;
+	struct gpio_chip *gc;
+	int ret;
+
+	adp5585_gpio = devm_kzalloc(&pdev->dev, sizeof(*adp5585_gpio), GFP_KERNEL);
+	if (!adp5585_gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, adp5585_gpio);
+
+	adp5585_gpio->regmap = adp5585->regmap;
+
+	mutex_init(&adp5585_gpio->lock);
+
+	gc = &adp5585_gpio->gpio_chip;
+	gc->parent = dev;
+	gc->direction_input  = adp5585_gpio_direction_input;
+	gc->direction_output = adp5585_gpio_direction_output;
+	gc->get = adp5585_gpio_get_value;
+	gc->set = adp5585_gpio_set_value;
+	gc->set_config = adp5585_gpio_set_config;
+	gc->can_sleep = true;
+
+	gc->base = -1;
+	gc->ngpio = ADP5585_GPIO_MAX;
+	gc->label = pdev->name;
+	gc->owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
+				     adp5585_gpio);
+	if (ret) {
+		mutex_destroy(&adp5585_gpio->lock);
+		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
+	}
+
+	return 0;
+}
+
+static void adp5585_gpio_remove(struct platform_device *pdev)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
+
+	mutex_destroy(&adp5585_gpio->lock);
+}
+
+static const struct of_device_id adp5585_of_match[] = {
+	{ .compatible = "adi,adp5585-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_of_match);
+
+static struct platform_driver adp5585_gpio_driver = {
+	.driver	= {
+		.name = "adp5585-gpio",
+		.of_match_table = adp5585_of_match,
+	},
+	.probe = adp5585_gpio_probe,
+	.remove_new = adp5585_gpio_remove,
+};
+module_platform_driver(adp5585_gpio_driver);
+
+MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>");
+MODULE_DESCRIPTION("GPIO ADP5585 Driver");
+MODULE_LICENSE("GPL");
-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] gpio: adp5585: Add Analog Devices ADP5585 support

[Linus Walleij]

Hi Laurent/Haibo,

thanks for your patch!

On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> From: Haibo Chen <haibo.chen@nxp.com>
>
> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> matrix decoder, programmable logic, reset generator, and PWM generator.
> This driver supports the GPIO function using the platform device
> registered by the core MFD driver.
>
> The driver is derived from an initial implementation from NXP, available
> in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add
> adp5585-gpio support") in their BSP kernel tree. It has been extensively
> rewritten.
>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(...)

> +static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
> +{
> +       struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
> +       unsigned int bank = ADP5585_BANK(off);
> +       unsigned int bit = ADP5585_BIT(off);
> +
> +       guard(mutex)(&adp5585_gpio->lock);
> +
> +       return regmap_update_bits(adp5585_gpio->regmap,
> +                                 ADP5585_GPIO_DIRECTION_A + bank,
> +                                 bit, 0);

First, I love the guarded mutex!

But doesn't regmap already contain a mutex? Or is this one of those
cases where regmap has been instantiated without a lock?

> +       gc = &adp5585_gpio->gpio_chip;
> +       gc->parent = dev;
> +       gc->direction_input  = adp5585_gpio_direction_input;
> +       gc->direction_output = adp5585_gpio_direction_output;

And chance to implemen ->get_direction()?

Other than this I think the driver is ready for merge, so with the
comments fixed or addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/5] gpio: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

Hi Linus,

On Tue, May 28, 2024 at 01:54:29PM +0200, Linus Walleij wrote:
> On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart wrote:
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > matrix decoder, programmable logic, reset generator, and PWM generator.
> > This driver supports the GPIO function using the platform device
> > registered by the core MFD driver.
> >
> > The driver is derived from an initial implementation from NXP, available
> > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add
> > adp5585-gpio support") in their BSP kernel tree. It has been extensively
> > rewritten.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> (...)
> 
> > +static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
> > +{
> > +       struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
> > +       unsigned int bank = ADP5585_BANK(off);
> > +       unsigned int bit = ADP5585_BIT(off);
> > +
> > +       guard(mutex)(&adp5585_gpio->lock);
> > +
> > +       return regmap_update_bits(adp5585_gpio->regmap,
> > +                                 ADP5585_GPIO_DIRECTION_A + bank,
> > +                                 bit, 0);
> 
> First, I love the guarded mutex!

Yes, it's neat :-)

> But doesn't regmap already contain a mutex? Or is this one of those
> cases where regmap has been instantiated without a lock?

regmap indeed includes a lock, but it will lock each register access
independently. In adp5585_gpio_get_value() we need to read two
registers atomically, so we need to cover them by a single lock.

I could drop the lock from regmap, but I would then likely need to
introduce a lock in the parent mfd device that both the PWM and GPIO
children would use to protect bus access. That may make sense in the
future, but is a bit overkill for now I think.

> > +       gc = &adp5585_gpio->gpio_chip;
> > +       gc->parent = dev;
> > +       gc->direction_input  = adp5585_gpio_direction_input;
> > +       gc->direction_output = adp5585_gpio_direction_output;
> 
> And chance to implemen ->get_direction()?

Sure, I'll add that to v2.

> Other than this I think the driver is ready for merge, so with the
> comments fixed or addressed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] gpio: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

On Tue, May 28, 2024 at 03:27:34PM +0300, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Tue, May 28, 2024 at 01:54:29PM +0200, Linus Walleij wrote:
> > On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart wrote:
> > 
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad
> > > matrix decoder, programmable logic, reset generator, and PWM generator.
> > > This driver supports the GPIO function using the platform device
> > > registered by the core MFD driver.
> > >
> > > The driver is derived from an initial implementation from NXP, available
> > > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add
> > > adp5585-gpio support") in their BSP kernel tree. It has been extensively
> > > rewritten.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > (...)
> > 
> > > +static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
> > > +{
> > > +       struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip);
> > > +       unsigned int bank = ADP5585_BANK(off);
> > > +       unsigned int bit = ADP5585_BIT(off);
> > > +
> > > +       guard(mutex)(&adp5585_gpio->lock);
> > > +
> > > +       return regmap_update_bits(adp5585_gpio->regmap,
> > > +                                 ADP5585_GPIO_DIRECTION_A + bank,
> > > +                                 bit, 0);
> > 
> > First, I love the guarded mutex!
> 
> Yes, it's neat :-)
> 
> > But doesn't regmap already contain a mutex? Or is this one of those
> > cases where regmap has been instantiated without a lock?
> 
> regmap indeed includes a lock, but it will lock each register access
> independently. In adp5585_gpio_get_value() we need to read two
> registers atomically, so we need to cover them by a single lock.
> 
> I could drop the lock from regmap, but I would then likely need to
> introduce a lock in the parent mfd device that both the PWM and GPIO
> children would use to protect bus access. That may make sense in the
> future, but is a bit overkill for now I think.

Actually, I think I can drop the lock. Concurrent access to the
registers from different GPIOs are protected by the regmap lock, and
concurrent access to groups of registers for the same GPIO isn't a valid
use case as callers shouldn't do that.

> > > +       gc = &adp5585_gpio->gpio_chip;
> > > +       gc->parent = dev;
> > > +       gc->direction_input  = adp5585_gpio_direction_input;
> > > +       gc->direction_output = adp5585_gpio_direction_output;
> > 
> > And chance to implemen ->get_direction()?
> 
> Sure, I'll add that to v2.
> 
> > Other than this I think the driver is ready for merge, so with the
> > comments fixed or addressed:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thank you.

-- 
Regards,

Laurent Pinchart

[PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

From: Clark Wang <xiaoning.wang@nxp.com>

The ADP5585 is a 10/11 input/output port expander with a built in keypad
matrix decoder, programmable logic, reset generator, and PWM generator.
This driver supports the PWM function using the platform device
registered by the core MFD driver.

The driver is derived from an initial implementation from NXP, available
in commit 113113742208 ("MLK-25922-1 pwm: adp5585: add adp5585 PWM
support") in their BSP kernel tree. It has been extensively rewritten.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes compared to the NXP original version

- Add MAINTAINERS entry
- Drop pwm_ops.owner
- Fix compilation
- Add prefix to compatible string
- Switch to regmap
- Use devm_pwmchip_add()
- Cleanup header includes
- White space fixes
- Drop ADP5585_REG_MASK
- Fix register field names
- Use mutex scope guards
- Clear OSC_EN when freeing PWM
- Reorder functions
- Clear PWM_IN_AND and PWM_MODE bits
- Support inverted polarity
- Clean up on/off computations
- Fix duty cycle computation in .get_state()
- Destroy mutex on remove
- Update copyright
- Update license to GPL-2.0-only
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |   7 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-adp5585.c | 230 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 drivers/pwm/pwm-adp5585.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5689fec270ef..280f97129598 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -505,6 +505,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/*/adi,adp5585*.yaml
 F:	drivers/gpio/gpio-adp5585.c
 F:	drivers/mfd/adp5585.c
+F:	drivers/pwm/pwm-adp5585.c
 F:	include/linux/mfd/adp5585.h
 
 ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..2393a50b3781 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -51,6 +51,13 @@ config PWM_AB8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-ab8500.
 
+config PWM_ADP5585
+	tristate "ADP5585 PWM support"
+	depends on MFD_ADP5585
+	help
+	  This option enables support for the PWM function found in the Analog
+	  Devices ADP5585.
+
 config PWM_APPLE
 	tristate "Apple SoC PWM support"
 	depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..100ac66b5f40 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
+obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
 obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
new file mode 100644
index 000000000000..709713d8f47a
--- /dev/null
+++ b/drivers/pwm/pwm-adp5585.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ADP5585 PWM driver
+ *
+ * Copyright 2022 NXP
+ * Copyright 2024 Ideas on Board Oy
+ */
+
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/time.h>
+
+#define ADP5585_PWM_CHAN_NUM		1
+
+#define ADP5585_PWM_OSC_FREQ_HZ		1000000U
+#define ADP5585_PWM_MIN_PERIOD_NS	(2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
+#define ADP5585_PWM_MAX_PERIOD_NS	(2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
+
+struct adp5585_pwm_chip {
+	struct pwm_chip chip;
+	struct regmap *regmap;
+	struct mutex lock;
+	u8 pin_config_val;
+};
+
+static inline struct adp5585_pwm_chip *
+to_adp5585_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct adp5585_pwm_chip, chip);
+}
+
+static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	unsigned int val;
+	int ret;
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	ret = regmap_read(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C, &val);
+	if (ret)
+		return ret;
+
+	adp5585_pwm->pin_config_val = val;
+
+	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
+				 ADP5585_R3_EXTEND_CFG_MASK,
+				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
+				 ADP5585_OSC_EN, ADP5585_OSC_EN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
+			   ADP5585_R3_EXTEND_CFG_MASK,
+			   adp5585_pwm->pin_config_val);
+	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
+			   ADP5585_OSC_EN, 0);
+}
+
+static int pwm_adp5585_apply(struct pwm_chip *chip,
+			     struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	u32 on, off;
+	int ret;
+
+	if (!state->enabled) {
+		guard(mutex)(&adp5585_pwm->lock);
+
+		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
+					  ADP5585_PWM_EN, 0);
+	}
+
+	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
+	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
+		return -EINVAL;
+
+	/*
+	 * Compute the on and off time. As the internal oscillator frequency is
+	 * 1MHz, the calculation can be simplified without loss of precision.
+	 */
+	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle,
+				   NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
+	off = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
+				    NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		swap(on, off);
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	ret = regmap_write(adp5585_pwm->regmap, ADP5585_PWM_OFFT_LOW,
+			   off & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(adp5585_pwm->regmap, ADP5585_PWM_OFFT_HIGH,
+			   (off >> 8) & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(adp5585_pwm->regmap, ADP5585_PWM_ONT_LOW,
+			   on & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(adp5585_pwm->regmap, ADP5585_PWM_ONT_HIGH,
+			   (on >> 8) & 0xff);
+	if (ret)
+		return ret;
+
+	/* Enable PWM in continuous mode and no external AND'ing. */
+	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
+				 ADP5585_PWM_IN_AND | ADP5585_PWM_MODE |
+				 ADP5585_PWM_EN, ADP5585_PWM_EN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int pwm_adp5585_get_state(struct pwm_chip *chip,
+				 struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	unsigned int on, off;
+	unsigned int val;
+
+	regmap_read(adp5585_pwm->regmap, ADP5585_PWM_OFFT_LOW, &off);
+	regmap_read(adp5585_pwm->regmap, ADP5585_PWM_OFFT_HIGH, &val);
+	off |= val << 8;
+
+	regmap_read(adp5585_pwm->regmap, ADP5585_PWM_ONT_LOW, &on);
+	regmap_read(adp5585_pwm->regmap, ADP5585_PWM_ONT_HIGH, &val);
+	on |= val << 8;
+
+	state->duty_cycle = on * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
+	state->period = (on + off) * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	regmap_read(adp5585_pwm->regmap, ADP5585_PWM_CFG, &val);
+	state->enabled = !!(val & ADP5585_PWM_EN);
+
+	return 0;
+}
+
+static const struct pwm_ops adp5585_pwm_ops = {
+	.request = pwm_adp5585_request,
+	.free = pwm_adp5585_free,
+	.apply = pwm_adp5585_apply,
+	.get_state = pwm_adp5585_get_state,
+};
+
+static int adp5585_pwm_probe(struct platform_device *pdev)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
+	struct adp5585_pwm_chip *adp5585_pwm;
+	int ret;
+
+	adp5585_pwm = devm_kzalloc(&pdev->dev, sizeof(*adp5585_pwm), GFP_KERNEL);
+	if (!adp5585_pwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, adp5585_pwm);
+
+	adp5585_pwm->regmap = adp5585->regmap;
+
+	mutex_init(&adp5585_pwm->lock);
+
+	adp5585_pwm->chip.dev = &pdev->dev;
+	adp5585_pwm->chip.ops = &adp5585_pwm_ops;
+	adp5585_pwm->chip.npwm = ADP5585_PWM_CHAN_NUM;
+
+	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
+	if (ret) {
+		mutex_destroy(&adp5585_pwm->lock);
+		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}
+
+	return 0;
+}
+
+static void adp5585_pwm_remove(struct platform_device *pdev)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
+
+	mutex_destroy(&adp5585_pwm->lock);
+}
+
+static const struct of_device_id adp5585_pwm_of_match[] = {
+	{ .compatible = "adi,adp5585-pwm" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
+
+static struct platform_driver adp5585_pwm_driver = {
+	.driver	= {
+		.name = "adp5585-pwm",
+		.of_match_table = adp5585_pwm_of_match,
+	},
+	.probe = adp5585_pwm_probe,
+	.remove_new = adp5585_pwm_remove,
+};
+module_platform_driver(adp5585_pwm_driver);
+
+MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");
+MODULE_DESCRIPTION("ADP5585 PWM Driver");
+MODULE_LICENSE("GPL");
-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Uwe Kleine-König]

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

Hello Laurent,

On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> new file mode 100644
> index 000000000000..709713d8f47a
> --- /dev/null
> +++ b/drivers/pwm/pwm-adp5585.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices ADP5585 PWM driver
> + *
> + * Copyright 2022 NXP
> + * Copyright 2024 Ideas on Board Oy
> + */

Please document some hardware properties here in the same format as many
other PWM drivers. The things I'd like to read there are:

 - Only supports normal polarity
 - How does the output pin behave when the hardware is disabled
   (typically "low" or "high-Z" or "freeze")
 - Does changing parameters or disabling complete the currently running
   period?
 - Are there glitches in .apply()? E.g. when the new duty_cycle is
   already written but the new period is not.

> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>

Do you need these all? I wounder about time.h.

> +#define ADP5585_PWM_CHAN_NUM		1
> +
> +#define ADP5585_PWM_OSC_FREQ_HZ		1000000U
> +#define ADP5585_PWM_MIN_PERIOD_NS	(2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> +#define ADP5585_PWM_MAX_PERIOD_NS	(2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> +
> +struct adp5585_pwm_chip {
> +	struct pwm_chip chip;
> +	struct regmap *regmap;
> +	struct mutex lock;

What does this mutex protect against? You can safely assume that there
are no concurrent calls of the callbacks. (This isn't ensured yet, but I
consider a consumer who does this buggy and it will soon be ensured.)

> +	u8 pin_config_val;
> +};
> +
> +static inline struct adp5585_pwm_chip *
> +to_adp5585_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct adp5585_pwm_chip, chip);
> +}
> +
> +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	ret = regmap_read(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C, &val);
> +	if (ret)
> +		return ret;
> +
> +	adp5585_pwm->pin_config_val = val;
> +
> +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> +				 ADP5585_R3_EXTEND_CFG_MASK,
> +				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

The last four lines are equivalent to

	return ret;

What is the purpose of this function? Setup some kind of pinmuxing? The
answer to that question goes into a code comment. If it's pinmuxing, is
this a hint to use the pinctrl subsystem? (Maybe it's overkill, but if
it's considered a good idea later, it might be hard to extend the dt
bindings, so thinking about that now might be a good idea.)

> +}
> +
> +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> +			   ADP5585_R3_EXTEND_CFG_MASK,
> +			   adp5585_pwm->pin_config_val);

I wonder if writing a deterministic value instead of whatever was in
that register before .request() would be more robust and less
surprising.

> +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> +			   ADP5585_OSC_EN, 0);
> +}
> +
> +static int pwm_adp5585_apply(struct pwm_chip *chip,
> +			     struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u32 on, off;
> +	int ret;
> +
> +	if (!state->enabled) {
> +		guard(mutex)(&adp5585_pwm->lock);
> +
> +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> +					  ADP5585_PWM_EN, 0);
> +	}
> +
> +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> +		return -EINVAL;

Make this:

	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
		return -EINVAL;

	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
	duty_cycle = min(period, state->period);

> +
> +	/*
> +	 * Compute the on and off time. As the internal oscillator frequency is
> +	 * 1MHz, the calculation can be simplified without loss of precision.
> +	 */
> +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle,
> +				   NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> +	off = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> +				    NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);

round-closest is wrong. Testing with PWM_DEBUG should point that out.
The right algorithm is:

	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on


> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		swap(on, off);

Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
you can.

> [...]
> +static int adp5585_pwm_probe(struct platform_device *pdev)
> +{
> +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> +	struct adp5585_pwm_chip *adp5585_pwm;
> +	int ret;
> +
> +	adp5585_pwm = devm_kzalloc(&pdev->dev, sizeof(*adp5585_pwm), GFP_KERNEL);
> +	if (!adp5585_pwm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, adp5585_pwm);
> +
> +	adp5585_pwm->regmap = adp5585->regmap;
> +
> +	mutex_init(&adp5585_pwm->lock);
> +
> +	adp5585_pwm->chip.dev = &pdev->dev;
> +	adp5585_pwm->chip.ops = &adp5585_pwm_ops;
> +	adp5585_pwm->chip.npwm = ADP5585_PWM_CHAN_NUM;

That is wrong since commit
05947224ff46 ("pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()")

> +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> +	if (ret) {
> +		mutex_destroy(&adp5585_pwm->lock);
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static void adp5585_pwm_remove(struct platform_device *pdev)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&adp5585_pwm->lock);

Huh, this is a bad idea. The mutex is gone while the pwmchip is still
registered. AFAIK calling mutex_destroy() is optional, and
adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
.probe().

> +}
> +
> +static const struct of_device_id adp5585_pwm_of_match[] = {
> +	{ .compatible = "adi,adp5585-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);

Is it normal/usual for mfd drivers to use of stuff? I thought they use
plain platform style binding, not sure though.

> +static struct platform_driver adp5585_pwm_driver = {
> +	.driver	= {
> +		.name = "adp5585-pwm",
> +		.of_match_table = adp5585_pwm_of_match,
> +	},
> +	.probe = adp5585_pwm_probe,
> +	.remove_new = adp5585_pwm_remove,
> +};
> +module_platform_driver(adp5585_pwm_driver);
> +
> +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");
> +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

Hi Uwe,

Thank you for the quick review.

On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > new file mode 100644
> > index 000000000000..709713d8f47a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-adp5585.c
> > @@ -0,0 +1,230 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices ADP5585 PWM driver
> > + *
> > + * Copyright 2022 NXP
> > + * Copyright 2024 Ideas on Board Oy
> > + */
> 
> Please document some hardware properties here in the same format as many
> other PWM drivers. The things I'd like to read there are:
> 
>  - Only supports normal polarity
>  - How does the output pin behave when the hardware is disabled
>    (typically "low" or "high-Z" or "freeze")
>  - Does changing parameters or disabling complete the currently running
>    period?
>  - Are there glitches in .apply()? E.g. when the new duty_cycle is
>    already written but the new period is not.
> 
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/math.h>
> > +#include <linux/minmax.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time.h>
> 
> Do you need these all? I wounder about time.h.

Yes I've checked them all :-) time.h is for NSEC_PER_SEC (defined in
vdso/time64.h, which I thought would be better replaced by time.h).

> > +#define ADP5585_PWM_CHAN_NUM		1
> > +
> > +#define ADP5585_PWM_OSC_FREQ_HZ		1000000U
> > +#define ADP5585_PWM_MIN_PERIOD_NS	(2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > +#define ADP5585_PWM_MAX_PERIOD_NS	(2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > +
> > +struct adp5585_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct regmap *regmap;
> > +	struct mutex lock;
> 
> What does this mutex protect against? You can safely assume that there
> are no concurrent calls of the callbacks. (This isn't ensured yet, but I
> consider a consumer who does this buggy and it will soon be ensured.)

That's good to know. I couldn't find that information. I'll revisit the
locking in v2, and add a comment to document the mutex in case it's
still needed.

> > +	u8 pin_config_val;
> > +};
> > +
> > +static inline struct adp5585_pwm_chip *
> > +to_adp5585_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct adp5585_pwm_chip, chip);
> > +}
> > +
> > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	ret = regmap_read(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	adp5585_pwm->pin_config_val = val;
> > +
> > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> > +				 ADP5585_R3_EXTEND_CFG_MASK,
> > +				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> The last four lines are equivalent to
> 
> 	return ret;

I prefer the existing code but can also change it.

> What is the purpose of this function? Setup some kind of pinmuxing? The
> answer to that question goes into a code comment. If it's pinmuxing, is
> this a hint to use the pinctrl subsystem? (Maybe it's overkill, but if
> it's considered a good idea later, it might be hard to extend the dt
> bindings, so thinking about that now might be a good idea.)

The ADP5585_R3_EXTEND_CFG_PWM_OUT bit is about pinmuxing, yes. I'll add
a comment. I considered pinctrl too, but I think it's overkill.

> > +}
> > +
> > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> > +			   ADP5585_R3_EXTEND_CFG_MASK,
> > +			   adp5585_pwm->pin_config_val);
> 
> I wonder if writing a deterministic value instead of whatever was in
> that register before .request() would be more robust and less
> surprising.

I'll change that. It looks like the last remains of the original code
are going away :-)

> > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > +			   ADP5585_OSC_EN, 0);
> > +}
> > +
> > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > +			     struct pwm_device *pwm,
> > +			     const struct pwm_state *state)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	int ret;
> > +
> > +	if (!state->enabled) {
> > +		guard(mutex)(&adp5585_pwm->lock);
> > +
> > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > +					  ADP5585_PWM_EN, 0);
> > +	}
> > +
> > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > +		return -EINVAL;
> 
> Make this:
> 
> 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> 		return -EINVAL;
> 
> 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> 	duty_cycle = min(period, state->period);

I haven't been able to find documentation about the expected behaviour.
What's the rationale for returning an error if the period is too low,
but silently clamping it if it's too high ?

> > +
> > +	/*
> > +	 * Compute the on and off time. As the internal oscillator frequency is
> > +	 * 1MHz, the calculation can be simplified without loss of precision.
> > +	 */
> > +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle,
> > +				   NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> > +	off = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> > +				    NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> 
> round-closest is wrong. Testing with PWM_DEBUG should point that out.
> The right algorithm is:
> 
> 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> 
> 
> > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > +		swap(on, off);
> 
> Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> you can.

OK, but what's the rationale ? This is also an area where I couldn't
find documentation.

> > [...]
> > +static int adp5585_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > +	struct adp5585_pwm_chip *adp5585_pwm;
> > +	int ret;
> > +
> > +	adp5585_pwm = devm_kzalloc(&pdev->dev, sizeof(*adp5585_pwm), GFP_KERNEL);
> > +	if (!adp5585_pwm)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, adp5585_pwm);
> > +
> > +	adp5585_pwm->regmap = adp5585->regmap;
> > +
> > +	mutex_init(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm->chip.dev = &pdev->dev;
> > +	adp5585_pwm->chip.ops = &adp5585_pwm_ops;
> > +	adp5585_pwm->chip.npwm = ADP5585_PWM_CHAN_NUM;
> 
> That is wrong since commit
> 05947224ff46 ("pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()")

I'll update the code.

> > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > +	if (ret) {
> > +		mutex_destroy(&adp5585_pwm->lock);
> > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > +
> > +	mutex_destroy(&adp5585_pwm->lock);
> 
> Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> registered. AFAIK calling mutex_destroy() is optional, and
> adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> .probe().

mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
option is selected, it gets more useful. I would prefer moving away from
the devm_* registration, and unregister the pwm_chip in .remove()
manually, before destroying the mutex.

> > +}
> > +
> > +static const struct of_device_id adp5585_pwm_of_match[] = {
> > +	{ .compatible = "adi,adp5585-pwm" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> 
> Is it normal/usual for mfd drivers to use of stuff? I thought they use
> plain platform style binding, not sure though.

I'll test it.

> > +static struct platform_driver adp5585_pwm_driver = {
> > +	.driver	= {
> > +		.name = "adp5585-pwm",
> > +		.of_match_table = adp5585_pwm_of_match,
> > +	},
> > +	.probe = adp5585_pwm_probe,
> > +	.remove_new = adp5585_pwm_remove,
> > +};
> > +module_platform_driver(adp5585_pwm_driver);
> > +
> > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");
> > +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> > +MODULE_LICENSE("GPL");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Uwe Kleine-König]

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

Hello,

[dropping Alexandru Ardelean from Cc as their address bounces]

On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > 
> > The last four lines are equivalent to
> > 
> > 	return ret;
> 
> I prefer the existing code but can also change it.

Well, I see the upside of your approach. If this was my only concern I
wouldn't refuse to apply the patch.

> > > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > +			   ADP5585_OSC_EN, 0);
> > > +}
> > > +
> > > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > > +			     struct pwm_device *pwm,
> > > +			     const struct pwm_state *state)
> > > +{
> > > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > > +	u32 on, off;
> > > +	int ret;
> > > +
> > > +	if (!state->enabled) {
> > > +		guard(mutex)(&adp5585_pwm->lock);
> > > +
> > > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > > +					  ADP5585_PWM_EN, 0);
> > > +	}
> > > +
> > > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > > +		return -EINVAL;
> > 
> > Make this:
> > 
> > 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> > 		return -EINVAL;
> > 
> > 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> > 	duty_cycle = min(period, state->period);
> 
> I haven't been able to find documentation about the expected behaviour.
> What's the rationale for returning an error if the period is too low,
> but silently clamping it if it's too high ?

Well, it's only implicitly documented in the implementation of
PWM_DEBUG. The reasoning is a combination of the following thoughts:

 - Requiring exact matches is hard to work with, so some deviation
   between request and configured value should be allowed.
 - Rounding in both directions has strange and surprising effects. The
   corner cases (for all affected parties (=consumer, lowlevel driver
   and pwm core)) are easier if you only round in one direction.
   One ugly corner case in your suggested patch is:
   ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks.
   If the consumer requests period=64000.2 clock ticks, you configure
   for 64000. If the consumer requests period=65535.2 clock ticks you
   return -EINVAL.
   Another strange corner case is: Consider a hardware that can
   implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then
   only values >502 ns.
   If you configure for 501 ns, you'd get 500.3 ns. get_state() would
   tell you it's running at 500 ns. If you then configure 500 ns you
   won't get 500.3 ns any more.
 - If you want to allow 66535.2 clock ticks (and return 65535), what
   should be the maximal value that should yield 65535? Each cut-off
   value is arbitrary, so using \infty looks reasonable (to me at
   least).
 - Rounding down is easier than rounding up, because that's what C's /
   does. (Well, this is admittedly a bit arbitrary, because if you round
   down in .apply() you have to round up in .get_state().)

> > round-closest is wrong. Testing with PWM_DEBUG should point that out.
> > The right algorithm is:
> > 
> > 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> > 
> > 
> > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > +		swap(on, off);
> > 
> > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > you can.
> 
> OK, but what's the rationale ? This is also an area where I couldn't
> find documentation.

I don't have a good rationale here. IMHO this inverted polarity stuff is
only a convenience for consumers because the start of the period isn't
visible from the output wave form (apart from (maybe) the moment where
you change the configuration) and so

	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL

isn't distinguishable from

	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED

. But it's a historic assumption of the pwm core that there is a
relevant difference between the two polarities and I want at least a
consistent behaviour among the lowlevel drivers. BTW, this convenience
is the reason I'm not yet clear how I want to implemement a duty_offset.

> > > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > > +	if (ret) {
> > > +		mutex_destroy(&adp5585_pwm->lock);
> > > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > > +
> > > +	mutex_destroy(&adp5585_pwm->lock);
> > 
> > Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> > registered. AFAIK calling mutex_destroy() is optional, and
> > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> > .probe().
> 
> mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
> option is selected, it gets more useful. I would prefer moving away from
> the devm_* registration, and unregister the pwm_chip in .remove()
> manually, before destroying the mutex.

In that case I'd prefer a devm_mutex_init()?!
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart]

On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> [dropping Alexandru Ardelean from Cc as their address bounces]
> 
> On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > 
> > > The last four lines are equivalent to
> > > 
> > > 	return ret;
> > 
> > I prefer the existing code but can also change it.
> 
> Well, I see the upside of your approach. If this was my only concern I
> wouldn't refuse to apply the patch.

While I have my preferences, I also favour consistency, so I'm happy to
comply with the preferred coding style for the subsystem :-) I'll
update this in the next version.

> > > > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > +			   ADP5585_OSC_EN, 0);
> > > > +}
> > > > +
> > > > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > > > +			     struct pwm_device *pwm,
> > > > +			     const struct pwm_state *state)
> > > > +{
> > > > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > > > +	u32 on, off;
> > > > +	int ret;
> > > > +
> > > > +	if (!state->enabled) {
> > > > +		guard(mutex)(&adp5585_pwm->lock);
> > > > +
> > > > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > > > +					  ADP5585_PWM_EN, 0);
> > > > +	}
> > > > +
> > > > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > > > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > > > +		return -EINVAL;
> > > 
> > > Make this:
> > > 
> > > 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> > > 		return -EINVAL;
> > > 
> > > 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> > > 	duty_cycle = min(period, state->period);
> > 
> > I haven't been able to find documentation about the expected behaviour.
> > What's the rationale for returning an error if the period is too low,
> > but silently clamping it if it's too high ?
> 
> Well, it's only implicitly documented in the implementation of
> PWM_DEBUG. The reasoning is a combination of the following thoughts:
> 
>  - Requiring exact matches is hard to work with, so some deviation
>    between request and configured value should be allowed.
>  - Rounding in both directions has strange and surprising effects. The
>    corner cases (for all affected parties (=consumer, lowlevel driver
>    and pwm core)) are easier if you only round in one direction.
>    One ugly corner case in your suggested patch is:
>    ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks.
>    If the consumer requests period=64000.2 clock ticks, you configure
>    for 64000. If the consumer requests period=65535.2 clock ticks you
>    return -EINVAL.
>    Another strange corner case is: Consider a hardware that can
>    implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then
>    only values >502 ns.
>    If you configure for 501 ns, you'd get 500.3 ns. get_state() would
>    tell you it's running at 500 ns. If you then configure 500 ns you
>    won't get 500.3 ns any more.
>  - If you want to allow 66535.2 clock ticks (and return 65535), what
>    should be the maximal value that should yield 65535? Each cut-off
>    value is arbitrary, so using \infty looks reasonable (to me at
>    least).
>  - Rounding down is easier than rounding up, because that's what C's /
>    does. (Well, this is admittedly a bit arbitrary, because if you round
>    down in .apply() you have to round up in .get_state().)

Thank you for the detailed explanation.

> > > round-closest is wrong. Testing with PWM_DEBUG should point that out.
> > > The right algorithm is:
> > > 
> > > 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > > 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> > > 
> > > 
> > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > +		swap(on, off);
> > > 
> > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > you can.
> > 
> > OK, but what's the rationale ? This is also an area where I couldn't
> > find documentation.
> 
> I don't have a good rationale here. IMHO this inverted polarity stuff is
> only a convenience for consumers because the start of the period isn't
> visible from the output wave form (apart from (maybe) the moment where
> you change the configuration) and so
> 
> 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> 
> isn't distinguishable from
> 
> 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> 
> . But it's a historic assumption of the pwm core that there is a
> relevant difference between the two polarities and I want at least a
> consistent behaviour among the lowlevel drivers. BTW, this convenience
> is the reason I'm not yet clear how I want to implemement a duty_offset.

Consistency is certainly good. Inverting the duty cycle to implement
inverted polarity would belong in the PWM core if we wanted to implement
it in software I suppose. I'll drop it from the driver.

> > > > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > > > +	if (ret) {
> > > > +		mutex_destroy(&adp5585_pwm->lock);
> > > > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > > > +
> > > > +	mutex_destroy(&adp5585_pwm->lock);
> > > 
> > > Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> > > registered. AFAIK calling mutex_destroy() is optional, and
> > > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> > > .probe().
> > 
> > mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
> > option is selected, it gets more useful. I would prefer moving away from
> > the devm_* registration, and unregister the pwm_chip in .remove()
> > manually, before destroying the mutex.
> 
> In that case I'd prefer a devm_mutex_init()?!

Maybe that would be useful :-) Let's see if I can drop the mutex though.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Uwe Kleine-König]

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

Hello Laurent,

On Wed, May 22, 2024 at 01:13:35PM +0300, Laurent Pinchart wrote:
> On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > > +		swap(on, off);
> > > > 
> > > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > > you can.
> > > 
> > > OK, but what's the rationale ? This is also an area where I couldn't
> > > find documentation.
> > 
> > I don't have a good rationale here. IMHO this inverted polarity stuff is
> > only a convenience for consumers because the start of the period isn't
> > visible from the output wave form (apart from (maybe) the moment where
> > you change the configuration) and so
> > 
> > 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> > 
> > isn't distinguishable from
> > 
> > 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> > 
> > . But it's a historic assumption of the pwm core that there is a
> > relevant difference between the two polarities and I want at least a
> > consistent behaviour among the lowlevel drivers. BTW, this convenience
> > is the reason I'm not yet clear how I want to implemement a duty_offset.
> 
> Consistency is certainly good. Inverting the duty cycle to implement
> inverted polarity would belong in the PWM core if we wanted to implement
> it in software I suppose. I'll drop it from the driver.

This isn't as easy as it sounds however. From the POV of the PWM core
the capabilities of the currently used hardware are unclear. So if a
request with (say) normal polarity and a certain duty_cycle + period
fails, it's unknown if it would be beneficial to try with inverted
polarity and if that is OK for the requesting consumer. So there is
information missing in both directions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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