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

[PATCH 00/13] 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 drops the
broken GPIO support for PM8008 which had already been upstreamed. The
last four 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


Johan Hovold (12):
  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
  dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008
  pinctrl: qcom: spmi-gpio: drop broken pm8008 support
  dt-bindings: mfd: pm8008: rework binding
  mfd: pm8008: rework driver
  arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

Satya Priya (1):
  regulator: add pm8008 pmic regulator driver

 .../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                     | 163 ++++++++-----
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c      |   1 -
 drivers/regulator/Kconfig                     |   7 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/qcom-pm8008-regulator.c     | 215 ++++++++++++++++++
 include/dt-bindings/mfd/qcom-pm8008.h         |  19 --
 10 files changed, 554 insertions(+), 137 deletions(-)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
 delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h

-- 
2.43.2

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

[Johan Hovold]

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

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.43.2

[PATCH 02/13] 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 | 66 +++++++++++++++------------------------
 1 file changed, 25 insertions(+), 41 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 3ac3742f438b..d53c987b0d49 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -56,15 +56,27 @@ 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),	\
+		},					\
+	}
+
+#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)
+
+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_ALL),
+	_IRQ(PM8008_IRQ_GPIO1,	      PM8008_GPIO1,	BIT(0), _IRQ_TYPE_ALL),
+	_IRQ(PM8008_IRQ_GPIO2,	      PM8008_GPIO2,	BIT(0), _IRQ_TYPE_ALL),
 };
 
 static const unsigned int pm8008_periph_base[] = {
@@ -143,38 +155,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 +170,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.43.2

[PATCH 03/13] 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.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index d53c987b0d49..d0f190c2ea2b 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>
@@ -158,6 +159,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;
@@ -169,6 +171,10 @@ 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);
+
 	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.43.2

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

[Johan Hovold]

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

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 d0f190c2ea2b..42dd4bf039c9 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,
@@ -131,7 +131,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.43.2

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

[Johan Hovold]

Use lower case hex notation for consistency.

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 42dd4bf039c9..f1c68b3da1b6 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
@@ -153,7 +153,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.43.2

[PATCH 06/13] mfd: pm8008: rename irq chip

[Johan Hovold]

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

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 f1c68b3da1b6..a04bae52a49a 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -132,7 +132,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.43.2

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

[Johan Hovold]

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

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 a04bae52a49a..c7a4f8a60cd4 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -169,8 +169,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.43.2

[PATCH 08/13] 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.

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.43.2

[PATCH 09/13] 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
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.43.2

[PATCH 10/13] 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  | 154 ++++++++++--------
 1 file changed, 90 insertions(+), 64 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index e1e05921afb4..ac1bab0261b6 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -19,116 +19,142 @@ properties:
     const: qcom,pm8008
 
   reg:
-    description:
-      I2C slave address.
-
     maxItems: 1
 
   interrupts:
     maxItems: 1
 
-    description: Parent interrupt.
-
   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.43.2

[PATCH 11/13] 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 two).

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's returned by default when
no name is provided in lookups.

Finally, note that the current QPNP GPIO and temperature alarm
subdrivers need some minor rework before they can be used with non-SPMI
devices like the PM8008. The MFD cell names therefore use a "qpnp"
rather than "spmi" prefix to prevent binding until the drivers have been
updated.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/Kconfig                   |  1 +
 drivers/mfd/qcom-pm8008.c             | 95 +++++++++++++++++++++++----
 include/dt-bindings/mfd/qcom-pm8008.h | 19 ------
 3 files changed, 85 insertions(+), 30 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 c7a4f8a60cd4..706a725428dd 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,
@@ -150,21 +160,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("qcom-pm8008-regulator"),
+	MFD_CELL_RES("qpnp-temp-alarm", pm8008_temp_res),
+	MFD_CELL_NAME("qpnp-gpio"),
+};
+
+static void devm_irq_domain_fwnode_release(void *res)
+{
+	struct fwnode_handle *fwnode = res;
+
+	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;
+	char *name;
 	int rc;
-	struct device *dev;
-	struct regmap *regmap;
 
-	dev = &client->dev;
+	dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1);
+	if (IS_ERR(dummy)) {
+		rc = PTR_ERR(dummy);
+		dev_err(&client->dev, "failed to claim second address: %d\n", rc);
+		return rc;
+	}
+
+	regmap2 = devm_regmap_init_i2c(dummy, &qcom_mfd_regmap_cfg);
+	if (IS_ERR(regmap2))
+		return PTR_ERR(regmap2);
+
+	rc = regmap_attach_dev(dev, regmap2, &pm8008_regmap_cfg_2);
+	if (rc)
+		return rc;
+
+	/* Default regmap must be attached last. */
 	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
@@ -173,14 +227,33 @@ static int pm8008_probe(struct i2c_client *client)
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 
-	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;
+
+	rc = devm_add_action_or_reset(dev, devm_irq_domain_fwnode_release, fwnode);
+	if (rc)
+		return rc;
+
+	rc = 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 (rc) {
+		dev_err(dev, "failed to add IRQ chip: %d\n", rc);
+		return rc;
 	}
 
-	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.43.2

[PATCH 12/13] regulator: add pm8008 pmic regulator driver

[Johan Hovold]

From: Satya Priya <quic_c_skakit@quicinc.com>

Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
seven LDO regulators. Add a PM8008 regulator driver to support PMIC
regulator management via the regulator framework.

Note that this driver, originally submitted by Satya Priya [1], has been
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 a
driver lookup table matching on the parent compatible.

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.

[1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Cc: Stephen Boyd <swboyd@chromium.org>
[ johan: rework probe to match new binding, amend commit message and
         Kconfig entry]
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/regulator/Kconfig                 |   7 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-pm8008-regulator.c | 215 ++++++++++++++++++++++
 3 files changed, 223 insertions(+)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7db0a29b5b8d..d07d034ef1a2 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 Technologies, Inc. PM8008 PMIC regulators"
+	depends on MFD_QCOM_PM8008
+	help
+	  Select this option to enable support for the voltage regulators in
+	  Qualcomm Technologies, Inc. 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..51f1ce5e043c
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,215 @@
+// 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.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#define VSET_STEP_MV			8
+#define VSET_STEP_UV			(VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base)		((base) + 0x46)
+#define ENABLE_BIT			BIT(7)
+
+#define LDO_VSET_LB_REG(base)		((base) + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base)	((base) + 0x3b)
+#define DEFAULT_VOLTAGE_STEPPER_RATE	38400
+#define STEP_RATE_MASK			GENMASK(1, 0)
+
+#define NLDO_MIN_UV			528000
+#define NLDO_MAX_UV			1504000
+
+#define PLDO_MIN_UV			1504000
+#define PLDO_MAX_UV			3400000
+
+struct pm8008_regulator_data {
+	const char			*name;
+	const char			*supply_name;
+	u16				base;
+	int				min_dropout_uv;
+	const struct linear_range	*voltage_range;
+};
+
+struct pm8008_regulator {
+	struct regmap		*regmap;
+	struct regulator_desc	rdesc;
+	u16			base;
+	int			step_rate;
+};
+
+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 reg_data[] = {
+	/* name	  parent       base    headroom_uv voltage_range */
+	{ "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_get_voltage(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	__le16 mV;
+	int uV;
+
+	regmap_bulk_read(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+
+	uV = le16_to_cpu(mV) * 1000;
+	return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+							int mV)
+{
+	__le16 vset_raw;
+
+	vset_raw = cpu_to_le16(mV);
+
+	return regmap_bulk_write(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base),
+			(const void *)&vset_raw, sizeof(vset_raw));
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+				int old_uV, int new_uv)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+	return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+					unsigned int selector)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc, mV;
+
+	rc = regulator_list_voltage_linear_range(rdev, selector);
+	if (rc < 0)
+		return rc;
+
+	/* voltage control register is set with voltage in millivolts */
+	mV = DIV_ROUND_UP(rc, 1000);
+
+	rc = pm8008_write_voltage(pm8008_reg, mV);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.set_voltage_sel	= pm8008_regulator_set_voltage,
+	.get_voltage_sel	= pm8008_regulator_get_voltage,
+	.list_voltage		= regulator_list_voltage_linear,
+	.set_voltage_time	= pm8008_regulator_set_voltage_time,
+};
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+	struct regulator_config reg_config = {};
+	struct pm8008_regulator *pm8008_reg;
+	struct device *dev = &pdev->dev;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+	unsigned int val;
+	int rc, i;
+
+	regmap = dev_get_regmap(dev->parent, "secondary");
+	if (!regmap)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
+		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+		if (!pm8008_reg)
+			return -ENOMEM;
+
+		pm8008_reg->regmap = regmap;
+		pm8008_reg->base = reg_data[i].base;
+
+		/* get slew rate */
+		rc = regmap_bulk_read(pm8008_reg->regmap,
+				LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
+		if (rc < 0) {
+			dev_err(dev, "failed to read step rate: %d\n", rc);
+			return rc;
+		}
+		val &= STEP_RATE_MASK;
+		pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
+
+		rdesc = &pm8008_reg->rdesc;
+		rdesc->type = REGULATOR_VOLTAGE;
+		rdesc->ops = &pm8008_regulator_ops;
+		rdesc->name = reg_data[i].name;
+		rdesc->supply_name = reg_data[i].supply_name;
+		rdesc->of_match = reg_data[i].name;
+		rdesc->uV_step = VSET_STEP_UV;
+		rdesc->linear_ranges = reg_data[i].voltage_range;
+		rdesc->n_linear_ranges = 1;
+		BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
+				(ARRAY_SIZE(nldo_ranges) != 1));
+
+		if (reg_data[i].voltage_range == nldo_ranges) {
+			rdesc->min_uV = NLDO_MIN_UV;
+			rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
+		} else {
+			rdesc->min_uV = PLDO_MIN_UV;
+			rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
+		}
+
+		rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
+		rdesc->enable_mask = ENABLE_BIT;
+		rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
+		rdesc->regulators_node = of_match_ptr("regulators");
+
+		reg_config.dev = dev->parent;
+		reg_config.driver_data = pm8008_reg;
+		reg_config.regmap = pm8008_reg->regmap;
+
+		rdev = devm_regulator_register(dev, rdesc, &reg_config);
+		if (IS_ERR(rdev)) {
+			rc = PTR_ERR(rdev);
+			dev_err(dev, "failed to register regulator %s: %d\n",
+					reg_data[i].name, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static struct platform_driver pm8008_regulator_driver = {
+	.driver	= {
+		.name = "qcom-pm8008-regulator",
+	},
+	.probe = pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom-pm8008-regulator");
-- 
2.43.2

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

[Johan Hovold]

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

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..78d85e722ab1 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.43.2

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

[Andy Shevchenko]

Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> The regmap irq array is potentially shared between multiple PMICs and

IRQ

> should only contain static data.
> 
> Use a custom macro to initialise also the type fields and drop the
> unnecessary updates on each probe.

...

> +#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)

This is repetition of IRQ_TYPE_DEFAULT.

...

> -			dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> +			dev_err(dev, "failed to add IRQ chip: %d\n", rc);

dev_err_probe(...); ?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti:
> 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.

...

> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);

Shouldn't you wait a bit to make chip settle down?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> From: Satya Priya <quic_c_skakit@quicinc.com>
> 
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
> 
> Note that this driver, originally submitted by Satya Priya [1], has been
> 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 a
> driver lookup table matching on the parent compatible.
> 
> 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.

> [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com

Make it Link: tag?

Link: URL [1]

...

> [ johan: rework probe to match new binding, amend commit message and
>          Kconfig entry]

Wouldn't be better on one line?

...

+ array_size.h
+ bits.h

> +#include <linux/device.h>

> +#include <linux/kernel.h>

What is this header for?

+ math.h

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

+ asm/byteorder.h

...

> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	__le16 mV;
> +	int uV;
> +
> +	regmap_bulk_read(pm8008_reg->regmap,
> +			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);

Why casting?

> +	uV = le16_to_cpu(mV) * 1000;
> +	return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> +							int mV)
> +{
> +	__le16 vset_raw;
> +
> +	vset_raw = cpu_to_le16(mV);

Can be joined to a single line.

> +	return regmap_bulk_write(pm8008_reg->regmap,
> +			LDO_VSET_LB_REG(pm8008_reg->base),
> +			(const void *)&vset_raw, sizeof(vset_raw));

Why casting?

> +}

...

> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> +					unsigned int selector)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc, mV;
> +
> +	rc = regulator_list_voltage_linear_range(rdev, selector);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* voltage control register is set with voltage in millivolts */
> +	mV = DIV_ROUND_UP(rc, 1000);

> +	rc = pm8008_write_voltage(pm8008_reg, mV);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;

	return pm8008_write_voltage(pm8008_reg, mV);

?

> +}

> +
> +	regmap = dev_get_regmap(dev->parent, "secondary");
> +	if (!regmap)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +		if (!pm8008_reg)
> +			return -ENOMEM;
> +
> +		pm8008_reg->regmap = regmap;
> +		pm8008_reg->base = reg_data[i].base;
> +
> +		/* get slew rate */
> +		rc = regmap_bulk_read(pm8008_reg->regmap,
> +				LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> +		if (rc < 0) {
> +			dev_err(dev, "failed to read step rate: %d\n", rc);
> +			return rc;

			return dev_err_probe(...);

> +		}
> +		val &= STEP_RATE_MASK;
> +		pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;

> +		rdesc = &pm8008_reg->rdesc;
> +		rdesc->type = REGULATOR_VOLTAGE;
> +		rdesc->ops = &pm8008_regulator_ops;
> +		rdesc->name = reg_data[i].name;
> +		rdesc->supply_name = reg_data[i].supply_name;
> +		rdesc->of_match = reg_data[i].name;
> +		rdesc->uV_step = VSET_STEP_UV;
> +		rdesc->linear_ranges = reg_data[i].voltage_range;
> +		rdesc->n_linear_ranges = 1;
> +		BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> +				(ARRAY_SIZE(nldo_ranges) != 1));
> +
> +		if (reg_data[i].voltage_range == nldo_ranges) {
> +			rdesc->min_uV = NLDO_MIN_UV;
> +			rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> +		} else {
> +			rdesc->min_uV = PLDO_MIN_UV;
> +			rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> +		}
> +
> +		rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> +		rdesc->enable_mask = ENABLE_BIT;
> +		rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> +		rdesc->regulators_node = of_match_ptr("regulators");
> +
> +		reg_config.dev = dev->parent;
> +		reg_config.driver_data = pm8008_reg;
> +		reg_config.regmap = pm8008_reg->regmap;
> +
> +		rdev = devm_regulator_register(dev, rdesc, &reg_config);
> +		if (IS_ERR(rdev)) {

> +			rc = PTR_ERR(rdev);
> +			dev_err(dev, "failed to register regulator %s: %d\n",
> +					reg_data[i].name, rc);
> +			return rc;

			return dev_err_probe(...);

> +		}
> +	}
> +
> +	return 0;
> +}

...

> +static struct platform_driver pm8008_regulator_driver = {
> +	.driver	= {
> +		.name = "qcom-pm8008-regulator",
> +	},
> +	.probe = pm8008_regulator_probe,
> +};

> +

Unneeded blank line.

> +module_platform_driver(pm8008_regulator_driver);

...

> +MODULE_ALIAS("platform:qcom-pm8008-regulator");

Use ID table instead.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Andy Shevchenko]

Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:
> 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 two).
> 
> 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's returned by default when
> no name is provided in lookups.
> 
> Finally, note that the current QPNP GPIO and temperature alarm
> subdrivers need some minor rework before they can be used with non-SPMI
> devices like the PM8008. The MFD cell names therefore use a "qpnp"
> rather than "spmi" prefix to prevent binding until the drivers have been
> updated.

...

> +static void devm_irq_domain_fwnode_release(void *res)
> +{

> +	struct fwnode_handle *fwnode = res;

Unneeded line, can be

static void devm_irq_domain_fwnode_release(void *fwnode)

> +	irq_domain_free_fwnode(fwnode);
> +}

...

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

		return dev_err_probe(...);

> +	}

...

> +	name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);

You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?

	name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));

> +	if (!name)
> +		return -ENOMEM;
> +
> +	name = strreplace(name, '/', ':');

> +	fwnode = irq_domain_alloc_named_fwnode(name);
> +	if (!fwnode)
> +		return -ENOMEM;

...

> +	rc = 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 (rc) {
> +		dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> +		return rc;

		return dev_err_probe(...);

>  	}

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring (Arm)]

On Mon, 06 May 2024 17:08:17 +0200, 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).
> 
> 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 drops the
> broken GPIO support for PM8008 which had already been upstreamed. The
> last four 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
> 
> 
> Johan Hovold (12):
>   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
>   dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008
>   pinctrl: qcom: spmi-gpio: drop broken pm8008 support
>   dt-bindings: mfd: pm8008: rework binding
>   mfd: pm8008: rework driver
>   arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic
> 
> Satya Priya (1):
>   regulator: add pm8008 pmic regulator driver
> 
>  .../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                     | 163 ++++++++-----
>  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c      |   1 -
>  drivers/regulator/Kconfig                     |   7 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/qcom-pm8008-regulator.c     | 215 ++++++++++++++++++
>  include/dt-bindings/mfd/qcom-pm8008.h         |  19 --
>  10 files changed, 554 insertions(+), 137 deletions(-)
>  create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
>  delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h
> 
> --
> 2.43.2
> 
> 
> 


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

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

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

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y qcom/sc8280xp-lenovo-thinkpad-x13s.dtb' for 20240506150830.23709-1-johan+linaro@kernel.org:

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a4f8800: interrupts-extended: [[1, 0, 130, 4], [1, 0, 135, 4], [1, 0, 857, 4], [1, 0, 856, 4], [1, 0, 131, 4], [1, 0, 136, 4], [1, 0, 860, 4], [1, 0, 859, 4], [136, 127, 3], [136, 126, 3], [136, 129, 3], [136, 128, 3], [136, 131, 3], [136, 130, 3], [136, 133, 3], [136, 132, 3], [136, 16, 4], [136, 17, 4]] is too long
	from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#

Re: [PATCH 01/13] dt-bindings: mfd: pm8008: add reset gpio

[Krzysztof Kozlowski]

On 06/05/2024 17:08, Johan Hovold wrote:
> Describe the optional reset gpio (which may not be wired up).
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 08/13] dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008

[Krzysztof Kozlowski]

On 06/05/2024 17:08, 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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml | 3 ---
>  1 file changed, 3 deletions(-)

Dropping compatibles from bindings must happen after they are dropped
from kernel, so this should go after spmi-gpio patch.

Best regards,
Krzysztof

Re: [PATCH 10/13] dt-bindings: mfd: pm8008: rework binding

[Krzysztof Kozlowski]

On 06/05/2024 17:08, 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).
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml  | 154 ++++++++++--------
>  1 file changed, 90 insertions(+), 64 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index e1e05921afb4..ac1bab0261b6 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -19,116 +19,142 @@ properties:
>      const: qcom,pm8008
>  
>    reg:
> -    description:
> -      I2C slave address.

Please split cleanups from actual functional/content rework.

> -
>      maxItems: 1
>  
>    interrupts:
>      maxItems: 1
>  
> -    description: Parent interrupt.
> -
>    reset-gpios:
>      maxItems: 1
>  
> -  "#interrupt-cells":
> +  vdd_l1_l2-supply: true

No underscores in property names.



Best regards,
Krzysztof

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

[Konrad Dybcio]

On 5/6/24 17:08, Johan Hovold wrote:
> From: Satya Priya <quic_c_skakit@quicinc.com>
> 
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
> 
> Note that this driver, originally submitted by Satya Priya [1], has been
> 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 a
> driver lookup table matching on the parent compatible.
> 
> 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.
> 
> [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> [ johan: rework probe to match new binding, amend commit message and
>           Kconfig entry]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
generic.. Would you know whether this code will also be used for e.g.
PM8010?

Konrad

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

[Johan Hovold]

On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > The regmap irq array is potentially shared between multiple PMICs and
> 
> IRQ

I'm referring to an array of struct regmap_irq. Perhaps I can add an
underscore.
 
> > should only contain static data.
> > 
> > Use a custom macro to initialise also the type fields and drop the
> > unnecessary updates on each probe.
> 
> ...
> 
> > +#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)
> 
> This is repetition of IRQ_TYPE_DEFAULT.

Thanks, I guess I should use IRQ_TYPE_SENSE_MASK here even.

> ...
> 
> > -			dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > +			dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> 
> dev_err_probe(...); ?

This function won't return -EPROBE_DEFER, and that would be a separate
change in any case.

Johan

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

[Johan Hovold]

On Mon, May 06, 2024 at 09:57:07PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti:
> > 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.
> 
> ...
> 
> > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(reset))
> > +		return PTR_ERR(reset);
> 
> Shouldn't you wait a bit to make chip settle down?

Yeah, probably. I actually asserted reset here for a while (e.g. to
reset the address), but didn't have to use a post-reset delay then.

I'll see if I can find someone with access to a datasheet or maybe add
some small delay here anyway.

Johan

Re: [PATCH 10/13] dt-bindings: mfd: pm8008: rework binding

[Johan Hovold]

On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote:
> On 06/05/2024 17:08, 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).
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
 
> >    reg:
> > -    description:
> > -      I2C slave address.
> 
> Please split cleanups from actual functional/content rework.

Sure.

> > -
> >      maxItems: 1
> >  
> >    interrupts:
> >      maxItems: 1
> >  
> > -    description: Parent interrupt.
> > -
> >    reset-gpios:
> >      maxItems: 1
> >  
> > -  "#interrupt-cells":
> > +  vdd_l1_l2-supply: true
> 
> No underscores in property names.

Indeed. These names come from Qualcomm's v15, but I should have caught
that. Thanks.

Johan

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

[Johan Hovold]

On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> > From: Satya Priya <quic_c_skakit@quicinc.com>
> > 
> > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > regulator management via the regulator framework.
> > 
> > Note that this driver, originally submitted by Satya Priya [1], has been
> > reworked to match the new devicetree binding which no longer describes
> > each regulator as a separate device.

> > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> 
> Make it Link: tag?
> 
> Link: URL [1]

Sure.

> > [ johan: rework probe to match new binding, amend commit message and
> >          Kconfig entry]
> 
> Wouldn't be better on one line?

Now you're really nit picking. ;) I think I prefer to stay within 72
columns.

> + array_size.h
> + bits.h

Ok.

> > +#include <linux/device.h>
> 
> > +#include <linux/kernel.h>
> 
> What is this header for?

Probably the ones that are not explicitly included.

> + math.h
> 
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> 
> + asm/byteorder.h

Ok, thanks.

> > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > +	__le16 mV;
> > +	int uV;
> > +
> > +	regmap_bulk_read(pm8008_reg->regmap,
> > +			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> 
> Why casting?

I tried not change things in the v15 from Qualcomm that I based this
on. I couldn't help cleaning up a few things in probe, which I was
touching anyway, but I left it there.

I'll drop the unnecessary cast.

> > +	uV = le16_to_cpu(mV) * 1000;
> > +	return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > +}
> > +
> > +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> > +							int mV)
> > +{
> > +	__le16 vset_raw;
> > +
> > +	vset_raw = cpu_to_le16(mV);
> 
> Can be joined to a single line.

Sure.

> > +	return regmap_bulk_write(pm8008_reg->regmap,
> > +			LDO_VSET_LB_REG(pm8008_reg->base),
> > +			(const void *)&vset_raw, sizeof(vset_raw));
> 
> Why casting?

Same answer as above. Will drop.

> > +}
> 
> ...
> 
> > +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> > +					unsigned int selector)
> > +{
> > +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > +	int rc, mV;
> > +
> > +	rc = regulator_list_voltage_linear_range(rdev, selector);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* voltage control register is set with voltage in millivolts */
> > +	mV = DIV_ROUND_UP(rc, 1000);
> 
> > +	rc = pm8008_write_voltage(pm8008_reg, mV);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	return 0;
> 
> 	return pm8008_write_voltage(pm8008_reg, mV);

Possibly, but I tend to prefer explicit error paths.

> > +}
> 
> > +
> > +	regmap = dev_get_regmap(dev->parent, "secondary");
> > +	if (!regmap)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> > +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> > +		if (!pm8008_reg)
> > +			return -ENOMEM;
> > +
> > +		pm8008_reg->regmap = regmap;
> > +		pm8008_reg->base = reg_data[i].base;
> > +
> > +		/* get slew rate */
> > +		rc = regmap_bulk_read(pm8008_reg->regmap,
> > +				LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> > +		if (rc < 0) {
> > +			dev_err(dev, "failed to read step rate: %d\n", rc);
> > +			return rc;
> 
> 			return dev_err_probe(...);

Nah, regmap won't trigger a probe deferral.

> > +static struct platform_driver pm8008_regulator_driver = {
> > +	.driver	= {
> > +		.name = "qcom-pm8008-regulator",
> > +	},
> > +	.probe = pm8008_regulator_probe,
> > +};
> 
> > +
> 
> Unneeded blank line.

I noticed that one too, but such things are up the author to decide.

> > +module_platform_driver(pm8008_regulator_driver);
> 
> ...
> 
> > +MODULE_ALIAS("platform:qcom-pm8008-regulator");
> 
> Use ID table instead.

No, the driver is not using an id-table for matching so the alias is
needed for module auto-loading.

Johan

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

[Johan Hovold]

On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> On 5/6/24 17:08, Johan Hovold wrote:
> > From: Satya Priya <quic_c_skakit@quicinc.com>
> > 
> > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > regulator management via the regulator framework.
> > 
> > Note that this driver, originally submitted by Satya Priya [1], has been
> > 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 a
> > driver lookup table matching on the parent compatible.
> > 
> > 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.
> > 
> > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> > 
> > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > [ johan: rework probe to match new binding, amend commit message and
> >           Kconfig entry]
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> 
> I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> generic.. Would you know whether this code will also be used for e.g.
> PM8010?

Yes, for any sufficiently similar PMICs, including SPMI ones. So
'qpnp-regulator' would be a generic name, but only Qualcomm knows what
PMICs they have and how they are related -- the rest of us is left doing
tedious code forensics to try to make some sense of this.

So just like for compatible strings, letting the first supported PMIC
name the driver makes sense as we don't know when we'll want to add a
second one for another set of devices (and we don't want to call that
one 'qpnp-regulator-2'). On the other hand, these names are now mostly
internal and can more easily be renamed later.

Johan

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

[Andy Shevchenko]

On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > The regmap irq array is potentially shared between multiple PMICs and

...

> > > -                   dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > +                   dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> >
> > dev_err_probe(...); ?
>
> This function won't return -EPROBE_DEFER,

This is not an argument for a long time (since documentation of
dev_err_probe() had been amended to encourage its use for any error
cases in probe).

> and that would be a separate
> change in any case.

Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:

...

> > > [ johan: rework probe to match new binding, amend commit message and
> > >          Kconfig entry]
> >
> > Wouldn't be better on one line?
>
> Now you're really nit picking. ;) I think I prefer to stay within 72
> columns.

Not really. The tag block is special and the format is rather one
entry per line. This might break some scriptings.

...

> > > +#include <linux/kernel.h>
> >
> > What is this header for?
>
> Probably the ones that are not explicitly included.

Please, remove it, it's a mess nowadays and most of what you need is
available via other headers.

...

> >                       return dev_err_probe(...);
>
> Nah, regmap won't trigger a probe deferral.

And it doesn't matter. What we gain with dev_err_probe() is:
- special handling of deferred probe
- unified format of messages in ->probe() stage

The second one is encouraged.

...

> > > +MODULE_ALIAS("platform:qcom-pm8008-regulator");
> >
> > Use ID table instead.
>
> No, the driver is not using an id-table for matching so the alias is
> needed for module auto-loading.

Then create one. Added Krzysztof for that. (He is working on dropping
MODULE_ALIAS() in cases like this one)

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

On 07/05/2024 19:22, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
>> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
>>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> 
> ...
> 
>>>> [ johan: rework probe to match new binding, amend commit message and
>>>>          Kconfig entry]
>>>
>>> Wouldn't be better on one line?
>>
>> Now you're really nit picking. ;) I think I prefer to stay within 72
>> columns.
> 
> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.
> 
> ...

I think [] can be wrapped, I saw it at least many times and I use as well...

...

> ...
> 
>>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
>>>
>>> Use ID table instead.
>>
>> No, the driver is not using an id-table for matching so the alias is
>> needed for module auto-loading.
> 
> Then create one. Added Krzysztof for that. (He is working on dropping
> MODULE_ALIAS() in cases like this one)

Yeah, please use ID table, since this is a driver (unless I missed
something). Module alias does not scale, leads to stale and duplicated
entries, so should not be used as substitute of ID table. Alias is
suitable for different cases.

Best regards,
Krzysztof

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

[Mark Brown]

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

On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:

> > > > [ johan: rework probe to match new binding, amend commit message and
> > > >          Kconfig entry]

> > > Wouldn't be better on one line?

> > Now you're really nit picking. ;) I think I prefer to stay within 72
> > columns.

> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.

No, really - the above is absolutely fine, random notes in the middle of
things are reasonably common and scripting that can't cope with them is
going to encounter a bunch of problems.  This stuff needs to be readable
by humans and this is just a stylistic preference on your part.

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

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

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold 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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/mfd/qcom-pm8008.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index d53c987b0d49..d0f190c2ea2b 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>
> @@ -158,6 +159,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;
> @@ -169,6 +171,10 @@ 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);
> +
>   	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);

So not resetting is fine and I understand you want to retain the address 
given by the firmware, I think that's the right thing to do.

You can add this now

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

In addition to adding a small delay suggested by Andy - a few 
microseconds pick a number, I think you should verify the chip is out of 
reset as we would do with many other i2c devices.

A common pattern with i2c devices is take the device out of reset then 
read back an identity register as a smoke test.

Take drivers/media/i2c/ov8856.c::ov8856_identify_module() as an example.

In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and 
REVID_PERPH_SUBTYPE @ 0x105

REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)

You can then add my

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

---
bod

Re: [PATCH 04/13] mfd: pm8008: mark regmap structures as const

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> The regmap irq chip structures can be const so mark them as such.
> 
> 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 d0f190c2ea2b..42dd4bf039c9 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,
> @@ -131,7 +131,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,

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

Re: [PATCH 05/13] mfd: pm8008: use lower case hex notation

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> Use lower case hex notation for consistency.
> 
> 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 42dd4bf039c9..f1c68b3da1b6 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
> @@ -153,7 +153,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)

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

Re: [PATCH 06/13] mfd: pm8008: rename irq chip

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> Drop the redundant "irq" suffix from the irq chip name.
> 
> 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 f1c68b3da1b6..a04bae52a49a 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -132,7 +132,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,

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

Re: [PATCH 07/13] mfd: pm8008: drop unused driver data

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> The i2c client driver data pointer has never been used so drop the
> unnecessary assignment.
> 
> 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 a04bae52a49a..c7a4f8a60cd4 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -169,8 +169,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);

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

Re: [PATCH 09/13] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold 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
> 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 },

Since there are no upstream dtsi users there's no harm in dropping and 
re-adding when i2c can be taken account of.

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

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

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> Enable the PM8008 PMIC which is used to power the camera sensors.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

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

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

[Bryan O'Donoghue]

On 06/05/2024 16:08, Johan Hovold wrote:
> From: Satya Priya <quic_c_skakit@quicinc.com>
> 
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
> 
> Note that this driver, originally submitted by Satya Priya [1], has been
> 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 a
> driver lookup table matching on the parent compatible.
> 
> 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.
> 
> [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> [ johan: rework probe to match new binding, amend commit message and
>           Kconfig entry]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/regulator/Kconfig                 |   7 +
>   drivers/regulator/Makefile                |   1 +
>   drivers/regulator/qcom-pm8008-regulator.c | 215 ++++++++++++++++++++++
>   3 files changed, 223 insertions(+)
>   create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 7db0a29b5b8d..d07d034ef1a2 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 Technologies, Inc. PM8008 PMIC regulators"
> +	depends on MFD_QCOM_PM8008
> +	help
> +	  Select this option to enable support for the voltage regulators in
> +	  Qualcomm Technologies, Inc. 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..51f1ce5e043c
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,215 @@
> +// 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +
> +#define VSET_STEP_MV			8
> +#define VSET_STEP_UV			(VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base)		((base) + 0x46)
> +#define ENABLE_BIT			BIT(7)
> +
> +#define LDO_VSET_LB_REG(base)		((base) + 0x40)
> +
> +#define LDO_STEPPER_CTL_REG(base)	((base) + 0x3b)
> +#define DEFAULT_VOLTAGE_STEPPER_RATE	38400
> +#define STEP_RATE_MASK			GENMASK(1, 0)
> +
> +#define NLDO_MIN_UV			528000
> +#define NLDO_MAX_UV			1504000
> +
> +#define PLDO_MIN_UV			1504000
> +#define PLDO_MAX_UV			3400000
> +
> +struct pm8008_regulator_data {
> +	const char			*name;
> +	const char			*supply_name;
> +	u16				base;
> +	int				min_dropout_uv;
> +	const struct linear_range	*voltage_range;
> +};
> +
> +struct pm8008_regulator {
> +	struct regmap		*regmap;
> +	struct regulator_desc	rdesc;
> +	u16			base;
> +	int			step_rate;
> +};
> +
> +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 reg_data[] = {
> +	/* name	  parent       base    headroom_uv voltage_range */
> +	{ "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_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	__le16 mV;
> +	int uV;
> +
> +	regmap_bulk_read(pm8008_reg->regmap,
> +			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> +
> +	uV = le16_to_cpu(mV) * 1000;
> +	return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> +							int mV)
> +{
> +	__le16 vset_raw;
> +
> +	vset_raw = cpu_to_le16(mV);
> +
> +	return regmap_bulk_write(pm8008_reg->regmap,
> +			LDO_VSET_LB_REG(pm8008_reg->base),
> +			(const void *)&vset_raw, sizeof(vset_raw));
> +}
> +
> +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
> +				int old_uV, int new_uv)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> +	return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
> +}
> +
> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> +					unsigned int selector)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc, mV;
> +
> +	rc = regulator_list_voltage_linear_range(rdev, selector);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* voltage control register is set with voltage in millivolts */
> +	mV = DIV_ROUND_UP(rc, 1000);
> +
> +	rc = pm8008_write_voltage(pm8008_reg, mV);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static const struct regulator_ops pm8008_regulator_ops = {
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.set_voltage_sel	= pm8008_regulator_set_voltage,
> +	.get_voltage_sel	= pm8008_regulator_get_voltage,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.set_voltage_time	= pm8008_regulator_set_voltage_time,
> +};
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +	struct regulator_config reg_config = {};
> +	struct pm8008_regulator *pm8008_reg;
> +	struct device *dev = &pdev->dev;
> +	struct regulator_desc *rdesc;
> +	struct regulator_dev *rdev;
> +	struct regmap *regmap;
> +	unsigned int val;
> +	int rc, i;
> +
> +	regmap = dev_get_regmap(dev->parent, "secondary");
> +	if (!regmap)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +		if (!pm8008_reg)
> +			return -ENOMEM;
> +
> +		pm8008_reg->regmap = regmap;
> +		pm8008_reg->base = reg_data[i].base;
> +
> +		/* get slew rate */
> +		rc = regmap_bulk_read(pm8008_reg->regmap,
> +				LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> +		if (rc < 0) {
> +			dev_err(dev, "failed to read step rate: %d\n", rc);
> +			return rc;
> +		}
> +		val &= STEP_RATE_MASK;
> +		pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> +
> +		rdesc = &pm8008_reg->rdesc;
> +		rdesc->type = REGULATOR_VOLTAGE;
> +		rdesc->ops = &pm8008_regulator_ops;
> +		rdesc->name = reg_data[i].name;
> +		rdesc->supply_name = reg_data[i].supply_name;
> +		rdesc->of_match = reg_data[i].name;
> +		rdesc->uV_step = VSET_STEP_UV;
> +		rdesc->linear_ranges = reg_data[i].voltage_range;
> +		rdesc->n_linear_ranges = 1;
> +		BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> +				(ARRAY_SIZE(nldo_ranges) != 1));
> +
> +		if (reg_data[i].voltage_range == nldo_ranges) {
> +			rdesc->min_uV = NLDO_MIN_UV;
> +			rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> +		} else {
> +			rdesc->min_uV = PLDO_MIN_UV;
> +			rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> +		}
> +
> +		rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> +		rdesc->enable_mask = ENABLE_BIT;
> +		rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> +		rdesc->regulators_node = of_match_ptr("regulators");
> +
> +		reg_config.dev = dev->parent;
> +		reg_config.driver_data = pm8008_reg;
> +		reg_config.regmap = pm8008_reg->regmap;
> +
> +		rdev = devm_regulator_register(dev, rdesc, &reg_config);
> +		if (IS_ERR(rdev)) {
> +			rc = PTR_ERR(rdev);
> +			dev_err(dev, "failed to register regulator %s: %d\n",
> +					reg_data[i].name, rc);
> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver pm8008_regulator_driver = {
> +	.driver	= {
> +		.name = "qcom-pm8008-regulator",
> +	},
> +	.probe = pm8008_regulator_probe,
> +};
> +
> +module_platform_driver(pm8008_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. PM8008 PMIC Regulator Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-pm8008-regulator");

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

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Bryan O'Donoghue]

On 06/05/2024 16:08, 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 two).
> 
> 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's returned by default when
> no name is provided in lookups.
> 
> Finally, note that the current QPNP GPIO and temperature alarm
> subdrivers need some minor rework before they can be used with non-SPMI
> devices like the PM8008. The MFD cell names therefore use a "qpnp"
> rather than "spmi" prefix to prevent binding until the drivers have been
> updated.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/mfd/Kconfig                   |  1 +
>   drivers/mfd/qcom-pm8008.c             | 95 +++++++++++++++++++++++----
>   include/dt-bindings/mfd/qcom-pm8008.h | 19 ------
>   3 files changed, 85 insertions(+), 30 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 c7a4f8a60cd4..706a725428dd 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,
> @@ -150,21 +160,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("qcom-pm8008-regulator"),
> +	MFD_CELL_RES("qpnp-temp-alarm", pm8008_temp_res),
> +	MFD_CELL_NAME("qpnp-gpio"),
> +};
> +
> +static void devm_irq_domain_fwnode_release(void *res)
> +{
> +	struct fwnode_handle *fwnode = res;
> +
> +	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;
> +	char *name;
>   	int rc;
> -	struct device *dev;
> -	struct regmap *regmap;
>   
> -	dev = &client->dev;
> +	dummy = devm_i2c_new_dummy_device(dev, client->adapter, client->addr + 1);
> +	if (IS_ERR(dummy)) {
> +		rc = PTR_ERR(dummy);
> +		dev_err(&client->dev, "failed to claim second address: %d\n", rc);
> +		return rc;
> +	}
> +
> +	regmap2 = devm_regmap_init_i2c(dummy, &qcom_mfd_regmap_cfg);
> +	if (IS_ERR(regmap2))
> +		return PTR_ERR(regmap2);
> +
> +	rc = regmap_attach_dev(dev, regmap2, &pm8008_regmap_cfg_2);
> +	if (rc)
> +		return rc;
> +
> +	/* Default regmap must be attached last. */
>   	regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>   	if (IS_ERR(regmap))
>   		return PTR_ERR(regmap);
> @@ -173,14 +227,33 @@ static int pm8008_probe(struct i2c_client *client)
>   	if (IS_ERR(reset))
>   		return PTR_ERR(reset);
>   
> -	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;
> +
> +	rc = devm_add_action_or_reset(dev, devm_irq_domain_fwnode_release, fwnode);
> +	if (rc)
> +		return rc;
> +
> +	rc = 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 (rc) {
> +		dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> +		return rc;
>   	}
>   
> -	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

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

Re: [PATCH 01/13] dt-bindings: mfd: pm8008: add reset gpio

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:18)
> Describe the optional reset gpio (which may not be wired up).
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 04/13] mfd: pm8008: mark regmap structures as const

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:21)
> The regmap irq chip structures can be const so mark them as such.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 05/13] mfd: pm8008: use lower case hex notation

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:22)
> Use lower case hex notation for consistency.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 06/13] mfd: pm8008: rename irq chip

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:23)
> Drop the redundant "irq" suffix from the irq chip name.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 07/13] mfd: pm8008: drop unused driver data

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:24)
> The i2c client driver data pointer has never been used so drop the
> unnecessary assignment.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 08/13] dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:25)
> 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.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 09/13] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:26)
> 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
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 10/13] dt-bindings: mfd: pm8008: rework binding

[Stephen Boyd]

Quoting Johan Hovold (2024-05-07 08:23:04)
> On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote:
> > > -
> > >      maxItems: 1
> > >
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > -    description: Parent interrupt.
> > > -
> > >    reset-gpios:
> > >      maxItems: 1
> > >
> > > -  "#interrupt-cells":
> > > +  vdd_l1_l2-supply: true
> >
> > No underscores in property names.
>
> Indeed. These names come from Qualcomm's v15, but I should have caught
> that. Thanks.

Drive by comment: we have underscores to match the label on the
datasheet. Not sure that will sway your opinion. Only trying to provide
some background rationale.

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

[Stephen Boyd]

Quoting Johan Hovold (2024-05-06 08:08:29)
> From: Satya Priya <quic_c_skakit@quicinc.com>
>
> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> regulator management via the regulator framework.
>
> Note that this driver, originally submitted by Satya Priya [1], has been
> 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.

Thanks. I had to remember this topic.

>
> 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 a
> driver lookup table matching on the parent compatible.
>
> 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.
>
> [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> [ johan: rework probe to match new binding, amend commit message and
>          Kconfig entry]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 000000000000..51f1ce5e043c
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,215 @@
> +// 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +
> +#define VSET_STEP_MV                   8
> +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
> +#define ENABLE_BIT                     BIT(7)
> +
> +#define LDO_VSET_LB_REG(base)          ((base) + 0x40)
> +
> +#define LDO_STEPPER_CTL_REG(base)      ((base) + 0x3b)
> +#define DEFAULT_VOLTAGE_STEPPER_RATE   38400
> +#define STEP_RATE_MASK                 GENMASK(1, 0)

Include bits.h?

> +
> +#define NLDO_MIN_UV                    528000
> +#define NLDO_MAX_UV                    1504000
> +
> +#define PLDO_MIN_UV                    1504000
> +#define PLDO_MAX_UV                    3400000
> +
> +struct pm8008_regulator_data {
> +       const char                      *name;
> +       const char                      *supply_name;
> +       u16                             base;
> +       int                             min_dropout_uv;
> +       const struct linear_range       *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> +       struct regmap           *regmap;
> +       struct regulator_desc   rdesc;
> +       u16                     base;
> +       int                     step_rate;

Is struct regulator_desc::vsel_step usable for this? If not, can it be
unsigned?

> +};
> +
> +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 reg_data[] = {
> +       /* name   parent       base    headroom_uv voltage_range */
> +       { "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_get_voltage(struct regulator_dev *rdev)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +       __le16 mV;
> +       int uV;

Can this be unsigned? Doubt we have negative voltage and this would
match rdesc.min_uV type.

> +
> +       regmap_bulk_read(pm8008_reg->regmap,
> +                       LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);

Is struct regulator_desc::vsel_reg usable for this?

> +
> +       uV = le16_to_cpu(mV) * 1000;
> +       return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> +                                                       int mV)
> +{
> +       __le16 vset_raw;
> +
> +       vset_raw = cpu_to_le16(mV);
> +
> +       return regmap_bulk_write(pm8008_reg->regmap,
> +                       LDO_VSET_LB_REG(pm8008_reg->base),
> +                       (const void *)&vset_raw, sizeof(vset_raw));

Is the cast to please sparse?

> +}
> +
> +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
> +                               int old_uV, int new_uv)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> +       return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
> +}
> +
> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> +                                       unsigned int selector)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +       int rc, mV;
> +
> +       rc = regulator_list_voltage_linear_range(rdev, selector);
> +       if (rc < 0)
> +               return rc;
> +
> +       /* voltage control register is set with voltage in millivolts */
> +       mV = DIV_ROUND_UP(rc, 1000);
> +
> +       rc = pm8008_write_voltage(pm8008_reg, mV);
> +       if (rc < 0)
> +               return rc;
> +
> +       return 0;

Can be shorter to save lines

	return pm8008_write_voltage(pm8008_reg, mV);

> +}
> +
> +static const struct regulator_ops pm8008_regulator_ops = {
> +       .enable                 = regulator_enable_regmap,
> +       .disable                = regulator_disable_regmap,
> +       .is_enabled             = regulator_is_enabled_regmap,
> +       .set_voltage_sel        = pm8008_regulator_set_voltage,
> +       .get_voltage_sel        = pm8008_regulator_get_voltage,
> +       .list_voltage           = regulator_list_voltage_linear,
> +       .set_voltage_time       = pm8008_regulator_set_voltage_time,
> +};
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +       struct regulator_config reg_config = {};
> +       struct pm8008_regulator *pm8008_reg;
> +       struct device *dev = &pdev->dev;
> +       struct regulator_desc *rdesc;
> +       struct regulator_dev *rdev;
> +       struct regmap *regmap;
> +       unsigned int val;
> +       int rc, i;
> +
> +       regmap = dev_get_regmap(dev->parent, "secondary");
> +       if (!regmap)
> +               return -EINVAL;
> +
> +       for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> +               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +               if (!pm8008_reg)
> +                       return -ENOMEM;
> +
> +               pm8008_reg->regmap = regmap;
> +               pm8008_reg->base = reg_data[i].base;
> +
> +               /* get slew rate */
> +               rc = regmap_bulk_read(pm8008_reg->regmap,
> +                               LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> +               if (rc < 0) {
> +                       dev_err(dev, "failed to read step rate: %d\n", rc);

Is it step rate or slew rate? The comment doesn't agree with the error
message.

> +                       return rc;
> +               }
> +               val &= STEP_RATE_MASK;
> +               pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> +
> +               rdesc = &pm8008_reg->rdesc;
> +               rdesc->type = REGULATOR_VOLTAGE;
> +               rdesc->ops = &pm8008_regulator_ops;
> +               rdesc->name = reg_data[i].name;
> +               rdesc->supply_name = reg_data[i].supply_name;
> +               rdesc->of_match = reg_data[i].name;
> +               rdesc->uV_step = VSET_STEP_UV;
> +               rdesc->linear_ranges = reg_data[i].voltage_range;
> +               rdesc->n_linear_ranges = 1;
> +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||

This should be an && not || right?

> +                               (ARRAY_SIZE(nldo_ranges) != 1));
> +
> +               if (reg_data[i].voltage_range == nldo_ranges) {
> +                       rdesc->min_uV = NLDO_MIN_UV;
> +                       rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> +               } else {
> +                       rdesc->min_uV = PLDO_MIN_UV;
> +                       rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> +               }
> +
> +               rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> +               rdesc->enable_mask = ENABLE_BIT;
> +               rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> +               rdesc->regulators_node = of_match_ptr("regulators");
> +
> +               reg_config.dev = dev->parent;
> +               reg_config.driver_data = pm8008_reg;
> +               reg_config.regmap = pm8008_reg->regmap;
> +
> +               rdev = devm_regulator_register(dev, rdesc, &reg_config);
> +               if (IS_ERR(rdev)) {
> +                       rc = PTR_ERR(rdev);
> +                       dev_err(dev, "failed to register regulator %s: %d\n",
> +                                       reg_data[i].name, rc);
> +                       return rc;

Could be return dev_err_probe() to simplify.

Re: [PATCH 10/13] dt-bindings: mfd: pm8008: rework binding

[Krzysztof Kozlowski]

On 09/05/2024 00:09, Stephen Boyd wrote:
> Quoting Johan Hovold (2024-05-07 08:23:04)
>> On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote:
>>>> -
>>>>      maxItems: 1
>>>>
>>>>    interrupts:
>>>>      maxItems: 1
>>>>
>>>> -    description: Parent interrupt.
>>>> -
>>>>    reset-gpios:
>>>>      maxItems: 1
>>>>
>>>> -  "#interrupt-cells":
>>>> +  vdd_l1_l2-supply: true
>>>
>>> No underscores in property names.
>>
>> Indeed. These names come from Qualcomm's v15, but I should have caught
>> that. Thanks.
> 
> Drive by comment: we have underscores to match the label on the
> datasheet. Not sure that will sway your opinion. Only trying to provide
> some background rationale.

I know, but if datasheet calls this "yellow_pony_!!!#1l33t-supply" we
still won't use datasheet names directly, so s/_/-/. That's also W=2
warning.

Best regards,
Krzysztof

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

[Johan Hovold]

[ +To: Krishna ]

On Mon, May 06, 2024 at 03:40:32PM -0500, Rob Herring wrote:
> On Mon, 06 May 2024 17:08:17 +0200, 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).

> My bot found new DTB warnings on the .dts files added or changed in this
> series.
> 
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
> 
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
> 
>   pip3 install dtschema --upgrade
> 
> 
> New warnings running 'make CHECK_DTBS=y qcom/sc8280xp-lenovo-thinkpad-x13s.dtb' for 20240506150830.23709-1-johan+linaro@kernel.org:
> 
> arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a4f8800: interrupts-extended: [[1, 0, 130, 4], [1, 0, 135, 4], [1, 0, 857, 4], [1, 0, 856, 4], [1, 0, 131, 4], [1, 0, 136, 4], [1, 0, 860, 4], [1, 0, 859, 4], [136, 127, 3], [136, 126, 3], [136, 129, 3], [136, 128, 3], [136, 131, 3], [136, 130, 3], [136, 133, 3], [136, 132, 3], [136, 16, 4], [136, 17, 4]] is too long
> 	from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#

This one is unrelated to this series and you should only see it with
linux-next which has:

	80adfb54044e ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport")		
	3170a2c906c6 ("arm64: dts: qcom: sc8280xp: Add USB DWC3 Multiport controller")
	eb24bd3c593f ("arm64: dts: qcom: sc8280xp-x13s: enable USB MP and fingerprint reader")

Apparently you already reported this two weeks ago without anyone
following up:

	https://lore.kernel.org/lkml/171449016553.3484108.5214033788092698309.robh@kernel.org/

I've just sent a fix here:

	https://lore.kernel.org/lkml/20240509083822.397-1-johan+linaro@kernel.org/
	
Johan

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

[Johan Hovold]

On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > > The regmap irq array is potentially shared between multiple PMICs and
> 
> ...
> 
> > > > -                   dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > > +                   dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> > >
> > > dev_err_probe(...); ?
> >
> > This function won't return -EPROBE_DEFER,
> 
> This is not an argument for a long time (since documentation of
> dev_err_probe() had been amended to encourage its use for any error
> cases in probe).

There was apparently a kernel doc update made in December 2023:

	532888a59505 ("driver core: Better advertise dev_err_probe()")

to clarify that people are *allowed* to use it also for functions not
returning -EPROBE_DEFER. That's hardly a long time ago and, importantly,
this is of course still nothing that is *required*.

> > and that would be a separate
> > change in any case.
> 
> Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?

This is not in any way technical debt.

Johan

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

[Johan Hovold]

On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:

> > > > [ johan: rework probe to match new binding, amend commit message and
> > > >          Kconfig entry]
> > >
> > > Wouldn't be better on one line?
> >
> > Now you're really nit picking. ;) I think I prefer to stay within 72
> > columns.
> 
> Not really. The tag block is special and the format is rather one
> entry per line. This might break some scriptings.

This is not a tag, and using line breaks here is perfectly fine.

> > >                       return dev_err_probe(...);
> >
> > Nah, regmap won't trigger a probe deferral.
> 
> And it doesn't matter. What we gain with dev_err_probe() is:
> - special handling of deferred probe
> - unified format of messages in ->probe() stage
> 
> The second one is encouraged.

I don't care about your personal preferences.

Johan

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

[Johan Hovold]

On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
> On 07/05/2024 19:22, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
> >> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> >>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:

> >>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
> >>>
> >>> Use ID table instead.
> >>
> >> No, the driver is not using an id-table for matching so the alias is
> >> needed for module auto-loading.
> > 
> > Then create one. Added Krzysztof for that. (He is working on dropping
> > MODULE_ALIAS() in cases like this one)
> 
> Yeah, please use ID table, since this is a driver (unless I missed
> something). Module alias does not scale, leads to stale and duplicated
> entries, so should not be used as substitute of ID table. Alias is
> suitable for different cases.

There's no scalability issue here. If the driver uses driver name
matching then there will always be exactly one alias needed.

That said, we may use a platform device id table instead of matching on
parent compatible if subdrivers are going to be reused by multiple
devices. I'll consider that.

Johan

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

[Johan Hovold]

On Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd wrote:
> Quoting Johan Hovold (2024-05-06 08:08:29)

> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> > +
> > +#define VSET_STEP_MV                   8
> > +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
> > +
> > +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
> > +#define ENABLE_BIT                     BIT(7)
> > +
> > +#define LDO_VSET_LB_REG(base)          ((base) + 0x40)
> > +
> > +#define LDO_STEPPER_CTL_REG(base)      ((base) + 0x3b)
> > +#define DEFAULT_VOLTAGE_STEPPER_RATE   38400
> > +#define STEP_RATE_MASK                 GENMASK(1, 0)
> 
> Include bits.h?

Sure.

I wanted to avoid changing Qualcomm's v15 driver too much and
essentially submitted it unchanged except for the probe rework. I'll
take closer look at things like this for v2.

> > +struct pm8008_regulator {
> > +       struct regmap           *regmap;
> > +       struct regulator_desc   rdesc;
> > +       u16                     base;
> > +       int                     step_rate;
> 
> Is struct regulator_desc::vsel_step usable for this? If not, can it be
> unsigned?

Not sure, I'll take a look when respinning.

> > +};

> > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > +       __le16 mV;
> > +       int uV;
> 
> Can this be unsigned? Doubt we have negative voltage and this would
> match rdesc.min_uV type.

Makes sense.

> > +
> > +       regmap_bulk_read(pm8008_reg->regmap,
> > +                       LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> 
> Is struct regulator_desc::vsel_reg usable for this?

Will look into that.
 
> > +
> > +       uV = le16_to_cpu(mV) * 1000;
> > +       return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > +}
> > +
> > +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> > +                                                       int mV)
> > +{
> > +       __le16 vset_raw;
> > +
> > +       vset_raw = cpu_to_le16(mV);
> > +
> > +       return regmap_bulk_write(pm8008_reg->regmap,
> > +                       LDO_VSET_LB_REG(pm8008_reg->base),
> > +                       (const void *)&vset_raw, sizeof(vset_raw));
> 
> Is the cast to please sparse?

No idea, I think it's just a stylistic preference that can be dropped.

> > +}

> > +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> > +                                       unsigned int selector)
> > +{
> > +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > +       int rc, mV;
> > +
> > +       rc = regulator_list_voltage_linear_range(rdev, selector);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       /* voltage control register is set with voltage in millivolts */
> > +       mV = DIV_ROUND_UP(rc, 1000);
> > +
> > +       rc = pm8008_write_voltage(pm8008_reg, mV);
> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       return 0;
> 
> Can be shorter to save lines
> 
> 	return pm8008_write_voltage(pm8008_reg, mV);

Possibly, but I tend to prefer explicit error paths (e.g. for symmetry).

> > +}

> > +static int pm8008_regulator_probe(struct platform_device *pdev)
> > +{
> > +       struct regulator_config reg_config = {};
> > +       struct pm8008_regulator *pm8008_reg;
> > +       struct device *dev = &pdev->dev;
> > +       struct regulator_desc *rdesc;
> > +       struct regulator_dev *rdev;
> > +       struct regmap *regmap;
> > +       unsigned int val;
> > +       int rc, i;
> > +
> > +       regmap = dev_get_regmap(dev->parent, "secondary");
> > +       if (!regmap)
> > +               return -EINVAL;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> > +               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> > +               if (!pm8008_reg)
> > +                       return -ENOMEM;
> > +
> > +               pm8008_reg->regmap = regmap;
> > +               pm8008_reg->base = reg_data[i].base;
> > +
> > +               /* get slew rate */
> > +               rc = regmap_bulk_read(pm8008_reg->regmap,
> > +                               LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> > +               if (rc < 0) {
> > +                       dev_err(dev, "failed to read step rate: %d\n", rc);
> 
> Is it step rate or slew rate? The comment doesn't agree with the error
> message.

Noticed that too, can update the comment.

> > +                       return rc;
> > +               }
> > +               val &= STEP_RATE_MASK;
> > +               pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> > +
> > +               rdesc = &pm8008_reg->rdesc;
> > +               rdesc->type = REGULATOR_VOLTAGE;
> > +               rdesc->ops = &pm8008_regulator_ops;
> > +               rdesc->name = reg_data[i].name;
> > +               rdesc->supply_name = reg_data[i].supply_name;
> > +               rdesc->of_match = reg_data[i].name;
> > +               rdesc->uV_step = VSET_STEP_UV;
> > +               rdesc->linear_ranges = reg_data[i].voltage_range;
> > +               rdesc->n_linear_ranges = 1;
> > +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> 
> This should be an && not || right?

No, I think this is correct as it stands if the intention is to prevent
anyone from extending either pldo_ranges or nldo_ranges.

> > +                               (ARRAY_SIZE(nldo_ranges) != 1));
> > +
> > +               if (reg_data[i].voltage_range == nldo_ranges) {
> > +                       rdesc->min_uV = NLDO_MIN_UV;
> > +                       rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> > +               } else {
> > +                       rdesc->min_uV = PLDO_MIN_UV;
> > +                       rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> > +               }
> > +
> > +               rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> > +               rdesc->enable_mask = ENABLE_BIT;
> > +               rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> > +               rdesc->regulators_node = of_match_ptr("regulators");
> > +
> > +               reg_config.dev = dev->parent;
> > +               reg_config.driver_data = pm8008_reg;
> > +               reg_config.regmap = pm8008_reg->regmap;
> > +
> > +               rdev = devm_regulator_register(dev, rdesc, &reg_config);
> > +               if (IS_ERR(rdev)) {
> > +                       rc = PTR_ERR(rdev);
> > +                       dev_err(dev, "failed to register regulator %s: %d\n",
> > +                                       reg_data[i].name, rc);
> > +                       return rc;
> 
> Could be return dev_err_probe() to simplify.

Possibly, but I think I prefer not using it when there is nothing that
can trigger a probe deferral.

Johan

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

[Johan Hovold]

On Wed, May 08, 2024 at 05:12:51PM +0100, Bryan O'Donoghue wrote:
> On 06/05/2024 16:08, Johan Hovold 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.

> > @@ -169,6 +171,10 @@ 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);
> > +
> >   	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);
> 
> So not resetting is fine and I understand you want to retain the address 
> given by the firmware, I think that's the right thing to do.

> In addition to adding a small delay suggested by Andy - a few 
> microseconds pick a number, I think you should verify the chip is out of 
> reset as we would do with many other i2c devices.

> In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and 
> REVID_PERPH_SUBTYPE @ 0x105
> 
> REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
> REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)

I'll consider it for v2.

Johan

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Johan Hovold]

On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:

> > +static void devm_irq_domain_fwnode_release(void *res)
> > +{
> 
> > +	struct fwnode_handle *fwnode = res;
> 
> Unneeded line, can be
> 
> static void devm_irq_domain_fwnode_release(void *fwnode)
> 
> > +	irq_domain_free_fwnode(fwnode);
> > +}

I think I prefer it this way for clarity and for type safety in the
unlikely even that the argument to irq_domain_free_fwnode() would ever
change.

> > +	name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> 
> You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?
> 
> 	name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));

This driver only support OF so why bother.

Johan

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

[Krzysztof Kozlowski]

On 09/05/2024 10:57, Johan Hovold wrote:
> On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
>> On 07/05/2024 19:22, Andy Shevchenko wrote:
>>> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
>>>> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
>>>>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:
> 
>>>>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator");
>>>>>
>>>>> Use ID table instead.
>>>>
>>>> No, the driver is not using an id-table for matching so the alias is
>>>> needed for module auto-loading.
>>>
>>> Then create one. Added Krzysztof for that. (He is working on dropping
>>> MODULE_ALIAS() in cases like this one)
>>
>> Yeah, please use ID table, since this is a driver (unless I missed
>> something). Module alias does not scale, leads to stale and duplicated
>> entries, so should not be used as substitute of ID table. Alias is
>> suitable for different cases.
> 
> There's no scalability issue here. If the driver uses driver name
> matching then there will always be exactly one alias needed.

And then we add one more ID with driver data and how does it scale?
There is a way to make drivers uniform, standard and easy to read. Why
doing some other way? What is the benefit of the alias comparing to
regular module ID table?

Best regards,
Krzysztof

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

[Andy Shevchenko]

Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> Quoting Johan Hovold (2024-05-06 08:08:29)

...

> > +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> 
> This should be an && not || right?

> > +                               (ARRAY_SIZE(nldo_ranges) != 1));

In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
better to have a static_assert() near to one of those arrays.

-- 
With Best Regards,
Andy Shevchenko

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

[Johan Hovold]

On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > Quoting Johan Hovold (2024-05-06 08:08:29)
> 
> ...
> 
> > > +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> > 
> > This should be an && not || right?
> 
> > > +                               (ARRAY_SIZE(nldo_ranges) != 1));
> 
> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> better to have a static_assert() near to one of those arrays.

I think the reason it is placed here is that the above line reads:

	rdesc->n_linear_ranges = 1;

and that would need to change if anyone expands the arrays.

Johan

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

[Johan Hovold]

On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote:
> On 09/05/2024 10:57, Johan Hovold wrote:
> > On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
> >> On 07/05/2024 19:22, Andy Shevchenko wrote:

> >> Yeah, please use ID table, since this is a driver (unless I missed
> >> something). Module alias does not scale, leads to stale and duplicated
> >> entries, so should not be used as substitute of ID table. Alias is
> >> suitable for different cases.
> > 
> > There's no scalability issue here. If the driver uses driver name
> > matching then there will always be exactly one alias needed.
> 
> And then we add one more ID with driver data and how does it scale?

That's what I wrote in the part of my reply that you left out. If a
driver is going to be used for multiple devices, then a module id table
makes sense, but there is no need to go around adding redundant tables
just for the sake of it when a simple alias will do.

Johan

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

[Andy Shevchenko]

Thu, May 09, 2024 at 10:53:02AM +0200, Johan Hovold kirjoitti:
> On Tue, May 07, 2024 at 08:22:34PM +0300, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote:
> > > > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti:

...

> > > >                       return dev_err_probe(...);
> > >
> > > Nah, regmap won't trigger a probe deferral.
> > 
> > And it doesn't matter. What we gain with dev_err_probe() is:
> > - special handling of deferred probe
> > - unified format of messages in ->probe() stage
> > 
> > The second one is encouraged.
> 
> I don't care about your personal preferences.

Did I say it's mine personal preference.
Or should I put it as preference of several (majority?) maintainers?

Whatever, it's up to Lee.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Thu, May 09, 2024 at 10:49:28AM +0200, Johan Hovold kirjoitti:
> On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote:
> > On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote:
> > > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti:
> > > > > The regmap irq array is potentially shared between multiple PMICs and

...

> > > > > -                   dev_err(dev, "Failed to probe irq periphs: %d\n", rc);
> > > > > +                   dev_err(dev, "failed to add IRQ chip: %d\n", rc);
> > > >
> > > > dev_err_probe(...); ?
> > >
> > > This function won't return -EPROBE_DEFER,
> > 
> > This is not an argument for a long time (since documentation of
> > dev_err_probe() had been amended to encourage its use for any error
> > cases in probe).
> 
> There was apparently a kernel doc update made in December 2023:
> 
> 	532888a59505 ("driver core: Better advertise dev_err_probe()")
> 
> to clarify that people are *allowed* to use it also for functions not
> returning -EPROBE_DEFER. That's hardly a long time ago and, importantly,
> this is of course still nothing that is *required*.

Fair enough.

> > > and that would be a separate
> > > change in any case.
> > 
> > Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?
> 
> This is not in any way technical debt.

OK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Andy Shevchenko]

On Thu, May 9, 2024 at 12:42 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:

...

> > > +static void devm_irq_domain_fwnode_release(void *res)
> > > +{
> >
> > > +   struct fwnode_handle *fwnode = res;
> >
> > Unneeded line, can be
> >
> > static void devm_irq_domain_fwnode_release(void *fwnode)
> >
> > > +   irq_domain_free_fwnode(fwnode);
> > > +}
>
> I think I prefer it this way for clarity and for type safety in the
> unlikely even that the argument to irq_domain_free_fwnode() would ever
> change.

If it ever changes, the allocation part most likely would need an
update and since devm_add_action() takes this type of function, I
don't believe the argument would ever change from void * to something
else. With this it just adds an additional burden on the conversion.

> > > +   name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> >
> > You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?
> >
> >       name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));
>
> This driver only support OF so why bother.

Sure, but it makes a bit of inconsistency. Besides that dereferencing
of_node might also add a burden one day we want to get rid of it or
move it somewhere else, or convert to the list_head or so.
dev_of_node(dev) in this case prevents from looking into this case.

-- 
With Best Regards,
Andy Shevchenko

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

[Satya Priya Kakitapalli]

> On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> > On 5/6/24 17:08, Johan Hovold wrote:
> > > From: Satya Priya <quic_c_skakit@quicinc.com>
> > > 
> > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > > regulator management via the regulator framework.
> > > 
> > > Note that this driver, originally submitted by Satya Priya [1], has been
> > > 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 a
> > > driver lookup table matching on the parent compatible.
> > > 
> > > 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.
> > > 
> > > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> > > 
> > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>

This is my old email which is discontinued, please use <quic_skakitap@quicinc.com>

> > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > [ johan: rework probe to match new binding, amend commit message and
> > >           Kconfig entry]
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > 
> > I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> > qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> > generic.. Would you know whether this code will also be used for e.g.
> > PM8010?
> 
> Yes, for any sufficiently similar PMICs, including SPMI ones. So
> 'qpnp-regulator' would be a generic name, but only Qualcomm knows what
> PMICs they have and how they are related -- the rest of us is left doing
> tedious code forensics to try to make some sense of this.
> 
> So just like for compatible strings, letting the first supported PMIC
> name the driver makes sense as we don't know when we'll want to add a
> second one for another set of devices (and we don't want to call that
> one 'qpnp-regulator-2'). On the other hand, these names are now mostly
> internal and can more easily be renamed later.

There is a PMIC called PM8010 which also is implemented over I2C, which could use the same pm8008 regulator driver.
Hence it is better to use device_id instead of a MODULE_ALIAS().

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

[Satya Priya Kakitapalli]

> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> > Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > > Quoting Johan Hovold (2024-05-06 08:08:29)
> > 
> > ...
> > 
> > > > +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> > > 
> > > This should be an && not || right?
> > 
> > > > +                               (ARRAY_SIZE(nldo_ranges) != 1));
> > 
> > In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> > better to have a static_assert() near to one of those arrays.
> 
> I think the reason it is placed here is that the above line reads:
> 
> 	rdesc->n_linear_ranges = 1;
> 
> and that would need to change if anyone expands the arrays.

Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
So, BUILD_BUG_ON is the only way to go here.

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

[Andy Shevchenko]

On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
<quic_skakitap@quicinc.com> wrote:
> > On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> > > Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> > > > Quoting Johan Hovold (2024-05-06 08:08:29)

...

> > > > > +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> > > >
> > > > This should be an && not || right?
> > >
> > > > > +                               (ARRAY_SIZE(nldo_ranges) != 1));
> > >
> > > In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> > > better to have a static_assert() near to one of those arrays.
> >
> > I think the reason it is placed here is that the above line reads:
> >
> >       rdesc->n_linear_ranges = 1;
> >
> > and that would need to change if anyone expands the arrays.
>
> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.

I didn't get this. The ARRAY_SIZE():s are defined at compile time
globally. How does this prevent from using static_assert()?

> So, BUILD_BUG_ON is the only way to go here.

I don't think so.

-- 
With Best Regards,
Andy Shevchenko

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

[Satya Priya Kakitapalli (Temp)]

On 5/14/2024 7:48 PM, Andy Shevchenko wrote:
> On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
> <quic_skakitap@quicinc.com> wrote:
>>> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
>>>> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
>>>>> Quoting Johan Hovold (2024-05-06 08:08:29)
> ...
>
>>>>>> +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
>>>>> This should be an && not || right?
>>>>>> +                               (ARRAY_SIZE(nldo_ranges) != 1));
>>>> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
>>>> better to have a static_assert() near to one of those arrays.
>>> I think the reason it is placed here is that the above line reads:
>>>
>>>        rdesc->n_linear_ranges = 1;
>>>
>>> and that would need to change if anyone expands the arrays.
>> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
> I didn't get this. The ARRAY_SIZE():s are defined at compile time
> globally. How does this prevent from using static_assert()?


The reason we added it here is to make sure the nlod_ranges and 
pldo_ranges doesn't become larger, and we forget updating the 
n_linear_ranges. Adding static_assert here is not feasible so adding a 
BUILD_BUG_ON at this point makes sure the n_linear_ranges is proper.


>> So, BUILD_BUG_ON is the only way to go here.
> I don't think so.
>

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

[Andy Shevchenko]

On Tue, May 14, 2024 at 6:05 PM Satya Priya Kakitapalli (Temp)
<quic_skakitap@quicinc.com> wrote:
> On 5/14/2024 7:48 PM, Andy Shevchenko wrote:
> > On Tue, May 14, 2024 at 5:05 PM Satya Priya Kakitapalli
> > <quic_skakitap@quicinc.com> wrote:
> >>> On Thu, May 09, 2024 at 03:07:02PM +0300, Andy Shevchenko wrote:
> >>>> Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd kirjoitti:
> >>>>> Quoting Johan Hovold (2024-05-06 08:08:29)

...

> >>>>>> +               BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
> >>>>> This should be an && not || right?
> >>>>>> +                               (ARRAY_SIZE(nldo_ranges) != 1));
> >>>> In any case BUILD_BUG_ON() is not encouraged for such cases, it would be much
> >>>> better to have a static_assert() near to one of those arrays.
> >>> I think the reason it is placed here is that the above line reads:
> >>>
> >>>        rdesc->n_linear_ranges = 1;
> >>>
> >>> and that would need to change if anyone expands the arrays.
> >> Correct. static_assert() cannot be used in the middle of code here, it can only be used at the declarations part which doesn't serve the purpose.
> > I didn't get this. The ARRAY_SIZE():s are defined at compile time
> > globally. How does this prevent from using static_assert()?

> The reason we added it here is to make sure the nlod_ranges and
> pldo_ranges doesn't become larger, and we forget updating the
> n_linear_ranges.

> Adding static_assert here is not feasible so adding a
> BUILD_BUG_ON at this point makes sure the n_linear_ranges is proper.

No, static_assert() will do _exactly_ the same with better error
reporting and location, but what you are trying to say is that the
location is chosen to be near to the n_liner_ranges assignment which
happens at runtime, that's why it can't be used as an argument to
BUILD_BUG_ON(). Based on this discussion I think the comment is
missing before BUILD_BUG_ON() to explain the semantics of 1 and all
that was just said.

> >> So, BUILD_BUG_ON is the only way to go here.
> > I don't think so.

As i said.

-- 
With Best Regards,
Andy Shevchenko

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

[Konrad Dybcio]

On 5/14/24 15:43, Satya Priya Kakitapalli wrote:
>> On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
>>> On 5/6/24 17:08, Johan Hovold wrote:
>>>> From: Satya Priya <quic_c_skakit@quicinc.com>
>>>>
>>>> Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
>>>> seven LDO regulators. Add a PM8008 regulator driver to support PMIC
>>>> regulator management via the regulator framework.
>>>>
>>>> Note that this driver, originally submitted by Satya Priya [1], has been
>>>> 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 a
>>>> driver lookup table matching on the parent compatible.
>>>>
>>>> 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.
>>>>
>>>> [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
>>>>
>>>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> 
> This is my old email which is discontinued, please use <quic_skakitap@quicinc.com>

Please submit an entry to the .mailmap file

Konrad

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

[Krzysztof Kozlowski]

On 09/05/2024 14:26, Johan Hovold wrote:
> On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote:
>> On 09/05/2024 10:57, Johan Hovold wrote:
>>> On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote:
>>>> On 07/05/2024 19:22, Andy Shevchenko wrote:
> 
>>>> Yeah, please use ID table, since this is a driver (unless I missed
>>>> something). Module alias does not scale, leads to stale and duplicated
>>>> entries, so should not be used as substitute of ID table. Alias is
>>>> suitable for different cases.
>>>
>>> There's no scalability issue here. If the driver uses driver name
>>> matching then there will always be exactly one alias needed.
>>
>> And then we add one more ID with driver data and how does it scale?
> 
> That's what I wrote in the part of my reply that you left out. If a
> driver is going to be used for multiple devices, then a module id table
> makes sense, but there is no need to go around adding redundant tables
> just for the sake of it when a simple alias will do.
> 

I still in general prefer ID tables, because I saw many times people
copy existing code while not understanding above subtleties thus they
just keep multiplying MODULE_ALIAS, but I understand your explanation
and it is reasonable.

FWIW:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Johan Hovold]

On Fri, May 10, 2024 at 04:15:43PM +0300, Andy Shevchenko wrote:
> On Thu, May 9, 2024 at 12:42 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> > > Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:

> > > > +static void devm_irq_domain_fwnode_release(void *res)
> > > > +{
> > >
> > > > +   struct fwnode_handle *fwnode = res;
> > >
> > > Unneeded line, can be
> > >
> > > static void devm_irq_domain_fwnode_release(void *fwnode)
> > >
> > > > +   irq_domain_free_fwnode(fwnode);
> > > > +}
> >
> > I think I prefer it this way for clarity and for type safety in the
> > unlikely even that the argument to irq_domain_free_fwnode() would ever
> > change.
> 
> If it ever changes, the allocation part most likely would need an
> update and since devm_add_action() takes this type of function, I
> don't believe the argument would ever change from void * to something
> else. With this it just adds an additional burden on the conversion.

I was referring to the irq_domain_free_fwnode() prototype.
 
> > > > +   name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> > >
> > > You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?
> > >
> > >       name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));
> >
> > This driver only support OF so why bother.
> 
> Sure, but it makes a bit of inconsistency.

No, I don't consider this an inconsistency. Again, *this* is an OF
driver, other subsystems need to deal with ACPI and use fwnode.

Johan

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Andy Shevchenko]

On Wed, May 22, 2024 at 9:49 AM Johan Hovold <johan@kernel.org> wrote:
> On Fri, May 10, 2024 at 04:15:43PM +0300, Andy Shevchenko wrote:
> > On Thu, May 9, 2024 at 12:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> > > > Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:

...

> > > > > +static void devm_irq_domain_fwnode_release(void *res)
> > > > > +{
> > > >
> > > > > +   struct fwnode_handle *fwnode = res;
> > > >
> > > > Unneeded line, can be
> > > >
> > > > static void devm_irq_domain_fwnode_release(void *fwnode)
> > > >
> > > > > +   irq_domain_free_fwnode(fwnode);
> > > > > +}
> > >
> > > I think I prefer it this way for clarity and for type safety in the
> > > unlikely even that the argument to irq_domain_free_fwnode() would ever
> > > change.
> >
> > If it ever changes, the allocation part most likely would need an
> > update and since devm_add_action() takes this type of function, I
> > don't believe the argument would ever change from void * to something
> > else. With this it just adds an additional burden on the conversion.
>
> I was referring to the irq_domain_free_fwnode() prototype.

And I also referred to that one. The release callback, i.e. the type
of the parameter, is solely defined by a caller of devm_add_action()
end friends, and in this case it means that if ever the type changes
(this is your argument why you want to have explicit line for that,
necessity of which I oppose) the devm_add_action() arguments also has
to be changed, it can't be done _just_ there, in
irq_domain_free_fwnode().

> > > > > +   name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> > > >
> > > > You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?
> > > >
> > > >       name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));
> > >
> > > This driver only support OF so why bother.
> >
> > Sure, but it makes a bit of inconsistency.
>
> No, I don't consider this an inconsistency. Again, *this* is an OF
> driver, other subsystems need to deal with ACPI and use fwnode.

OK.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/13] mfd: pm8008: rework driver

[Johan Hovold]

On Wed, May 22, 2024 at 10:13:33AM +0300, Andy Shevchenko wrote:
> On Wed, May 22, 2024 at 9:49 AM Johan Hovold <johan@kernel.org> wrote:
> > On Fri, May 10, 2024 at 04:15:43PM +0300, Andy Shevchenko wrote:
> > > On Thu, May 9, 2024 at 12:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > > On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> > > > > Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:

> > > > > > +static void devm_irq_domain_fwnode_release(void *res)
> > > > > > +{
> > > > >
> > > > > > +   struct fwnode_handle *fwnode = res;
> > > > >
> > > > > Unneeded line, can be
> > > > >
> > > > > static void devm_irq_domain_fwnode_release(void *fwnode)
> > > > >
> > > > > > +   irq_domain_free_fwnode(fwnode);
> > > > > > +}
> > > >
> > > > I think I prefer it this way for clarity and for type safety in the
> > > > unlikely even that the argument to irq_domain_free_fwnode() would ever
> > > > change.
> > >
> > > If it ever changes, the allocation part most likely would need an
> > > update and since devm_add_action() takes this type of function, I
> > > don't believe the argument would ever change from void * to something
> > > else. With this it just adds an additional burden on the conversion.
> >
> > I was referring to the irq_domain_free_fwnode() prototype.
> 
> And I also referred to that one. The release callback, i.e. the type
> of the parameter, is solely defined by a caller of devm_add_action()
> end friends, and in this case it means that if ever the type changes
> (this is your argument why you want to have explicit line for that,
> necessity of which I oppose) the devm_add_action() arguments also has
> to be changed, it can't be done _just_ there, in
> irq_domain_free_fwnode().

No, not necessarily, but as I already wrote above this is unlikely to
ever be of practical concern.

Johan

Re: [PATCH 01/13] dt-bindings: mfd: pm8008: add reset gpio

[Linus Walleij]

On Mon, May 6, 2024 at 5:10 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> Describe the optional reset gpio (which may not be wired up).
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 09/13] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Linus Walleij]

On Mon, May 6, 2024 at 5:10 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
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Is this something I can just apply, maybe with the DT binding drop
patch right (8/13) after it?

IIUC it does not need to go into fixes because there are no regressions,
right?

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, May 6, 2024 at 5:10 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.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Johan Hovold]

On Thu, May 09, 2024 at 11:10:41AM +0200, Johan Hovold wrote:
> On Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd wrote:
> > Quoting Johan Hovold (2024-05-06 08:08:29)

> > > +struct pm8008_regulator {
> > > +       struct regmap           *regmap;
> > > +       struct regulator_desc   rdesc;
> > > +       u16                     base;
> > > +       int                     step_rate;
> > 
> > Is struct regulator_desc::vsel_step usable for this? If not, can it be
> > unsigned?
> 
> Not sure, I'll take a look when respinning.

No, vsel_step is unrelated to this, which is really a slew rate.

I've reworked the driver and dropped this field in favour of
regulator_desc::ramp_delay.

> > > +};
 
> > > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > > +{
> > > +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > > +       __le16 mV;
> > > +       int uV;

> > > +
> > > +       regmap_bulk_read(pm8008_reg->regmap,
> > > +                       LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> > 
> > Is struct regulator_desc::vsel_reg usable for this?
> 
> Will look into that.

I don't think vsel_reg can be used here as the voltage is set using two
registers (LSB and MSB).
  
> > > +
> > > +       uV = le16_to_cpu(mV) * 1000;
> > > +       return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > > +}

Johan

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

[Johan Hovold]

On Tue, May 14, 2024 at 07:13:17PM +0530, Satya Priya Kakitapalli wrote:
> > On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote:
> > > On 5/6/24 17:08, Johan Hovold wrote:
> > > > From: Satya Priya <quic_c_skakit@quicinc.com>
> > > > 
> > > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing
> > > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC
> > > > regulator management via the regulator framework.
> > > > 
> > > > Note that this driver, originally submitted by Satya Priya [1], has been
> > > > reworked to match the new devicetree binding which no longer describes
> > > > each regulator as a separate device.

> > > > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com
> > > > 
> > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> 
> This is my old email which is discontinued, please use <quic_skakitap@quicinc.com>

I've cleaned up and reworked the driver for v2 and changed the
authorship to myself in the process, but I'll make sure to CC your new
address when submitting.

You should add an alias as Konrad suggested as you apparently have
commits that use your old address.

> > > > Cc: Stephen Boyd <swboyd@chromium.org>
> > > > [ johan: rework probe to match new binding, amend commit message and
> > > >           Kconfig entry]
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > > ---
> > > 
> > > I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then
> > > qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly
> > > generic.. Would you know whether this code will also be used for e.g.
> > > PM8010?
> > 
> > Yes, for any sufficiently similar PMICs, including SPMI ones. So
> > 'qpnp-regulator' would be a generic name, but only Qualcomm knows what
> > PMICs they have and how they are related -- the rest of us is left doing
> > tedious code forensics to try to make some sense of this.
> > 
> > So just like for compatible strings, letting the first supported PMIC
> > name the driver makes sense as we don't know when we'll want to add a
> > second one for another set of devices (and we don't want to call that
> > one 'qpnp-regulator-2'). On the other hand, these names are now mostly
> > internal and can more easily be renamed later.
> 
> There is a PMIC called PM8010 which also is implemented over I2C,
> which could use the same pm8008 regulator driver.
> Hence it is better to use device_id instead of a MODULE_ALIAS().

Right, I've had PM8010 in mind all along, but it's hard to tell whether
it will be using the same (sub)driver until code is submitted since you
guys (Qualcomm) don't publish any documentation.

I've changed the regulator (and GPIO) drivers to use platform device id
matching for now either way.

Johan

Re: [PATCH 09/13] pinctrl: qcom: spmi-gpio: drop broken pm8008 support

[Johan Hovold]

On Mon, May 27, 2024 at 03:35:41PM +0200, Linus Walleij wrote:
> On Mon, May 6, 2024 at 5:10 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
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Is this something I can just apply, maybe with the DT binding drop
> patch right (8/13) after it?

Yes, I guess so, unless it's easier to let everything go through MFD
(except possibly the regulator driver).

I'll be posting a v2 in a bit and include these two there too. You can
either pick them there or ack them as you prefer.

> IIUC it does not need to go into fixes because there are no regressions,
> right?

As I mentioned in the commit message, the driver is backcasting a
pointer to an incorrect type, which could lead to all sorts of trouble
even if it does not seem to be the case currently (I did not check 5.13
for example).

Since it has always been broken, I'd rather err on the safe side and
just drop it also from the stable trees.

Johan

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

[Johan Hovold]

On Thu, May 09, 2024 at 11:31:22AM +0200, Johan Hovold wrote:
> On Wed, May 08, 2024 at 05:12:51PM +0100, Bryan O'Donoghue wrote:
> > On 06/05/2024 16:08, Johan Hovold 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.
> 
> > > @@ -169,6 +171,10 @@ 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);
> > > +
> > >   	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);
> > 
> > So not resetting is fine and I understand you want to retain the address 
> > given by the firmware, I think that's the right thing to do.
> 
> > In addition to adding a small delay suggested by Andy - a few 
> > microseconds pick a number, I think you should verify the chip is out of 
> > reset as we would do with many other i2c devices.
> 
> > In this case, suggest reading REVID_PERPH_TYPE @ 0x104 and 
> > REVID_PERPH_SUBTYPE @ 0x105
> > 
> > REVID_PERPH_TYPE @ 0x104 == 0x51 (PMIC)
> > REVID_PERPH_SUBYTE @ 0x105 == 0x2C (PM8008)
> 
> I'll consider it for v2.

I decided to not add any revid checks for v2 as it's not strictly
needed. This is something can be added later when/if needed.

The irqchip registration will also fail if there's no from reply from
this address.

Johan

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

[Bryan O'Donoghue]

On 29/05/2024 17:17, Johan Hovold wrote:
> The irqchip registration will also fail if there's no from reply from
> this address.

That's acceptable too.

---
bod