[PATCH 0/6] mfd: add adp5585 gpio and pwm support

[PATCH 0/6] mfd: add adp5585 gpio and pwm support

[Frank Li]

adp5585 is totally difference adp5588, which in
drivers/input/keyboard/adp5588-keys.c.

So create new driver for it.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Clark Wang (1):
      pwm: adp5585: add adp5585 PWM support

Frank Li (3):
      dt-bindings: mfd: Add i2c device adi5585
      arm64: dts: imx93-11x11-evk: add adi,adp5585 gpio and pwm
      arm64: dts: imx95-19x19-evk: add i2c2 and adi,adp5585

Haibo Chen (2):
      mfd: adp5585: add ADI adp5585 core support
      gpio: adp5585-gpio: add adp5585-gpio support

 .../devicetree/bindings/mfd/adi,adp5585.yaml       |  83 ++++++++
 arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts  |  16 ++
 arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts  |  30 +++
 drivers/gpio/Kconfig                               |   7 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-adp5585.c                        | 184 ++++++++++++++++++
 drivers/mfd/Kconfig                                |   9 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/adp5585.c                              | 134 +++++++++++++
 drivers/pwm/Kconfig                                |   8 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-adp5585.c                          | 215 +++++++++++++++++++++
 include/linux/mfd/adp5585.h                        | 100 ++++++++++
 13 files changed, 789 insertions(+)
---
base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf
change-id: 20240715-adi-11ff2d3d35ec

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH 1/6] dt-bindings: mfd: Add i2c device adi5585

[Frank Li]

Add adi5585, which provide gpio, pwm and keypad controller.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../devicetree/bindings/mfd/adi,adp5585.yaml       | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
new file mode 100644
index 0000000000000..03c4760242ddd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: GPL-2.0-only 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: ADI adp5585 GPIO, PWM, keypad controller
+
+maintainers:
+  - Frank Li <Frank.Li@nxp.com>
+
+description:
+  adp5585 is a multifunctional device that can provide GPIO, PWM and keypad.
+
+properties:
+  compatible:
+    const: adi,adp5585
+
+  reg:
+    items:
+      description: I2C device address.
+
+  gpio:
+    type: object
+    properties:
+      compatible:
+        const: adp5585-gpio
+
+      gpio-controller: true
+
+      "#gpio-cells":
+        const: 2
+
+    required:
+      - compatible
+      - gpio-controller
+      - "#gpio-cells"
+
+    additionalProperties: false
+
+  pwm:
+    $ref: /schemas/pwm/pwm.yaml
+    properties:
+      compatible:
+        const: adp5585-pwm
+
+      "#pwm-cells":
+        const: 3
+
+    required:
+      - compatible
+      - "#pwm-cells"
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mfd@34 {
+            compatible = "adi,adp5585";
+            reg = <0x34>;
+
+            gpio {
+                compatible = "adp5585-gpio";
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            pwm {
+                compatible = "adp5585-pwm";
+                #pwm-cells = <3>;
+            };
+        };
+    };

-- 
2.34.1

[PATCH 2/6] mfd: adp5585: add ADI adp5585 core support

[Frank Li]

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

ADP5585 support multi function, include expander GPIO, pwm, keypad
controller.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/mfd/Kconfig         |   9 +++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/adp5585.c       | 134 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/adp5585.h | 100 +++++++++++++++++++++++++++++++++
 4 files changed, 244 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6b..62a967ee8ae1c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -20,6 +20,15 @@ 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 "ADI ADP5585 core functions"
+	select MFD_CORE
+	depends on I2C && OF
+
+	help
+	  This is ADI adp5585 core support, implement the support for
+	  communication through i2c bus.
+
 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 02b651cd75352..2a9f91e81af83 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -193,6 +193,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 0000000000000..52cc0d38bf2c3
--- /dev/null
+++ b/drivers/mfd/adp5585.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* ADP5585 IO Expander, Key controller core driver.
+ *
+ * Copyright 2024 NXP
+ */
+
+#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>
+
+static const struct mfd_cell adp5585_devs[] = {
+	{
+		.name = "adp5585-gpio",
+		.of_compatible = "adp5585-gpio",
+	},
+	{
+		.name = "adp5585-pwm",
+		.of_compatible = "adp5585-pwm",
+	},
+};
+
+static int adp5585_i2c_read_reg(struct adp5585_dev *adp5585, u8 reg, u8 *val)
+{
+	struct i2c_client *i2c = adp5585->i2c_client;
+	struct i2c_msg xfer[2];
+	int ret;
+
+	/* Write register */
+	xfer[0].addr = i2c->addr;
+	xfer[0].flags = 0;
+	xfer[0].len = 1;
+	xfer[0].buf = &reg;
+
+	/* Read data */
+	xfer[1].addr = i2c->addr;
+	xfer[1].flags = I2C_M_RD;
+	xfer[1].len = 1;
+	xfer[1].buf = val;
+
+	ret = i2c_transfer(i2c->adapter, xfer, 2);
+	if (ret == 2)
+		return 0;
+
+	dev_err(&i2c->dev, "Failed to read reg 0x%02x, ret is %d\n", reg, ret);
+
+	return ret >= 0 ? -EIO : ret;
+}
+
+static int adp5585_i2c_write_reg(struct adp5585_dev *adp5585, u8 reg, u8 val)
+{
+	struct i2c_client *i2c = adp5585->i2c_client;
+	u8 msg[2];
+	int ret;
+
+	msg[0] = reg;
+	msg[1] = val;
+	ret = i2c_master_send(i2c, msg, 2);
+	if (ret == 2)
+		return 0;
+
+	dev_err(&i2c->dev, "Failed to write reg 0x%02x, ret is %d\n", reg, ret);
+
+	return ret >= 0 ? -EIO : ret;
+}
+
+static int adp5585_i2c_probe(struct i2c_client *i2c)
+{
+	struct adp5585_dev *adp;
+	u8 reg;
+	int ret;
+
+	adp = devm_kzalloc(&i2c->dev, sizeof(struct adp5585_dev), GFP_KERNEL);
+	if (!adp)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, adp);
+	adp->dev = &i2c->dev;
+	adp->i2c_client = i2c;
+	adp->read_reg = adp5585_i2c_read_reg;
+	adp->write_reg = adp5585_i2c_write_reg;
+
+	ret = adp5585_i2c_read_reg(adp, ADP5585_ID, &reg);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(adp->dev, PLATFORM_DEVID_AUTO,
+				    adp5585_devs, ARRAY_SIZE(adp5585_devs),
+				    NULL, 0, NULL);
+}
+
+static const struct i2c_device_id adp5585_i2c_id[] = {
+	{ "adp5585", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, adp5585_i2c_id);
+
+static const struct of_device_id adp5585_of_match[] = {
+	{.compatible = "adi,adp5585", },
+	{ /* 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),
+	},
+	.probe = adp5585_i2c_probe,
+	.id_table = adp5585_i2c_id,
+};
+
+static int __init adp5585_i2c_init(void)
+{
+	return i2c_add_driver(&adp5585_i2c_driver);
+}
+
+/* init early so consumer devices can complete system boot */
+subsys_initcall(adp5585_i2c_init);
+
+static void __exit adp5585_i2c_exit(void)
+{
+	i2c_del_driver(&adp5585_i2c_driver);
+}
+module_exit(adp5585_i2c_exit);
+
+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 0000000000000..58a9f84c9a75a
--- /dev/null
+++ b/include/linux/mfd/adp5585.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Analog Devices ADP5585 I/O Expander, keypad controller,
+ * PWM contorller.
+ *
+ * Copyright 2022 NXP
+ */
+
+#ifndef __ADP5585_H_
+#define __ADP5585_H_
+
+#define ADP5585_ID			0x00
+#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_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_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_GENERAL_CFG		0x3B
+#define ADP5585_INT_EN			0x3C
+
+/* ID Register */
+#define ADP5585_DEVICE_ID_MASK	0xF
+#define ADP5585_MAN_ID_MASK	0xF
+#define ADP5585_MAN_ID_SHIFT	4
+#define ADP5585_MAN_ID		0x02
+
+#define ADP5585_PWM_CFG_EN		0x1
+#define ADP5585_PWM_CFG_MODE		0x2
+#define ADP5585_PIN_CONFIG_R3_PWM	0x8
+#define ADP5585_PIN_CONFIG_R3_MASK	0xC
+#define ADP5585_GENERAL_CFG_OSC_EN	0x80
+
+#define ADP5585_REG_MASK		0xFF
+
+#define ADP5585_BANK(offs)		((offs) > 4)
+#define ADP5585_BIT(offs)		((offs) > 4 ? \
+					1u << ((offs) - 5) : 1u << (offs))
+
+struct adp5585_dev {
+	struct device *dev;
+	struct i2c_client *i2c_client;
+
+	int (*read_reg)(struct adp5585_dev *adp5585, u8 reg, u8 *val);
+	int (*write_reg)(struct adp5585_dev *adp5585, u8 reg, u8 val);
+};
+
+#endif

-- 
2.34.1

[PATCH 3/6] gpio: adp5585-gpio: add adp5585-gpio support

[Frank Li]

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

Add gpio function support for MFD adp5585.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/gpio/Kconfig        |   7 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-adp5585.c | 184 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 58f43bcced7c1..20daa3d70abc7 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 on-chip GPIO found on 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 64dd6d9d730d5..1429e8c0229b9 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 0000000000000..7c9edbc16975b
--- /dev/null
+++ b/drivers/gpio/gpio-adp5585.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO driver for Analog Devices ADP5585 MFD
+ *
+ * Copyright 2024 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/gpio/driver.h>
+
+#define ADP5585_GPIO_MAX 10
+
+struct adp5585_gpio_dev {
+	struct device *parent;
+	struct gpio_chip gpio_chip;
+	struct mutex lock;
+	u8 dat_out[2];
+	u8 dir[2];
+};
+
+static int adp5585_gpio_reg_read(struct adp5585_gpio_dev *adp5585_gpio, u8 reg, u8 *val)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(adp5585_gpio->parent);
+
+	return adp5585->read_reg(adp5585, reg, val);
+}
+
+static int adp5585_gpio_reg_write(struct adp5585_gpio_dev *adp5585_gpio, u8 reg, u8 val)
+{
+	struct adp5585_dev *adp5585 = dev_get_drvdata(adp5585_gpio->parent);
+
+	return adp5585->write_reg(adp5585, reg, val);
+}
+
+static int adp5585_gpio_get_value(struct gpio_chip *chip, unsigned int off)
+{
+	struct adp5585_gpio_dev *adp5585_gpio;
+	unsigned int bank, bit;
+	u8 val;
+
+	adp5585_gpio = gpiochip_get_data(chip);
+	bank = ADP5585_BANK(off);
+	bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	/* There are dedicated registers for GPIO IN/OUT. */
+	if (adp5585_gpio->dir[bank] & bit)
+		val = adp5585_gpio->dat_out[bank];
+	else
+		adp5585_gpio_reg_read(adp5585_gpio, ADP5585_GPI_STATUS_A + bank, &val);
+
+	return !!(val & bit);
+}
+
+static void adp5585_gpio_set_value(struct gpio_chip *chip, unsigned int off, int val)
+{
+	struct adp5585_gpio_dev *adp5585_gpio;
+	unsigned int bank, bit;
+
+	adp5585_gpio = gpiochip_get_data(chip);
+	bank = ADP5585_BANK(off);
+	bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	if (val)
+		adp5585_gpio->dat_out[bank] |= bit;
+	else
+		adp5585_gpio->dat_out[bank] &= ~bit;
+
+	adp5585_gpio_reg_write(adp5585_gpio, ADP5585_GPO_DATA_OUT_A + bank,
+			       adp5585_gpio->dat_out[bank]);
+}
+
+static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
+{
+	struct adp5585_gpio_dev *adp5585_gpio;
+	unsigned int bank, bit;
+	int ret;
+
+	adp5585_gpio = gpiochip_get_data(chip);
+	bank = ADP5585_BANK(off);
+	bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	adp5585_gpio->dir[bank] &= ~bit;
+	ret = adp5585_gpio_reg_write(adp5585_gpio, ADP5585_GPIO_DIRECTION_A + bank,
+				     adp5585_gpio->dir[bank]);
+	return ret;
+}
+
+static int adp5585_gpio_direction_output(struct gpio_chip *chip, unsigned int off, int val)
+{
+	struct adp5585_gpio_dev *adp5585_gpio;
+	unsigned int bank, bit;
+	int ret;
+
+	adp5585_gpio = gpiochip_get_data(chip);
+	bank = ADP5585_BANK(off);
+	bit = ADP5585_BIT(off);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	adp5585_gpio->dir[bank] |= bit;
+
+	if (val)
+		adp5585_gpio->dat_out[bank] |= bit;
+	else
+		adp5585_gpio->dat_out[bank] &= ~bit;
+
+	ret = adp5585_gpio_reg_write(adp5585_gpio, ADP5585_GPO_DATA_OUT_A + bank,
+				     adp5585_gpio->dat_out[bank]);
+	ret |= adp5585_gpio_reg_write(adp5585_gpio, ADP5585_GPIO_DIRECTION_A + bank,
+				      adp5585_gpio->dir[bank]);
+
+	return ret;
+}
+
+static int adp5585_gpio_probe(struct platform_device *pdev)
+{
+	struct adp5585_gpio_dev *adp5585_gpio;
+	struct device *dev = &pdev->dev;
+	struct gpio_chip *gc;
+	int i;
+
+	adp5585_gpio = devm_kzalloc(&pdev->dev, sizeof(struct adp5585_gpio_dev), GFP_KERNEL);
+	if (!adp5585_gpio)
+		return -ENOMEM;
+
+	adp5585_gpio->parent = pdev->dev.parent;
+
+	gc = &adp5585_gpio->gpio_chip;
+	gc->parent = dev;
+	gc->direction_input  = adp5585_gpio_direction_input;
+	gc->direction_output = adp5585_gpio_direction_output;
+	gc->get = adp5585_gpio_get_value;
+	gc->set = adp5585_gpio_set_value;
+	gc->can_sleep = true;
+
+	gc->base = -1;
+	gc->ngpio = ADP5585_GPIO_MAX;
+	gc->label = pdev->name;
+	gc->owner = THIS_MODULE;
+
+	mutex_init(&adp5585_gpio->lock);
+
+	for (i = 0; i < 2; i++) {
+		u8 *dat_out, *dir;
+
+		dat_out = adp5585_gpio->dat_out;
+		dir = adp5585_gpio->dir;
+		adp5585_gpio_reg_read(adp5585_gpio, ADP5585_GPO_DATA_OUT_A + i, dat_out + i);
+		adp5585_gpio_reg_read(adp5585_gpio, ADP5585_GPIO_DIRECTION_A + i, dir + i);
+	}
+
+	return devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip, adp5585_gpio);
+}
+
+static const struct of_device_id adp5585_of_match[] = {
+	{.compatible = "adp5585-gpio", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_of_match);
+
+static struct platform_driver adp5585_gpio_driver = {
+	.driver	= {
+		.name	= "adp5585-gpio",
+		.of_match_table = adp5585_of_match,
+	},
+	.probe		= adp5585_gpio_probe,
+};
+
+module_platform_driver(adp5585_gpio_driver);
+
+MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>");
+MODULE_DESCRIPTION("GPIO ADP5585 Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH 4/6] pwm: adp5585: add adp5585 PWM support

[Frank Li]

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

Add PWM function support for MFD adp5585.

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pwm/Kconfig       |   8 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-adp5585.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3e53838990f5b..baaadf877b9c6 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -38,6 +38,14 @@ config PWM_DEBUG
 	  It is expected to introduce some runtime overhead and diagnostic
 	  output to the kernel log, so only enable while working on a driver.
 
+config PWM_ADP5585
+	tristate "ADP5585 PWM support"
+	depends on MFD_ADP5585
+	help
+	  This option enables support for on-chip PWM found
+	  on Analog Devices ADP5585.
+
+
 config PWM_AB8500
 	tristate "AB8500 PWM support"
 	depends on AB8500_CORE && ARCH_U8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 0be4f3e6dd432..161131a261e94 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
+obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
new file mode 100644
index 0000000000000..f578d24df5c74
--- /dev/null
+++ b/drivers/pwm/pwm-adp5585.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PWM driver for Analog Devices ADP5585 MFD
+ *
+ * Copyright 2024 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+
+#define ADP5585_PWM_CHAN_NUM		1
+#define ADP5585_PWM_FASTEST_PERIOD_NS	2000
+#define ADP5585_PWM_SLOWEST_PERIOD_NS	131070000
+
+struct adp5585_pwm_chip {
+	struct device *parent;
+	struct mutex lock;
+	u8 pin_config_val;
+};
+
+static inline struct adp5585_pwm_chip *
+to_adp5585_pwm_chip(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int adp5585_pwm_reg_read(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 *val)
+{
+	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
+
+	return adp5585->read_reg(adp5585, reg, val);
+}
+
+static int adp5585_pwm_reg_write(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 val)
+{
+	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
+
+	return adp5585->write_reg(adp5585, reg, val);
+}
+
+static int pwm_adp5585_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	u32 on, off;
+	u8 temp;
+
+	/* get period */
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_LOW, &temp);
+	off = temp;
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_HIGH, &temp);
+	off |= temp << 8;
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_LOW, &temp);
+	on = temp;
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_HIGH, &temp);
+	on |= temp << 8;
+	state->period = (on + off) * NSEC_PER_USEC;
+
+	state->duty_cycle = on;
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	/* get channel status */
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &temp);
+	state->enabled = temp & ADP5585_PWM_CFG_EN;
+
+	return 0;
+}
+
+static int pwm_adp5585_apply(struct pwm_chip *chip,
+			     struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	u32 on, off;
+	u8 enabled;
+	int ret;
+
+	if (state->period > ADP5585_PWM_SLOWEST_PERIOD_NS ||
+	    state->period < ADP5585_PWM_FASTEST_PERIOD_NS)
+		return -EINVAL;
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	/* set on/off cycle*/
+	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, NSEC_PER_USEC);
+	off = DIV_ROUND_CLOSEST_ULL((state->period - state->duty_cycle), NSEC_PER_USEC);
+
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_LOW, off & ADP5585_REG_MASK);
+	if (ret)
+		return ret;
+
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_HIGH,
+				    (off >> 8) & ADP5585_REG_MASK);
+	if (ret)
+		return ret;
+
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_LOW, on & ADP5585_REG_MASK);
+	if (ret)
+		return ret;
+
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_HIGH,
+				    (on >> 8) & ADP5585_REG_MASK);
+	if (ret)
+		return ret;
+
+	/* enable PWM and set to continuous PWM mode*/
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &enabled);
+	if (state->enabled)
+		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, ADP5585_PWM_CFG_EN);
+	else
+		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, 0);
+
+	return ret;
+}
+
+static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	u8 reg_cfg;
+	int ret;
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &adp5585_pwm->pin_config_val);
+	reg_cfg = adp5585_pwm->pin_config_val & ~ADP5585_PIN_CONFIG_R3_MASK;
+	reg_cfg |= ADP5585_PIN_CONFIG_R3_PWM;
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
+
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_GENERAL_CFG, &adp5585_pwm->pin_config_val);
+	reg_cfg |= ADP5585_GENERAL_CFG_OSC_EN;
+	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_GENERAL_CFG, reg_cfg);
+
+	return ret;
+}
+
+static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
+	u8 reg_cfg;
+
+	guard(mutex)(&adp5585_pwm->lock);
+
+	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &reg_cfg);
+	reg_cfg &= ~ADP5585_PIN_CONFIG_R3_MASK;
+	reg_cfg |= adp5585_pwm->pin_config_val & ADP5585_PIN_CONFIG_R3_MASK;
+	adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
+}
+
+static const struct pwm_ops adp5585_pwm_ops = {
+	.request = pwm_adp5585_request,
+	.free = pwm_adp5585_free,
+	.get_state = pwm_adp5585_get_state,
+	.apply = pwm_adp5585_apply,
+};
+
+static int adp5585_pwm_probe(struct platform_device *pdev)
+{
+	struct adp5585_pwm_chip *adp5585_pwm;
+	struct pwm_chip *chip;
+	unsigned int npwm = ADP5585_PWM_CHAN_NUM;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*adp5585_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	adp5585_pwm = to_adp5585_pwm_chip(chip);
+	adp5585_pwm->parent = pdev->dev.parent;
+
+	platform_set_drvdata(pdev, adp5585_pwm);
+
+	chip->ops = &adp5585_pwm_ops;
+	mutex_init(&adp5585_pwm->lock);
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret)
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+
+	return ret;
+}
+
+static void adp5585_pwm_remove(struct platform_device *pdev)
+{
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+
+	pwmchip_remove(chip);
+}
+
+static const struct of_device_id adp5585_pwm_of_match[] = {
+	{.compatible = "adp5585-pwm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
+
+static struct platform_driver adp5585_pwm_driver = {
+	.driver	= {
+		.name	= "adp5585-pwm",
+		.of_match_table = adp5585_pwm_of_match,
+	},
+	.probe		= adp5585_pwm_probe,
+	.remove		= adp5585_pwm_remove,
+};
+module_platform_driver(adp5585_pwm_driver);
+
+MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");
+MODULE_DESCRIPTION("ADP5585 PWM Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH 5/6] arm64: dts: imx93-11x11-evk: add adi,adp5585 gpio and pwm

[Frank Li]

Add adi,adp5585 gpio and pwm for imx93-11x11-evk board.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
index a15987f49e8d6..6bfb90f4da63a 100644
--- a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
@@ -241,6 +241,22 @@ ldo5: LDO5 {
 			};
 		};
 	};
+
+	adp5585: mfd@34 {
+		compatible = "adi,adp5585";
+		reg = <0x34>;
+
+		adp5585gpio: gpio {
+			compatible = "adp5585-gpio";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		adp5585pwm: pwm {
+			compatible = "adp5585-pwm";
+			#pwm-cells = <3>;
+		};
+	};
 };
 
 &lpi2c3 {

-- 
2.34.1

[PATCH 6/6] arm64: dts: imx95-19x19-evk: add i2c2 and adi,adp5585

[Frank Li]

Add adi,adp5585 gpio and pwm for imx95-19x19-evk board.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
index d14a54ab4fd47..8c52fec79535f 100644
--- a/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts
@@ -81,6 +81,29 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
 	};
 };
 
+&lpi2c2 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lpi2c2>;
+	status = "okay";
+
+	adp5585: mfd@34 {
+		compatible = "adi,adp5585";
+		reg = <0x34>;
+
+		adp5585gpio: gpio-adp5585 {
+			compatible = "adp5585-gpio";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		adp5585pwm: pwm-adp5585 {
+			compatible = "adp5585-pwm";
+			#pwm-cells = <3>;
+		};
+	};
+};
+
 &lpi2c7 {
 	clock-frequency = <1000000>;
 	pinctrl-names = "default";
@@ -159,6 +182,13 @@ &wdog3 {
 };
 
 &scmi_iomuxc {
+	pinctrl_lpi2c2: lpi2c2grp {
+		fsl,pins = <
+			IMX95_PAD_I2C2_SCL__AONMIX_TOP_LPI2C2_SCL	0x40000b9e
+			IMX95_PAD_I2C2_SDA__AONMIX_TOP_LPI2C2_SDA	0x40000b9e
+		>;
+	};
+
 	pinctrl_i2c7_pcal6524: i2c7pcal6524grp {
 		fsl,pins = <
 			IMX95_PAD_GPIO_IO36__GPIO5_IO_BIT16			0x31e

-- 
2.34.1

Re: [PATCH 1/6] dt-bindings: mfd: Add i2c device adi5585

[Rob Herring]

On Tue, Jul 16, 2024 at 03:28:24PM -0400, Frank Li wrote:
> Add adi5585, which provide gpio, pwm and keypad controller.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../devicetree/bindings/mfd/adi,adp5585.yaml       | 83 ++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> new file mode 100644
> index 0000000000000..03c4760242ddd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: GPL-2.0-only 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: ADI adp5585 GPIO, PWM, keypad controller
> +
> +maintainers:
> +  - Frank Li <Frank.Li@nxp.com>
> +
> +description:
> +  adp5585 is a multifunctional device that can provide GPIO, PWM and keypad.
> +
> +properties:
> +  compatible:
> +    const: adi,adp5585
> +
> +  reg:
> +    items:
> +      description: I2C device address.
> +
> +  gpio:
> +    type: object
> +    properties:
> +      compatible:
> +        const: adp5585-gpio
> +
> +      gpio-controller: true
> +
> +      "#gpio-cells":
> +        const: 2
> +
> +    required:
> +      - compatible
> +      - gpio-controller
> +      - "#gpio-cells"
> +
> +    additionalProperties: false
> +
> +  pwm:
> +    $ref: /schemas/pwm/pwm.yaml
> +    properties:
> +      compatible:
> +        const: adp5585-pwm
> +
> +      "#pwm-cells":
> +        const: 3
> +
> +    required:
> +      - compatible
> +      - "#pwm-cells"
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mfd@34 {
> +            compatible = "adi,adp5585";
> +            reg = <0x34>;
> +
> +            gpio {
> +                compatible = "adp5585-gpio";

Missing vendor prefix. However, ...

> +                gpio-controller;
> +                #gpio-cells = <2>;
> +            };
> +
> +            pwm {
> +                compatible = "adp5585-pwm";
> +                #pwm-cells = <3>;
> +            };

There's no need for these child nodes. This can be just:

mfd@34 {
    compatible = "adi,adp5585";
    reg = <0x34>;
    gpio-controller;
    #gpio-cells = <2>;
    #pwm-cells = <3>;
};

Rob

Re: [PATCH 0/6] mfd: add adp5585 gpio and pwm support

[Rob Herring (Arm)]

On Tue, 16 Jul 2024 15:28:23 -0400, Frank Li wrote:
> adp5585 is totally difference adp5588, which in
> drivers/input/keyboard/adp5588-keys.c.
> 
> So create new driver for it.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Clark Wang (1):
>       pwm: adp5585: add adp5585 PWM support
> 
> Frank Li (3):
>       dt-bindings: mfd: Add i2c device adi5585
>       arm64: dts: imx93-11x11-evk: add adi,adp5585 gpio and pwm
>       arm64: dts: imx95-19x19-evk: add i2c2 and adi,adp5585
> 
> Haibo Chen (2):
>       mfd: adp5585: add ADI adp5585 core support
>       gpio: adp5585-gpio: add adp5585-gpio support
> 
>  .../devicetree/bindings/mfd/adi,adp5585.yaml       |  83 ++++++++
>  arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts  |  16 ++
>  arch/arm64/boot/dts/freescale/imx95-19x19-evk.dts  |  30 +++
>  drivers/gpio/Kconfig                               |   7 +
>  drivers/gpio/Makefile                              |   1 +
>  drivers/gpio/gpio-adp5585.c                        | 184 ++++++++++++++++++
>  drivers/mfd/Kconfig                                |   9 +
>  drivers/mfd/Makefile                               |   1 +
>  drivers/mfd/adp5585.c                              | 134 +++++++++++++
>  drivers/pwm/Kconfig                                |   8 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-adp5585.c                          | 215 +++++++++++++++++++++
>  include/linux/mfd/adp5585.h                        | 100 ++++++++++
>  13 files changed, 789 insertions(+)
> ---
> base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf
> change-id: 20240715-adi-11ff2d3d35ec
> 
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y freescale/imx93-11x11-evk.dtb freescale/imx95-19x19-evk.dtb' for 20240716-adi-v1-0-79c0122986e7@nxp.com:

arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb: mfd@34: 'gpio', 'pwm' do not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#
arch/arm64/boot/dts/freescale/imx95-19x19-evk.dtb: mfd@34: 'gpio-adp5585', 'pwm-adp5585' do not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#

Re: [PATCH 0/6] mfd: add adp5585 gpio and pwm support

[Krzysztof Kozlowski]

On 16/07/2024 21:28, Frank Li wrote:
> adp5585 is totally difference adp5588, which in
> drivers/input/keyboard/adp5588-keys.c.
> 
> So create new driver for it.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

There is ongoing work (v4 if I recall) for adp5585, so please rebase on
that. Existing patches do not look abandoned.

Best regards,
Krzysztof

Re: [PATCH 4/6] pwm: adp5585: add adp5585 PWM support

[Uwe Kleine-König]

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

Hello Clark,

On Tue, Jul 16, 2024 at 03:28:27PM -0400, Frank Li wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> Add PWM function support for MFD adp5585.
> 
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pwm/Kconfig       |   8 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-adp5585.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3e53838990f5b..baaadf877b9c6 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -38,6 +38,14 @@ config PWM_DEBUG
>  	  It is expected to introduce some runtime overhead and diagnostic
>  	  output to the kernel log, so only enable while working on a driver.
>  
> +config PWM_ADP5585
> +	tristate "ADP5585 PWM support"
> +	depends on MFD_ADP5585
> +	help
> +	  This option enables support for on-chip PWM found
> +	  on Analog Devices ADP5585.
> +
> +
>  config PWM_AB8500
>  	tristate "AB8500 PWM support"
>  	depends on AB8500_CORE && ARCH_U8500

alphabetic ordering (by config symbol) please.

> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0be4f3e6dd432..161131a261e94 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_PWM)		+= core.o
>  obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
>  obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
> +obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
>  obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
>  obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
>  obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o

alphabetic ordering please.

> diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> new file mode 100644
> index 0000000000000..f578d24df5c74
> --- /dev/null
> +++ b/drivers/pwm/pwm-adp5585.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PWM driver for Analog Devices ADP5585 MFD
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +
> +#define ADP5585_PWM_CHAN_NUM		1

This is only used once. I'd prefer to pass the 1 verbatim to
pwmchip_alloc.

> +#define ADP5585_PWM_FASTEST_PERIOD_NS	2000
> +#define ADP5585_PWM_SLOWEST_PERIOD_NS	131070000

Funny number. I wonder where this comes from.

> +struct adp5585_pwm_chip {
> +	struct device *parent;
> +	struct mutex lock;
> +	u8 pin_config_val;
> +};
> +
> +static inline struct adp5585_pwm_chip *
> +to_adp5585_pwm_chip(struct pwm_chip *chip)
> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static int adp5585_pwm_reg_read(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 *val)
> +{
> +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);

s/  / /;
ditto below in adp5585_pwm_reg_write().

> +
> +	return adp5585->read_reg(adp5585, reg, val);
> +}
> +
> +static int adp5585_pwm_reg_write(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 val)
> +{
> +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
> +
> +	return adp5585->write_reg(adp5585, reg, val);
> +}
> +
> +static int pwm_adp5585_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u32 on, off;
> +	u8 temp;
> +
> +	/* get period */
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_LOW, &temp);
> +	off = temp;
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_HIGH, &temp);
> +	off |= temp << 8;
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_LOW, &temp);
> +	on = temp;
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_HIGH, &temp);
> +	on |= temp << 8;
> +	state->period = (on + off) * NSEC_PER_USEC;
> +
> +	state->duty_cycle = on;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	/* get channel status */
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &temp);
> +	state->enabled = temp & ADP5585_PWM_CFG_EN;
> +
> +	return 0;
> +}
> +
> +static int pwm_adp5585_apply(struct pwm_chip *chip,
> +			     struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u32 on, off;
> +	u8 enabled;
> +	int ret;
> +
> +	if (state->period > ADP5585_PWM_SLOWEST_PERIOD_NS ||
> +	    state->period < ADP5585_PWM_FASTEST_PERIOD_NS)
> +		return -EINVAL;
> +
> +	guard(mutex)(&adp5585_pwm->lock);

What does this protect? You're allowed (and expected) to assume that the
consumer serializes calls to .apply() for a single pwm_device. Given
that you have npwm=1 I think this lock can be dropped.

> +	/* set on/off cycle*/
> +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, NSEC_PER_USEC);
> +	off = DIV_ROUND_CLOSEST_ULL((state->period - state->duty_cycle), NSEC_PER_USEC);

Please enable PWM_DEBUG your tests and make sure it doesn't produce
warnings. (Hint: round_closest is wrong)

> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_LOW, off & ADP5585_REG_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_HIGH,
> +				    (off >> 8) & ADP5585_REG_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_LOW, on & ADP5585_REG_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_HIGH,
> +				    (on >> 8) & ADP5585_REG_MASK);
> +	if (ret)
> +		return ret;

How does the hardware behave in between these register writes? Can it
happen that an intermediate state is visible on the output pin? (E.g.
because off is already written but on is still the old value. Or even
off is only partly written after the first byte write.)

Please document this behaviour in a paragraph at the top of the driver
in the same way as many other PWM drivers do it. The details should be
extractable by

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

> +
> +	/* enable PWM and set to continuous PWM mode*/

Missing space before comment ending delimiter

> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &enabled);
> +	if (state->enabled)
> +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, ADP5585_PWM_CFG_EN);
> +	else
> +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, 0);
> +
> +	return ret;
> +}
> +
> +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u8 reg_cfg;
> +	int ret;
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &adp5585_pwm->pin_config_val);
> +	reg_cfg = adp5585_pwm->pin_config_val & ~ADP5585_PIN_CONFIG_R3_MASK;
> +	reg_cfg |= ADP5585_PIN_CONFIG_R3_PWM;
> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);

ret is only written to here, ditto for &adp5585_pwm->pin_config_val.

> +
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_GENERAL_CFG, &adp5585_pwm->pin_config_val);
> +	reg_cfg |= ADP5585_GENERAL_CFG_OSC_EN;
> +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_GENERAL_CFG, reg_cfg);

Please add a comment about what is happening here. I assume this sets up
pinmuxing and enabled the oscillator. I wonder if it is sensible to do
the latter only in .apply() iff state->enabled = true.

> +
> +	return ret;
> +}
> +
> +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u8 reg_cfg;
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &reg_cfg);
> +	reg_cfg &= ~ADP5585_PIN_CONFIG_R3_MASK;
> +	reg_cfg |= adp5585_pwm->pin_config_val & ADP5585_PIN_CONFIG_R3_MASK;
> +	adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);

It would be consequent to clear ADP5585_GENERAL_CFG_OSC_EN in this
function given that it's set in .request().

> +}
> +
> +static const struct pwm_ops adp5585_pwm_ops = {
> +	.request = pwm_adp5585_request,
> +	.free = pwm_adp5585_free,
> +	.get_state = pwm_adp5585_get_state,
> +	.apply = pwm_adp5585_apply,
> +};
> +
> +static int adp5585_pwm_probe(struct platform_device *pdev)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm;
> +	struct pwm_chip *chip;
> +	unsigned int npwm = ADP5585_PWM_CHAN_NUM;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*adp5585_pwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);

Error message using dev_err_probe() please.

> +
> +	adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	adp5585_pwm->parent = pdev->dev.parent;
> +
> +	platform_set_drvdata(pdev, adp5585_pwm);
> +
> +	chip->ops = &adp5585_pwm_ops;
> +	mutex_init(&adp5585_pwm->lock);
> +
> +	ret = devm_pwmchip_add(&pdev->dev, chip);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);

Please use dev_err_probe().

> +
> +	return ret;
> +}
> +
> +static void adp5585_pwm_remove(struct platform_device *pdev)
> +{
> +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(chip);

Did you test this? I'd expect this to explode.

> +}
> +
> +static const struct of_device_id adp5585_pwm_of_match[] = {
> +	{.compatible = "adp5585-pwm", },

Missing space after the opening brace.

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> +
> +static struct platform_driver adp5585_pwm_driver = {
> +	.driver	= {
> +		.name	= "adp5585-pwm",
> +		.of_match_table = adp5585_pwm_of_match,
> +	},
> +	.probe		= adp5585_pwm_probe,
> +	.remove		= adp5585_pwm_remove,
> +};
> +module_platform_driver(adp5585_pwm_driver);
> +
> +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");

The email address matches one of the S-o-b lines, the name however is
different. Is this by mistake?

> +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> +MODULE_LICENSE("GPL");

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

Re: [PATCH 4/6] pwm: adp5585: add adp5585 PWM support

[Frank Li]

On Wed, Jul 17, 2024 at 11:04:50AM +0200, Uwe Kleine-König wrote:
> Hello Clark,
> 
> On Tue, Jul 16, 2024 at 03:28:27PM -0400, Frank Li wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> > 
> > Add PWM function support for MFD adp5585.
> > 
> > Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Signed-off-by: Jindong Yue <jindong.yue@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pwm/Kconfig       |   8 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-adp5585.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 224 insertions(+)
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 3e53838990f5b..baaadf877b9c6 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -38,6 +38,14 @@ config PWM_DEBUG
> >  	  It is expected to introduce some runtime overhead and diagnostic
> >  	  output to the kernel log, so only enable while working on a driver.
> >  
> > +config PWM_ADP5585
> > +	tristate "ADP5585 PWM support"
> > +	depends on MFD_ADP5585
> > +	help
> > +	  This option enables support for on-chip PWM found
> > +	  on Analog Devices ADP5585.
> > +
> > +
> >  config PWM_AB8500
> >  	tristate "AB8500 PWM support"
> >  	depends on AB8500_CORE && ARCH_U8500
> 
> alphabetic ordering (by config symbol) please.

Thank you for you review. I just found someone already submit similar patch

https://lore.kernel.org/linux-gpio/20240608141633.2562-5-laurent.pinchart@ideasonboard.com/

Let's wait for laurent. If he is busy, I can rework base on the above one.

Frank

> 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 0be4f3e6dd432..161131a261e94 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -2,6 +2,7 @@
> >  obj-$(CONFIG_PWM)		+= core.o
> >  obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
> >  obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
> > +obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
> >  obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
> >  obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
> >  obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
> 
> alphabetic ordering please.
> 
> > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > new file mode 100644
> > index 0000000000000..f578d24df5c74
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-adp5585.c
> > @@ -0,0 +1,215 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * PWM driver for Analog Devices ADP5585 MFD
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +#include <linux/time.h>
> > +
> > +#define ADP5585_PWM_CHAN_NUM		1
> 
> This is only used once. I'd prefer to pass the 1 verbatim to
> pwmchip_alloc.
> 
> > +#define ADP5585_PWM_FASTEST_PERIOD_NS	2000
> > +#define ADP5585_PWM_SLOWEST_PERIOD_NS	131070000
> 
> Funny number. I wonder where this comes from.
> 
> > +struct adp5585_pwm_chip {
> > +	struct device *parent;
> > +	struct mutex lock;
> > +	u8 pin_config_val;
> > +};
> > +
> > +static inline struct adp5585_pwm_chip *
> > +to_adp5585_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return pwmchip_get_drvdata(chip);
> > +}
> > +
> > +static int adp5585_pwm_reg_read(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 *val)
> > +{
> > +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
> 
> s/  / /;
> ditto below in adp5585_pwm_reg_write().
> 
> > +
> > +	return adp5585->read_reg(adp5585, reg, val);
> > +}
> > +
> > +static int adp5585_pwm_reg_write(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 val)
> > +{
> > +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
> > +
> > +	return adp5585->write_reg(adp5585, reg, val);
> > +}
> > +
> > +static int pwm_adp5585_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				 struct pwm_state *state)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	u8 temp;
> > +
> > +	/* get period */
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_LOW, &temp);
> > +	off = temp;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_HIGH, &temp);
> > +	off |= temp << 8;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_LOW, &temp);
> > +	on = temp;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_HIGH, &temp);
> > +	on |= temp << 8;
> > +	state->period = (on + off) * NSEC_PER_USEC;
> > +
> > +	state->duty_cycle = on;
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	/* get channel status */
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &temp);
> > +	state->enabled = temp & ADP5585_PWM_CFG_EN;
> > +
> > +	return 0;
> > +}
> > +
> > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > +			     struct pwm_device *pwm,
> > +			     const struct pwm_state *state)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	u8 enabled;
> > +	int ret;
> > +
> > +	if (state->period > ADP5585_PWM_SLOWEST_PERIOD_NS ||
> > +	    state->period < ADP5585_PWM_FASTEST_PERIOD_NS)
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> 
> What does this protect? You're allowed (and expected) to assume that the
> consumer serializes calls to .apply() for a single pwm_device. Given
> that you have npwm=1 I think this lock can be dropped.
> 
> > +	/* set on/off cycle*/
> > +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, NSEC_PER_USEC);
> > +	off = DIV_ROUND_CLOSEST_ULL((state->period - state->duty_cycle), NSEC_PER_USEC);
> 
> Please enable PWM_DEBUG your tests and make sure it doesn't produce
> warnings. (Hint: round_closest is wrong)
> 
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_LOW, off & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_HIGH,
> > +				    (off >> 8) & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_LOW, on & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_HIGH,
> > +				    (on >> 8) & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> 
> How does the hardware behave in between these register writes? Can it
> happen that an intermediate state is visible on the output pin? (E.g.
> because off is already written but on is still the old value. Or even
> off is only partly written after the first byte write.)
> 
> Please document this behaviour in a paragraph at the top of the driver
> in the same way as many other PWM drivers do it. The details should be
> extractable by
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> > +
> > +	/* enable PWM and set to continuous PWM mode*/
> 
> Missing space before comment ending delimiter
> 
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &enabled);
> > +	if (state->enabled)
> > +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, ADP5585_PWM_CFG_EN);
> > +	else
> > +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u8 reg_cfg;
> > +	int ret;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &adp5585_pwm->pin_config_val);
> > +	reg_cfg = adp5585_pwm->pin_config_val & ~ADP5585_PIN_CONFIG_R3_MASK;
> > +	reg_cfg |= ADP5585_PIN_CONFIG_R3_PWM;
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
> 
> ret is only written to here, ditto for &adp5585_pwm->pin_config_val.
> 
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_GENERAL_CFG, &adp5585_pwm->pin_config_val);
> > +	reg_cfg |= ADP5585_GENERAL_CFG_OSC_EN;
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_GENERAL_CFG, reg_cfg);
> 
> Please add a comment about what is happening here. I assume this sets up
> pinmuxing and enabled the oscillator. I wonder if it is sensible to do
> the latter only in .apply() iff state->enabled = true.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u8 reg_cfg;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &reg_cfg);
> > +	reg_cfg &= ~ADP5585_PIN_CONFIG_R3_MASK;
> > +	reg_cfg |= adp5585_pwm->pin_config_val & ADP5585_PIN_CONFIG_R3_MASK;
> > +	adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
> 
> It would be consequent to clear ADP5585_GENERAL_CFG_OSC_EN in this
> function given that it's set in .request().
> 
> > +}
> > +
> > +static const struct pwm_ops adp5585_pwm_ops = {
> > +	.request = pwm_adp5585_request,
> > +	.free = pwm_adp5585_free,
> > +	.get_state = pwm_adp5585_get_state,
> > +	.apply = pwm_adp5585_apply,
> > +};
> > +
> > +static int adp5585_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm;
> > +	struct pwm_chip *chip;
> > +	unsigned int npwm = ADP5585_PWM_CHAN_NUM;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*adp5585_pwm));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> 
> Error message using dev_err_probe() please.
> 
> > +
> > +	adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	adp5585_pwm->parent = pdev->dev.parent;
> > +
> > +	platform_set_drvdata(pdev, adp5585_pwm);
> > +
> > +	chip->ops = &adp5585_pwm_ops;
> > +	mutex_init(&adp5585_pwm->lock);
> > +
> > +	ret = devm_pwmchip_add(&pdev->dev, chip);
> > +	if (ret)
> > +		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> 
> Please use dev_err_probe().
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> > +
> > +	pwmchip_remove(chip);
> 
> Did you test this? I'd expect this to explode.
> 
> > +}
> > +
> > +static const struct of_device_id adp5585_pwm_of_match[] = {
> > +	{.compatible = "adp5585-pwm", },
> 
> Missing space after the opening brace.
> 
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> > +
> > +static struct platform_driver adp5585_pwm_driver = {
> > +	.driver	= {
> > +		.name	= "adp5585-pwm",
> > +		.of_match_table = adp5585_pwm_of_match,
> > +	},
> > +	.probe		= adp5585_pwm_probe,
> > +	.remove		= adp5585_pwm_remove,
> > +};
> > +module_platform_driver(adp5585_pwm_driver);
> > +
> > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>");
> 
> The email address matches one of the S-o-b lines, the name however is
> different. Is this by mistake?
> 
> > +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> > +MODULE_LICENSE("GPL");

Re: [PATCH 4/6] pwm: adp5585: add adp5585 PWM support

[Fabio Estevam]

On Wed, Jul 17, 2024 at 11:29 AM Frank Li <Frank.li@nxp.com> wrote:

> Thank you for you review. I just found someone already submit similar patch
>
> https://lore.kernel.org/linux-gpio/20240608141633.2562-5-laurent.pinchart@ideasonboard.com/
>
> Let's wait for laurent. If he is busy, I can rework base on the above one.

Adding Laurent.

Re: [PATCH 4/6] pwm: adp5585: add adp5585 PWM support

[Laurent Pinchart]

On Wed, Jul 17, 2024 at 12:35:29PM -0300, Fabio Estevam wrote:
> On Wed, Jul 17, 2024 at 11:29 AM Frank Li <Frank.li@nxp.com> wrote:
> 
> > Thank you for you review. I just found someone already submit similar patch
> >
> > https://lore.kernel.org/linux-gpio/20240608141633.2562-5-laurent.pinchart@ideasonboard.com/
> >
> > Let's wait for laurent. If he is busy, I can rework base on the above one.
> 
> Adding Laurent.

Thanks Fabio.

I had a quick look at this series, and I think mine is better as it
supports more features and more chip variants.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] mfd: adp5585: add ADI adp5585 core support

[Linus Walleij]

Hi Frank,

thanks for your patch!

On Tue, Jul 16, 2024 at 9:29 PM Frank Li <Frank.Li@nxp.com> wrote:

> +struct adp5585_dev {
> +       struct device *dev;
> +       struct i2c_client *i2c_client;
> +
> +       int (*read_reg)(struct adp5585_dev *adp5585, u8 reg, u8 *val);
> +       int (*write_reg)(struct adp5585_dev *adp5585, u8 reg, u8 val);
> +};

The read_reg() and write_reg() accessors looks like a reimplementation
of regmap.

Can you check if you can just let the subdrivers access a regmap
for the parent MFD device and use that to read/write registers?

There are many examples of this in recent MFD drivers.

Yours,
Linus Walleij