[PATCH v2 00/14] arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

[PATCH v2 00/14] arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

[Johan Hovold]

The Qualcomm PM8008 PMIC is a so called QPNP PMIC with seven LDO
regulators, a temperature alarm block and two GPIO pins (which are also
used for interrupt signalling and reset).

Unlike previous QPNP PMICs it uses an I2C rather than SPMI interface,
which has implications for how interrupts are handled.

A previous attempt by Qualcomm to upstream support for PM8008 stalled
two years ago at version 15 after a lot of back and forth discussion on
how best to describe this device in the devicetree. [1]

After reviewing the backstory on this and surveying the current SPMI
PMIC bindings and implementation, I opted for a new approach that does
not describe internal details like register offsets and interrupts in
the devicetree.

The original decision to include registers offsets and internal
interrupts for SPMI PMICs has led to a number of PMIC dtsi being created
to avoid copying lots of boiler plate declarations. This in turn causes
trouble when the PMIC USID address is configurable as the address is
included in every interrupt specifier.

The current SPMI bindings still do not describe the devices fully and
additional data is therefore already provided by drivers (e.g.
additional register blocks, supplies, additional interrupt specifiers).

The fact that PMICs which use two USIDs (addresses) are modelled as two
separate devices causes trouble, for example, when there are
dependencies between subfunctions. [2]

Subfunctions also do not necessarily map neatly onto the 256-register
block partitioning of the SPMI register space, something which has lead
to unresolved inconsistencies in how functions like PWM are described.
[3]

In short, it's a bit of a mess.

With the new style of bindings, by contrast, only essential information
that actually differs between machines would be included in the
devicetree. The bindings would also be mostly decoupled from the
implementation, which has started to leak out into the binding (e.g. how
the QPNP interrupts are handled). This also allows for extending the
implementation without having to update the binding, which is especially
important as Qualcomm does not publish any documentation (e.g. to later
enable regulator over-current protection).

Some PMICs support both I2C and SPMI interfaces (e.g. PM8010) and we
want to be able to reuse the same bindings regardless of the interface.

As a proof concept I have written a new pmc8280 driver for one of the
SPMI PMICs in the Lenovo ThinkPad X13s that uses the new style of
bindings and I've been using that one to control backlight and
peripheral regulators for a while now. Specifically, the gpio and
temperature-alarm blocks can be used with some minor updates to the
current drivers.

That work still needs a bit of polish before posting, but my working PoC
means that I'm confident enough that the new model will work and that we
can go ahead and merge regulator support for the PM8008.

This series is specifically needed for the camera sensors in the X13s,
for which camera subsystem (camss) support has now been merged for 6.10.

The first seven patches are preparatory and can possibly be merged
separately from the rest of the series. The next two patches drop the
broken GPIO support for PM8008 which had already been upstreamed. The
last five patches rework the binding and MFD driver, add support for the
regulators and enable the camera PMIC on the X13s.

Johan

[1] https://lore.kernel.org/all/1655200111-18357-1-git-send-email-quic_c_skakit@quicinc.com
[2] https://lore.kernel.org/lkml/20231003152927.15000-3-johan+linaro@kernel.org
[3] https://lore.kernel.org/r/20220828132648.3624126-3-bryan.odonoghue@linaro.org


Changes in v2
 - use IRQ_TYPE_SENSE_MASK in regmap_irq table
 - add post-reset delay
 - reorder pinctrl binding and driver update
 - split out binding cleanups
 - use platform_device_id matching
 - replace underscore in supply names with dash
 - use more fine-grained includes in regulator driver
 - rework regulator driver and update authorship


Johan Hovold (14):
  dt-bindings: mfd: pm8008: add reset gpio
  mfd: pm8008: fix regmap irq chip initialisation
  mfd: pm8008: deassert reset on probe
  mfd: pm8008: mark regmap structures as const
  mfd: pm8008: use lower case hex notation
  mfd: pm8008: rename irq chip
  mfd: pm8008: drop unused driver data
  pinctrl: qcom: spmi-gpio: drop broken pm8008 support
  dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008
  dt-bindings: mfd: pm8008: drop redundant descriptions
  dt-bindings: mfd: pm8008: rework binding
  mfd: pm8008: rework driver
  regulator: add pm8008 pmic regulator driver
  arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

 .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 158 ++++++++------
 .../bindings/pinctrl/qcom,pmic-gpio.yaml      |   3 -
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 123 +++++++++++
 drivers/mfd/Kconfig                           |   1 +
 drivers/mfd/qcom-pm8008.c                     | 169 ++++++++++-----
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c      |   1 -
 drivers/regulator/Kconfig                     |   7 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/qcom-pm8008-regulator.c     | 198 ++++++++++++++++++
 include/dt-bindings/mfd/qcom-pm8008.h         |  19 --
 10 files changed, 542 insertions(+), 138 deletions(-)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
 delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h

-- 
2.44.1

[PATCH v2 01/14] dt-bindings: mfd: pm8008: add reset gpio

[Johan Hovold]

Describe the optional reset gpio (which may not be wired up).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index 0c75d8bde568..e1e05921afb4 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -29,6 +29,9 @@ properties:
 
     description: Parent interrupt.
 
+  reset-gpios:
+    maxItems: 1
+
   "#interrupt-cells":
     const: 2
 
@@ -97,6 +100,7 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/mfd/qcom-pm8008.h>
     #include <dt-bindings/interrupt-controller/irq.h>
 
@@ -115,6 +119,8 @@ examples:
         interrupt-parent = <&tlmm>;
         interrupts = <32 IRQ_TYPE_EDGE_RISING>;
 
+        reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
+
         pm8008_gpios: gpio@c000 {
           compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
           reg = <0xc000>;
-- 
2.44.1

[PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

[Johan Hovold]

The regmap irq array is potentially shared between multiple PMICs and
should only contain static data.

Use a custom macro to initialise also the type fields and drop the
unnecessary updates on each probe.

Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 41 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 3ac3742f438b..f71c490f25c8 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
 	INT_POL_LOW_OFFSET,
 };
 
-static struct regmap_irq pm8008_irqs[] = {
-	REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO,	PM8008_MISC,	BIT(0)),
-	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO,	PM8008_MISC,	BIT(1)),
-	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2,	PM8008_MISC,	BIT(2)),
-	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3,	PM8008_MISC,	BIT(3)),
-	REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP,	PM8008_MISC,	BIT(4)),
-	REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM,	PM8008_TEMP_ALARM, BIT(0)),
-	REGMAP_IRQ_REG(PM8008_IRQ_GPIO1,	PM8008_GPIO1,	BIT(0)),
-	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
+#define _IRQ(_irq, _off, _mask, _types)			\
+	[_irq] = {					\
+		.reg_offset = (_off),			\
+		.mask = (_mask),			\
+		.type = {				\
+			.type_reg_offset = (_off),	\
+			.types_supported = (_types),	\
+		},					\
+	}
+
+static const struct regmap_irq pm8008_irqs[] = {
+	_IRQ(PM8008_IRQ_MISC_UVLO,    PM8008_MISC,	BIT(0), IRQ_TYPE_EDGE_RISING),
+	_IRQ(PM8008_IRQ_MISC_OVLO,    PM8008_MISC,	BIT(1), IRQ_TYPE_EDGE_RISING),
+	_IRQ(PM8008_IRQ_MISC_OTST2,   PM8008_MISC,	BIT(2), IRQ_TYPE_EDGE_RISING),
+	_IRQ(PM8008_IRQ_MISC_OTST3,   PM8008_MISC,	BIT(3), IRQ_TYPE_EDGE_RISING),
+	_IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC,	BIT(4), IRQ_TYPE_EDGE_RISING),
+	_IRQ(PM8008_IRQ_TEMP_ALARM,   PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
+	_IRQ(PM8008_IRQ_GPIO1,	      PM8008_GPIO1,	BIT(0), IRQ_TYPE_SENSE_MASK),
+	_IRQ(PM8008_IRQ_GPIO2,	      PM8008_GPIO2,	BIT(0), IRQ_TYPE_SENSE_MASK),
 };
 
 static const unsigned int pm8008_periph_base[] = {
@@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
 	.max_register	= 0xFFFF,
 };
 
-static int pm8008_probe_irq_peripherals(struct device *dev,
-					struct regmap *regmap,
-					int client_irq)
-{
-	int rc, i;
-	struct regmap_irq_type *type;
-	struct regmap_irq_chip_data *irq_data;
-
-	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
-		type = &pm8008_irqs[i].type;
-
-		type->type_reg_offset = pm8008_irqs[i].reg_offset;
-
-		if (type->type_reg_offset == PM8008_MISC)
-			type->types_supported = IRQ_TYPE_EDGE_RISING;
-		else
-			type->types_supported = (IRQ_TYPE_EDGE_BOTH |
-				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
-	}
-
-	rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
-			IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
-	if (rc) {
-		dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
-		return rc;
-	}
-
-	return 0;
-}
-
 static int pm8008_probe(struct i2c_client *client)
 {
+	struct regmap_irq_chip_data *irq_data;
 	int rc;
 	struct device *dev;
 	struct regmap *regmap;
@@ -187,9 +168,10 @@ static int pm8008_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, regmap);
 
 	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
-		rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
+		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
+				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
 		if (rc)
-			dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
+			dev_err(dev, "failed to add IRQ chip: %d\n", rc);
 	}
 
 	return devm_of_platform_populate(dev);
-- 
2.44.1

[PATCH v2 03/14] mfd: pm8008: deassert reset on probe

[Johan Hovold]

Request and deassert any (optional) reset gpio during probe in case it
has been left asserted by the boot firmware.

Note the reset line is not asserted to avoid reverting to the default
I2C address in case the firmware has configured an alternate address.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index f71c490f25c8..5a77155a63d7 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -156,6 +157,7 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
 static int pm8008_probe(struct i2c_client *client)
 {
 	struct regmap_irq_chip_data *irq_data;
+	struct gpio_desc *reset;
 	int rc;
 	struct device *dev;
 	struct regmap *regmap;
@@ -167,6 +169,16 @@ static int pm8008_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, regmap);
 
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	/*
+	 * The PMIC does not appear to require a post-reset delay, but wait
+	 * for a millisecond for now anyway.
+	 */
+	usleep_range(1000, 2000);
+
 	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
 		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
 				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
-- 
2.44.1

[PATCH v2 04/14] mfd: pm8008: mark regmap structures as const

[Johan Hovold]

The regmap irq chip structures can be const so mark them as such.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 5a77155a63d7..ab55d524c27b 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -51,7 +51,7 @@ enum {
 	POLARITY_LO_INDEX,
 };
 
-static unsigned int pm8008_config_regs[] = {
+static const unsigned int pm8008_config_regs[] = {
 	INT_SET_TYPE_OFFSET,
 	INT_POL_HIGH_OFFSET,
 	INT_POL_LOW_OFFSET,
@@ -129,7 +129,7 @@ static int pm8008_set_type_config(unsigned int **buf, unsigned int type,
 	return 0;
 }
 
-static struct regmap_irq_chip pm8008_irq_chip = {
+static const struct regmap_irq_chip pm8008_irq_chip = {
 	.name			= "pm8008_irq",
 	.main_status		= I2C_INTR_STATUS_BASE,
 	.num_main_regs		= 1,
-- 
2.44.1

[PATCH v2 05/14] mfd: pm8008: use lower case hex notation

[Johan Hovold]

Use lower case hex notation for consistency.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index ab55d524c27b..18173c7a7e7b 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -38,8 +38,8 @@ enum {
 
 #define PM8008_PERIPH_0_BASE	0x900
 #define PM8008_PERIPH_1_BASE	0x2400
-#define PM8008_PERIPH_2_BASE	0xC000
-#define PM8008_PERIPH_3_BASE	0xC100
+#define PM8008_PERIPH_2_BASE	0xc000
+#define PM8008_PERIPH_3_BASE	0xc100
 
 #define PM8008_TEMP_ALARM_ADDR	PM8008_PERIPH_1_BASE
 #define PM8008_GPIO1_ADDR	PM8008_PERIPH_2_BASE
@@ -151,7 +151,7 @@ static const struct regmap_irq_chip pm8008_irq_chip = {
 static struct regmap_config qcom_mfd_regmap_cfg = {
 	.reg_bits	= 16,
 	.val_bits	= 8,
-	.max_register	= 0xFFFF,
+	.max_register	= 0xffff,
 };
 
 static int pm8008_probe(struct i2c_client *client)
-- 
2.44.1

[PATCH v2 06/14] mfd: pm8008: rename irq chip

[Johan Hovold]

Drop the redundant "irq" suffix from the irq chip name.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 18173c7a7e7b..bab17417aeec 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -130,7 +130,7 @@ static int pm8008_set_type_config(unsigned int **buf, unsigned int type,
 }
 
 static const struct regmap_irq_chip pm8008_irq_chip = {
-	.name			= "pm8008_irq",
+	.name			= "pm8008",
 	.main_status		= I2C_INTR_STATUS_BASE,
 	.num_main_regs		= 1,
 	.irqs			= pm8008_irqs,
-- 
2.44.1

[PATCH v2 07/14] mfd: pm8008: drop unused driver data

[Johan Hovold]

The i2c client driver data pointer has never been used so drop the
unnecessary assignment.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index bab17417aeec..72199840231e 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -167,8 +167,6 @@ static int pm8008_probe(struct i2c_client *client)
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-	i2c_set_clientdata(client, regmap);
-
 	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
-- 
2.44.1

[PATCH v2 08/14] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Johan Hovold]

The SPMI GPIO driver assumes that the parent device is an SPMI device
and accesses random data when backcasting the parent struct device
pointer for non-SPMI devices.

Fortunately this does not seem to cause any issues currently when the
parent device is an I2C client like the PM8008, but this could change if
the structures are reorganised (e.g. using structure randomisation).

Notably the interrupt implementation is also broken for non-SPMI devices.

Also note that the two GPIO pins on PM8008 are used for interrupts and
reset so their practical use should be limited.

Drop the broken GPIO support for PM8008 for now.

Fixes: ea119e5a482a ("pinctrl: qcom-pmic-gpio: Add support for pm8008")
Cc: stable@vger.kernel.org	# 5.13
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index f4e2c88a7c82..e61be7d05494 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -1206,7 +1206,6 @@ static const struct of_device_id pmic_gpio_of_match[] = {
 	{ .compatible = "qcom,pm7325-gpio", .data = (void *) 10 },
 	{ .compatible = "qcom,pm7550ba-gpio", .data = (void *) 8},
 	{ .compatible = "qcom,pm8005-gpio", .data = (void *) 4 },
-	{ .compatible = "qcom,pm8008-gpio", .data = (void *) 2 },
 	{ .compatible = "qcom,pm8019-gpio", .data = (void *) 6 },
 	/* pm8150 has 10 GPIOs with holes on 2, 5, 7 and 8 */
 	{ .compatible = "qcom,pm8150-gpio", .data = (void *) 10 },
-- 
2.44.1

[PATCH v2 09/14] dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008

[Johan Hovold]

The binding for PM8008 is being reworked so that internal details like
interrupts and register offsets are no longer described. This
specifically also involves dropping the gpio child node and its
compatible string which is no longer needed.

Note that there are currently no users of the upstream binding and
driver.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
index 3f8ad07c7cfd..58807212a601 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml
@@ -28,7 +28,6 @@ properties:
           - qcom,pm7325-gpio
           - qcom,pm7550ba-gpio
           - qcom,pm8005-gpio
-          - qcom,pm8008-gpio
           - qcom,pm8018-gpio
           - qcom,pm8019-gpio
           - qcom,pm8038-gpio
@@ -122,7 +121,6 @@ allOf:
         compatible:
           contains:
             enum:
-              - qcom,pm8008-gpio
               - qcom,pmi8950-gpio
               - qcom,pmr735d-gpio
     then:
@@ -421,7 +419,6 @@ $defs:
                  - gpio1-gpio10 for pm7325
                  - gpio1-gpio8 for pm7550ba
                  - gpio1-gpio4 for pm8005
-                 - gpio1-gpio2 for pm8008
                  - gpio1-gpio6 for pm8018
                  - gpio1-gpio12 for pm8038
                  - gpio1-gpio40 for pm8058
-- 
2.44.1

[PATCH v2 10/14] dt-bindings: mfd: pm8008: drop redundant descriptions

[Johan Hovold]

In preparation for reworking the binding, drop the redundant
descriptions of the standard 'reg' and 'interrupts' properties.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index e1e05921afb4..d71657f488db 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -19,16 +19,11 @@ properties:
     const: qcom,pm8008
 
   reg:
-    description:
-      I2C slave address.
-
     maxItems: 1
 
   interrupts:
     maxItems: 1
 
-    description: Parent interrupt.
-
   reset-gpios:
     maxItems: 1
 
-- 
2.44.1

[PATCH v2 11/14] dt-bindings: mfd: pm8008: rework binding

[Johan Hovold]

Rework the pm8008 binding by dropping internal details like register
offsets and interrupts and by adding the missing regulator and
temperature alarm properties.

Note that child nodes are still used for pinctrl and regulator
configuration.

Also note that the pinctrl state definition will be extended later and
could eventually also be shared with other PMICs (e.g. by breaking out
bits of qcom,pmic-gpio.yaml).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
 1 file changed, 90 insertions(+), 59 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index d71657f488db..ccf472e7f552 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -27,103 +27,134 @@ properties:
   reset-gpios:
     maxItems: 1
 
-  "#interrupt-cells":
+  vdd-l1-l2-supply: true
+  vdd-l3-l4-supply: true
+  vdd-l5-supply: true
+  vdd-l6-supply: true
+  vdd-l7-supply: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
     const: 2
 
-    description: |
-      The first cell is the IRQ number, the second cell is the IRQ trigger
-      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
+  gpio-ranges:
+    maxItems: 1
 
   interrupt-controller: true
 
-  "#address-cells":
-    const: 1
+  "#interrupt-cells":
+    const: 2
 
-  "#size-cells":
+  "#thermal-sensor-cells":
     const: 0
 
-patternProperties:
-  "^gpio@[0-9a-f]+$":
+  pinctrl:
     type: object
+    additionalProperties: false
+    patternProperties:
+      "-state$":
+        type: object
+        $ref: "#/$defs/qcom-pm8008-pinctrl-state"
+        unevaluatedProperties: false
 
-    description: |
-      The GPIO peripheral. This node may be specified twice, one for each GPIO.
-
-    properties:
-      compatible:
-        items:
-          - const: qcom,pm8008-gpio
-          - const: qcom,spmi-gpio
+  regulators:
+    type: object
+    additionalProperties: false
+    patternProperties:
+      "^ldo[1-7]$":
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+        unevaluatedProperties: false
 
-      reg:
-        description: Peripheral address of one of the two GPIO peripherals.
-        maxItems: 1
+required:
+  - compatible
+  - reg
+  - interrupts
+  - vdd-l1-l2-supply
+  - vdd-l3-l4-supply
+  - vdd-l5-supply
+  - vdd-l6-supply
+  - vdd-l7-supply
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+  - interrupt-controller
+  - "#interrupt-cells"
+  - "#thermal-sensor-cells"
 
-      gpio-controller: true
+additionalProperties: false
 
-      gpio-ranges:
-        maxItems: 1
+$defs:
+  qcom-pm8008-pinctrl-state:
+    type: object
 
-      interrupt-controller: true
+    allOf:
+      - $ref: /schemas/pinctrl/pinmux-node.yaml
+      - $ref: /schemas/pinctrl/pincfg-node.yaml
 
-      "#interrupt-cells":
-        const: 2
+    properties:
+      pins:
+        items:
+          pattern: "^gpio[12]$"
 
-      "#gpio-cells":
-        const: 2
+      function:
+        items:
+          - enum:
+              - normal
 
     required:
-      - compatible
-      - reg
-      - gpio-controller
-      - interrupt-controller
-      - "#gpio-cells"
-      - gpio-ranges
-      - "#interrupt-cells"
+      - pins
+      - function
 
     additionalProperties: false
 
-required:
-  - compatible
-  - reg
-  - interrupts
-  - "#address-cells"
-  - "#size-cells"
-  - "#interrupt-cells"
-
-additionalProperties: false
-
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
-    #include <dt-bindings/mfd/qcom-pm8008.h>
     #include <dt-bindings/interrupt-controller/irq.h>
 
     i2c {
       #address-cells = <1>;
       #size-cells = <0>;
 
-      pmic@8 {
+      pm8008: pmic@8 {
         compatible = "qcom,pm8008";
         reg = <0x8>;
-        #address-cells = <1>;
-        #size-cells = <0>;
-        interrupt-controller;
-        #interrupt-cells = <2>;
 
         interrupt-parent = <&tlmm>;
         interrupts = <32 IRQ_TYPE_EDGE_RISING>;
 
         reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
 
-        pm8008_gpios: gpio@c000 {
-          compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
-          reg = <0xc000>;
-          gpio-controller;
-          gpio-ranges = <&pm8008_gpios 0 0 2>;
-          #gpio-cells = <2>;
-          interrupt-controller;
-          #interrupt-cells = <2>;
+        vdd-l1-l2-supply = <&vreg_s8b_1p2>;
+        vdd-l3-l4-supply = <&vreg_s1b_1p8>;
+        vdd-l5-supply = <&vreg_bob>;
+        vdd-l6-supply = <&vreg_bob>;
+        vdd-l7-supply = <&vreg_bob>;
+
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&pm8008 0 0 2>;
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+
+        #thermal-sensor-cells = <0>;
+
+        pinctrl {
+          gpio-keys-state {
+            pins = "gpio1";
+            function = "normal";
+          };
+        };
+
+        regulators {
+          ldo1 {
+            regulator-name = "vreg_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+          };
         };
       };
     };
-- 
2.44.1

[PATCH v2 12/14] mfd: pm8008: rework driver

[Johan Hovold]

Rework the pm8008 driver to match the new binding which no longer
describes internal details like interrupts and register offsets
(including which of the two consecutive I2C addresses the registers
belong to).

Instead make the interrupt controller implementation internal and pass
interrupts to the subdrivers using MFD cell resources.

Note that subdrivers may either get their resources, like register block
offsets, from the parent MFD or this can be included in the subdrivers
directly.

In the current implementation, the temperature alarm driver is generic
enough to just get its base address and alarm interrupt from the parent
driver, which already uses this information to implement the interrupt
controller.

The regulator driver, however, needs additional information like parent
supplies and regulator characteristics so in that case it is easier to
just augment its table with the regulator register base addresses.

Similarly, the current GPIO driver already holds the number of pins and
that lookup table can therefore also be extended with register offsets.

Note that subdrivers can now access the two regmaps by name, even if the
primary regmap is registered last so that it is returned by default when
no name is provided in lookups.

Finally, note that the temperature alarm and GPIO subdrivers need some
minor rework before they can be used with non-SPMI devices like the
PM8008. The temperature alarm MFD cell name specifically uses a "qpnp"
rather than "spmi" prefix to prevent binding until the driver has been
updated.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/Kconfig                   |  1 +
 drivers/mfd/qcom-pm8008.c             | 97 +++++++++++++++++++++++----
 include/dt-bindings/mfd/qcom-pm8008.h | 19 ------
 3 files changed, 86 insertions(+), 31 deletions(-)
 delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..bfcb68c62b07 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2208,6 +2208,7 @@ config MFD_ACER_A500_EC
 config MFD_QCOM_PM8008
 	tristate "QCOM PM8008 Power Management IC"
 	depends on I2C && OF
+	select MFD_CORE
 	select REGMAP_I2C
 	select REGMAP_IRQ
 	help
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 72199840231e..246b5fe9819d 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -7,8 +7,10 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/ioport.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -16,8 +18,6 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-#include <dt-bindings/mfd/qcom-pm8008.h>
-
 #define I2C_INTR_STATUS_BASE		0x0550
 #define INT_RT_STS_OFFSET		0x10
 #define INT_SET_TYPE_OFFSET		0x11
@@ -45,6 +45,16 @@ enum {
 #define PM8008_GPIO1_ADDR	PM8008_PERIPH_2_BASE
 #define PM8008_GPIO2_ADDR	PM8008_PERIPH_3_BASE
 
+/* PM8008 IRQ numbers */
+#define PM8008_IRQ_MISC_UVLO	0
+#define PM8008_IRQ_MISC_OVLO	1
+#define PM8008_IRQ_MISC_OTST2	2
+#define PM8008_IRQ_MISC_OTST3	3
+#define PM8008_IRQ_MISC_LDO_OCP	4
+#define PM8008_IRQ_TEMP_ALARM	5
+#define PM8008_IRQ_GPIO1	6
+#define PM8008_IRQ_GPIO2	7
+
 enum {
 	SET_TYPE_INDEX,
 	POLARITY_HI_INDEX,
@@ -148,21 +158,65 @@ static const struct regmap_irq_chip pm8008_irq_chip = {
 	.get_irq_reg		= pm8008_get_irq_reg,
 };
 
-static struct regmap_config qcom_mfd_regmap_cfg = {
+static const struct regmap_config qcom_mfd_regmap_cfg = {
+	.name		= "primary",
+	.reg_bits	= 16,
+	.val_bits	= 8,
+	.max_register	= 0xffff,
+};
+
+static const struct regmap_config pm8008_regmap_cfg_2 = {
+	.name		= "secondary",
 	.reg_bits	= 16,
 	.val_bits	= 8,
 	.max_register	= 0xffff,
 };
 
+static const struct resource pm8008_temp_res[] = {
+	DEFINE_RES_MEM(PM8008_TEMP_ALARM_ADDR, 0x100),
+	DEFINE_RES_IRQ(PM8008_IRQ_TEMP_ALARM),
+};
+
+static const struct mfd_cell pm8008_cells[] = {
+	MFD_CELL_NAME("pm8008-regulator"),
+	MFD_CELL_RES("qpnp-temp-alarm", pm8008_temp_res),
+	MFD_CELL_NAME("pm8008-gpio"),
+};
+
+static void devm_irq_domain_fwnode_release(void *data)
+{
+	struct fwnode_handle *fwnode = data;
+
+	irq_domain_free_fwnode(fwnode);
+}
+
 static int pm8008_probe(struct i2c_client *client)
 {
 	struct regmap_irq_chip_data *irq_data;
+	struct device *dev = &client->dev;
+	struct regmap *regmap, *regmap2;
+	struct fwnode_handle *fwnode;
+	struct i2c_client *dummy;
 	struct gpio_desc *reset;
-	int rc;
-	struct device *dev;
-	struct regmap *regmap;
+	char *name;
+	int ret;
+
+	dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1);
+	if (IS_ERR(dummy)) {
+		ret = PTR_ERR(dummy);
+		dev_err(dev, "failed to claim second address: %d\n", ret);
+		return ret;
+	}
+
+	regmap2 = devm_regmap_init_i2c(dummy, &qcom_mfd_regmap_cfg);
+	if (IS_ERR(regmap2))
+		return PTR_ERR(regmap2);
 
-	dev = &client->dev;
+	ret = regmap_attach_dev(dev, regmap2, &pm8008_regmap_cfg_2);
+	if (ret)
+		return ret;
+
+	/* Default regmap must be attached last. */
 	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
@@ -177,14 +231,33 @@ static int pm8008_probe(struct i2c_client *client)
 	 */
 	usleep_range(1000, 2000);
 
-	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
-		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
+	name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
+	if (!name)
+		return -ENOMEM;
+
+	name = strreplace(name, '/', ':');
+
+	fwnode = irq_domain_alloc_named_fwnode(name);
+	if (!fwnode)
+		return -ENOMEM;
+
+	ret = devm_add_action_or_reset(dev, devm_irq_domain_fwnode_release, fwnode);
+	if (ret)
+		return ret;
+
+	ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq,
 				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
-		if (rc)
-			dev_err(dev, "failed to add IRQ chip: %d\n", rc);
+	if (ret) {
+		dev_err(dev, "failed to add IRQ chip: %d\n", ret);
+		return ret;
 	}
 
-	return devm_of_platform_populate(dev);
+	/* Needed by GPIO driver. */
+	dev_set_drvdata(dev, regmap_irq_get_domain(irq_data));
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, pm8008_cells,
+				ARRAY_SIZE(pm8008_cells), NULL, 0,
+				regmap_irq_get_domain(irq_data));
 }
 
 static const struct of_device_id pm8008_match[] = {
diff --git a/include/dt-bindings/mfd/qcom-pm8008.h b/include/dt-bindings/mfd/qcom-pm8008.h
deleted file mode 100644
index eca9448df228..000000000000
--- a/include/dt-bindings/mfd/qcom-pm8008.h
+++ /dev/null
@@ -1,19 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (c) 2021 The Linux Foundation. All rights reserved.
- */
-
-#ifndef __DT_BINDINGS_MFD_QCOM_PM8008_H
-#define __DT_BINDINGS_MFD_QCOM_PM8008_H
-
-/* PM8008 IRQ numbers */
-#define PM8008_IRQ_MISC_UVLO	0
-#define PM8008_IRQ_MISC_OVLO	1
-#define PM8008_IRQ_MISC_OTST2	2
-#define PM8008_IRQ_MISC_OTST3	3
-#define PM8008_IRQ_MISC_LDO_OCP	4
-#define PM8008_IRQ_TEMP_ALARM	5
-#define PM8008_IRQ_GPIO1	6
-#define PM8008_IRQ_GPIO2	7
-
-#endif
-- 
2.44.1

[PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Johan Hovold]

The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO
regulators.

The driver is based on a driver submitted by Satya Priya, but it has
been cleaned up and reworked to match the new devicetree binding which
no longer describes each regulator as a separate device.

This avoids describing internal details like register offsets in the
devicetree and allows for extending the implementation with features
like over-current protection without having to update the binding.

Specifically note that the regulator interrupts are shared between all
regulators.

Note that the secondary regmap is looked up by name and that if the
driver ever needs to be generalised to support regulators provided by
the primary regmap (I2C address) such information could be added to the
device-id table.

This also fixes the original implementation, which looked up regulators
by 'regulator-name' property rather than devicetree node name and which
prevented the regulators from being named to match board schematics.

Link: https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
Cc: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/regulator/Kconfig                 |   7 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-pm8008-regulator.c | 198 ++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7db0a29b5b8d..546533735be8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1027,6 +1027,13 @@ config REGULATOR_PWM
 	  This driver supports PWM controlled voltage regulators. PWM
 	  duty cycle can increase or decrease the voltage.
 
+config REGULATOR_QCOM_PM8008
+	tristate "Qualcomm PM8008 PMIC regulators"
+	depends on MFD_QCOM_PM8008
+	help
+	  Select this option to enable support for the voltage regulators in
+	  Qualcomm PM8008 PMICs.
+
 config REGULATOR_QCOM_REFGEN
 	tristate "Qualcomm REFGEN regulator driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 46fb569e6be8..0457b0925b3e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -112,6 +112,7 @@ obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
 obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_REFGEN) += qcom-refgen-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
new file mode 100644
index 000000000000..da017c1969d0
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2024 Linaro Limited
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#include <asm/byteorder.h>
+
+#define DEFAULT_VOLTAGE_STEPPER_RATE	38400
+
+#define LDO_STEPPER_CTL_REG		0x3b
+#define STEP_RATE_MASK			GENMASK(1, 0)
+
+#define LDO_VSET_LB_REG			0x40
+
+#define LDO_ENABLE_REG			0x46
+#define ENABLE_BIT			BIT(7)
+
+struct pm8008_regulator {
+	struct regmap		*regmap;
+	struct regulator_desc	desc;
+	unsigned int		base;
+};
+
+struct pm8008_regulator_data {
+	const char			*name;
+	const char			*supply_name;
+	unsigned int			base;
+	int				min_dropout_uV;
+	const struct linear_range	*voltage_range;
+};
+
+static const struct linear_range nldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
+};
+
+static const struct linear_range pldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
+};
+
+static const struct pm8008_regulator_data pm8008_reg_data[] = {
+	{ "ldo1", "vdd-l1-l2", 0x4000, 225000, nldo_ranges, },
+	{ "ldo2", "vdd-l1-l2", 0x4100, 225000, nldo_ranges, },
+	{ "ldo3", "vdd-l3-l4", 0x4200, 300000, pldo_ranges, },
+	{ "ldo4", "vdd-l3-l4", 0x4300, 300000, pldo_ranges, },
+	{ "ldo5", "vdd-l5",    0x4400, 200000, pldo_ranges, },
+	{ "ldo6", "vdd-l6",    0x4500, 200000, pldo_ranges, },
+	{ "ldo7", "vdd-l7",    0x4600, 200000, pldo_ranges, },
+};
+
+static int pm8008_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
+{
+	struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
+	unsigned int mV;
+	__le16 val;
+	int ret;
+
+	ret = regulator_list_voltage_linear_range(rdev, sel);
+	if (ret < 0)
+		return ret;
+
+	mV = DIV_ROUND_UP(ret, 1000);
+
+	val = cpu_to_le16(mV);
+
+	ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG,
+			&val, sizeof(val));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int pm8008_regulator_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
+	unsigned int uV;
+	__le16 val;
+	int ret;
+
+	ret = regmap_bulk_read(preg->regmap, preg->base + LDO_VSET_LB_REG,
+			&val, sizeof(val));
+	if (ret < 0)
+		return ret;
+
+	uV = le16_to_cpu(val) * 1000;
+
+	return (uV - preg->desc.min_uV) / preg->desc.uV_step;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.set_voltage_sel	= pm8008_regulator_set_voltage_sel,
+	.get_voltage_sel	= pm8008_regulator_get_voltage_sel,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+	const struct pm8008_regulator_data *data;
+	struct regulator_config config = {};
+	struct device *dev = &pdev->dev;
+	struct pm8008_regulator *preg;
+	struct regulator_desc *desc;
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+	unsigned int val;
+	int ret, i;
+
+	regmap = dev_get_regmap(dev->parent, "secondary");
+	if (!regmap)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(pm8008_reg_data); i++) {
+		data = &pm8008_reg_data[i];
+
+		preg = devm_kzalloc(dev, sizeof(*preg), GFP_KERNEL);
+		if (!preg)
+			return -ENOMEM;
+
+		preg->regmap = regmap;
+		preg->base = data->base;
+
+		desc = &preg->desc;
+
+		desc->name = data->name;
+		desc->supply_name = data->supply_name;
+		desc->of_match = data->name;
+		desc->regulators_node = of_match_ptr("regulators");
+		desc->ops = &pm8008_regulator_ops;
+		desc->type = REGULATOR_VOLTAGE;
+		desc->owner = THIS_MODULE;
+
+		desc->linear_ranges = data->voltage_range;
+		desc->n_linear_ranges = 1;
+		desc->uV_step = desc->linear_ranges[0].step;
+		desc->min_uV = desc->linear_ranges[0].min;
+		desc->n_voltages = linear_range_values_in_range(&desc->linear_ranges[0]);
+
+		ret = regmap_read(regmap, preg->base + LDO_STEPPER_CTL_REG, &val);
+		if (ret < 0) {
+			dev_err(dev, "failed to read step rate: %d\n", ret);
+			return ret;
+		}
+		val &= STEP_RATE_MASK;
+		desc->ramp_delay = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
+
+		desc->min_dropout_uV = data->min_dropout_uV;
+
+		desc->enable_reg = preg->base + LDO_ENABLE_REG;
+		desc->enable_mask = ENABLE_BIT;
+
+		config.dev = dev->parent;
+		config.driver_data = preg;
+		config.regmap = regmap;
+
+		rdev = devm_regulator_register(dev, desc, &config);
+		if (IS_ERR(rdev)) {
+			ret = PTR_ERR(rdev);
+			dev_err(dev, "failed to register regulator %s: %d\n",
+					desc->name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id pm8008_regulator_id_table[] = {
+	{ "pm8008-regulator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, pm8008_regulator_id_table);
+
+static struct platform_driver pm8008_regulator_driver = {
+	.driver	= {
+		.name = "qcom-pm8008-regulator",
+	},
+	.probe = pm8008_regulator_probe,
+	.id_table = pm8008_regulator_id_table,
+};
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm PM8008 PMIC regulator driver");
+MODULE_LICENSE("GPL");
-- 
2.44.1

[PATCH v2 14/14] arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

[Johan Hovold]

Enable the PM8008 PMIC which is used to power the camera sensors.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 123 ++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 98c1b75513be..93a44f803e8d 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -295,6 +295,27 @@ linux,cma {
 	};
 
 	thermal-zones {
+		pm8008-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+
+			thermal-sensors = <&pm8008>;
+
+			trips {
+				trip0 {
+					temperature = <95000>;
+					hysteresis = <0>;
+					type = "passive";
+				};
+
+				trip1 {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
 		skin-temp-thermal {
 			polling-delay-passive = <250>;
 			polling-delay = <0>;
@@ -669,6 +690,85 @@ touchscreen@10 {
 	};
 };
 
+&i2c11 {
+	clock-frequency = <400000>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c11_default>;
+
+	status = "okay";
+
+	pm8008: pmic@c {
+		compatible = "qcom,pm8008";
+		reg = <0xc>;
+
+		interrupts-extended = <&tlmm 41 IRQ_TYPE_EDGE_RISING>;
+		reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
+
+		vdd-l1-l2-supply = <&vreg_s11b>;
+		vdd-l3-l4-supply = <&vreg_bob>;
+		vdd-l5-supply = <&vreg_bob>;
+		vdd-l6-supply = <&vreg_bob>;
+		vdd-l7-supply = <&vreg_bob>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pm8008_default>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-ranges = <&pm8008 0 0 2>;
+
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		#thermal-sensor-cells = <0>;
+
+		regulators {
+			vreg_l1q: ldo1 {
+				regulator-name = "vreg_l1q";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+			};
+
+			vreg_l2q: ldo2 {
+				regulator-name = "vreg_l2q";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+			};
+
+			vreg_l3q: ldo3 {
+				regulator-name = "vreg_l3q";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+			};
+
+			vreg_l4q: ldo4 {
+				regulator-name = "vreg_l4q";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+			};
+
+			vreg_l5q: ldo5 {
+				regulator-name = "vreg_l5q";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+			};
+
+			vreg_l6q: ldo6 {
+				regulator-name = "vreg_l6q";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+			};
+
+			vreg_l7q: ldo7 {
+				regulator-name = "vreg_l7q";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+			};
+		};
+	};
+};
+
 &i2c21 {
 	clock-frequency = <400000>;
 
@@ -1367,6 +1467,13 @@ i2c4_default: i2c4-default-state {
 		bias-disable;
 	};
 
+	i2c11_default: i2c11-default-state {
+		pins = "gpio18", "gpio19";
+		function = "qup11";
+		drive-strength = <16>;
+		bias-disable;
+	};
+
 	i2c21_default: i2c21-default-state {
 		pins = "gpio81", "gpio82";
 		function = "qup21";
@@ -1470,6 +1577,22 @@ wake-n-pins {
 		};
 	};
 
+	pm8008_default: pm8008-default-state {
+		int-pins {
+			pins = "gpio41";
+			function = "gpio";
+			drive-strength = <2>;
+			bias-pull-down;
+		};
+
+		reset-n-pins {
+			pins = "gpio42";
+			function = "gpio";
+			drive-strength = <2>;
+			bias-disable;
+		};
+	};
+
 	spkr_1_sd_n_default: spkr-1-sd-n-default-state {
 		perst-n-pins {
 			pins = "gpio178";
-- 
2.44.1

Re: [PATCH v2 03/14] mfd: pm8008: deassert reset on probe

[Andy Shevchenko]

On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Request and deassert any (optional) reset gpio during probe in case it
> has been left asserted by the boot firmware.
>
> Note the reset line is not asserted to avoid reverting to the default
> I2C address in case the firmware has configured an alternate address.

...

> +       /*
> +        * The PMIC does not appear to require a post-reset delay, but wait
> +        * for a millisecond for now anyway.
> +        */

> +       usleep_range(1000, 2000);

fsleep() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/14] mfd: pm8008: rework driver

[Andy Shevchenko]

On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Rework the pm8008 driver to match the new binding which no longer
> describes internal details like interrupts and register offsets
> (including which of the two consecutive I2C addresses the registers
> belong to).
>
> Instead make the interrupt controller implementation internal and pass
> interrupts to the subdrivers using MFD cell resources.
>
> Note that subdrivers may either get their resources, like register block
> offsets, from the parent MFD or this can be included in the subdrivers
> directly.
>
> In the current implementation, the temperature alarm driver is generic
> enough to just get its base address and alarm interrupt from the parent
> driver, which already uses this information to implement the interrupt
> controller.
>
> The regulator driver, however, needs additional information like parent
> supplies and regulator characteristics so in that case it is easier to
> just augment its table with the regulator register base addresses.
>
> Similarly, the current GPIO driver already holds the number of pins and
> that lookup table can therefore also be extended with register offsets.
>
> Note that subdrivers can now access the two regmaps by name, even if the
> primary regmap is registered last so that it is returned by default when
> no name is provided in lookups.
>
> Finally, note that the temperature alarm and GPIO subdrivers need some
> minor rework before they can be used with non-SPMI devices like the
> PM8008. The temperature alarm MFD cell name specifically uses a "qpnp"
> rather than "spmi" prefix to prevent binding until the driver has been
> updated.

...

> +       dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1);
> +       if (IS_ERR(dummy)) {
> +               ret = PTR_ERR(dummy);
> +               dev_err(dev, "failed to claim second address: %d\n", ret);
> +               return ret;
> +       }


> +       ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq,
>                                 IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> +       if (ret) {
> +               dev_err(dev, "failed to add IRQ chip: %d\n", ret);
> +               return ret;
>         }

I believe there is no harm to use

  return dev_err_probe(...);

for these. But it seems you don't like that API. Whatever, no-one will
die, just additional work for the future :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Andy Shevchenko]

On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO
> regulators.
>
> The driver is based on a driver submitted by Satya Priya, but it has
> been cleaned up and reworked to match the new devicetree binding which
> no longer describes each regulator as a separate device.
>
> This avoids describing internal details like register offsets in the
> devicetree and allows for extending the implementation with features
> like over-current protection without having to update the binding.
>
> Specifically note that the regulator interrupts are shared between all
> regulators.
>
> Note that the secondary regmap is looked up by name and that if the
> driver ever needs to be generalised to support regulators provided by
> the primary regmap (I2C address) such information could be added to the
> device-id table.
>
> This also fixes the original implementation, which looked up regulators
> by 'regulator-name' property rather than devicetree node name and which
> prevented the regulators from being named to match board schematics.

...

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/math.h>
> +#include <linux/module.h>

> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>

+ types.h

+ asm/byteorder.h

...

> +static int pm8008_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
> +{
> +       struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
> +       unsigned int mV;
> +       __le16 val;
> +       int ret;
> +
> +       ret = regulator_list_voltage_linear_range(rdev, sel);
> +       if (ret < 0)
> +               return ret;
> +
> +       mV = DIV_ROUND_UP(ret, 1000);

MILLI from units.h ?

> +       val = cpu_to_le16(mV);

> +       ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG,
> +                       &val, sizeof(val));
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;

May be written as

  return regmap_bulk_write(...);

> +}
> +
> +static int pm8008_regulator_get_voltage_sel(struct regulator_dev *rdev)
> +{
> +       struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
> +       unsigned int uV;
> +       __le16 val;
> +       int ret;
> +
> +       ret = regmap_bulk_read(preg->regmap, preg->base + LDO_VSET_LB_REG,
> +                       &val, sizeof(val));
> +       if (ret < 0)
> +               return ret;
> +
> +       uV = le16_to_cpu(val) * 1000;

MILLI ?

> +       return (uV - preg->desc.min_uV) / preg->desc.uV_step;
> +}

...

> +               rdev = devm_regulator_register(dev, desc, &config);
> +               if (IS_ERR(rdev)) {
> +                       ret = PTR_ERR(rdev);
> +                       dev_err(dev, "failed to register regulator %s: %d\n",
> +                                       desc->name, ret);
> +                       return ret;

It's possible to use

  return dev_err_probe(...);

even for non-probe functions.

> +               }
> +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/14] mfd: pm8008: deassert reset on probe

[Johan Hovold]

On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > Request and deassert any (optional) reset gpio during probe in case it
> > has been left asserted by the boot firmware.
> >
> > Note the reset line is not asserted to avoid reverting to the default
> > I2C address in case the firmware has configured an alternate address.
> 
> ...
> 
> > +       /*
> > +        * The PMIC does not appear to require a post-reset delay, but wait
> > +        * for a millisecond for now anyway.
> > +        */
> 
> > +       usleep_range(1000, 2000);
> 
> fsleep() ?

No, I'd only use fsleep() when the argument is variable.

Johan

Re: [PATCH v2 12/14] mfd: pm8008: rework driver

[Johan Hovold]

On Wed, May 29, 2024 at 10:53:09PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> > +       ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq,
> >                                 IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> > +       if (ret) {
> > +               dev_err(dev, "failed to add IRQ chip: %d\n", ret);
> > +               return ret;
> >         }
> 
> I believe there is no harm to use
> 
>   return dev_err_probe(...);
> 
> for these. But it seems you don't like that API. Whatever, no-one will
> die, just additional work for the future :-)

Correct. And there's no additional work for the future here either.

Johan

Re: [PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Johan Hovold]

On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> > +#include <linux/array_size.h>
> > +#include <linux/bits.h>
> > +#include <linux/device.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> 
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> 
> + types.h

This one is already pulled in indirectly and I'm not going to respin for
this.

> + asm/byteorder.h

Already explicitly included in the code you left out.

> > +static int pm8008_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
> > +{
> > +       struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
> > +       unsigned int mV;
> > +       __le16 val;
> > +       int ret;
> > +
> > +       ret = regulator_list_voltage_linear_range(rdev, sel);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       mV = DIV_ROUND_UP(ret, 1000);
> 
> MILLI from units.h ?

Nah. 
 
> > +       val = cpu_to_le16(mV);
> 
> > +       ret = regmap_bulk_write(preg->regmap, preg->base + LDO_VSET_LB_REG,
> > +                       &val, sizeof(val));
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return 0;
> 
> May be written as
> 
>   return regmap_bulk_write(...);

I obviously prefer it the way I wrote it.

> > +               rdev = devm_regulator_register(dev, desc, &config);
> > +               if (IS_ERR(rdev)) {
> > +                       ret = PTR_ERR(rdev);
> > +                       dev_err(dev, "failed to register regulator %s: %d\n",
> > +                                       desc->name, ret);
> > +                       return ret;
> 
> It's possible to use
> 
>   return dev_err_probe(...);
> 
> even for non-probe functions.

This is a probe function(), but as I've told you repeatedly I'm not
going to use dev_err_probe() here.

Johan

Re: [PATCH v2 03/14] mfd: pm8008: deassert reset on probe

[Andy Shevchenko]

On Thu, May 30, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > Request and deassert any (optional) reset gpio during probe in case it
> > > has been left asserted by the boot firmware.
> > >
> > > Note the reset line is not asserted to avoid reverting to the default
> > > I2C address in case the firmware has configured an alternate address.

...

> > > +       /*
> > > +        * The PMIC does not appear to require a post-reset delay, but wait
> > > +        * for a millisecond for now anyway.
> > > +        */
> >
> > > +       usleep_range(1000, 2000);
> >
> > fsleep() ?
>
> No, I'd only use fsleep() when the argument is variable.

Okay, this is basically the same issue as with use of dev_err_probe()
with known errors. fsleep() hides the choice between let's say
msleep() / usleep_range() / udelay() from the caller. This, in
particular, might allow shifting constraints if the timer core is
changed or becomes more granular. It's independent to the variable or
constant parameter(s). Whatever, I'm not going to insist.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Andy Shevchenko]

On Thu, May 30, 2024 at 11:14 AM Johan Hovold <johan@kernel.org> wrote:
> On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:

...

> > > +#include <linux/array_size.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/device.h>
> > > +#include <linux/math.h>
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/driver.h>
> >
> > + types.h
>
> This one is already pulled in indirectly and I'm not going to respin for
> this.
>
> > + asm/byteorder.h
>
> Already explicitly included in the code you left out.

Is there any guarantee it will be like this? I don't think so. That's
why there is an IWYU principle to give more flexibility of reshuffling
the (core) headers. And I believe you know that we have way too far
dependency hell in the headers in the kernel. Have you seen what Ingo
tried to do and what the potential achievements are?

...

> > > +               rdev = devm_regulator_register(dev, desc, &config);
> > > +               if (IS_ERR(rdev)) {
> > > +                       ret = PTR_ERR(rdev);
> > > +                       dev_err(dev, "failed to register regulator %s: %d\n",
> > > +                                       desc->name, ret);
> > > +                       return ret;
> >
> > It's possible to use
> >
> >   return dev_err_probe(...);
> >
> > even for non-probe functions.

(this should be "non-probe deferred functions")

> This is a probe function(), but as I've told you repeatedly I'm not
> going to use dev_err_probe() here.

Yeah, I got it, some developers are leaving in the previous decades to
make code very verbose for no benefit, no problem.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/14] mfd: pm8008: deassert reset on probe

[Johan Hovold]

On Thu, May 30, 2024 at 11:34:55AM +0300, Andy Shevchenko wrote:
> On Thu, May 30, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> > > >
> > > > Request and deassert any (optional) reset gpio during probe in case it
> > > > has been left asserted by the boot firmware.
> > > >
> > > > Note the reset line is not asserted to avoid reverting to the default
> > > > I2C address in case the firmware has configured an alternate address.
> 
> ...
> 
> > > > +       /*
> > > > +        * The PMIC does not appear to require a post-reset delay, but wait
> > > > +        * for a millisecond for now anyway.
> > > > +        */
> > >
> > > > +       usleep_range(1000, 2000);
> > >
> > > fsleep() ?
> >
> > No, I'd only use fsleep() when the argument is variable.
> 
> Okay, this is basically the same issue as with use of dev_err_probe()
> with known errors. fsleep() hides the choice between let's say
> msleep() / usleep_range() / udelay() from the caller. This, in
> particular, might allow shifting constraints if the timer core is
> changed or becomes more granular. It's independent to the variable or
> constant parameter(s). Whatever, I'm not going to insist.

I prefer that developers are aware of what they are doing and understand
the difference between, say, usleep_range() and udelay(), instead of
hiding things away in obscure helper functions.

Johan

Re: [PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Johan Hovold]

On Thu, May 30, 2024 at 11:46:12AM +0300, Andy Shevchenko wrote:
> On Thu, May 30, 2024 at 11:14 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, May 29, 2024 at 11:02:57PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> 
> ...
> 
> > > > +#include <linux/array_size.h>
> > > > +#include <linux/bits.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/math.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/regulator/driver.h>
> > >
> > > + types.h
> >
> > This one is already pulled in indirectly and I'm not going to respin for
> > this.
> >
> > > + asm/byteorder.h
> >
> > Already explicitly included in the code you left out.
> 
> Is there any guarantee it will be like this? I don't think so. That's
> why there is an IWYU principle to give more flexibility of reshuffling
> the (core) headers. And I believe you know that we have way too far
> dependency hell in the headers in the kernel. Have you seen what Ingo
> tried to do and what the potential achievements are?

The driver is using cpu_to_le16() from asm/byteorder.h so the __le16
type definition will be pulled in.

> 
> ...
> 
> > > > +               rdev = devm_regulator_register(dev, desc, &config);
> > > > +               if (IS_ERR(rdev)) {
> > > > +                       ret = PTR_ERR(rdev);
> > > > +                       dev_err(dev, "failed to register regulator %s: %d\n",
> > > > +                                       desc->name, ret);
> > > > +                       return ret;
> > >
> > > It's possible to use
> > >
> > >   return dev_err_probe(...);
> > >
> > > even for non-probe functions.
> 
> (this should be "non-probe deferred functions")
> 
> > This is a probe function(), but as I've told you repeatedly I'm not
> > going to use dev_err_probe() here.
> 
> Yeah, I got it, some developers are leaving in the previous decades to
> make code very verbose for no benefit, no problem.

And some developers write unreadable code just to save a few lines of
code. I prefer clarity.

Johan

Re: [PATCH v2 13/14] regulator: add pm8008 pmic regulator driver

[Mark Brown]

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

On Wed, May 29, 2024 at 06:29:57PM +0200, Johan Hovold wrote:
> The Qualcomm PM8008 is an I2C-controlled PMIC containing seven LDO
> regulators.
> 
> The driver is based on a driver submitted by Satya Priya, but it has
> been cleaned up and reworked to match the new devicetree binding which
> no longer describes each regulator as a separate device.

Reviewed-by: Mark Brown <broonie@kernel.org>

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

Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

[Bryan O'Donoghue]

On 29/05/2024 17:29, Johan Hovold wrote:
> The regmap irq array is potentially shared between multiple PMICs and
> should only contain static data.
> 
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.
> 
> Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
>   1 file changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 3ac3742f438b..f71c490f25c8 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
>   	INT_POL_LOW_OFFSET,
>   };
>   
> -static struct regmap_irq pm8008_irqs[] = {
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO,	PM8008_MISC,	BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO,	PM8008_MISC,	BIT(1)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2,	PM8008_MISC,	BIT(2)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3,	PM8008_MISC,	BIT(3)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP,	PM8008_MISC,	BIT(4)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM,	PM8008_TEMP_ALARM, BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO1,	PM8008_GPIO1,	BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
> +#define _IRQ(_irq, _off, _mask, _types)			\
> +	[_irq] = {					\
> +		.reg_offset = (_off),			\
> +		.mask = (_mask),			\
> +		.type = {				\
> +			.type_reg_offset = (_off),	\
> +			.types_supported = (_types),	\
> +		},					\
> +	}
> +
> +static const struct regmap_irq pm8008_irqs[] = {
> +	_IRQ(PM8008_IRQ_MISC_UVLO,    PM8008_MISC,	BIT(0), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OVLO,    PM8008_MISC,	BIT(1), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OTST2,   PM8008_MISC,	BIT(2), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OTST3,   PM8008_MISC,	BIT(3), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC,	BIT(4), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_TEMP_ALARM,   PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> +	_IRQ(PM8008_IRQ_GPIO1,	      PM8008_GPIO1,	BIT(0), IRQ_TYPE_SENSE_MASK),
> +	_IRQ(PM8008_IRQ_GPIO2,	      PM8008_GPIO2,	BIT(0), IRQ_TYPE_SENSE_MASK),
>   };
>   
>   static const unsigned int pm8008_periph_base[] = {
> @@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>   	.max_register	= 0xFFFF,
>   };
>   
> -static int pm8008_probe_irq_peripherals(struct device *dev,
> -					struct regmap *regmap,
> -					int client_irq)
> -{
> -	int rc, i;
> -	struct regmap_irq_type *type;
> -	struct regmap_irq_chip_data *irq_data;
> -
> -	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
> -		type = &pm8008_irqs[i].type;
> -
> -		type->type_reg_offset = pm8008_irqs[i].reg_offset;
> -
> -		if (type->type_reg_offset == PM8008_MISC)
> -			type->types_supported = IRQ_TYPE_EDGE_RISING;
> -		else
> -			type->types_supported = (IRQ_TYPE_EDGE_BOTH |
> -				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> -	}
ick

A no-brainer improvement.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

[Lee Jones]

On Wed, 29 May 2024, Johan Hovold wrote:

> The regmap irq array is potentially shared between multiple PMICs and
> should only contain static data.
> 
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.
> 
> Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/mfd/qcom-pm8008.c | 64 ++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 3ac3742f438b..f71c490f25c8 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -56,15 +56,25 @@ static unsigned int pm8008_config_regs[] = {
>  	INT_POL_LOW_OFFSET,
>  };
>  
> -static struct regmap_irq pm8008_irqs[] = {
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO,	PM8008_MISC,	BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO,	PM8008_MISC,	BIT(1)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2,	PM8008_MISC,	BIT(2)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3,	PM8008_MISC,	BIT(3)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP,	PM8008_MISC,	BIT(4)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM,	PM8008_TEMP_ALARM, BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO1,	PM8008_GPIO1,	BIT(0)),
> -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
> +#define _IRQ(_irq, _off, _mask, _types)			\
> +	[_irq] = {					\
> +		.reg_offset = (_off),			\
> +		.mask = (_mask),			\
> +		.type = {				\
> +			.type_reg_offset = (_off),	\
> +			.types_supported = (_types),	\
> +		},					\
> +	}

Any reason why this can't be generic and be tucked away somewhere in a
header file?

> +static const struct regmap_irq pm8008_irqs[] = {
> +	_IRQ(PM8008_IRQ_MISC_UVLO,    PM8008_MISC,	BIT(0), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OVLO,    PM8008_MISC,	BIT(1), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OTST2,   PM8008_MISC,	BIT(2), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_OTST3,   PM8008_MISC,	BIT(3), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC,	BIT(4), IRQ_TYPE_EDGE_RISING),
> +	_IRQ(PM8008_IRQ_TEMP_ALARM,   PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> +	_IRQ(PM8008_IRQ_GPIO1,	      PM8008_GPIO1,	BIT(0), IRQ_TYPE_SENSE_MASK),
> +	_IRQ(PM8008_IRQ_GPIO2,	      PM8008_GPIO2,	BIT(0), IRQ_TYPE_SENSE_MASK),
>  };
>  
>  static const unsigned int pm8008_periph_base[] = {
> @@ -143,38 +153,9 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>  	.max_register	= 0xFFFF,
>  };
>  
> -static int pm8008_probe_irq_peripherals(struct device *dev,
> -					struct regmap *regmap,
> -					int client_irq)
> -{
> -	int rc, i;
> -	struct regmap_irq_type *type;
> -	struct regmap_irq_chip_data *irq_data;
> -
> -	for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) {
> -		type = &pm8008_irqs[i].type;
> -
> -		type->type_reg_offset = pm8008_irqs[i].reg_offset;
> -
> -		if (type->type_reg_offset == PM8008_MISC)
> -			type->types_supported = IRQ_TYPE_EDGE_RISING;
> -		else
> -			type->types_supported = (IRQ_TYPE_EDGE_BOTH |
> -				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
> -	}
> -
> -	rc = devm_regmap_add_irq_chip(dev, regmap, client_irq,
> -			IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> -	if (rc) {
> -		dev_err(dev, "Failed to add IRQ chip: %d\n", rc);
> -		return rc;
> -	}
> -
> -	return 0;
> -}
> -
>  static int pm8008_probe(struct i2c_client *client)
>  {
> +	struct regmap_irq_chip_data *irq_data;
>  	int rc;
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -187,9 +168,10 @@ static int pm8008_probe(struct i2c_client *client)
>  	i2c_set_clientdata(client, regmap);
>  
>  	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> -		rc = pm8008_probe_irq_peripherals(dev, regmap, client->irq);
> +		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> +				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>  		if (rc)
> -			dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> +			dev_err(dev, "failed to add IRQ chip: %d\n", rc);
>  	}
>  
>  	return devm_of_platform_populate(dev);
> -- 
> 2.44.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 12/14] mfd: pm8008: rework driver

[Lee Jones]

Please improve the subject line.

I'll come back to review the whole set once Andy has had his wicked way with you!

</end>

On Wed, 29 May 2024, Johan Hovold wrote:

> Rework the pm8008 driver to match the new binding which no longer
> describes internal details like interrupts and register offsets
> (including which of the two consecutive I2C addresses the registers
> belong to).
> 
> Instead make the interrupt controller implementation internal and pass
> interrupts to the subdrivers using MFD cell resources.
> 
> Note that subdrivers may either get their resources, like register block
> offsets, from the parent MFD or this can be included in the subdrivers
> directly.
> 
> In the current implementation, the temperature alarm driver is generic
> enough to just get its base address and alarm interrupt from the parent
> driver, which already uses this information to implement the interrupt
> controller.
> 
> The regulator driver, however, needs additional information like parent
> supplies and regulator characteristics so in that case it is easier to
> just augment its table with the regulator register base addresses.
> 
> Similarly, the current GPIO driver already holds the number of pins and
> that lookup table can therefore also be extended with register offsets.
> 
> Note that subdrivers can now access the two regmaps by name, even if the
> primary regmap is registered last so that it is returned by default when
> no name is provided in lookups.
> 
> Finally, note that the temperature alarm and GPIO subdrivers need some
> minor rework before they can be used with non-SPMI devices like the
> PM8008. The temperature alarm MFD cell name specifically uses a "qpnp"
> rather than "spmi" prefix to prevent binding until the driver has been
> updated.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/mfd/Kconfig                   |  1 +
>  drivers/mfd/qcom-pm8008.c             | 97 +++++++++++++++++++++++----
>  include/dt-bindings/mfd/qcom-pm8008.h | 19 ------
>  3 files changed, 86 insertions(+), 31 deletions(-)
>  delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4b023ee229cf..bfcb68c62b07 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2208,6 +2208,7 @@ config MFD_ACER_A500_EC
>  config MFD_QCOM_PM8008
>  	tristate "QCOM PM8008 Power Management IC"
>  	depends on I2C && OF
> +	select MFD_CORE
>  	select REGMAP_I2C
>  	select REGMAP_IRQ
>  	help
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 72199840231e..246b5fe9819d 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -7,8 +7,10 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> +#include <linux/ioport.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
> @@ -16,8 +18,6 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> -#include <dt-bindings/mfd/qcom-pm8008.h>
> -
>  #define I2C_INTR_STATUS_BASE		0x0550
>  #define INT_RT_STS_OFFSET		0x10
>  #define INT_SET_TYPE_OFFSET		0x11
> @@ -45,6 +45,16 @@ enum {
>  #define PM8008_GPIO1_ADDR	PM8008_PERIPH_2_BASE
>  #define PM8008_GPIO2_ADDR	PM8008_PERIPH_3_BASE
>  
> +/* PM8008 IRQ numbers */
> +#define PM8008_IRQ_MISC_UVLO	0
> +#define PM8008_IRQ_MISC_OVLO	1
> +#define PM8008_IRQ_MISC_OTST2	2
> +#define PM8008_IRQ_MISC_OTST3	3
> +#define PM8008_IRQ_MISC_LDO_OCP	4
> +#define PM8008_IRQ_TEMP_ALARM	5
> +#define PM8008_IRQ_GPIO1	6
> +#define PM8008_IRQ_GPIO2	7
> +
>  enum {
>  	SET_TYPE_INDEX,
>  	POLARITY_HI_INDEX,
> @@ -148,21 +158,65 @@ static const struct regmap_irq_chip pm8008_irq_chip = {
>  	.get_irq_reg		= pm8008_get_irq_reg,
>  };
>  
> -static struct regmap_config qcom_mfd_regmap_cfg = {
> +static const struct regmap_config qcom_mfd_regmap_cfg = {
> +	.name		= "primary",
> +	.reg_bits	= 16,
> +	.val_bits	= 8,
> +	.max_register	= 0xffff,
> +};
> +
> +static const struct regmap_config pm8008_regmap_cfg_2 = {
> +	.name		= "secondary",
>  	.reg_bits	= 16,
>  	.val_bits	= 8,
>  	.max_register	= 0xffff,
>  };
>  
> +static const struct resource pm8008_temp_res[] = {
> +	DEFINE_RES_MEM(PM8008_TEMP_ALARM_ADDR, 0x100),
> +	DEFINE_RES_IRQ(PM8008_IRQ_TEMP_ALARM),
> +};
> +
> +static const struct mfd_cell pm8008_cells[] = {
> +	MFD_CELL_NAME("pm8008-regulator"),
> +	MFD_CELL_RES("qpnp-temp-alarm", pm8008_temp_res),
> +	MFD_CELL_NAME("pm8008-gpio"),
> +};
> +
> +static void devm_irq_domain_fwnode_release(void *data)
> +{
> +	struct fwnode_handle *fwnode = data;
> +
> +	irq_domain_free_fwnode(fwnode);
> +}
> +
>  static int pm8008_probe(struct i2c_client *client)
>  {
>  	struct regmap_irq_chip_data *irq_data;
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap, *regmap2;
> +	struct fwnode_handle *fwnode;
> +	struct i2c_client *dummy;
>  	struct gpio_desc *reset;
> -	int rc;
> -	struct device *dev;
> -	struct regmap *regmap;
> +	char *name;
> +	int ret;
> +
> +	dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1);
> +	if (IS_ERR(dummy)) {
> +		ret = PTR_ERR(dummy);
> +		dev_err(dev, "failed to claim second address: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap2 = devm_regmap_init_i2c(dummy, &qcom_mfd_regmap_cfg);
> +	if (IS_ERR(regmap2))
> +		return PTR_ERR(regmap2);
>  
> -	dev = &client->dev;
> +	ret = regmap_attach_dev(dev, regmap2, &pm8008_regmap_cfg_2);
> +	if (ret)
> +		return ret;
> +
> +	/* Default regmap must be attached last. */
>  	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
> @@ -177,14 +231,33 @@ static int pm8008_probe(struct i2c_client *client)
>  	 */
>  	usleep_range(1000, 2000);
>  
> -	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
> -		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	name = strreplace(name, '/', ':');
> +
> +	fwnode = irq_domain_alloc_named_fwnode(name);
> +	if (!fwnode)
> +		return -ENOMEM;
> +
> +	ret = devm_add_action_or_reset(dev, devm_irq_domain_fwnode_release, fwnode);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regmap_add_irq_chip_fwnode(dev, fwnode, regmap, client->irq,
>  				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
> -		if (rc)
> -			dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> +	if (ret) {
> +		dev_err(dev, "failed to add IRQ chip: %d\n", ret);
> +		return ret;
>  	}
>  
> -	return devm_of_platform_populate(dev);
> +	/* Needed by GPIO driver. */
> +	dev_set_drvdata(dev, regmap_irq_get_domain(irq_data));
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, pm8008_cells,
> +				ARRAY_SIZE(pm8008_cells), NULL, 0,
> +				regmap_irq_get_domain(irq_data));
>  }
>  
>  static const struct of_device_id pm8008_match[] = {
> diff --git a/include/dt-bindings/mfd/qcom-pm8008.h b/include/dt-bindings/mfd/qcom-pm8008.h
> deleted file mode 100644
> index eca9448df228..000000000000
> --- a/include/dt-bindings/mfd/qcom-pm8008.h
> +++ /dev/null
> @@ -1,19 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (c) 2021 The Linux Foundation. All rights reserved.
> - */
> -
> -#ifndef __DT_BINDINGS_MFD_QCOM_PM8008_H
> -#define __DT_BINDINGS_MFD_QCOM_PM8008_H
> -
> -/* PM8008 IRQ numbers */
> -#define PM8008_IRQ_MISC_UVLO	0
> -#define PM8008_IRQ_MISC_OVLO	1
> -#define PM8008_IRQ_MISC_OTST2	2
> -#define PM8008_IRQ_MISC_OTST3	3
> -#define PM8008_IRQ_MISC_LDO_OCP	4
> -#define PM8008_IRQ_TEMP_ALARM	5
> -#define PM8008_IRQ_GPIO1	6
> -#define PM8008_IRQ_GPIO2	7
> -
> -#endif
> -- 
> 2.44.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 00/14] arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

[Lee Jones]

On Wed, 29 May 2024, Johan Hovold wrote:

> The Qualcomm PM8008 PMIC is a so called QPNP PMIC with seven LDO
> regulators, a temperature alarm block and two GPIO pins (which are also
> used for interrupt signalling and reset).

I don't see any issues with the MFD commits.

When you submit this, would you do me a favour and change the subject
lines to match that of the subsystem.  I usually silently change them,
but this is a large set and it'll become annoying real quick.

`git log --oneline -- <subsystem>` is your friend.

Thanks.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 12/14] mfd: pm8008: rework driver

[Andy Shevchenko]

On Fri, May 31, 2024 at 8:08 PM Lee Jones <lee@kernel.org> wrote:
>
> Please improve the subject line.
>
> I'll come back to review the whole set once Andy has had his wicked way with you!

I guess we came to the equilibrium, I still disagree on some points,
but Johan has a strong opinion to not follow my comments. So, it's up
to you now :)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/14] dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008

[Rob Herring (Arm)]

On Wed, 29 May 2024 18:29:53 +0200, Johan Hovold wrote:
> The binding for PM8008 is being reworked so that internal details like
> interrupts and register offsets are no longer described. This
> specifically also involves dropping the gpio child node and its
> compatible string which is no longer needed.
> 
> Note that there are currently no users of the upstream binding and
> driver.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 10/14] dt-bindings: mfd: pm8008: drop redundant descriptions

[Rob Herring (Arm)]

On Wed, 29 May 2024 18:29:54 +0200, Johan Hovold wrote:
> In preparation for reworking the binding, drop the redundant
> descriptions of the standard 'reg' and 'interrupts' properties.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 5 -----
>  1 file changed, 5 deletions(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v2 11/14] dt-bindings: mfd: pm8008: rework binding

[Rob Herring]

On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> Rework the pm8008 binding by dropping internal details like register
> offsets and interrupts and by adding the missing regulator and
> temperature alarm properties.
> 
> Note that child nodes are still used for pinctrl and regulator
> configuration.
> 
> Also note that the pinctrl state definition will be extended later and
> could eventually also be shared with other PMICs (e.g. by breaking out
> bits of qcom,pmic-gpio.yaml).

This commit message should also state there are no users.

> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
>  1 file changed, 90 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index d71657f488db..ccf472e7f552 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -27,103 +27,134 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> -  "#interrupt-cells":
> +  vdd-l1-l2-supply: true
> +  vdd-l3-l4-supply: true
> +  vdd-l5-supply: true
> +  vdd-l6-supply: true
> +  vdd-l7-supply: true
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
>      const: 2
>  
> -    description: |
> -      The first cell is the IRQ number, the second cell is the IRQ trigger
> -      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
> +  gpio-ranges:
> +    maxItems: 1
>  
>    interrupt-controller: true
>  
> -  "#address-cells":
> -    const: 1
> +  "#interrupt-cells":
> +    const: 2
>  
> -  "#size-cells":
> +  "#thermal-sensor-cells":
>      const: 0
>  
> -patternProperties:
> -  "^gpio@[0-9a-f]+$":
> +  pinctrl:
>      type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "-state$":
> +        type: object
> +        $ref: "#/$defs/qcom-pm8008-pinctrl-state"

There's only 1 reference to this, so just move the $def contents here.

Rob

Re: [PATCH v2 11/14] dt-bindings: mfd: pm8008: rework binding

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> Rework the pm8008 binding by dropping internal details like register
> offsets and interrupts and by adding the missing regulator and
> temperature alarm properties.
> 
> Note that child nodes are still used for pinctrl and regulator
> configuration.
> 
> Also note that the pinctrl state definition will be extended later and
> could eventually also be shared with other PMICs (e.g. by breaking out
> bits of qcom,pmic-gpio.yaml).

Obviously we want to adapt this style of bindings for the other PMICs
too. My main concern here are PMICs which have two kinds of controlled
pins: GPIOs and MPPs. With the existing bindings style those are
declared as two subdevices. What would be your suggested way to support
MPPs with the proposed kind of bindings?

> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 149 +++++++++++-------
>  1 file changed, 90 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index d71657f488db..ccf472e7f552 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -27,103 +27,134 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> -  "#interrupt-cells":
> +  vdd-l1-l2-supply: true
> +  vdd-l3-l4-supply: true
> +  vdd-l5-supply: true
> +  vdd-l6-supply: true
> +  vdd-l7-supply: true
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
>      const: 2
>  
> -    description: |
> -      The first cell is the IRQ number, the second cell is the IRQ trigger
> -      flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h.
> +  gpio-ranges:
> +    maxItems: 1
>  
>    interrupt-controller: true
>  
> -  "#address-cells":
> -    const: 1
> +  "#interrupt-cells":
> +    const: 2
>  
> -  "#size-cells":
> +  "#thermal-sensor-cells":
>      const: 0
>  
> -patternProperties:
> -  "^gpio@[0-9a-f]+$":
> +  pinctrl:
>      type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "-state$":
> +        type: object
> +        $ref: "#/$defs/qcom-pm8008-pinctrl-state"
> +        unevaluatedProperties: false
>  
> -    description: |
> -      The GPIO peripheral. This node may be specified twice, one for each GPIO.
> -
> -    properties:
> -      compatible:
> -        items:
> -          - const: qcom,pm8008-gpio
> -          - const: qcom,spmi-gpio
> +  regulators:
> +    type: object
> +    additionalProperties: false
> +    patternProperties:
> +      "^ldo[1-7]$":
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#
> +        unevaluatedProperties: false
>  
> -      reg:
> -        description: Peripheral address of one of the two GPIO peripherals.
> -        maxItems: 1
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - vdd-l1-l2-supply
> +  - vdd-l3-l4-supply
> +  - vdd-l5-supply
> +  - vdd-l6-supply
> +  - vdd-l7-supply
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - "#thermal-sensor-cells"
>  
> -      gpio-controller: true
> +additionalProperties: false
>  
> -      gpio-ranges:
> -        maxItems: 1
> +$defs:
> +  qcom-pm8008-pinctrl-state:
> +    type: object
>  
> -      interrupt-controller: true
> +    allOf:
> +      - $ref: /schemas/pinctrl/pinmux-node.yaml
> +      - $ref: /schemas/pinctrl/pincfg-node.yaml
>  
> -      "#interrupt-cells":
> -        const: 2
> +    properties:
> +      pins:
> +        items:
> +          pattern: "^gpio[12]$"
>  
> -      "#gpio-cells":
> -        const: 2
> +      function:
> +        items:
> +          - enum:
> +              - normal
>  
>      required:
> -      - compatible
> -      - reg
> -      - gpio-controller
> -      - interrupt-controller
> -      - "#gpio-cells"
> -      - gpio-ranges
> -      - "#interrupt-cells"
> +      - pins
> +      - function
>  
>      additionalProperties: false
>  
> -required:
> -  - compatible
> -  - reg
> -  - interrupts
> -  - "#address-cells"
> -  - "#size-cells"
> -  - "#interrupt-cells"
> -
> -additionalProperties: false
> -
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> -    #include <dt-bindings/mfd/qcom-pm8008.h>
>      #include <dt-bindings/interrupt-controller/irq.h>
>  
>      i2c {
>        #address-cells = <1>;
>        #size-cells = <0>;
>  
> -      pmic@8 {
> +      pm8008: pmic@8 {
>          compatible = "qcom,pm8008";
>          reg = <0x8>;
> -        #address-cells = <1>;
> -        #size-cells = <0>;
> -        interrupt-controller;
> -        #interrupt-cells = <2>;
>  
>          interrupt-parent = <&tlmm>;
>          interrupts = <32 IRQ_TYPE_EDGE_RISING>;
>  
>          reset-gpios = <&tlmm 42 GPIO_ACTIVE_LOW>;
>  
> -        pm8008_gpios: gpio@c000 {
> -          compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
> -          reg = <0xc000>;
> -          gpio-controller;
> -          gpio-ranges = <&pm8008_gpios 0 0 2>;
> -          #gpio-cells = <2>;
> -          interrupt-controller;
> -          #interrupt-cells = <2>;
> +        vdd-l1-l2-supply = <&vreg_s8b_1p2>;
> +        vdd-l3-l4-supply = <&vreg_s1b_1p8>;
> +        vdd-l5-supply = <&vreg_bob>;
> +        vdd-l6-supply = <&vreg_bob>;
> +        vdd-l7-supply = <&vreg_bob>;
> +
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&pm8008 0 0 2>;
> +
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +
> +        #thermal-sensor-cells = <0>;
> +
> +        pinctrl {
> +          gpio-keys-state {
> +            pins = "gpio1";
> +            function = "normal";
> +          };
> +        };
> +
> +        regulators {
> +          ldo1 {
> +            regulator-name = "vreg_l1";
> +            regulator-min-microvolt = <950000>;
> +            regulator-max-microvolt = <1300000>;
> +          };
>          };
>        };
>      };
> -- 
> 2.44.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 08/14] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Linus Walleij]

On Wed, May 29, 2024 at 6:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> The SPMI GPIO driver assumes that the parent device is an SPMI device
> and accesses random data when backcasting the parent struct device
> pointer for non-SPMI devices.
>
> Fortunately this does not seem to cause any issues currently when the
> parent device is an I2C client like the PM8008, but this could change if
> the structures are reorganised (e.g. using structure randomisation).
>
> Notably the interrupt implementation is also broken for non-SPMI devices.
>
> Also note that the two GPIO pins on PM8008 are used for interrupts and
> reset so their practical use should be limited.
>
> Drop the broken GPIO support for PM8008 for now.
>
> Fixes: ea119e5a482a ("pinctrl: qcom-pmic-gpio: Add support for pm8008")
> Cc: stable@vger.kernel.org      # 5.13
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

This patch applied to pinctrl fixes.

Yours,
Linus Walleij

Re: [PATCH v2 09/14] dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008

[Linus Walleij]

On Wed, May 29, 2024 at 6:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> The binding for PM8008 is being reworked so that internal details like
> interrupts and register offsets are no longer described. This
> specifically also involves dropping the gpio child node and its
> compatible string which is no longer needed.
>
> Note that there are currently no users of the upstream binding and
> driver.
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

This patch applied to pinctrl fixes.

Yours,
Linus Walleij

Re: [PATCH v2 11/14] dt-bindings: mfd: pm8008: rework binding

[Johan Hovold]

On Wed, Jun 05, 2024 at 11:43:16AM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> > Rework the pm8008 binding by dropping internal details like register
> > offsets and interrupts and by adding the missing regulator and
> > temperature alarm properties.
> > 
> > Note that child nodes are still used for pinctrl and regulator
> > configuration.
> > 
> > Also note that the pinctrl state definition will be extended later and
> > could eventually also be shared with other PMICs (e.g. by breaking out
> > bits of qcom,pmic-gpio.yaml).
> 
> Obviously we want to adapt this style of bindings for the other PMICs
> too. My main concern here are PMICs which have two kinds of controlled
> pins: GPIOs and MPPs. With the existing bindings style those are
> declared as two subdevices. What would be your suggested way to support
> MPPs with the proposed kind of bindings?

As far as I understand newer PMICs do not have MPP blocks and we do not
necessarily want to convert the existing bindings.

That said, if there is ever a need to describe two separate gpio blocks
this can, for example, be done using subnodes on those PMICs.

Johan

Re: [PATCH v2 02/14] mfd: pm8008: fix regmap irq chip initialisation

[Johan Hovold]

On Fri, May 31, 2024 at 06:03:53PM +0100, Lee Jones wrote:
> On Wed, 29 May 2024, Johan Hovold wrote:
> 
> > The regmap irq array is potentially shared between multiple PMICs and
> > should only contain static data.
> > 
> > Use a custom macro to initialise also the type fields and drop the
> > unnecessary updates on each probe.
> > 
> > Fixes: 6b149f3310a4 ("mfd: pm8008: Add driver for QCOM PM8008 PMIC")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
 
> > -static struct regmap_irq pm8008_irqs[] = {
> > -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_UVLO,	PM8008_MISC,	BIT(0)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OVLO,	PM8008_MISC,	BIT(1)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST2,	PM8008_MISC,	BIT(2)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_OTST3,	PM8008_MISC,	BIT(3)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_MISC_LDO_OCP,	PM8008_MISC,	BIT(4)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_TEMP_ALARM,	PM8008_TEMP_ALARM, BIT(0)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO1,	PM8008_GPIO1,	BIT(0)),
> > -	REGMAP_IRQ_REG(PM8008_IRQ_GPIO2,	PM8008_GPIO2,	BIT(0)),
> > +#define _IRQ(_irq, _off, _mask, _types)			\
> > +	[_irq] = {					\
> > +		.reg_offset = (_off),			\
> > +		.mask = (_mask),			\
> > +		.type = {				\
> > +			.type_reg_offset = (_off),	\
> > +			.types_supported = (_types),	\
> > +		},					\
> > +	}
> 
> Any reason why this can't be generic and be tucked away somewhere in a
> header file?

These macros tend to be quite driver specific so not sure it makes sense
to try to generalise beyond the basic ones already provided by regmap.

Either way, I don't think that should be a prerequisite for fixing this
driver.

I'm also considering replacing the current irq chip implementation as
part of unifying with the SPMI implementation.
 
> > +static const struct regmap_irq pm8008_irqs[] = {
> > +	_IRQ(PM8008_IRQ_MISC_UVLO,    PM8008_MISC,	BIT(0), IRQ_TYPE_EDGE_RISING),
> > +	_IRQ(PM8008_IRQ_MISC_OVLO,    PM8008_MISC,	BIT(1), IRQ_TYPE_EDGE_RISING),
> > +	_IRQ(PM8008_IRQ_MISC_OTST2,   PM8008_MISC,	BIT(2), IRQ_TYPE_EDGE_RISING),
> > +	_IRQ(PM8008_IRQ_MISC_OTST3,   PM8008_MISC,	BIT(3), IRQ_TYPE_EDGE_RISING),
> > +	_IRQ(PM8008_IRQ_MISC_LDO_OCP, PM8008_MISC,	BIT(4), IRQ_TYPE_EDGE_RISING),
> > +	_IRQ(PM8008_IRQ_TEMP_ALARM,   PM8008_TEMP_ALARM,BIT(0), IRQ_TYPE_SENSE_MASK),
> > +	_IRQ(PM8008_IRQ_GPIO1,	      PM8008_GPIO1,	BIT(0), IRQ_TYPE_SENSE_MASK),
> > +	_IRQ(PM8008_IRQ_GPIO2,	      PM8008_GPIO2,	BIT(0), IRQ_TYPE_SENSE_MASK),
> >  };

Johan

Re: [PATCH v2 11/14] dt-bindings: mfd: pm8008: rework binding

[Dmitry Baryshkov]

On Sat, Jun 08, 2024 at 05:36:36PM +0200, Johan Hovold wrote:
> On Wed, Jun 05, 2024 at 11:43:16AM +0300, Dmitry Baryshkov wrote:
> > On Wed, May 29, 2024 at 06:29:55PM +0200, Johan Hovold wrote:
> > > Rework the pm8008 binding by dropping internal details like register
> > > offsets and interrupts and by adding the missing regulator and
> > > temperature alarm properties.
> > > 
> > > Note that child nodes are still used for pinctrl and regulator
> > > configuration.
> > > 
> > > Also note that the pinctrl state definition will be extended later and
> > > could eventually also be shared with other PMICs (e.g. by breaking out
> > > bits of qcom,pmic-gpio.yaml).
> > 
> > Obviously we want to adapt this style of bindings for the other PMICs
> > too. My main concern here are PMICs which have two kinds of controlled
> > pins: GPIOs and MPPs. With the existing bindings style those are
> > declared as two subdevices. What would be your suggested way to support
> > MPPs with the proposed kind of bindings?
> 
> As far as I understand newer PMICs do not have MPP blocks and we do not
> necessarily want to convert the existing bindings.

Well, I definitely want to do so.

> That said, if there is ever a need to describe two separate gpio blocks
> this can, for example, be done using subnodes on those PMICs.

This creates an asymmetry between older and newer PMICs. Wouldn't it be
better to always use gpios subnode for GPIO pins? This way older PMICS
will use the same approach _plus_ mpps {} subnode instead of having
either nothing or two subnodes.

The same approach probably applies to some other subdevices: temp-alarm
vs adc-tm, etc.

-- 
With best wishes
Dmitry