[RFC PATCH 0/5] support for Marvell 88PM886 PMIC

[RFC PATCH 0/5] support for Marvell 88PM886 PMIC

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Hello,

the following implements basic support for Marvell's 88PM886 PMIC which
is found for instance as a component of the samsung,coreprimevelte
smartphone which inspired this and also serves as a testing platform.

The code for the MFD is based primarily on this old series [1] with the
addition of poweroff based on the smartphone's downstream kernel tree
[2]. The onkey driver is based on the latter. I am not in possesion of
the datasheet.

The vendor version of this driver includes support for a similar chip:
88PM880. While that is not included here it was written with it in mind
and it should be quite straighforward to add it.

[1] https://lore.kernel.org/all/1434098601-3498-1-git-send-email-yizhang@marvell.com/
[2] https://github.com/CoderCharmander/g361f-kernel

Thank you and kind regards,
K. B.

Karel Balej (5):
  dt-bindings: mfd: add entry for the Marvell 88PM88X PMICs
  mfd: add 88pm88x driver
  dt-bindings: input: add entry for 88pm88x-onkey
  input: add onkey driver for Marvell 88PM88X PMICs
  MAINTAINERS: add myself for Marvell 88PM88X PMICs

 .../bindings/input/marvell,88pm88x-onkey.yaml |  30 +++
 .../bindings/mfd/marvell,88pm88x.yaml         |  59 ++++++
 MAINTAINERS                                   |   9 +
 drivers/input/misc/88pm88x-onkey.c            | 103 +++++++++
 drivers/input/misc/Kconfig                    |  10 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/mfd/88pm88x.c                         | 199 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 include/linux/mfd/88pm88x.h                   |  60 ++++++
 10 files changed, 483 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
 create mode 100644 drivers/input/misc/88pm88x-onkey.c
 create mode 100644 drivers/mfd/88pm88x.c
 create mode 100644 include/linux/mfd/88pm88x.h

-- 
2.43.0

[RFC PATCH 1/5] dt-bindings: mfd: add entry for the Marvell 88PM88X PMICs

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Marvell 88PM880 and 88PM886 are two similar PMICs with mostly matching
register mapping and subdevices such as onkey, regulators or battery and
charger. Both seem to come in two revisions which seem to be handled
slightly differently in some subdevice drivers.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 .../bindings/mfd/marvell,88pm88x.yaml         | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml

diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
new file mode 100644
index 000000000000..e075729c360f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/marvell,88pm88x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell 88PM88X PMIC core MFD
+
+maintainers:
+  - Karel Balej <balejk@matfyz.cz>
+
+description: |
+  Marvell 88PM880 and 88PM886 are two similar PMICs providing
+  several functions such as onkey, regulators or battery and
+  charger. Both seem to come in two revisions -- A0 and A1.
+
+properties:
+  compatible:
+    const: marvell,88pm886-a1
+
+  reg:
+    description: I2C device address
+    maxItems: 1
+
+  interrupt-controller: true
+
+  interrupts:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      pmic0: 88pm886@30 {
+        compatible = "marvell,88pm886-a1";
+        reg = <0x30>;
+        interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-parent = <&gic>;
+        interrupt-controller;
+        #interrupt-cells = <1>;
+      };
+    };
+...
-- 
2.43.0

[RFC PATCH 2/5] mfd: add 88pm88x driver

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Marvell 88PM880 and 8PM886 are two similar PMICs with mostly matching
register mapping. They provide various functions such as onkey, battery,
charger and regulators.

Add support for 88PM886 found for instance in the samsung,coreprimevelte
smartphone with which this was tested. Support for 88PM880 is not
implemented here but should be straightforward to add.

Implement only the most basic support omitting the currently unused
registers and I2C subclients which should thus be added with the
respective subdevices. However, add support for the onkey already.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/mfd/88pm88x.c       | 199 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig         |  11 ++
 drivers/mfd/Makefile        |   1 +
 include/linux/mfd/88pm88x.h |  60 +++++++++++
 4 files changed, 271 insertions(+)
 create mode 100644 drivers/mfd/88pm88x.c
 create mode 100644 include/linux/mfd/88pm88x.h

diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
new file mode 100644
index 000000000000..5db6c65b667d
--- /dev/null
+++ b/drivers/mfd/88pm88x.c
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/mfd/core.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+
+#include <linux/mfd/88pm88x.h>
+
+/* interrupt status registers */
+#define PM88X_REG_INT_STATUS1			0x05
+
+#define PM88X_REG_INT_ENA_1			0x0a
+#define PM88X_INT_ENA1_ONKEY			BIT(0)
+
+enum pm88x_irq_number {
+	PM88X_IRQ_ONKEY,
+
+	PM88X_MAX_IRQ
+};
+
+static struct regmap_irq pm88x_regmap_irqs[] = {
+	REGMAP_IRQ_REG(PM88X_IRQ_ONKEY, 0, PM88X_INT_ENA1_ONKEY),
+};
+
+static struct regmap_irq_chip pm88x_regmap_irq_chip = {
+	.name = "88pm88x",
+	.irqs = pm88x_regmap_irqs,
+	.num_irqs = ARRAY_SIZE(pm88x_regmap_irqs),
+	.num_regs = 4,
+	.status_base = PM88X_REG_INT_STATUS1,
+	.ack_base = PM88X_REG_INT_STATUS1,
+	.unmask_base = PM88X_REG_INT_ENA_1,
+};
+
+static struct reg_sequence pm886_presets[] = {
+	/* disable watchdog */
+	REG_SEQ0(PM88X_REG_WDOG, 0x01),
+	/* GPIO1: DVC, GPIO0: input */
+	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
+	/* GPIO2: input */
+	REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
+	/* DVC2, DVC1 */
+	REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
+	/* GPIO5V_1:input, GPIO5V_2: input */
+	REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
+	/* output 32 kHz from XO */
+	REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
+	/* OSC_FREERUN = 1, to lock FLL */
+	REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
+	/* XO_LJ = 1, enable low jitter for 32 kHz */
+	REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
+	/* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
+	REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
+	/* set the duty cycle of charger DC/DC to max */
+	REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
+};
+
+static struct resource onkey_resources[] = {
+	DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
+};
+
+static struct mfd_cell pm88x_devs[] = {
+	{
+		.name = "88pm88x-onkey",
+		.num_resources = ARRAY_SIZE(onkey_resources),
+		.resources = onkey_resources,
+		.id = -1,
+	},
+};
+
+static struct pm88x_data pm886_a1_data = {
+	.whoami = PM886_A1_WHOAMI,
+	.presets = pm886_presets,
+	.num_presets = ARRAY_SIZE(pm886_presets),
+};
+
+static const struct regmap_config pm88x_i2c_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xfe,
+};
+
+static int pm88x_power_off_handler(struct sys_off_data *data)
+{
+	struct pm88x_chip *chip = data->cb_data;
+	int ret;
+
+	ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG1,
+			PM88X_SW_PDOWN, PM88X_SW_PDOWN);
+	if (ret) {
+		dev_err(&chip->client->dev, "Failed to power off the device: %d\n", ret);
+		return NOTIFY_BAD;
+	}
+	return NOTIFY_DONE;
+}
+
+static int pm88x_setup_irq(struct pm88x_chip *chip)
+{
+	int ret;
+
+	/* set interrupt clearing mode to clear on write */
+	ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG2,
+			PM88X_INT_INV | PM88X_INT_CLEAR | PM88X_INT_MASK_MODE,
+			PM88X_INT_WC);
+	if (ret) {
+		dev_err(&chip->client->dev, "Failed to set interrupt clearing mode: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_regmap_add_irq_chip(&chip->client->dev, chip->regmaps[PM88X_REGMAP_BASE],
+			chip->client->irq, IRQF_ONESHOT, -1, &pm88x_regmap_irq_chip,
+			&chip->irq_data);
+	if (ret) {
+		dev_err(&chip->client->dev, "Failed to request IRQ: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int pm88x_probe(struct i2c_client *client)
+{
+	struct pm88x_chip *chip;
+	int ret = 0;
+	unsigned int chip_id;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->client = client;
+	chip->data = device_get_match_data(&client->dev);
+	i2c_set_clientdata(client, chip);
+
+	device_init_wakeup(&client->dev, 1);
+
+	chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap);
+	if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {
+		ret = PTR_ERR(chip->regmaps[PM88X_REGMAP_BASE]);
+		dev_err(&client->dev, "Failed to initialize regmap: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_read(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_ID, &chip_id);
+	if (ret) {
+		dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
+		return ret;
+	}
+	if (chip->data->whoami != chip_id) {
+		dev_err(&client->dev, "Device reported wrong chip ID: %u\n", chip_id);
+		return -EINVAL;
+	}
+
+	ret = pm88x_setup_irq(chip);
+	if (ret)
+		return ret;
+
+	ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs),
+			NULL, 0, regmap_irq_get_domain(chip->irq_data));
+	if (ret) {
+		dev_err(&client->dev, "Failed to add devices: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_register_patch(chip->regmaps[PM88X_REGMAP_BASE], chip->data->presets,
+			chip->data->num_presets);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register regmap patch: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_register_power_off_handler(&client->dev, pm88x_power_off_handler, chip);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register power off handler: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+const struct of_device_id pm88x_of_match[] = {
+	{ .compatible = "marvell,88pm886-a1", .data = &pm886_a1_data },
+	{ },
+};
+
+static struct i2c_driver pm88x_i2c_driver = {
+	.driver = {
+		.name = "88pm88x",
+		.of_match_table = of_match_ptr(pm88x_of_match),
+	},
+	.probe = pm88x_probe,
+};
+module_i2c_driver(pm88x_i2c_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM88X PMIC driver");
+MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 90ce58fd629e..c593279fd766 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -794,6 +794,17 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_88PM88X
+	bool "Marvell 88PM886"
+	depends on I2C=y
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  This enables support for Marvell 88PM886 Power Management IC.
+	  This includes the I2C driver and the core APIs _only_, you have to
+	  select individual components like onkey under the corresponding menus.
+
 config MFD_MAX14577
 	tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
 	depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..14e42b045c92 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_MFD_88PM860X)	+= 88pm860x.o
 obj-$(CONFIG_MFD_88PM800)	+= 88pm800.o 88pm80x.o
 obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
+obj-$(CONFIG_MFD_88PM88X)	+= 88pm88x.o
 obj-$(CONFIG_MFD_ACT8945A)	+= act8945a.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
new file mode 100644
index 000000000000..a34c57447827
--- /dev/null
+++ b/include/linux/mfd/88pm88x.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_MFD_88PM88X_H
+#define __LINUX_MFD_88PM88X_H
+
+#include <linux/mfd/core.h>
+
+#define PM886_A1_WHOAMI		0xa1
+
+#define PM88X_REG_ID			0x00
+
+#define PM88X_REG_STATUS1		0x01
+#define PM88X_ONKEY_STS1		BIT(0)
+
+#define PM88X_REG_MISC_CONFIG1		0x14
+#define PM88X_SW_PDOWN			BIT(5)
+
+#define PM88X_REG_MISC_CONFIG2		0x15
+#define PM88X_INT_INV			BIT(0)
+#define PM88X_INT_CLEAR			BIT(1)
+#define PM88X_INT_RC			0x00
+#define PM88X_INT_WC			BIT(1)
+#define PM88X_INT_MASK_MODE		BIT(2)
+
+#define PM88X_REG_WDOG			0x1d
+
+#define PM88X_REG_LOWPOWER2		0x21
+#define PM88X_REG_LOWPOWER4		0x23
+
+#define PM88X_REG_GPIO_CTRL1		0x30
+
+#define PM88X_REG_GPIO_CTRL2		0x31
+
+#define PM88X_REG_GPIO_CTRL3		0x32
+
+#define PM88X_REG_GPIO_CTRL4		0x33
+
+#define PM88X_REG_BK_OSC_CTRL1		0x50
+#define PM88X_REG_BK_OSC_CTRL3		0x52
+
+#define PM88X_REG_AON_CTRL2		0xe2
+
+enum pm88x_regmap_index {
+	PM88X_REGMAP_BASE,
+
+	PM88X_REGMAP_NR
+};
+
+struct pm88x_data {
+	unsigned int whoami;
+	struct reg_sequence *presets;
+	unsigned int num_presets;
+};
+
+struct pm88x_chip {
+	struct i2c_client *client;
+	struct regmap_irq_chip_data *irq_data;
+	const struct pm88x_data *data;
+	struct regmap *regmaps[PM88X_REGMAP_NR];
+};
+#endif /* __LINUX_MFD_88PM88X_H */
-- 
2.43.0

[RFC PATCH 3/5] dt-bindings: input: add entry for 88pm88x-onkey

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Marvell 88PM88X PMICs provide onkey functionality. Document it.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 .../bindings/input/marvell,88pm88x-onkey.yaml | 30 +++++++++++++++++++
 .../bindings/mfd/marvell,88pm88x.yaml         |  4 +++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml

diff --git a/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
new file mode 100644
index 000000000000..aeb7673189f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/marvell,88pm88x-onkey.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Onkey driver for Marvell 88PM88X PMICs.
+
+maintainers:
+  - Karel Balej <balejk@matfyz.cz>
+
+description: |
+  This module is part of the 88PM88X MFD device. For more details
+  see Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml.
+
+  The onkey controller is represented as a sub-node of the PMIC node in
+  the device tree.
+
+allOf:
+  - $ref: input.yaml#
+
+properties:
+  compatible:
+    const: marvell,88pm88x-onkey
+
+required:
+  - compatible
+
+additionalProperties: false
+...
diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
index e075729c360f..115b41c9f22c 100644
--- a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
+++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
@@ -50,6 +50,10 @@ examples:
         interrupt-parent = <&gic>;
         interrupt-controller;
         #interrupt-cells = <1>;
+
+        onkey {
+          compatible = "marvell,88pm88x-onkey";
+        };
       };
     };
 ...
-- 
2.43.0

[RFC PATCH 4/5] input: add onkey driver for Marvell 88PM88X PMICs

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

The Marvell 88PM88X PMICs provide onkey among other things. Add client
driver to handle it. The driver currently only provides a basic support
omitting additional functions found in the vendor version, such as long
onkey and GPIO integration.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/input/misc/88pm88x-onkey.c | 103 +++++++++++++++++++++++++++++
 drivers/input/misc/Kconfig         |  10 +++
 drivers/input/misc/Makefile        |   1 +
 3 files changed, 114 insertions(+)
 create mode 100644 drivers/input/misc/88pm88x-onkey.c

diff --git a/drivers/input/misc/88pm88x-onkey.c b/drivers/input/misc/88pm88x-onkey.c
new file mode 100644
index 000000000000..0d6056a3cab2
--- /dev/null
+++ b/drivers/input/misc/88pm88x-onkey.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/regmap.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#include <linux/mfd/88pm88x.h>
+
+struct pm88x_onkey {
+	struct input_dev *idev;
+	struct pm88x_chip *chip;
+	int irq;
+};
+
+static irqreturn_t pm88x_onkey_irq_handler(int irq, void *data)
+{
+	struct pm88x_onkey *onkey = data;
+	unsigned int val;
+	int ret = 0;
+
+	ret = regmap_read(onkey->chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_STATUS1, &val);
+	if (ret) {
+		dev_err(onkey->idev->dev.parent, "Failed to read status: %d\n", ret);
+		return IRQ_NONE;
+	}
+	val &= PM88X_ONKEY_STS1;
+
+	input_report_key(onkey->idev, KEY_POWER, val);
+	input_sync(onkey->idev);
+
+	return IRQ_HANDLED;
+}
+
+static int pm88x_onkey_probe(struct platform_device *pdev)
+{
+	struct pm88x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+	struct pm88x_onkey *onkey;
+	int err;
+
+	onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
+	if (!onkey)
+		return -ENOMEM;
+
+	onkey->chip = chip;
+
+	onkey->irq = platform_get_irq(pdev, 0);
+	if (onkey->irq < 0) {
+		dev_err(&pdev->dev, "Failed to get IRQ\n");
+		return -EINVAL;
+	}
+
+	onkey->idev = devm_input_allocate_device(&pdev->dev);
+	if (!onkey->idev) {
+		dev_err(&pdev->dev, "Failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	onkey->idev->name = "88pm88x-onkey";
+	onkey->idev->phys = "88pm88x-onkey/input0";
+	onkey->idev->id.bustype = BUS_I2C;
+	onkey->idev->dev.parent = &pdev->dev;
+	onkey->idev->evbit[0] = BIT_MASK(EV_KEY);
+	onkey->idev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
+
+	err = devm_request_threaded_irq(&pdev->dev, onkey->irq, NULL, pm88x_onkey_irq_handler,
+			IRQF_ONESHOT | IRQF_NO_SUSPEND, "onkey", onkey);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to request IRQ: %d\n", err);
+		return err;
+	}
+
+	err = input_register_device(onkey->idev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to register input device: %d\n", err);
+		return err;
+	}
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+}
+
+static const struct of_device_id pm88x_onkey_of_match[] = {
+	{ .compatible = "marvell,88pm88x-onkey", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pm88x_onkey_of_match);
+
+static struct platform_driver pm88x_onkey_driver = {
+	.driver = {
+		.name = "88pm88x-onkey",
+		.of_match_table = of_match_ptr(pm88x_onkey_of_match),
+	},
+	.probe = pm88x_onkey_probe,
+};
+module_platform_driver(pm88x_onkey_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM88X onkey driver");
+MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6ba984d7f0b1..fdfa3e23c3cf 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -33,6 +33,16 @@ config INPUT_88PM80X_ONKEY
 	  To compile this driver as a module, choose M here: the module
 	  will be called 88pm80x_onkey.
 
+config INPUT_88PM88X_ONKEY
+	tristate "Marvell 88PM88X onkey support"
+	depends on MFD_88PM88X
+	help
+	  Support the onkey of Marvell 88PM88X PMICs as an input device
+	  reporting power button status.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called 88pm88x-onkey.
+
 config INPUT_AB8500_PONKEY
 	tristate "AB8500 Pon (PowerOn) Key"
 	depends on AB8500_CORE
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 04296a4abe8e..eab7a364188c 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -7,6 +7,7 @@
 
 obj-$(CONFIG_INPUT_88PM860X_ONKEY)	+= 88pm860x_onkey.o
 obj-$(CONFIG_INPUT_88PM80X_ONKEY)	+= 88pm80x_onkey.o
+obj-$(CONFIG_INPUT_88PM88X_ONKEY)	+= 88pm88x-onkey.o
 obj-$(CONFIG_INPUT_AB8500_PONKEY)	+= ab8500-ponkey.o
 obj-$(CONFIG_INPUT_AD714X)		+= ad714x.o
 obj-$(CONFIG_INPUT_AD714X_I2C)		+= ad714x-i2c.o
-- 
2.43.0

[RFC PATCH 5/5] MAINTAINERS: add myself for Marvell 88PM88X PMICs

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Add an entry to MAINTAINERS for the Marvell 88PM88X PMICs MFD and onkey
drivers.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e2c6187a3ac8..eb0171cd2323 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12737,6 +12737,15 @@ F:	drivers/net/dsa/mv88e6xxx/
 F:	include/linux/dsa/mv88e6xxx.h
 F:	include/linux/platform_data/mv88e6xxx.h
 
+MARVELL 88PM88X PMIC MFD DRIVER
+M:	Karel Balej <balejk@matfyz.cz>
+S:	Maintained
+F:	Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
+F:	Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
+F:	drivers/input/misc/88pm88x-onkey.c
+F:	drivers/mfd/88pm88x.c
+F:	include/linux/mfd/88pm88x.h
+
 MARVELL ARMADA 3700 PHY DRIVERS
 M:	Miquel Raynal <miquel.raynal@bootlin.com>
 S:	Maintained
-- 
2.43.0

Re: [RFC PATCH 1/5] dt-bindings: mfd: add entry for the Marvell 88PM88X PMICs

[Rob Herring]

On Sun, 17 Dec 2023 14:16:59 +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> Marvell 88PM880 and 88PM886 are two similar PMICs with mostly matching
> register mapping and subdevices such as onkey, regulators or battery and
> charger. Both seem to come in two revisions which seem to be handled
> slightly differently in some subdevice drivers.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../bindings/mfd/marvell,88pm88x.yaml         | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/mfd/marvell,88pm88x.example.dts:30.31-32 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/mfd/marvell,88pm88x.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231217131838.7569-2-karelb@gimli.ms.mff.cuni.cz

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: [RFC PATCH 3/5] dt-bindings: input: add entry for 88pm88x-onkey

[Rob Herring]

On Sun, 17 Dec 2023 14:17:01 +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> Marvell 88PM88X PMICs provide onkey functionality. Document it.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../bindings/input/marvell,88pm88x-onkey.yaml | 30 +++++++++++++++++++
>  .../bindings/mfd/marvell,88pm88x.yaml         |  4 +++
>  2 files changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231217131838.7569-4-karelb@gimli.ms.mff.cuni.cz

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: [RFC PATCH 1/5] dt-bindings: mfd: add entry for the Marvell 88PM88X PMICs

[Rob Herring]

On Sun, Dec 17, 2023 at 02:16:59PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> Marvell 88PM880 and 88PM886 are two similar PMICs with mostly matching
> register mapping and subdevices such as onkey, regulators or battery and
> charger. Both seem to come in two revisions which seem to be handled
> slightly differently in some subdevice drivers.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../bindings/mfd/marvell,88pm88x.yaml         | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> new file mode 100644
> index 000000000000..e075729c360f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/marvell,88pm88x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell 88PM88X PMIC core MFD

Drop 'MFD'.

> +
> +maintainers:
> +  - Karel Balej <balejk@matfyz.cz>
> +
> +description: |

Don't need '|' as there is no formatting to preserve.

> +  Marvell 88PM880 and 88PM886 are two similar PMICs providing
> +  several functions such as onkey, regulators or battery and
> +  charger. Both seem to come in two revisions -- A0 and A1.
> +
> +properties:
> +  compatible:
> +    const: marvell,88pm886-a1

The description talks about 4 different devices, but only 1 here. 

Do you expect to need A0 support? Devices with these PMICs should be 
known and few, right? 

> +
> +  reg:
> +    description: I2C device address

Drop.

> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      pmic0: 88pm886@30 {

pmic@30

Drop the unused label.

> +        compatible = "marvell,88pm886-a1";
> +        reg = <0x30>;
> +        interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;

You need the header for this.

You'll find the input binding fails too. Please test your bindings 
before sending.

> +        interrupt-parent = <&gic>;
> +        interrupt-controller;
> +        #interrupt-cells = <1>;
> +      };
> +    };
> +...
> -- 
> 2.43.0
> 

Re: [RFC PATCH 3/5] dt-bindings: input: add entry for 88pm88x-onkey

[Rob Herring]

On Sun, Dec 17, 2023 at 02:17:01PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> Marvell 88PM88X PMICs provide onkey functionality. Document it.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../bindings/input/marvell,88pm88x-onkey.yaml | 30 +++++++++++++++++++
>  .../bindings/mfd/marvell,88pm88x.yaml         |  4 +++
>  2 files changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> new file mode 100644
> index 000000000000..aeb7673189f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/marvell,88pm88x-onkey.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Onkey driver for Marvell 88PM88X PMICs.
> +
> +maintainers:
> +  - Karel Balej <balejk@matfyz.cz>
> +
> +description: |
> +  This module is part of the 88PM88X MFD device. For more details
> +  see Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml.
> +
> +  The onkey controller is represented as a sub-node of the PMIC node in
> +  the device tree.
> +
> +allOf:
> +  - $ref: input.yaml#
> +
> +properties:
> +  compatible:
> +    const: marvell,88pm88x-onkey
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +...
> diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> index e075729c360f..115b41c9f22c 100644
> --- a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> +++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> @@ -50,6 +50,10 @@ examples:
>          interrupt-parent = <&gic>;
>          interrupt-controller;
>          #interrupt-cells = <1>;
> +
> +        onkey {
> +          compatible = "marvell,88pm88x-onkey";
> +        };

Why do you need this? You have no properties for it. The parent driver 
can instantiate child drivers. You don't need a DT node for that.

Rob

Re: [RFC PATCH 4/5] input: add onkey driver for Marvell 88PM88X PMICs

[Dmitry Torokhov]

Hi Karel,

On Sun, Dec 17, 2023 at 02:17:02PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> The Marvell 88PM88X PMICs provide onkey among other things. Add client
> driver to handle it. The driver currently only provides a basic support
> omitting additional functions found in the vendor version, such as long
> onkey and GPIO integration.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  drivers/input/misc/88pm88x-onkey.c | 103 +++++++++++++++++++++++++++++
>  drivers/input/misc/Kconfig         |  10 +++
>  drivers/input/misc/Makefile        |   1 +
>  3 files changed, 114 insertions(+)
>  create mode 100644 drivers/input/misc/88pm88x-onkey.c
> 
> diff --git a/drivers/input/misc/88pm88x-onkey.c b/drivers/input/misc/88pm88x-onkey.c
> new file mode 100644
> index 000000000000..0d6056a3cab2
> --- /dev/null
> +++ b/drivers/input/misc/88pm88x-onkey.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/regmap.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>

Please sort alphabetically.

> +
> +#include <linux/mfd/88pm88x.h>
> +
> +struct pm88x_onkey {
> +	struct input_dev *idev;
> +	struct pm88x_chip *chip;
> +	int irq;

Since you are using devm to request interrupt I do not think you need to
store it here.

> +};
> +
> +static irqreturn_t pm88x_onkey_irq_handler(int irq, void *data)
> +{
> +	struct pm88x_onkey *onkey = data;
> +	unsigned int val;
> +	int ret = 0;

"error". Also no need to initialize it to 0.

> +
> +	ret = regmap_read(onkey->chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_STATUS1, &val);

I still prefer to keep withing 80 columns, unless longer lines are a
clear win.

> +	if (ret) {
> +		dev_err(onkey->idev->dev.parent, "Failed to read status: %d\n", ret);

Maybe have "dev" in onkey instead of poking through input?

> +		return IRQ_NONE;
> +	}
> +	val &= PM88X_ONKEY_STS1;
> +
> +	input_report_key(onkey->idev, KEY_POWER, val);
> +	input_sync(onkey->idev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int pm88x_onkey_probe(struct platform_device *pdev)
> +{
> +	struct pm88x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> +	struct pm88x_onkey *onkey;
> +	int err;
> +
> +	onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
> +	if (!onkey)
> +		return -ENOMEM;
> +
> +	onkey->chip = chip;
> +
> +	onkey->irq = platform_get_irq(pdev, 0);
> +	if (onkey->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get IRQ\n");
> +		return -EINVAL;

Do not clobber the return from platform_get_irq(). So "return
onkey->irq" (or simply irq if you drop it from onkey and use temporary).

> +	}
> +
> +	onkey->idev = devm_input_allocate_device(&pdev->dev);
> +	if (!onkey->idev) {
> +		dev_err(&pdev->dev, "Failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	onkey->idev->name = "88pm88x-onkey";
> +	onkey->idev->phys = "88pm88x-onkey/input0";
> +	onkey->idev->id.bustype = BUS_I2C;
> +	onkey->idev->dev.parent = &pdev->dev;

No need to do this since you are using devm_input_allocate_device().

> +	onkey->idev->evbit[0] = BIT_MASK(EV_KEY);
> +	onkey->idev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);

Please use

	input_set_capability(onkey->idev, EV_KEY, KEY_POWER);

> +
> +	err = devm_request_threaded_irq(&pdev->dev, onkey->irq, NULL, pm88x_onkey_irq_handler,
> +			IRQF_ONESHOT | IRQF_NO_SUSPEND, "onkey", onkey);

Please align arguments.

> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to request IRQ: %d\n", err);

I was persuaded to stop my crusade against dev_err_probe() so you may
use it here.

> +		return err;
> +	}
> +
> +	err = input_register_device(onkey->idev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);

Are there cases where we woudl not want wakeup? Maybe use standard
"wakeup-source" property?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pm88x_onkey_of_match[] = {
> +	{ .compatible = "marvell,88pm88x-onkey", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, pm88x_onkey_of_match);
> +
> +static struct platform_driver pm88x_onkey_driver = {
> +	.driver = {
> +		.name = "88pm88x-onkey",
> +		.of_match_table = of_match_ptr(pm88x_onkey_of_match),

Given that you are not guarding pm88x_onkey_of_match definition with
#ifdef CONFIG_OF you shoudl not use of_match_ptr().

Thanks.

-- 
Dmitry

Re: [RFC PATCH 1/5] dt-bindings: mfd: add entry for the Marvell 88PM88X PMICs

[Karel Balej]

Rob,

thank you very much for your feedback.

On Mon Dec 18, 2023 at 4:17 PM CET, Rob Herring wrote:
> > +  Marvell 88PM880 and 88PM886 are two similar PMICs providing
> > +  several functions such as onkey, regulators or battery and
> > +  charger. Both seem to come in two revisions -- A0 and A1.
> > +
> > +properties:
> > +  compatible:
> > +    const: marvell,88pm886-a1
>
> The description talks about 4 different devices, but only 1 here. 
>
> Do you expect to need A0 support? Devices with these PMICs should be 
> known and few, right? 

I know of three smartphones which have 88PM886 and all of them (at least
the revisions that have been tested) seem to use A1. So no, I don't know
of any device that would need A0, but I wanted have the driver ready in
case somebody needed to add it later. What change do you then suggest?

Thank you and kind regards,
K. B.

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Lee Jones]

On Sun, 17 Dec 2023, Karel Balej wrote:

> From: Karel Balej <balejk@matfyz.cz>
> 
> Marvell 88PM880 and 8PM886 are two similar PMICs with mostly matching
> register mapping. They provide various functions such as onkey, battery,
> charger and regulators.
> 
> Add support for 88PM886 found for instance in the samsung,coreprimevelte
> smartphone with which this was tested. Support for 88PM880 is not
> implemented here but should be straightforward to add.
> 
> Implement only the most basic support omitting the currently unused
> registers and I2C subclients which should thus be added with the
> respective subdevices. However, add support for the onkey already.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  drivers/mfd/88pm88x.c       | 199 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/Kconfig         |  11 ++
>  drivers/mfd/Makefile        |   1 +
>  include/linux/mfd/88pm88x.h |  60 +++++++++++
>  4 files changed, 271 insertions(+)
>  create mode 100644 drivers/mfd/88pm88x.c
>  create mode 100644 include/linux/mfd/88pm88x.h
> 
> diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> new file mode 100644
> index 000000000000..5db6c65b667d
> --- /dev/null
> +++ b/drivers/mfd/88pm88x.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/core.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>

Alphabetical

> +#include <linux/mfd/88pm88x.h>
> +
> +/* interrupt status registers */

Use correct grammar in comments, including capital letters.

 - Applies throughout

The comment is not required - we can see what they are from the
nomenclature.

> +#define PM88X_REG_INT_STATUS1			0x05
> +
> +#define PM88X_REG_INT_ENA_1			0x0a
> +#define PM88X_INT_ENA1_ONKEY			BIT(0)
> +
> +enum pm88x_irq_number {
> +	PM88X_IRQ_ONKEY,
> +
> +	PM88X_MAX_IRQ
> +};

An enum for a single IRQ?

> +static struct regmap_irq pm88x_regmap_irqs[] = {
> +	REGMAP_IRQ_REG(PM88X_IRQ_ONKEY, 0, PM88X_INT_ENA1_ONKEY),
> +};
> +
> +static struct regmap_irq_chip pm88x_regmap_irq_chip = {
> +	.name = "88pm88x",
> +	.irqs = pm88x_regmap_irqs,
> +	.num_irqs = ARRAY_SIZE(pm88x_regmap_irqs),
> +	.num_regs = 4,
> +	.status_base = PM88X_REG_INT_STATUS1,
> +	.ack_base = PM88X_REG_INT_STATUS1,
> +	.unmask_base = PM88X_REG_INT_ENA_1,
> +};
> +
> +static struct reg_sequence pm886_presets[] = {
> +	/* disable watchdog */
> +	REG_SEQ0(PM88X_REG_WDOG, 0x01),

Easier to read if you place spaces between them.

> +	/* GPIO1: DVC, GPIO0: input */
> +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),

Shouldn't you set these up using Pintrl?

> +	/* GPIO2: input */
> +	REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> +	/* DVC2, DVC1 */

Please unify all of the comments.

They all use a different structure.

> +	REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> +	/* GPIO5V_1:input, GPIO5V_2: input */
> +	REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> +	/* output 32 kHz from XO */
> +	REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> +	/* OSC_FREERUN = 1, to lock FLL */
> +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> +	/* XO_LJ = 1, enable low jitter for 32 kHz */
> +	REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> +	/* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
> +	REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> +	/* set the duty cycle of charger DC/DC to max */
> +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),

These all looks like they should be handled in their respective drivers?

"patch"ing these in seems like a hack.

> +};

Why this instead of 
> +static struct resource onkey_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> +};
> +
> +static struct mfd_cell pm88x_devs[] = {
> +	{
> +		.name = "88pm88x-onkey",
> +		.num_resources = ARRAY_SIZE(onkey_resources),
> +		.resources = onkey_resources,
> +		.id = -1,
> +	},
> +};

It's not an MFD if it only supports a single device.

> +static struct pm88x_data pm886_a1_data = {
> +	.whoami = PM886_A1_WHOAMI,
> +	.presets = pm886_presets,
> +	.num_presets = ARRAY_SIZE(pm886_presets),
> +};

Just pass the device ID through DT's .data, then match on that instead
of passing pointer to random data structures.

> +static const struct regmap_config pm88x_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xfe,

Define this please.

> +};
> +
> +static int pm88x_power_off_handler(struct sys_off_data *data)

'data' is a terrible variable name.  Please change throughout.

> +{
> +	struct pm88x_chip *chip = data->cb_data;
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG1,
> +			PM88X_SW_PDOWN, PM88X_SW_PDOWN);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to power off the device: %d\n", ret);
> +		return NOTIFY_BAD;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
> +static int pm88x_setup_irq(struct pm88x_chip *chip)
> +{
> +	int ret;
> +
> +	/* set interrupt clearing mode to clear on write */
> +	ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG2,
> +			PM88X_INT_INV | PM88X_INT_CLEAR | PM88X_INT_MASK_MODE,
> +			PM88X_INT_WC);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to set interrupt clearing mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&chip->client->dev, chip->regmaps[PM88X_REGMAP_BASE],
> +			chip->client->irq, IRQF_ONESHOT, -1, &pm88x_regmap_irq_chip,
> +			&chip->irq_data);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to request IRQ: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pm88x_probe(struct i2c_client *client)
> +{
> +	struct pm88x_chip *chip;
> +	int ret = 0;
> +	unsigned int chip_id;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->client = client;
> +	chip->data = device_get_match_data(&client->dev);

Now where is this being past to?

What is going to consume this?

> +	i2c_set_clientdata(client, chip);
> +
> +	device_init_wakeup(&client->dev, 1);
> +
> +	chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap);
> +	if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {

Just define different regmaps if you really need them.

I only see one being used anyway.

> +		ret = PTR_ERR(chip->regmaps[PM88X_REGMAP_BASE]);
> +		dev_err(&client->dev, "Failed to initialize regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_ID, &chip_id);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
> +		return ret;
> +	}
> +	if (chip->data->whoami != chip_id) {
> +		dev_err(&client->dev, "Device reported wrong chip ID: %u\n", chip_id);

Use dev_err_probe() throughout.

> +		return -EINVAL;
> +	}
> +
> +	ret = pm88x_setup_irq(chip);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs),
> +			NULL, 0, regmap_irq_get_domain(chip->irq_data));
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to add devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_register_patch(chip->regmaps[PM88X_REGMAP_BASE], chip->data->presets,
> +			chip->data->num_presets);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register regmap patch: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_register_power_off_handler(&client->dev, pm88x_power_off_handler, chip);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register power off handler: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct of_device_id pm88x_of_match[] = {
> +	{ .compatible = "marvell,88pm886-a1", .data = &pm886_a1_data },
> +	{ },
> +};
> +
> +static struct i2c_driver pm88x_i2c_driver = {
> +	.driver = {
> +		.name = "88pm88x",
> +		.of_match_table = of_match_ptr(pm88x_of_match),
> +	},
> +	.probe = pm88x_probe,
> +};
> +module_i2c_driver(pm88x_i2c_driver);
> +
> +MODULE_DESCRIPTION("Marvell 88PM88X PMIC driver");
> +MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 90ce58fd629e..c593279fd766 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -794,6 +794,17 @@ config MFD_88PM860X
>  	  select individual components like voltage regulators, RTC and
>  	  battery-charger under the corresponding menus.
>  
> +config MFD_88PM88X

"MFD_88PM88X_PMIC"?

> +	bool "Marvell 88PM886"

"Marvell 88PM886 PMIC"?

> +	depends on I2C=y
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	select MFD_CORE
> +	help
> +	  This enables support for Marvell 88PM886 Power Management IC.
> +	  This includes the I2C driver and the core APIs _only_, you have to
> +	  select individual components like onkey under the corresponding menus.
> +
>  config MFD_MAX14577
>  	tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..14e42b045c92 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_MFD_88PM860X)	+= 88pm860x.o
>  obj-$(CONFIG_MFD_88PM800)	+= 88pm800.o 88pm80x.o
>  obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
> +obj-$(CONFIG_MFD_88PM88X)	+= 88pm88x.o
>  obj-$(CONFIG_MFD_ACT8945A)	+= act8945a.o
>  obj-$(CONFIG_MFD_SM501)		+= sm501.o
>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
> diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> new file mode 100644
> index 000000000000..a34c57447827
> --- /dev/null
> +++ b/include/linux/mfd/88pm88x.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_MFD_88PM88X_H
> +#define __LINUX_MFD_88PM88X_H

Drop the LINUX part.

> +
> +#include <linux/mfd/core.h>
> +
> +#define PM886_A1_WHOAMI		0xa1

s/WHOAMI/CHIP_ID/

> +#define PM88X_REG_ID			0x00
> +
> +#define PM88X_REG_STATUS1		0x01
> +#define PM88X_ONKEY_STS1		BIT(0)
> +
> +#define PM88X_REG_MISC_CONFIG1		0x14
> +#define PM88X_SW_PDOWN			BIT(5)
> +
> +#define PM88X_REG_MISC_CONFIG2		0x15
> +#define PM88X_INT_INV			BIT(0)
> +#define PM88X_INT_CLEAR			BIT(1)
> +#define PM88X_INT_RC			0x00
> +#define PM88X_INT_WC			BIT(1)
> +#define PM88X_INT_MASK_MODE		BIT(2)
> +
> +#define PM88X_REG_WDOG			0x1d
> +
> +#define PM88X_REG_LOWPOWER2		0x21
> +#define PM88X_REG_LOWPOWER4		0x23
> +
> +#define PM88X_REG_GPIO_CTRL1		0x30

These don't really need to be spaced out, do they?

> +#define PM88X_REG_GPIO_CTRL2		0x31
> +
> +#define PM88X_REG_GPIO_CTRL3		0x32
> +
> +#define PM88X_REG_GPIO_CTRL4		0x33
> +
> +#define PM88X_REG_BK_OSC_CTRL1		0x50
> +#define PM88X_REG_BK_OSC_CTRL3		0x52
> +
> +#define PM88X_REG_AON_CTRL2		0xe2
> +
> +enum pm88x_regmap_index {
> +	PM88X_REGMAP_BASE,
> +
> +	PM88X_REGMAP_NR
> +};
> +
> +struct pm88x_data {
> +	unsigned int whoami;
> +	struct reg_sequence *presets;
> +	unsigned int num_presets;
> +};
> +
> +struct pm88x_chip {
> +	struct i2c_client *client;
> +	struct regmap_irq_chip_data *irq_data;

Group this with the other regmap related member(s).

What are you using this for?

> +	const struct pm88x_data *data;

Remove this.

> +	struct regmap *regmaps[PM88X_REGMAP_NR];
> +};
> +#endif /* __LINUX_MFD_88PM88X_H */
> -- 
> 2.43.0
> 

-- 
9)
4)
Lee Jones [李琼斯]

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Karel Balej]

Lee,

thank you for your feedback.

On Thu Jan 25, 2024 at 1:26 PM CET, Lee Jones wrote:

[...]

> > +#define PM88X_REG_INT_STATUS1			0x05
> > +
> > +#define PM88X_REG_INT_ENA_1			0x0a
> > +#define PM88X_INT_ENA1_ONKEY			BIT(0)
> > +
> > +enum pm88x_irq_number {
> > +	PM88X_IRQ_ONKEY,
> > +
> > +	PM88X_MAX_IRQ
> > +};
>
> An enum for a single IRQ?

There will be a lot more IRQs but I have only added this one so far as
it is the only one used by this series -- is that OK?

> > +static struct reg_sequence pm886_presets[] = {
> > +	/* disable watchdog */
> > +	REG_SEQ0(PM88X_REG_WDOG, 0x01),
>
> Easier to read if you place spaces between them.
>
> > +	/* GPIO1: DVC, GPIO0: input */
> > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
>
> Shouldn't you set these up using Pintrl?

You mean to add a new MFD cell for the pins and write the respective
driver? The downstream implementation has no such thing so I'm not sure
if I would be able to do that from scratch.

> > +	/* GPIO2: input */
> > +	REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> > +	/* DVC2, DVC1 */
>
> Please unify all of the comments.
>
> They all use a different structure.
>
> > +	REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> > +	/* GPIO5V_1:input, GPIO5V_2: input */
> > +	REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> > +	/* output 32 kHz from XO */
> > +	REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> > +	/* OSC_FREERUN = 1, to lock FLL */
> > +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> > +	/* XO_LJ = 1, enable low jitter for 32 kHz */
> > +	REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> > +	/* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
> > +	REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> > +	/* set the duty cycle of charger DC/DC to max */
> > +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
>
> These all looks like they should be handled in their respective drivers?
>
> "patch"ing these in seems like a hack.

To be honest, I don't really know why these are required and what effect
they have -- the comments above taken from the downstream version are
the only thing I have to go by. I might try removing them to see if
there is any noticable change and whether they could be added only later
with the respective drivers.

>
> > +};
>
> Why this instead of 

What are you refering to here please?

> > +static struct resource onkey_resources[] = {
> > +	DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> > +};
> > +
> > +static struct mfd_cell pm88x_devs[] = {
> > +	{
> > +		.name = "88pm88x-onkey",
> > +		.num_resources = ARRAY_SIZE(onkey_resources),
> > +		.resources = onkey_resources,
> > +		.id = -1,
> > +	},
> > +};
>
> It's not an MFD if it only supports a single device.

As I have noted above with respect to the IRQ enum and also in the
commit message, I have so far only added the parts which there is
already use for. I intend to add the other parts along with the
respective subdevice drivers, please see my regulator series [1] for an
example.

I thought this approach would make for shorter and simpler patches and
also would allow me to make more informed decisions as I familiarize
myself with the downstream subdevice drivers more closely one by one.

> > +	i2c_set_clientdata(client, chip);
> > +
> > +	device_init_wakeup(&client->dev, 1);
> > +
> > +	chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap);
> > +	if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {
>
> Just define different regmaps if you really need them.

You mean not to use an array of regmaps but add new struct members
instead? One for each regmap?

> > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > new file mode 100644
> > index 000000000000..a34c57447827
> > --- /dev/null
> > +++ b/include/linux/mfd/88pm88x.h

[...]

> > +#define PM88X_REG_ID			0x00
> > +
> > +#define PM88X_REG_STATUS1		0x01
> > +#define PM88X_ONKEY_STS1		BIT(0)
> > +
> > +#define PM88X_REG_MISC_CONFIG1		0x14
> > +#define PM88X_SW_PDOWN			BIT(5)
> > +
> > +#define PM88X_REG_MISC_CONFIG2		0x15
> > +#define PM88X_INT_INV			BIT(0)
> > +#define PM88X_INT_CLEAR			BIT(1)
> > +#define PM88X_INT_RC			0x00
> > +#define PM88X_INT_WC			BIT(1)
> > +#define PM88X_INT_MASK_MODE		BIT(2)
> > +
> > +#define PM88X_REG_WDOG			0x1d
> > +
> > +#define PM88X_REG_LOWPOWER2		0x21
> > +#define PM88X_REG_LOWPOWER4		0x23
> > +
> > +#define PM88X_REG_GPIO_CTRL1		0x30
>
> These don't really need to be spaced out, do they?

I have spaced them out already as I expect to add some related
definitions to each of these in the future and thought it would then
perhaps be more easily readable like this.

[1] https://lore.kernel.org/all/20231228100208.2932-1-karelb@gimli.ms.mff.cuni.cz/

Kind regards,
K. B.

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Lee Jones]

On Sun, 28 Jan 2024, Karel Balej wrote:
> > > +	/* GPIO1: DVC, GPIO0: input */
> > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> >
> > Shouldn't you set these up using Pintrl?
> 
> You mean to add a new MFD cell for the pins and write the respective
> driver? The downstream implementation has no such thing so I'm not sure
> if I would be able to do that from scratch.

This is not a Pinctrl driver.

Isn't there a generic API you can use?

> > > +	/* GPIO2: input */
> > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> > > +	/* DVC2, DVC1 */
> >
> > Please unify all of the comments.
> >
> > They all use a different structure.
> >
> > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> > > +	/* GPIO5V_1:input, GPIO5V_2: input */
> > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> > > +	/* output 32 kHz from XO */
> > > +	REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> > > +	/* OSC_FREERUN = 1, to lock FLL */
> > > +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> > > +	/* XO_LJ = 1, enable low jitter for 32 kHz */
> > > +	REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> > > +	/* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
> > > +	REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> > > +	/* set the duty cycle of charger DC/DC to max */
> > > +	REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
> >
> > These all looks like they should be handled in their respective drivers?
> >
> > "patch"ing these in seems like a hack.
> 
> To be honest, I don't really know why these are required and what effect
> they have -- the comments above taken from the downstream version are
> the only thing I have to go by. I might try removing them to see if
> there is any noticable change and whether they could be added only later
> with the respective drivers.

If you don't know that they are or what they do and you haven't tested
them, they should not be submitted upstream.

> > > +static struct mfd_cell pm88x_devs[] = {
> > > +	{
> > > +		.name = "88pm88x-onkey",
> > > +		.num_resources = ARRAY_SIZE(onkey_resources),
> > > +		.resources = onkey_resources,
> > > +		.id = -1,
> > > +	},
> > > +};
> >
> > It's not an MFD if it only supports a single device.
> 
> As I have noted above with respect to the IRQ enum and also in the
> commit message, I have so far only added the parts which there is
> already use for. I intend to add the other parts along with the
> respective subdevice drivers, please see my regulator series [1] for an
> example.
> 
> I thought this approach would make for shorter and simpler patches and
> also would allow me to make more informed decisions as I familiarize
> myself with the downstream subdevice drivers more closely one by one.

One device doesn't warrant an MFD.  Please add more devices.

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Karel Balej]

Lee Jones, 2024-01-31T11:03:11+00:00:
> On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > +	/* GPIO1: DVC, GPIO0: input */
> > > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > >
> > > Shouldn't you set these up using Pintrl?
> > 
> > You mean to add a new MFD cell for the pins and write the respective
> > driver? The downstream implementation has no such thing so I'm not sure
> > if I would be able to do that from scratch.
>
> This is not a Pinctrl driver.
>
> Isn't there a generic API you can use?

I'm sorry, I don't think I understand what you mean.

Thank you,
K. B.

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Lee Jones]

On Thu, 01 Feb 2024, Karel Balej wrote:

> Lee Jones, 2024-01-31T11:03:11+00:00:
> > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > +	/* GPIO1: DVC, GPIO0: input */
> > > > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > > >
> > > > Shouldn't you set these up using Pintrl?
> > > 
> > > You mean to add a new MFD cell for the pins and write the respective
> > > driver? The downstream implementation has no such thing so I'm not sure
> > > if I would be able to do that from scratch.
> >
> > This is not a Pinctrl driver.
> >
> > Isn't there a generic API you can use?
> 
> I'm sorry, I don't think I understand what you mean.

Perhaps I misunderstand the code.  It looks like this regmap patch hack
is configuring pins and a bunch of other things.  Would that be a
correct assessment?

If so, where do we draw the line here?  Do we accept a 1000 line driver
which configures a large SoC with a bunch of bespoke register writes?

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Karel Balej]

Lee Jones, 2024-02-02T12:45:50+00:00:
> On Thu, 01 Feb 2024, Karel Balej wrote:
>
> > Lee Jones, 2024-01-31T11:03:11+00:00:
> > > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > > +	/* GPIO1: DVC, GPIO0: input */
> > > > > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
> > > > >
> > > > > Shouldn't you set these up using Pintrl?
> > > > 
> > > > You mean to add a new MFD cell for the pins and write the respective
> > > > driver? The downstream implementation has no such thing so I'm not sure
> > > > if I would be able to do that from scratch.
> > >
> > > This is not a Pinctrl driver.
> > >
> > > Isn't there a generic API you can use?
> > 
> > I'm sorry, I don't think I understand what you mean.
>
> Perhaps I misunderstand the code.  It looks like this regmap patch hack
> is configuring pins and a bunch of other things.  Would that be a
> correct assessment?

Yes, that sounds correct.

> If so, where do we draw the line here?  Do we accept a 1000 line driver
> which configures a large SoC with a bunch of bespoke register writes?

I understand, I just don't know what you mean by "a generic API". I'm
also not clear on whether what you have in mind is simply adding a
dedicated driver for the pins as a new subdevice of this MFD.

Thanks,
K. B.

Re: [RFC PATCH 2/5] mfd: add 88pm88x driver

[Lee Jones]

Linus,

On Fri, 02 Feb 2024, Karel Balej wrote:

> Lee Jones, 2024-02-02T12:45:50+00:00:
> > On Thu, 01 Feb 2024, Karel Balej wrote:
> >
> > > Lee Jones, 2024-01-31T11:03:11+00:00:
> > > > On Sun, 28 Jan 2024, Karel Balej wrote:
> > > > > > > +	/* GPIO1: DVC, GPIO0: input */
> > > > > > > +	REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),

Do we have a precedent for drivers setting up their own pins like this?

> > > > > > Shouldn't you set these up using Pintrl?
> > > > > 
> > > > > You mean to add a new MFD cell for the pins and write the respective
> > > > > driver? The downstream implementation has no such thing so I'm not sure
> > > > > if I would be able to do that from scratch.
> > > >
> > > > This is not a Pinctrl driver.
> > > >
> > > > Isn't there a generic API you can use?
> > > 
> > > I'm sorry, I don't think I understand what you mean.
> >
> > Perhaps I misunderstand the code.  It looks like this regmap patch hack
> > is configuring pins and a bunch of other things.  Would that be a
> > correct assessment?
> 
> Yes, that sounds correct.
> 
> > If so, where do we draw the line here?  Do we accept a 1000 line driver
> > which configures a large SoC with a bunch of bespoke register writes?
> 
> I understand, I just don't know what you mean by "a generic API". I'm
> also not clear on whether what you have in mind is simply adding a
> dedicated driver for the pins as a new subdevice of this MFD.

-- 
Lee Jones [李琼斯]