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

[PATCH v2 0/4] 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 (1/4), an MFD driver (2/4) and drivers
for the GPIO (3/4) and PWM (4/4) 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.

All review comments from v1 have been taken into account as far as I can
tell. The most notable changes are in the DT binding that now merge all
functions in a single node, and the corresponding changes to the
drivers.

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 (1):
  dt-bindings: Add bindings for the Analog Devices ADP5585

 .../devicetree/bindings/mfd/adi,adp5585.yaml  | 107 ++++++++
 .../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                         | 200 +++++++++++++++
 drivers/pwm/Kconfig                           |   7 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-adp5585.c                     | 187 ++++++++++++++
 include/linux/mfd/adp5585.h                   | 127 ++++++++++
 13 files changed, 892 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.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: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/4] 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.

Drop the existing adi,adp5585 and adi,adp5585-02 compatible strings from
trivial-devices.yaml. They have been added there by mistake as the
driver that was submitted at the same time used different compatible
strings. We can take them over safely.

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.

Changes since v1:

- Squash "dt-bindings: trivial-devices: Drop adi,adp5585 and
  adi,adp5585-02" into this patch
- Merge child nodes into parent node
---
 .../devicetree/bindings/mfd/adi,adp5585.yaml  | 107 ++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |   4 -
 MAINTAINERS                                   |   7 ++
 3 files changed, 114 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml

diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
new file mode 100644
index 000000000000..45bbfadbb9d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
@@ -0,0 +1,107 @@
+# 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-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-reserved-ranges: true
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - gpio
+  - gpio-controller
+  - "#gpio-cells"
+  - "#pwm-cells"
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,adp5585-01
+    then:
+      properties:
+        gpio-reserved-ranges: false
+    else:
+      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-controller;
+            #gpio-cells = <2>;
+            gpio-reserved-ranges = <5 1>;
+
+            #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-controller;
+            #gpio-cells = <2>;
+
+            #pwm-cells = <3>;
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 0a419453d183..91e62df4b296 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
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..3016b003ead3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -526,6 +526,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 v2 1/4] dt-bindings: Add bindings for the Analog Devices ADP5585

[Rob Herring (Arm)]

On Tue, 28 May 2024 22:03:11 +0300, 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.
> 
> Drop the existing adi,adp5585 and adi,adp5585-02 compatible strings from
> trivial-devices.yaml. They have been added there by mistake as the
> driver that was submitted at the same time used different compatible
> strings. We can take them over safely.
> 
> 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.
> 
> Changes since v1:
> 
> - Squash "dt-bindings: trivial-devices: Drop adi,adp5585 and
>   adi,adp5585-02" into this patch
> - Merge child nodes into parent node
> ---
>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 107 ++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |   4 -
>  MAINTAINERS                                   |   7 ++
>  3 files changed, 114 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240528190315.3865-2-laurent.pinchart@ideasonboard.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/4] dt-bindings: Add bindings for the Analog Devices ADP5585

[Laurent Pinchart]

On Tue, May 28, 2024 at 03:41:48PM -0500, Rob Herring (Arm) wrote:
> 
> On Tue, 28 May 2024 22:03:11 +0300, 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.
> > 
> > Drop the existing adi,adp5585 and adi,adp5585-02 compatible strings from
> > trivial-devices.yaml. They have been added there by mistake as the
> > driver that was submitted at the same time used different compatible
> > strings. We can take them over safely.
> > 
> > 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.
> > 
> > Changes since v1:
> > 
> > - Squash "dt-bindings: trivial-devices: Drop adi,adp5585 and
> >   adi,adp5585-02" into this patch
> > - Merge child nodes into parent node
> > ---
> >  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 107 ++++++++++++++++++
> >  .../devicetree/bindings/trivial-devices.yaml  |   4 -
> >  MAINTAINERS                                   |   7 ++
> >  3 files changed, 114 insertions(+), 4 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:

My bad, I messed up. Will be fixed in v3.

> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property
> 	from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property
> 	from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240528190315.3865-2-laurent.pinchart@ideasonboard.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/4] 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 since v1:

- Add comment to explain BANK and BIT macros
- Drop compatible strings from cells
- White space fixes
- Fix comparison to NULL

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       | 200 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/adp5585.h | 127 +++++++++++++++++++++++
 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 3016b003ead3..77b93fbdf5cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -532,6 +532,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 266b4f54af60..8a7ab8af217f 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..4f7e065e5314
--- /dev/null
+++ b/drivers/mfd/adp5585.c
@@ -0,0 +1,200 @@
+// 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", },
+	{ .name = "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)
+		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..7682182b10d1
--- /dev/null
+++ b/include/linux/mfd/adp5585.h
@@ -0,0 +1,127 @@
+/* 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
+
+/*
+ * Bank 0 covers pins "GPIO 1/R0" to "GPIO 6/R5", numbered 0 to 5 by the
+ * driver, and bank 1 covers pins "GPIO 7/C0" to "GPIO 11/C4", numbered 6 to
+ * 10. Some variants of the ADP5585 don't support "GPIO 6/R5". As the driver
+ * uses identical GPIO numbering for all variants to avoid confusion, GPIO 5 is
+ * marked as reserved in the device tree for variants that don't support it.
+ */
+#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 v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Andy Shevchenko]

Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti:
> 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.

...

> +	tristate "Analog Devices ADP5585 MFD driver"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C && OF

Why OF?
No COMPILE_TEST?

...

+ array_size.h
+ device.h // e.g., devm_kzalloc()

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

You don't need them, instead of proxying...

> +#include <linux/mfd/core.h>
> +#include <linux/mfd/adp5585.h>

m is earlier than 'o', but with above drop no more issue :-)

...just include mod_devicetable.h.

> +#include <linux/regmap.h>

+ types.h // e.g., u8

...

> +	regmap_config = of_device_get_match_data(&i2c->dev);

We have i2c_get_match_data().

...

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

...

> +#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)

GENMASK()

...

> +#define		ADP5585_Rx_PULL_CFG_MASK	(3)

GENMASK()

Why parentheses in all of them, btw?

...

> +#define		ADP5585_C4_EXTEND_CFG_MASK	(1U << 6)

> +#define		ADP5585_R4_EXTEND_CFG_MASK	(1U << 5)

> +#define		ADP5585_R3_EXTEND_CFG_MASK	(3U << 2)

> +#define		ADP5585_R0_EXTEND_CFG_MASK	(1U << 0)

> +#define		ADP5585_OSC_FREQ_MASK		(3U << 5)

BIT() / GENMASK()

> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Laurent Pinchart]

On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote:
> Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti:
> > 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.
> 
> ...
> 
> > +	tristate "Analog Devices ADP5585 MFD driver"
> > +	select MFD_CORE
> > +	select REGMAP_I2C
> > +	depends on I2C && OF
> 
> Why OF?

Because the driver works on OF systems only.

> No COMPILE_TEST?

The driver won't compile without CONFIG_I2C, so I can use

	depends on I2C
	depends on OF || COMPILE_TEST

Do you think that's better ?

> 
> ...
> 
> + array_size.h
> + device.h // e.g., devm_kzalloc()
> 
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>

I'll drop those 3 headers, there's not needed anymore.

> > +#include <linux/i2c.h>
> 
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> You don't need them, instead of proxying...

of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the
former, but I need the latter, so I'll keep of.h

of_device.h for historical reasons probably, I'll drop it.

> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/adp5585.h>
> 
> m is earlier than 'o', but with above drop no more issue :-)
> 
> ...just include mod_devicetable.h.
> 
> > +#include <linux/regmap.h>
> 
> + types.h // e.g., u8
> 
> ...
> 
> > +	regmap_config = of_device_get_match_data(&i2c->dev);
> 
> We have i2c_get_match_data().

Sounds good.

> ...
> 
> > +#ifndef __LINUX_MFD_ADP5585_H_
> > +#define __LINUX_MFD_ADP5585_H_
> > +
> > +#include <linux/bits.h>
> 
> ...
> 
> > +#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
> 
> GENMASK()

This is not a mask. Or do you mean

	(((v) & GENMASK(7, 4)) >> 4)

? I think that's overkill.

> ...
> 
> > +#define		ADP5585_Rx_PULL_CFG_MASK	(3)
> 
> GENMASK()

Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG().

> Why parentheses in all of them, btw?

Probably for consistency, but I don't mind dropping them.

> ...
> 
> > +#define		ADP5585_C4_EXTEND_CFG_MASK	(1U << 6)
> 
> > +#define		ADP5585_R4_EXTEND_CFG_MASK	(1U << 5)
> 
> > +#define		ADP5585_R3_EXTEND_CFG_MASK	(3U << 2)
> 
> > +#define		ADP5585_R0_EXTEND_CFG_MASK	(1U << 0)
> 
> > +#define		ADP5585_OSC_FREQ_MASK		(3U << 5)
> 
> BIT() / GENMASK()

I'll use GENMASK for the masks.

> > +#endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Andy Shevchenko]

On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote:
> > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti:

...

> > > +   depends on I2C && OF
> >
> > Why OF?
>
> Because the driver works on OF systems only.
>
> > No COMPILE_TEST?
>
> The driver won't compile without CONFIG_I2C, so I can use
>
>         depends on I2C
>         depends on OF || COMPILE_TEST
>
> Do you think that's better ?

I think that dropping OF completely is the best.
OF || COMPILE_TEST would work as well, but I still don't know why we need this.

...

> > + array_size.h
> > + device.h // e.g., devm_kzalloc()
> >
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
>
> I'll drop those 3 headers, there's not needed anymore.
>
> > > +#include <linux/i2c.h>
> >
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> >
> > You don't need them, instead of proxying...
>
> of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the
> former, but I need the latter, so I'll keep of.h

Why do you need of_match_ptr()? What for?

> of_device.h for historical reasons probably, I'll drop it.
>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/adp5585.h>
> >
> > m is earlier than 'o', but with above drop no more issue :-)
> >
> > ...just include mod_devicetable.h.
> >
> > > +#include <linux/regmap.h>
> >
> > + types.h // e.g., u8

I assume that all marked with + in my previous reply you agree on?

...

> > > +#define            ADP5585_MAN_ID(v)               (((v) & 0xf0) >> 4)
> >
> > GENMASK()
>
> This is not a mask. Or do you mean
>
>         (((v) & GENMASK(7, 4)) >> 4)
>
> ?

Yes.

> I think that's overkill.

Why? You have a mask, use it for less error prone code.

...

> > > +#define            ADP5585_Rx_PULL_CFG_MASK        (3)
> >
> > GENMASK()
>
> Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG().

Why is it marked as a mask? Rename it to _ALL or alike.

...

> > > +#define            ADP5585_C4_EXTEND_CFG_MASK      (1U << 6)
> >
> > > +#define            ADP5585_R4_EXTEND_CFG_MASK      (1U << 5)
> >
> > > +#define            ADP5585_R3_EXTEND_CFG_MASK      (3U << 2)
> >
> > > +#define            ADP5585_R0_EXTEND_CFG_MASK      (1U << 0)
> >
> > > +#define            ADP5585_OSC_FREQ_MASK           (3U << 5)
> >
> > BIT() / GENMASK()
>
> I'll use GENMASK for the masks.

For a single bit the BIT() is okay, and TBH I don't remember if
GENMASK() supports h == l cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Laurent Pinchart]

Hi Andy,

On Wed, May 29, 2024 at 08:44:26AM +0300, Andy Shevchenko wrote:
> On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart wrote:
> > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote:
> > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti:
> 
> ...
> 
> > > > +   depends on I2C && OF
> > >
> > > Why OF?
> >
> > Because the driver works on OF systems only.
> >
> > > No COMPILE_TEST?
> >
> > The driver won't compile without CONFIG_I2C, so I can use
> >
> >         depends on I2C
> >         depends on OF || COMPILE_TEST
> >
> > Do you think that's better ?
> 
> I think that dropping OF completely is the best.
> OF || COMPILE_TEST would work as well, but I still don't know why we need this.

For the same reason that many drivers depend on specific CONFIG_$ARCH.
They can't run on other platforms, the dependency hides the symbol for
users who can't use the driver. This driver works on OF platforms only.

> ...
> 
> > > + array_size.h
> > > + device.h // e.g., devm_kzalloc()
> > >
> > > > +#include <linux/module.h>
> > > > +#include <linux/moduleparam.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/slab.h>
> >
> > I'll drop those 3 headers, there's not needed anymore.
> >
> > > > +#include <linux/i2c.h>
> > >
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > >
> > > You don't need them, instead of proxying...
> >
> > of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the
> > former, but I need the latter, so I'll keep of.h
> 
> Why do you need of_match_ptr()? What for?

That's actually not needed, I'll drop it.

> > of_device.h for historical reasons probably, I'll drop it.
> >
> > > > +#include <linux/mfd/core.h>
> > > > +#include <linux/mfd/adp5585.h>
> > >
> > > m is earlier than 'o', but with above drop no more issue :-)
> > >
> > > ...just include mod_devicetable.h.
> > >
> > > > +#include <linux/regmap.h>
> > >
> > > + types.h // e.g., u8
> 
> I assume that all marked with + in my previous reply you agree on?

If I don't reply to a particular comment it means I agree with it and
will address it, yes.

> ...
> 
> > > > +#define            ADP5585_MAN_ID(v)               (((v) & 0xf0) >> 4)
> > >
> > > GENMASK()
> >
> > This is not a mask. Or do you mean
> >
> >         (((v) & GENMASK(7, 4)) >> 4)
> >
> > ?
> 
> Yes.
> 
> > I think that's overkill.
> 
> Why? You have a mask, use it for less error prone code.

I'll change this to

diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
index fa4092a5c97f..924758b8a3cd 100644
--- a/drivers/mfd/adp5585.c
+++ b/drivers/mfd/adp5585.c
@@ -125,7 +125,7 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
 		return dev_err_probe(&i2c->dev, ret,
 				     "Failed to read device ID\n");
 
-	if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
+	if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE)
 		return dev_err_probe(&i2c->dev, -ENODEV,
 				     "Invalid device ID 0x%02x\n", id);
 
diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h
index f06a574afedf..f5776ee844dc 100644
--- a/include/linux/mfd/adp5585.h
+++ b/include/linux/mfd/adp5585.h
@@ -12,8 +12,8 @@
 #include <linux/bits.h>
 
 #define ADP5585_ID			0x00
-#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
-#define		ADP5585_MAN_ID_VALUE		0x02
+#define		ADP5585_MAN_ID_VALUE		0x20
+#define		ADP5585_MAN_ID_MASK		GENMASK(7, 4)
 #define ADP5585_INT_STATUS		0x01
 #define ADP5585_STATUS			0x02
 #define ADP5585_FIFO_1			0x03

> ...
> 
> > > > +#define            ADP5585_Rx_PULL_CFG_MASK        (3)
> > >
> > > GENMASK()
> >
> > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG().
> 
> Why is it marked as a mask? Rename it to _ALL or alike.

It's a mask, but used as

	ADP5585_Rx_PULL_CFG(ADP5585_Rx_PULL_CFG_MASK)

We're reaching a level of bike-shedding that even I find impressive :-)
As with a few other of your review comments that I think are related to
personal taste more than anything else, I'll defer to the subsystem
maintainer and follow their preference on this one.

> ...
> 
> > > > +#define            ADP5585_C4_EXTEND_CFG_MASK      (1U << 6)
> > >
> > > > +#define            ADP5585_R4_EXTEND_CFG_MASK      (1U << 5)
> > >
> > > > +#define            ADP5585_R3_EXTEND_CFG_MASK      (3U << 2)
> > >
> > > > +#define            ADP5585_R0_EXTEND_CFG_MASK      (1U << 0)
> > >
> > > > +#define            ADP5585_OSC_FREQ_MASK           (3U << 5)
> > >
> > > BIT() / GENMASK()
> >
> > I'll use GENMASK for the masks.
> 
> For a single bit the BIT() is okay, and TBH I don't remember if
> GENMASK() supports h == l cases.

I've tested it and it works.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Andy Shevchenko]

On Wed, May 29, 2024 at 12:35 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, May 29, 2024 at 08:44:26AM +0300, Andy Shevchenko wrote:
> > On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart wrote:
> > > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote:
> > > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti:

...

> > > > > +   depends on I2C && OF
> > > >
> > > > Why OF?
> > >
> > > Because the driver works on OF systems only.
> > >
> > > > No COMPILE_TEST?
> > >
> > > The driver won't compile without CONFIG_I2C, so I can use
> > >
> > >         depends on I2C
> > >         depends on OF || COMPILE_TEST
> > >
> > > Do you think that's better ?
> >
> > I think that dropping OF completely is the best.
> > OF || COMPILE_TEST would work as well, but I still don't know why we need this.
>
> For the same reason that many drivers depend on specific CONFIG_$ARCH.

It's different. You may not do in many cases the $ARCH ||
COMPILE_TEST, while OF || COMPILE_TEST should just work in 100% cases.

> They can't run on other platforms, the dependency hides the symbol for
> users who can't use the driver. This driver works on OF platforms only.

What you are talking about is functional dependency, what I'm talking
about is the compilation one.
So, it's a pity that Kbuild doesn't distinguish these two basic
concepts in dependencies and
FOO || COMPILE_TEST is basically an artificial hack to tell "hey, FOO
is _functional_ dependency, I do not care if I can compile without
it".

...

> > > > > +#define            ADP5585_MAN_ID(v)               (((v) & 0xf0) >> 4)
> > > >
> > > > GENMASK()
> > >
> > > This is not a mask. Or do you mean
> > >
> > >         (((v) & GENMASK(7, 4)) >> 4)
> > >
> > > ?
> >
> > Yes.
> >
> > > I think that's overkill.
> >
> > Why? You have a mask, use it for less error prone code.
>
> I'll change this to

...

> -       if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
> +       if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE)

(Don't forget inner parentheses)

...

> > > > > +#define            ADP5585_Rx_PULL_CFG_MASK        (3)
> > > >
> > > > GENMASK()
> > >
> > > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG().
> >
> > Why is it marked as a mask? Rename it to _ALL or alike.
>
> It's a mask, but used as
>
>         ADP5585_Rx_PULL_CFG(ADP5585_Rx_PULL_CFG_MASK)
>
> We're reaching a level of bike-shedding that even I find impressive :-)
> As with a few other of your review comments that I think are related to
> personal taste more than anything else, I'll defer to the subsystem
> maintainer and follow their preference on this one.

I would name it _ALL and use (BIT(2) - 1) notation to show that you
want all of them to be set. But okay, let's leave this to the
maintainer.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes compared to v1:

- Drop OF match table
- Fix .get() for GPOs
- Add platform ID table
- Set struct device of_node manually
- Merge child DT node into parent node
- Implement .get_direction()
- Drop mutex

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 77b93fbdf5cc..9c560d9a590a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -532,6 +532,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 3dbddec07028..67286886db5e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1233,6 +1233,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 e2a53013780e..04bfa2bc7e11 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..bd6b87ec5dac
--- /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/platform_device.h>
+#include <linux/regmap.h>
+
+#define ADP5585_GPIO_MAX	11
+
+struct adp5585_gpio_dev {
+	struct gpio_chip gpio_chip;
+	struct regmap *regmap;
+};
+
+static int adp5585_gpio_get_direction(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;
+
+	regmap_read(adp5585_gpio->regmap, ADP5585_GPIO_DIRECTION_A + bank, &val);
+
+	return val & bit ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+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);
+
+	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;
+
+	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 reg;
+	unsigned int val;
+
+	/*
+	 * The input status register doesn't reflect the pin state when the
+	 * GPIO is configured as an output. Check the direction, and read the
+	 * input status from GPI_STATUS or output value from GPO_DATA_OUT
+	 * accordingly.
+	 *
+	 * We don't need any locking, as concurrent access to the same GPIO
+	 * isn't allowed by the GPIO API, so there's no risk of the
+	 * .direction_input(), .direction_output() or .set() operations racing
+	 * with this.
+	 */
+	regmap_read(adp5585_gpio->regmap, ADP5585_GPIO_DIRECTION_A + bank, &val);
+	reg = val & bit ? ADP5585_GPO_DATA_OUT_A : ADP5585_GPI_STATUS_A;
+	regmap_read(adp5585_gpio->regmap, reg + 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);
+
+	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);
+
+	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);
+
+	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);
+
+	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(dev, sizeof(*adp5585_gpio), GFP_KERNEL);
+	if (!adp5585_gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, adp5585_gpio);
+
+	adp5585_gpio->regmap = adp5585->regmap;
+
+	device_set_of_node_from_dev(dev, dev->parent);
+
+	gc = &adp5585_gpio->gpio_chip;
+	gc->parent = dev;
+	gc->get_direction = adp5585_gpio_get_direction;
+	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(dev, &adp5585_gpio->gpio_chip,
+				     adp5585_gpio);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add GPIO chip\n");
+
+	return 0;
+}
+
+static const struct platform_device_id adp5585_gpio_id_table[] = {
+	{ "adp5585-gpio" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(platform, adp5585_gpio_id_table);
+
+static struct platform_driver adp5585_gpio_driver = {
+	.driver	= {
+		.name = "adp5585-gpio",
+	},
+	.probe = adp5585_gpio_probe,
+	.id_table = adp5585_gpio_id_table,
+};
+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 v2 3/4] gpio: adp5585: Add Analog Devices ADP5585 support

[Andy Shevchenko]

Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti:
> 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.

Why is this not using gpio-regmap?

...

> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

+ types.h

...

> +	bit = off * 2 + (off > 5 ? 4 : 0);

Right, but can you use >= 6 here which immediately follows to the next
question, i.e. why not use bank in this conditional?

...

> +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);

(see below)

> +	struct adp5585_gpio_dev *adp5585_gpio;
> +	struct device *dev = &pdev->dev;

	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);

> +	struct gpio_chip *gc;
> +	int ret;

...

> +	platform_set_drvdata(pdev, adp5585_gpio);

Any use of driver data?

...

> +	device_set_of_node_from_dev(dev, dev->parent);

Why not device_set_node()?

-- 
With Best Regards,
Andy Shevchenko

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

[Laurent Pinchart]

Hi Andy,

Thank you for the patch.

On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti:
> > 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.
> 
> Why is this not using gpio-regmap?
> 
> ...
> 
> > +#include <linux/device.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> + types.h
> 
> ...
> 
> > +	bit = off * 2 + (off > 5 ? 4 : 0);
> 
> Right, but can you use >= 6 here which immediately follows to the next
> question, i.e. why not use bank in this conditional?

The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
set of registers with the same layout. Here the layout is different, the
registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
rather not use ADP5585_BANK() either. I have decided to use > 5 instead
of >= 6 to match the R5 field name in the comment above:

        /*
         * 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.
         */

> ...
> 
> > +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> 
> (see below)
> 
> > +	struct adp5585_gpio_dev *adp5585_gpio;
> > +	struct device *dev = &pdev->dev;
> 
> 	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);

I prefer keeping the current ordering, with long lines first, I think
that's more readable.

> > +	struct gpio_chip *gc;
> > +	int ret;
> 
> ...
> 
> > +	platform_set_drvdata(pdev, adp5585_gpio);
> 
> Any use of driver data?

In v1, not v2. I'll drop it.

> ...
> 
> > +	device_set_of_node_from_dev(dev, dev->parent);
> 
> Why not device_set_node()?

Because device_set_of_node_from_dev() is meant for this exact use case,
where the same node is used for multiple devices. It also puts any
previous dev->of_node, ensuring proper refcounting when devices are
unbound and rebound, without being deleted.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Tue, May 28, 2024 at 11:20:45PM +0300, Laurent Pinchart wrote:
> Hi Andy,
> 
> Thank you for the patch.
> 
> On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> > Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti:
> > > 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.
> > 
> > Why is this not using gpio-regmap?

I forgot to answer this:

I don't think it's a good match for the hardware.

> > ...
> > 
> > > +#include <linux/device.h>
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/mfd/adp5585.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > 
> > + types.h
> > 
> > ...
> > 
> > > +	bit = off * 2 + (off > 5 ? 4 : 0);
> > 
> > Right, but can you use >= 6 here which immediately follows to the next
> > question, i.e. why not use bank in this conditional?
> 
> The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
> set of registers with the same layout. Here the layout is different, the
> registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
> rather not use ADP5585_BANK() either. I have decided to use > 5 instead
> of >= 6 to match the R5 field name in the comment above:
> 
>         /*
>          * 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.
>          */
> 
> > ...
> > 
> > > +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > 
> > (see below)
> > 
> > > +	struct adp5585_gpio_dev *adp5585_gpio;
> > > +	struct device *dev = &pdev->dev;
> > 
> > 	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
> 
> I prefer keeping the current ordering, with long lines first, I think
> that's more readable.
> 
> > > +	struct gpio_chip *gc;
> > > +	int ret;
> > 
> > ...
> > 
> > > +	platform_set_drvdata(pdev, adp5585_gpio);
> > 
> > Any use of driver data?
> 
> In v1, not v2. I'll drop it.
> 
> > ...
> > 
> > > +	device_set_of_node_from_dev(dev, dev->parent);
> > 
> > Why not device_set_node()?
> 
> Because device_set_of_node_from_dev() is meant for this exact use case,
> where the same node is used for multiple devices. It also puts any
> previous dev->of_node, ensuring proper refcounting when devices are
> unbound and rebound, without being deleted.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Tue, May 28, 2024 at 11:22:18PM +0300, Laurent Pinchart wrote:
> On Tue, May 28, 2024 at 11:20:45PM +0300, Laurent Pinchart wrote:
> > Hi Andy,
> > 
> > Thank you for the patch.

I meant for the review. It's getting late.

> > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> > > Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti:
> > > > 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.
> > > 
> > > Why is this not using gpio-regmap?
> 
> I forgot to answer this:
> 
> I don't think it's a good match for the hardware.
> 
> > > ...
> > > 
> > > > +#include <linux/device.h>
> > > > +#include <linux/gpio/driver.h>
> > > > +#include <linux/mfd/adp5585.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > 
> > > + types.h
> > > 
> > > ...
> > > 
> > > > +	bit = off * 2 + (off > 5 ? 4 : 0);
> > > 
> > > Right, but can you use >= 6 here which immediately follows to the next
> > > question, i.e. why not use bank in this conditional?
> > 
> > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
> > set of registers with the same layout. Here the layout is different, the
> > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
> > rather not use ADP5585_BANK() either. I have decided to use > 5 instead
> > of >= 6 to match the R5 field name in the comment above:
> > 
> >         /*
> >          * 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.
> >          */
> > 
> > > ...
> > > 
> > > > +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > > 
> > > (see below)
> > > 
> > > > +	struct adp5585_gpio_dev *adp5585_gpio;
> > > > +	struct device *dev = &pdev->dev;
> > > 
> > > 	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
> > 
> > I prefer keeping the current ordering, with long lines first, I think
> > that's more readable.
> > 
> > > > +	struct gpio_chip *gc;
> > > > +	int ret;
> > > 
> > > ...
> > > 
> > > > +	platform_set_drvdata(pdev, adp5585_gpio);
> > > 
> > > Any use of driver data?
> > 
> > In v1, not v2. I'll drop it.
> > 
> > > ...
> > > 
> > > > +	device_set_of_node_from_dev(dev, dev->parent);
> > > 
> > > Why not device_set_node()?
> > 
> > Because device_set_of_node_from_dev() is meant for this exact use case,
> > where the same node is used for multiple devices. It also puts any
> > previous dev->of_node, ensuring proper refcounting when devices are
> > unbound and rebound, without being deleted.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,

Laurent Pinchart

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

[Andy Shevchenko]

On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:

...

> > > +   bit = off * 2 + (off > 5 ? 4 : 0);
> >
> > Right, but can you use >= 6 here which immediately follows to the next
> > question, i.e. why not use bank in this conditional?
>
> The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
> set of registers with the same layout. Here the layout is different, the
> registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
> rather not use ADP5585_BANK() either. I have decided to use > 5 instead
> of >= 6 to match the R5 field name in the comment above:
>
>         /*
>          * 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.
>          */

First of all, the 5 sounds misleading as one needs to think about "how
many are exactly per the register" and the answer AFAIU is 6. >= 6
shows this. Second, I haven't mentioned _BANK(), what I meant is
something to

  unsigned int bank = ... >= 6 ? : ;

...

> > > +   struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> >
> > (see below)
> >
> > > +   struct adp5585_gpio_dev *adp5585_gpio;
> > > +   struct device *dev = &pdev->dev;
> >
> >       struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
>
> I prefer keeping the current ordering, with long lines first, I think
> that's more readable.

Does the compiler optimise these two?

> > > +   struct gpio_chip *gc;
> > > +   int ret;

...

> > > +   device_set_of_node_from_dev(dev, dev->parent);
> >
> > Why not device_set_node()?
>
> Because device_set_of_node_from_dev() is meant for this exact use case,
> where the same node is used for multiple devices. It also puts any
> previous dev->of_node, ensuring proper refcounting when devices are
> unbound and rebound, without being deleted.

When will the refcount be dropped (in case of removal of this device)?
Or you mean it shouldn't?

-- 
With Best Regards,
Andy Shevchenko

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

[Laurent Pinchart]

On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > +   bit = off * 2 + (off > 5 ? 4 : 0);
> > >
> > > Right, but can you use >= 6 here which immediately follows to the next
> > > question, i.e. why not use bank in this conditional?
> >
> > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
> > set of registers with the same layout. Here the layout is different, the
> > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
> > rather not use ADP5585_BANK() either. I have decided to use > 5 instead
> > of >= 6 to match the R5 field name in the comment above:
> >
> >         /*
> >          * 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.
> >          */
> 
> First of all, the 5 sounds misleading as one needs to think about "how
> many are exactly per the register" and the answer AFAIU is 6. >= 6
> shows this. Second, I haven't mentioned _BANK(), what I meant is
> something to
> 
>   unsigned int bank = ... >= 6 ? : ;

That doesn't reflect the organisation of the bits in the registers. If
you're interested, please check the datasheet.

> ...
> 
> > > > +   struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > >
> > > (see below)
> > >
> > > > +   struct adp5585_gpio_dev *adp5585_gpio;
> > > > +   struct device *dev = &pdev->dev;
> > >
> > >       struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
> >
> > I prefer keeping the current ordering, with long lines first, I think
> > that's more readable.
> 
> Does the compiler optimise these two?

If anyone is interested in figuring out, I'll let them test :-)

> > > > +   struct gpio_chip *gc;
> > > > +   int ret;
> 
> ...
> 
> > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > >
> > > Why not device_set_node()?
> >
> > Because device_set_of_node_from_dev() is meant for this exact use case,
> > where the same node is used for multiple devices. It also puts any
> > previous dev->of_node, ensuring proper refcounting when devices are
> > unbound and rebound, without being deleted.
> 
> When will the refcount be dropped (in case of removal of this device)?
> Or you mean it shouldn't?

Any refcount taken on the OF node needs to be dropped. The device core
only drops the refcount when the device is being deleted, not when
there's an unbind-rebind cycle without deletion of the device (as
happens for instance when the module is unloaded and reloaded). This has
to be handled by the driver. device_set_of_node_from_dev() handles it.

-- 
Regards,

Laurent Pinchart

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

[Andy Shevchenko]

On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:

...

> > > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > > >
> > > > Why not device_set_node()?
> > >
> > > Because device_set_of_node_from_dev() is meant for this exact use case,
> > > where the same node is used for multiple devices. It also puts any
> > > previous dev->of_node, ensuring proper refcounting when devices are
> > > unbound and rebound, without being deleted.
> >
> > When will the refcount be dropped (in case of removal of this device)?
> > Or you mean it shouldn't?
>
> Any refcount taken on the OF node needs to be dropped. The device core
> only drops the refcount when the device is being deleted, not when
> there's an unbind-rebind cycle without deletion of the device (as
> happens for instance when the module is unloaded and reloaded).

Under "device" you meant the real hardware, as Linux device (instance
of the struct device object) is being rebuilt AFAIK)?

> This has
> to be handled by the driver. device_set_of_node_from_dev() handles it.

But why do you need to keep a parent node reference bumped?
Only very few drivers in the kernel use this API and I believe either
nobody knows what they are doing and you are right, or you are doing
something which is not needed.

--
With Best Regards,
Andy Shevchenko

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

[Laurent Pinchart]

On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > > > >
> > > > > Why not device_set_node()?
> > > >
> > > > Because device_set_of_node_from_dev() is meant for this exact use case,
> > > > where the same node is used for multiple devices. It also puts any
> > > > previous dev->of_node, ensuring proper refcounting when devices are
> > > > unbound and rebound, without being deleted.
> > >
> > > When will the refcount be dropped (in case of removal of this device)?
> > > Or you mean it shouldn't?
> >
> > Any refcount taken on the OF node needs to be dropped. The device core
> > only drops the refcount when the device is being deleted, not when
> > there's an unbind-rebind cycle without deletion of the device (as
> > happens for instance when the module is unloaded and reloaded).
> 
> Under "device" you meant the real hardware, as Linux device (instance
> of the struct device object) is being rebuilt AFAIK)?

I mean struct device. The driver core will drop the reference in
platform_device_release(), called when the last reference to the
platform device is released, just before freeing the platform_device
instance. This happens after the device is removed from the system (e.g.
hot-unplug), but not when a device is unbound from a driver and rebound
(e.g. module unload and reload).

> > This has
> > to be handled by the driver. device_set_of_node_from_dev() handles it.
> 
> But why do you need to keep a parent node reference bumped?
> Only very few drivers in the kernel use this API and I believe either
> nobody knows what they are doing and you are right, or you are doing
> something which is not needed.

I need to set the of_node and fwnode fields of struct device to enable
OF-based lookups of GPIOs and PWMs. The of_node field is meant to be
populated by the driver core when the device is created, with a
reference to the OF node. When populated directly by driver, this needs
to be taken into account, and drivers need to ensure the reference will
be released correctly. device_set_of_node_from_dev() is meant for that.

-- 
Regards,

Laurent Pinchart

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

[Andy Shevchenko]

On Wed, May 29, 2024 at 5:35 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote:
> > > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:

...

> > > > > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > > > > >
> > > > > > Why not device_set_node()?
> > > > >
> > > > > Because device_set_of_node_from_dev() is meant for this exact use case,
> > > > > where the same node is used for multiple devices. It also puts any
> > > > > previous dev->of_node, ensuring proper refcounting when devices are
> > > > > unbound and rebound, without being deleted.
> > > >
> > > > When will the refcount be dropped (in case of removal of this device)?
> > > > Or you mean it shouldn't?
> > >
> > > Any refcount taken on the OF node needs to be dropped. The device core
> > > only drops the refcount when the device is being deleted, not when
> > > there's an unbind-rebind cycle without deletion of the device (as
> > > happens for instance when the module is unloaded and reloaded).
> >
> > Under "device" you meant the real hardware, as Linux device (instance
> > of the struct device object) is being rebuilt AFAIK)?
>
> I mean struct device. The driver core will drop the reference in
> platform_device_release(), called when the last reference to the
> platform device is released, just before freeing the platform_device
> instance. This happens after the device is removed from the system (e.g.
> hot-unplug), but not when a device is unbound from a driver and rebound
> (e.g. module unload and reload).

This is something I need to refresh in my memory. Any pointers (the
links to the exact code lines are also okay) where I can find the
proof of what you are saying. (It's not that I untrust you, it's just
that I take my time on studying it.)

> > > This has
> > > to be handled by the driver. device_set_of_node_from_dev() handles it.
> >
> > But why do you need to keep a parent node reference bumped?
> > Only very few drivers in the kernel use this API and I believe either

s/very/a/ (sorry for the confusion)

> > nobody knows what they are doing and you are right, or you are doing
> > something which is not needed.
>
> I need to set the of_node and fwnode fields of struct device to enable
> OF-based lookups of GPIOs and PWMs. The of_node field is meant to be
> populated by the driver core when the device is created, with a
> reference to the OF node. When populated directly by driver, this needs
> to be taken into account, and drivers need to ensure the reference will
> be released correctly. device_set_of_node_from_dev() is meant for that.

What you are doing is sharing the parent node with the child, but at
the same time you bump the parent's reference count. As this is a
child of MFD I don't think you need this as MFD already takes care of
it via parent -> child natural dependencies. Is it incorrect
understanding?

-- 
With Best Regards,
Andy Shevchenko

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

[Laurent Pinchart]

On Wed, May 29, 2024 at 06:00:19PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 5:35 PM Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote:
> > > > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > > > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > > > > > >
> > > > > > > Why not device_set_node()?
> > > > > >
> > > > > > Because device_set_of_node_from_dev() is meant for this exact use case,
> > > > > > where the same node is used for multiple devices. It also puts any
> > > > > > previous dev->of_node, ensuring proper refcounting when devices are
> > > > > > unbound and rebound, without being deleted.
> > > > >
> > > > > When will the refcount be dropped (in case of removal of this device)?
> > > > > Or you mean it shouldn't?
> > > >
> > > > Any refcount taken on the OF node needs to be dropped. The device core
> > > > only drops the refcount when the device is being deleted, not when
> > > > there's an unbind-rebind cycle without deletion of the device (as
> > > > happens for instance when the module is unloaded and reloaded).
> > >
> > > Under "device" you meant the real hardware, as Linux device (instance
> > > of the struct device object) is being rebuilt AFAIK)?
> >
> > I mean struct device. The driver core will drop the reference in
> > platform_device_release(), called when the last reference to the
> > platform device is released, just before freeing the platform_device
> > instance. This happens after the device is removed from the system (e.g.
> > hot-unplug), but not when a device is unbound from a driver and rebound
> > (e.g. module unload and reload).
> 
> This is something I need to refresh in my memory. Any pointers (the
> links to the exact code lines are also okay) where I can find the
> proof of what you are saying. (It's not that I untrust you, it's just
> that I take my time on studying it.)

I wish this was documented, I could then point you to a nice text that
explains it all :-) My understanding comes from reading
platform_device_release(), and from failing to find other of_node_put()
calls that would be relevant to this.

> > > > This has
> > > > to be handled by the driver. device_set_of_node_from_dev() handles it.
> > >
> > > But why do you need to keep a parent node reference bumped?
> > > Only very few drivers in the kernel use this API and I believe either
> 
> s/very/a/ (sorry for the confusion)
> 
> > > nobody knows what they are doing and you are right, or you are doing
> > > something which is not needed.
> >
> > I need to set the of_node and fwnode fields of struct device to enable
> > OF-based lookups of GPIOs and PWMs. The of_node field is meant to be
> > populated by the driver core when the device is created, with a
> > reference to the OF node. When populated directly by driver, this needs
> > to be taken into account, and drivers need to ensure the reference will
> > be released correctly. device_set_of_node_from_dev() is meant for that.
> 
> What you are doing is sharing the parent node with the child, but at
> the same time you bump the parent's reference count. As this is a
> child of MFD I don't think you need this as MFD already takes care of
> it via parent -> child natural dependencies. Is it incorrect
> understanding?

It is true that the parent device will not be destroyed as long as the
child device exists. The parent device's of_node will thus be kept
around by the reference that the parent device holds on it. However, if
I set dev.of_node for the child device without acquiring a reference, we
will have dev.of_node pointing to the same device_node, with a single
reference to it. This means that when the child device is destroyed and
platform_device_release() is called for it, of_node_put() will release
that reference, and the parent dev.of_node will point to a device_node
without holding a reference. Then, when the parent device, which is an
i2c_client, is unregistered, the call to i2c_unregister_device() will
call of_node_put(), releasing a reference we don't have.

The order of i2c_unregister_device() and platform_device_release() may
be swapped in practice, I haven't double-checked, but the reasoning
still holds. We have two functions that expect dev.of_node to hold a
reference, so both dev.of_node need to hold a reference.

Another way to handle the problem would be to borrow the parent's
reference in the probe function of the child, and set dev.of_node to
NULL manually in the child .remove() function. This will ensure that
platform_device_release(), which is called after .remove() (not
necessarily directly after, but certainly not before), will not attempt
to incorrectly release a reference on dev.of_node. This will however not
be safe if i2c_unregister_device() is called before the child .remove(),
as the child dev.of_node would then for some amount of time point to a
device_node without a reference.

TL;DR: Using device_set_of_node_from_dev() seems the safest option,
especially given that it was designed for this purpose.

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/4] 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 since v1:

- Drop mutex
- Restore R3 pinconfig to known value
- Simplify error check in pwm_adp5585_request()
- Don't fake PWM_POLARITY_INVERSED
- Fix rounding of period and duty cycle
- Drop OF match table
- Drop empty .remove() handler
- Allocate pwm_chip dynamically
- Document limitations
- Add platform ID table
- Set struct device of_node manually
- Merge child DT node into parent node

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 | 187 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 drivers/pwm/pwm-adp5585.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9c560d9a590a..9877fa342931 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -534,6 +534,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 1dd7921194f5..b778ecee3e9b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -47,6 +47,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 90913519f11a..f24d518d20f2 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PWM)		+= core.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..483846cd7334
--- /dev/null
+++ b/drivers/pwm/pwm-adp5585.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ADP5585 PWM driver
+ *
+ * Copyright 2022 NXP
+ * Copyright 2024 Ideas on Board Oy
+ *
+ * Limitations:
+ * - The .apply() operation executes atomically, but may not wait for the
+ *   period to complete (this is not documented and would need to be tested).
+ * - Disabling the PWM drives the output pin to a low level immediately.
+ * - The hardware can only generate normal polarity output.
+ */
+
+#include <linux/device.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/module.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)
+
+static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct regmap *regmap = pwmchip_get_drvdata(chip);
+	int ret;
+
+	ret = regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C,
+				 ADP5585_R3_EXTEND_CFG_MASK,
+				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(regmap, ADP5585_GENERAL_CFG,
+				  ADP5585_OSC_EN, ADP5585_OSC_EN);
+}
+
+static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct regmap *regmap = pwmchip_get_drvdata(chip);
+
+	regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C,
+			   ADP5585_R3_EXTEND_CFG_MASK,
+			   ADP5585_R3_EXTEND_CFG_GPIO4);
+	regmap_update_bits(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 regmap *regmap = pwmchip_get_drvdata(chip);
+	u64 period, duty_cycle;
+	u32 on, off;
+	int ret;
+
+	if (!state->enabled)
+		return regmap_update_bits(regmap, ADP5585_PWM_CFG,
+					  ADP5585_PWM_EN, 0);
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
+		return -EINVAL;
+
+	period = min(state->period, ADP5585_PWM_MAX_PERIOD_NS);
+	duty_cycle = min(state->duty_cycle, 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_u64(duty_cycle, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
+	off = div_u64(period, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on;
+
+	ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW,
+			   off & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH,
+			   (off >> 8) & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW,
+			   on & 0xff);
+	if (ret)
+		return ret;
+	ret = regmap_write(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(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 regmap *regmap = pwmchip_get_drvdata(chip);
+	unsigned int on, off;
+	unsigned int val;
+
+	regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off);
+	regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val);
+	off |= val << 8;
+
+	regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on);
+	regmap_read(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(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 device *dev = &pdev->dev;
+	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
+	struct pwm_chip *chip;
+	int ret;
+
+	chip = devm_pwmchip_alloc(dev, ADP5585_PWM_CHAN_NUM, 0);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	device_set_of_node_from_dev(dev, dev->parent);
+
+	pwmchip_set_drvdata(chip, adp5585->regmap);
+	chip->ops = &adp5585_pwm_ops;
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
+
+	return 0;
+}
+
+static const struct platform_device_id adp5585_pwm_id_table[] = {
+	{ "adp5585-pwm" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(platform, adp5585_pwm_id_table);
+
+static struct platform_driver adp5585_pwm_driver = {
+	.driver	= {
+		.name = "adp5585-pwm",
+	},
+	.probe = adp5585_pwm_probe,
+	.id_table = adp5585_pwm_id_table,
+};
+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 v2 4/4] pwm: adp5585: Add Analog Devices ADP5585 support

[Andy Shevchenko]

Tue, May 28, 2024 at 10:03:14PM +0300, Laurent Pinchart kirjoitti:
> 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.

...

> +#include <linux/device.h>

+ err.h

> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>

...

> +#define ADP5585_PWM_OSC_FREQ_HZ		1000000U

(1 * HZ_PER_MHZ) ?

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

Wouldn't be better to use GENMASK() or (BIT(x) - 1) notation to show that
the limit is due to HW register bits in use?

...

> +	ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW,
> +			   off & 0xff);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH,
> +			   (off >> 8) & 0xff);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW,
> +			   on & 0xff);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH,
> +			   (on >> 8) & 0xff);
> +	if (ret)
> +		return ret;

Can be proper __le16/__be16 be used in conjunction with regmap bulk API?

...

> +	/* Enable PWM in continuous mode and no external AND'ing. */
> +	ret = regmap_update_bits(regmap, ADP5585_PWM_CFG,
> +				 ADP5585_PWM_IN_AND | ADP5585_PWM_MODE |
> +				 ADP5585_PWM_EN, ADP5585_PWM_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return regmap_update_bits(...);

...

> +	regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off);
> +	regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val);
> +	off |= val << 8;
> +
> +	regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on);
> +	regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val);
> +	on |= val << 8;

As per above, can it be converted to use proper __le16/__be16 type and
regmap bulk API?

...

> +	device_set_of_node_from_dev(dev, dev->parent);

Why this one? What's wrong with device_set_node()?

-- 
With Best Regards,
Andy Shevchenko

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

[Laurent Pinchart]

Hi Andy,

Thank you for the review.

On Tue, May 28, 2024 at 10:41:51PM +0300, Andy Shevchenko wrote:
> Tue, May 28, 2024 at 10:03:14PM +0300, Laurent Pinchart kirjoitti:
> > 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.
> 
> ...
> 
> > +#include <linux/device.h>
> 
> + err.h
> 
> > +#include <linux/math64.h>
> > +#include <linux/minmax.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time.h>

You forgot to mention types.h :-)

> 
> ...
> 
> > +#define ADP5585_PWM_OSC_FREQ_HZ		1000000U
> 
> (1 * HZ_PER_MHZ) ?

If we had an MHZ macro I would use 1 * MHZ, but I don't think HZ_PER_MHZ
improves readability here.

> > +#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)
> 
> Wouldn't be better to use GENMASK() or (BIT(x) - 1) notation to show that
> the limit is due to HW register bits in use?

I think that would decrease readability to be honest.

> ...
> 
> > +	ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW,
> > +			   off & 0xff);
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH,
> > +			   (off >> 8) & 0xff);
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW,
> > +			   on & 0xff);
> > +	if (ret)
> > +		return ret;
> > +	ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH,
> > +			   (on >> 8) & 0xff);
> > +	if (ret)
> > +		return ret;
> 
> Can be proper __le16/__be16 be used in conjunction with regmap bulk API?

What I would really like is regmap growing an API similar to
include/media/v4l2-cci.h. Any volunteer ? :-)

> ...
> 
> > +	/* Enable PWM in continuous mode and no external AND'ing. */
> > +	ret = regmap_update_bits(regmap, ADP5585_PWM_CFG,
> > +				 ADP5585_PWM_IN_AND | ADP5585_PWM_MODE |
> > +				 ADP5585_PWM_EN, ADP5585_PWM_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> 	return regmap_update_bits(...);
> 
> ...
> 
> > +	regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off);
> > +	regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val);
> > +	off |= val << 8;
> > +
> > +	regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on);
> > +	regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val);
> > +	on |= val << 8;
> 
> As per above, can it be converted to use proper __le16/__be16 type and
> regmap bulk API?

As there are only 2 registers, I think that's a bit overkill really.

> ...
> 
> > +	device_set_of_node_from_dev(dev, dev->parent);
> 
> Why this one? What's wrong with device_set_node()?

See my reply to 3/4.

-- 
Regards,

Laurent Pinchart

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

[Andy Shevchenko]

On Tue, May 28, 2024 at 11:27 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, May 28, 2024 at 10:41:51PM +0300, Andy Shevchenko wrote:
> > Tue, May 28, 2024 at 10:03:14PM +0300, Laurent Pinchart kirjoitti:

...

> > > +#define ADP5585_PWM_OSC_FREQ_HZ            1000000U
> >
> > (1 * HZ_PER_MHZ) ?
>
> If we had an MHZ macro I would use 1 * MHZ, but I don't think HZ_PER_MHZ
> improves readability here.

We have MEGA. HZ is already the suffix in this definition.

> > > +#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)
> >
> > Wouldn't be better to use GENMASK() or (BIT(x) - 1) notation to show that
> > the limit is due to HW register bits in use?
>
> I think that would decrease readability to be honest.

I think it improves the robustness of the code. I always fail to count
3,4,5,6 f:s in those masks, using BIT()/GENMASK() notation makes it
better.

...

> > > +   ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW,
> > > +                      off & 0xff);
> > > +   if (ret)
> > > +           return ret;
> > > +   ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH,
> > > +                      (off >> 8) & 0xff);
> > > +   if (ret)
> > > +           return ret;
> > > +   ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW,
> > > +                      on & 0xff);
> > > +   if (ret)
> > > +           return ret;
> > > +   ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH,
> > > +                      (on >> 8) & 0xff);
> > > +   if (ret)
> > > +           return ret;
> >
> > Can be proper __le16/__be16 be used in conjunction with regmap bulk API?
>
> What I would really like is regmap growing an API similar to
> include/media/v4l2-cci.h. Any volunteer ? :-)

So, the answer here is yes?

...

> > > +   regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off);
> > > +   regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val);
> > > +   off |= val << 8;
> > > +
> > > +   regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on);
> > > +   regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val);
> > > +   on |= val << 8;
> >
> > As per above, can it be converted to use proper __le16/__be16 type and
> > regmap bulk API?
>
> As there are only 2 registers, I think that's a bit overkill really.

I do not think so. It increases readability (less LoCs) and improves a
lot of understanding of the hardware layout from reading the code.
Please, consider using it.

...

> > > +   device_set_of_node_from_dev(dev, dev->parent);
> >
> > Why this one? What's wrong with device_set_node()?
>
> See my reply to 3/4.

See additional questions there as well.

-- 
With Best Regards,
Andy Shevchenko