[PATCH v2 00/11] Use correct LDO5 control registers for PCA9450

[PATCH v2 00/11] Use correct LDO5 control registers for PCA9450

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This is a follow-up of [1].

The main objective of this is to fix the PCA9450 driver to
use the correct control register for the LDO5 regulator.

Currently the control register to use for LDO5 is hardcoded to
LDO5CTRL_H. This is wrong for two reasons:

1. LDO5CTRL_H doesn't contain the bits for enabling/disabling
   the regulator. Only LDO5CTRL_L does.

2. The actual output voltage of the regulator is determined by
   the LDO5CTRL_H only if the SD_VSEL input is HIGH. If it is
   low, then LDO5CTRL_L is used. The driver does not take this
   into account.

This can cause several problems:

1. LDO5 can not be turned on/off and we rely on the bootloader
   to leave it turned on. On the other hand we can't save
   power if LDO5 is unused.

2. There is a potential for corner-cases where switching
   SD_VSEL via USDHC_VSELECT and writing to the (wrong)
   control register can cause wrong output voltage and therfore
   SD card failures (not observed yet).

3. Reading the current voltage of the LDO5 regulator (e. g. via
   sysfs can yield the wrong value as the voltage is read from
   the wrong control register.

At the same time there is now hardware that hardwires SD_VSEL
to a fixed LOW level and therefore relies on switching the
voltage only via a single control register. We add support for
this through an additional property "nxp,sd-vsel-fixed-low" in
the LDO5 node.

Summary of binding changes (patch 1-3):

1. Adjust the bindings to remove the old and abandoned use of
   sd-vsel-gpios property.

2. Adjust the bindings to use sd-vsel-gpios in the LDO5 node to
   retrieve an input that can be used to sample the SD_VSEL
   status.

3. Adjust bindings to allow "nxp,sd-vsel-fixed-low" to be used
   for boards that have SD_VSEL hardwired to low level.

Summary of driver changes (patch 4-7):

1. Remove the old sd-vsel-gpios handling.

2. Use the new sd-vsel-gpios property to determine the correct
   control register for LDO5.

3. Fix the enable register for LDO5.

4. Support hardware with fixed low level of SD_VSEL.

Summary of devicetree changes (patch 8-10):

Implement the changes in the devicetrees for Kontron hardware
(i.MX8MM, i.MX8MP and i.MX93).

Changelog:

v1 -> v2:

* Split binding patch
* Add solution for hardwired SD_VSEL
* Leave regulator core untouched as requested by Mark
* Add devicetree changes for i.MX8MP and i.MX93

[1] https://lore.kernel.org/lkml/20230213155833.1644366-1-frieder@fris.de/

Frieder Schrempf (11):
  Revert "regulator: pca9450: Add sd-vsel GPIO"
  dt-bindings: regulator: pca9450: Add sd-vsel-gpios to read back LDO5
    status
  dt-bindings: regulator: pca9450: Document nxp,sd-vsel-fixed-low
    property for LDO5
  arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT
  Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"
  regulator: pca9450: Fix control register for LDO5
  regulator: pca9450: Fix enable register for LDO5
  regulator: pca9450: Handle hardware with fixed SD_VSEL for LDO5
  arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal
  arm64: dts: imx93-kontron: Fix SD card IO voltage control
  arm64: dts: imx8mp-kontron: Add support for reading SD_VSEL signal

 .../regulator/nxp,pca9450-regulator.yaml      | 29 ++++--
 .../boot/dts/freescale/imx8mm-kontron-bl.dts  | 10 ++-
 .../dts/freescale/imx8mm-kontron-osm-s.dtsi   |  7 +-
 .../dts/freescale/imx8mp-kontron-osm-s.dtsi   |  7 +-
 .../boot/dts/freescale/imx8mp-skov-reva.dtsi  |  5 +-
 .../dts/freescale/imx93-kontron-osm-s.dtsi    |  5 +-
 drivers/regulator/pca9450-regulator.c         | 88 ++++++++++++++++---
 7 files changed, 119 insertions(+), 32 deletions(-)

-- 
2.46.1

[PATCH v2 01/11] Revert "regulator: pca9450: Add sd-vsel GPIO"

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This reverts commit 27866e3e8a7e93494f8374f48061aa73ee46ceb2.

It turned out that this feature was implemented based on
the wrong assumption that the SD_VSEL signal needs to be
controlled as GPIO in any case.

In fact the straight-forward approach is to mux the signal
as USDHC_VSELECT and let the USDHC controller do the job.

Most users never even used this property and the few who
did have been or are getting migrated to the alternative
approach.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* split revert into separate patch
---
 .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
index f8057bba747a5..79fc0baf5fa2f 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
@@ -77,11 +77,6 @@ properties:
 
     additionalProperties: false
 
-  sd-vsel-gpios:
-    description: GPIO that is used to switch LDO5 between being configured by
-      LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is
-      connected to a host GPIO.
-
   nxp,i2c-lt-enable:
     type: boolean
     description:
-- 
2.46.1

[PATCH v2 02/11] dt-bindings: regulator: pca9450: Add sd-vsel-gpios to read back LDO5 status

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

In order to know the current status (which of the two control
registers is used) for the LDO5 regulator, we need to route back the
USDHC_VSELECT signal by setting the SION bit in the IOMUX.

By adding the according GPIO as sd-vsel-gpios to the LDO5 node, we
allow the regulator driver to sample the current status of the
SD_VSEL signal that is used to select the correct control register.

The SD_VSEL on the PMIC is always an input. It's driven by the SoC's
VSELECT signal (controlled by the USDHC controller) and we use the
SION bit in the IOMUX to internally loop back the signal in order to
sample it using the GPIO.

As the SD_VSEL pin is directly routed to the LDO5 regulator in the
PMIC, make the sd-vsel-gpios property part of the LDO5 node.

SoC                                  PMIC
+-----------------------+           +-------------------+
|                       |           |                   |
|                       |           |                   |
|  GPIO <----------+    |           |                   |
|                  |    |    SD_VSEL|   +-------+       |
|  USDHC_VSELECT ->+------------------->| LDO5  |       |
|                       |           |   +-------+       |
|                       |           |                   |
+-----------------------+           +-------------------+

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* extend commit message
* split into two patches (revert old sd-vsel-gpios separately)
---
 .../regulator/nxp,pca9450-regulator.yaml       | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
index 79fc0baf5fa2f..5d0d684186c96 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
@@ -41,8 +41,24 @@ properties:
     description: |
       list of regulators provided by this controller
 
+    properties:
+      LDO5:
+        type: object
+        $ref: regulator.yaml#
+        description:
+          Properties for single LDO5 regulator.
+
+        properties:
+          sd-vsel-gpios:
+            description:
+              GPIO that can be used to read the current status of the SD_VSEL
+              signal in order for the driver to know if LDO5CTRL_L or LDO5CTRL_H
+              is used by the hardware.
+
+        unevaluatedProperties: false
+
     patternProperties:
-      "^LDO[1-5]$":
+      "^LDO[1-4]$":
         type: object
         $ref: regulator.yaml#
         description:
-- 
2.46.1

[PATCH v2 03/11] dt-bindings: regulator: pca9450: Document nxp,sd-vsel-fixed-low property for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This new property can be used for boards which have the SD_VSEL tied
to a fixed low level. The voltage of LDO5 is therefore only controlled
by writing to the LDO5CTRL_L register.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* new patch
---
 .../bindings/regulator/nxp,pca9450-regulator.yaml           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
index 5d0d684186c96..0e19c54aa5f8a 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
@@ -49,6 +49,12 @@ properties:
           Properties for single LDO5 regulator.
 
         properties:
+          nxp,sd-vsel-fixed-low:
+            type: boolean
+            description:
+              Let the driver know that SD_VSEL is hardwired to low level and
+              there is no GPIO to get the actual value from.
+
           sd-vsel-gpios:
             description:
               GPIO that can be used to read the current status of the SD_VSEL
-- 
2.46.1

[PATCH v2 04/11] arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The USDHC controller is able to control the IO voltage of the SD card.
There is no reason to use a GPIO to control it.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* new patch
---
 arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
index 59813ef8e2bb3..33031e946329d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
@@ -232,7 +232,6 @@ pmic@25 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&pinctrl_pmic>;
 		interrupts-extended = <&gpio1 3 IRQ_TYPE_EDGE_RISING>;
-		sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
 
 		regulators {
 			reg_vdd_soc: BUCK1 {
@@ -555,7 +554,6 @@ MX8MP_IOMUXC_I2C4_SDA__I2C4_SDA				0x400001c3
 	pinctrl_pmic: pmicirqgrp {
 		fsl,pins = <
 			MX8MP_IOMUXC_GPIO1_IO03__GPIO1_IO03			0x41
-			MX8MP_IOMUXC_GPIO1_IO04__GPIO1_IO04			0x41
 		>;
 	};
 
@@ -623,6 +621,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0			0x1d0
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1			0x1d0
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2			0x1d0
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3			0x1d0
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT			0xc0
 		>;
 	};
 
@@ -634,6 +633,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0			0x1d4
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1			0x1d4
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2			0x1d4
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3			0x1d4
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT			0xc0
 		>;
 	};
 
@@ -645,6 +645,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0			0x1d6
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1			0x1d6
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2			0x1d6
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3			0x1d6
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT			0xc0
 		>;
 	};
 
-- 
2.46.1

[PATCH v2 05/11] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This reverts commit 8c67a11bae889f51fe5054364c3c789dfae3ad73.

It turns out that all boards using the PCA9450 actually have the
SD_VSEL input connected to the VSELECT signal of the SoCs SD/MMC
interface or use a fixed level.

The assumptions on which this was implemented were wrong. There
is no need for a GPIO-only-based approach and keeping this will
cause confusion and lead people to implement non-standard setups.

All in-tree users of this have been migrated and we can savely
remove this now and allow for a more future-proof approach
of syncing the actual status of SD_VSEL and the PMIC driver.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* rebase to current master
---
 drivers/regulator/pca9450-regulator.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 9714afe347dcc..0f8a515a0b11a 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -5,7 +5,6 @@
  */
 
 #include <linux/err.h>
-#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -32,7 +31,6 @@ struct pca9450_regulator_desc {
 struct pca9450 {
 	struct device *dev;
 	struct regmap *regmap;
-	struct gpio_desc *sd_vsel_gpio;
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
@@ -1015,18 +1013,6 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		}
 	}
 
-	/*
-	 * The driver uses the LDO5CTRL_H register to control the LDO5 regulator.
-	 * This is only valid if the SD_VSEL input of the PMIC is high. Let's
-	 * check if the pin is available as GPIO and set it to high.
-	 */
-	pca9450->sd_vsel_gpio = gpiod_get_optional(pca9450->dev, "sd-vsel", GPIOD_OUT_HIGH);
-
-	if (IS_ERR(pca9450->sd_vsel_gpio)) {
-		dev_err(&i2c->dev, "Failed to get SD_VSEL GPIO\n");
-		return PTR_ERR(pca9450->sd_vsel_gpio);
-	}
-
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" :
 		(type == PCA9450_TYPE_PCA9451A ? "pca9451a" : "pca9450bc"));
-- 
2.46.1

[PATCH v2 06/11] regulator: pca9450: Fix control register for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

For LDO5 we need to be able to check the status of the SD_VSEL input in
order to know which control register is used. Read the status of the
SD_VSEL signal via GPIO and use the correct register accordingly.

To use this, the LDO5 node in the devicetree needs the sd-vsel-gpios
property to reference the GPIO that is used to read back the SD_VSEL
status internally. Please note that the SION bit in the IOMUX must be
set if the signal is muxed as VSELECT and controlled by the USDHC
controller.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* rebase to current master and stop using generic helper from
  regulator core
---
 drivers/regulator/pca9450-regulator.c | 77 +++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 0f8a515a0b11a..8e525deaff0b7 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -31,6 +32,7 @@ struct pca9450_regulator_desc {
 struct pca9450 {
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *sd_vsel_gpio;
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
@@ -96,6 +98,58 @@ static const struct regulator_ops pca9450_ldo_regulator_ops = {
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 };
 
+static unsigned int pca9450_ldo5_get_reg_voltage_sel(struct regulator_dev *rdev)
+{
+	struct pca9450 *pca9450 = rdev_get_drvdata(rdev);
+
+	if (pca9450->sd_vsel_gpio && !gpiod_get_value(pca9450->sd_vsel_gpio))
+		return PCA9450_REG_LDO5CTRL_L;
+
+	return rdev->desc->vsel_reg;
+}
+
+static int pca9450_ldo5_get_voltage_sel_regmap(struct regulator_dev *rdev)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, pca9450_ldo5_get_reg_voltage_sel(rdev), &val);
+	if (ret != 0)
+		return ret;
+
+	val &= rdev->desc->vsel_mask;
+	val >>= ffs(rdev->desc->vsel_mask) - 1;
+
+	return val;
+}
+
+static int pca9450_ldo5_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned int sel)
+{
+	int ret;
+
+	sel <<= ffs(rdev->desc->vsel_mask) - 1;
+
+	ret = regmap_update_bits(rdev->regmap, pca9450_ldo5_get_reg_voltage_sel(rdev),
+				  rdev->desc->vsel_mask, sel);
+	if (ret)
+		return ret;
+
+	if (rdev->desc->apply_bit)
+		ret = regmap_update_bits(rdev->regmap, rdev->desc->apply_reg,
+					 rdev->desc->apply_bit,
+					 rdev->desc->apply_bit);
+	return ret;
+}
+
+static const struct regulator_ops pca9450_ldo5_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.set_voltage_sel = pca9450_ldo5_set_voltage_sel_regmap,
+	.get_voltage_sel = pca9450_ldo5_get_voltage_sel_regmap,
+};
+
 /*
  * BUCK1/2/3
  * 0.60 to 2.1875V (12.5mV step)
@@ -445,7 +499,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.of_match = of_match_ptr("LDO5"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO5,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo5_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo5_volts,
@@ -654,7 +708,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.of_match = of_match_ptr("LDO5"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO5,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo5_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo5_volts,
@@ -826,7 +880,7 @@ static const struct pca9450_regulator_desc pca9451a_regulators[] = {
 			.of_match = of_match_ptr("LDO5"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO5,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo5_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo5_volts,
@@ -884,6 +938,7 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 				      of_device_get_match_data(&i2c->dev);
 	const struct pca9450_regulator_desc	*regulator_desc;
 	struct regulator_config config = { };
+	struct regulator_dev *ldo5;
 	struct pca9450 *pca9450;
 	unsigned int device_id, i;
 	unsigned int reset_ctrl;
@@ -949,6 +1004,7 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 
 		config.regmap = pca9450->regmap;
 		config.dev = pca9450->dev;
+		config.driver_data = pca9450;
 
 		rdev = devm_regulator_register(pca9450->dev, desc, &config);
 		if (IS_ERR(rdev)) {
@@ -958,6 +1014,9 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 				desc->name, ret);
 			return ret;
 		}
+
+		if (!strcmp(desc->name, "ldo5"))
+			ldo5 = rdev;
 	}
 
 	if (pca9450->irq) {
@@ -1013,6 +1072,18 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		}
 	}
 
+	/*
+	 * For LDO5 we need to be able to check the status of the SD_VSEL input in
+	 * order to know which control register is used. Most boards connect SD_VSEL
+	 * to the VSELECT signal, so we can use the GPIO that is internally routed
+	 * to this signal (if SION bit is set in IOMUX).
+	 */
+	pca9450->sd_vsel_gpio = gpiod_get_optional(&ldo5->dev, "sd-vsel", GPIOD_IN);
+	if (IS_ERR(pca9450->sd_vsel_gpio)) {
+		dev_err(&i2c->dev, "Failed to get SD_VSEL GPIO\n");
+		return ret;
+	}
+
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" :
 		(type == PCA9450_TYPE_PCA9451A ? "pca9451a" : "pca9450bc"));
-- 
2.46.1

[PATCH v2 07/11] regulator: pca9450: Fix enable register for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The LDO5 regulator has two configuration registers, but only
LDO5CTRL_L contains the bits for enabling/disabling the regulator.

Fixes: 0935ff5f1f0a ("regulator: pca9450: add pca9450 pmic driver")
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Reviewed-by: Marek Vasut <marex@denx.de>
---
Changes for v2:
* none
---
 drivers/regulator/pca9450-regulator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 8e525deaff0b7..7f7e176bef452 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -506,7 +506,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
 			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
-			.enable_reg = PCA9450_REG_LDO5CTRL_H,
+			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
 			.owner = THIS_MODULE,
 		},
@@ -715,7 +715,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
 			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
-			.enable_reg = PCA9450_REG_LDO5CTRL_H,
+			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
 			.owner = THIS_MODULE,
 		},
@@ -887,7 +887,7 @@ static const struct pca9450_regulator_desc pca9451a_regulators[] = {
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
 			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
-			.enable_reg = PCA9450_REG_LDO5CTRL_H,
+			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
 			.owner = THIS_MODULE,
 		},
-- 
2.46.1

[PATCH v2 08/11] regulator: pca9450: Handle hardware with fixed SD_VSEL for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

There are two ways to set the output voltage of the LD05
regulator. First by writing to the voltage selection registers
and second by toggling the SD_VSEL signal.

Usually board designers connect SD_VSEL to the VSELECT signal
controlled by the USDHC controller, but in some cases the
signal is hardwired to a fixed low level (therefore selecting
3.3V as initial value for allowing to boot from the SD card).

In these cases, the voltage is only determined by the value
of the LDO5CTRL_L register. Introduce a property
nxp,sd-vsel-fixed-low to let the driver know that SD_VSEL
is low and there is no GPIO to actually get that
information from dynamically.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* new patch
---
 drivers/regulator/pca9450-regulator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 7f7e176bef452..6024de9656e8e 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -36,6 +36,7 @@ struct pca9450 {
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
+	bool sd_vsel_fixed_low;
 };
 
 static const struct regmap_range pca9450_status_range = {
@@ -102,6 +103,9 @@ static unsigned int pca9450_ldo5_get_reg_voltage_sel(struct regulator_dev *rdev)
 {
 	struct pca9450 *pca9450 = rdev_get_drvdata(rdev);
 
+	if (pca9450->sd_vsel_fixed_low)
+		return PCA9450_REG_LDO5CTRL_L;
+
 	if (pca9450->sd_vsel_gpio && !gpiod_get_value(pca9450->sd_vsel_gpio))
 		return PCA9450_REG_LDO5CTRL_L;
 
@@ -1084,6 +1088,9 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		return ret;
 	}
 
+	pca9450->sd_vsel_fixed_low =
+		of_property_read_bool(ldo5->dev.of_node, "nxp,sd-vsel-fixed-low");
+
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" :
 		(type == PCA9450_TYPE_PCA9451A ? "pca9451a" : "pca9450bc"));
-- 
2.46.1

[PATCH v2 09/11] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This fixes the LDO5 regulator handling of the pca9450 driver by
taking the status of the SD_VSEL into account to determine which
configuration register is used for the voltage setting.

Even without this change there is no functional issue, as the code
for switching the voltage in sdhci.c currently switches both, the
VSELECT/SD_VSEL signal and the regulator voltage at the same time
and doesn't run into an invalid corner case.

We should still make sure, that we always use the correct register
when controlling the regulator. At least in U-Boot this fixes an
actual bug where the wrong IO voltage is used and it makes sure
that the correct voltage can be read from sysfs.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* rebase to current master
---
 arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts    | 10 +++++++---
 .../arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi |  7 ++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
index a8ef4fba16a9e..d16490d876874 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
@@ -254,6 +254,10 @@ &pwm2 {
 	status = "okay";
 };
 
+&reg_nvcc_sd {
+	sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1>;
@@ -454,7 +458,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x19
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0xd0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000d0
 		>;
 	};
 
@@ -467,7 +471,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x19
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0xd0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000d0
 		>;
 	};
 
@@ -480,7 +484,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x19
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0xd0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000d0
 		>;
 	};
 };
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
index 663ae52b48526..d455429652305 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
@@ -342,6 +342,7 @@ reg_nvcc_sd: LDO5 {
 				regulator-name = "NVCC_SD (LDO5)";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
+				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
 			};
 		};
 	};
@@ -794,7 +795,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0 /* SDIO_A_D1 */
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0 /* SDIO_A_D2 */
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0 /* SDIO_A_D3 */
 			MX8MM_IOMUXC_SD2_WP_USDHC2_WP			0x400000d6 /* SDIO_A_WP */
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x90
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000090
 		>;
 	};
 
@@ -807,7 +808,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4 /* SDIO_A_D1 */
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4 /* SDIO_A_D2 */
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4 /* SDIO_A_D3 */
 			MX8MM_IOMUXC_SD2_WP_USDHC2_WP			0x400000d6 /* SDIO_A_WP */
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x90
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000090
 		>;
 	};
 
@@ -820,7 +821,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6 /* SDIO_A_D1 */
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6 /* SDIO_A_D2 */
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6 /* SDIO_A_D3 */
 			MX8MM_IOMUXC_SD2_WP_USDHC2_WP			0x400000d6 /* SDIO_A_WP */
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x90
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x40000090
 		>;
 	};
 
-- 
2.46.1

[PATCH v2 10/11] arm64: dts: imx93-kontron: Fix SD card IO voltage control

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The OSM-S i.MX93 SoM doesn't have the VSELECT signal of the USDHC
controller connected to the PMICs SD_VSEL input. Instead SD_VSEL
is hardwired to low level. Let the driver know this in order to
use the proper register for reading and writing the voltage level.

This fixes SD card access with the latest hardware revision of
the Kontron OSM-S i.MX93 SoM.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* new patch
---
 arch/arm64/boot/dts/freescale/imx93-kontron-osm-s.dtsi | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx93-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx93-kontron-osm-s.dtsi
index 47c1363a2f99a..119a162070596 100644
--- a/arch/arm64/boot/dts/freescale/imx93-kontron-osm-s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93-kontron-osm-s.dtsi
@@ -189,6 +189,7 @@ reg_nvcc_sd: LDO5 {
 				regulator-name = "NVCC_SD (LDO5)";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
+				nxp,sd-vsel-fixed-low;
 			};
 		};
 	};
@@ -282,6 +283,7 @@ &usdhc2 { /* OSM-S SDIO_A */
 	pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;
 	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>;
 	vmmc-supply = <&reg_usdhc2_vcc>;
+	vqmmc-supply = <&reg_nvcc_sd>;
 	cd-gpios = <&gpio3 0 GPIO_ACTIVE_LOW>;
 };
 
@@ -553,7 +555,6 @@ MX93_PAD_SD2_DATA0__USDHC2_DATA0		0x40001382 /* SDIO_A_D0 */
 			MX93_PAD_SD2_DATA1__USDHC2_DATA1		0x40001382 /* SDIO_A_D1 */
 			MX93_PAD_SD2_DATA2__USDHC2_DATA2		0x40001382 /* SDIO_A_D2 */
 			MX93_PAD_SD2_DATA3__USDHC2_DATA3		0x40001382 /* SDIO_A_D3 */
-			MX93_PAD_SD2_VSELECT__USDHC2_VSELECT		0x1d0
 		>;
 	};
 
@@ -565,7 +566,6 @@ MX93_PAD_SD2_DATA0__USDHC2_DATA0		0x4000138e /* SDIO_A_D0 */
 			MX93_PAD_SD2_DATA1__USDHC2_DATA1		0x4000138e /* SDIO_A_D1 */
 			MX93_PAD_SD2_DATA2__USDHC2_DATA2		0x4000138e /* SDIO_A_D2 */
 			MX93_PAD_SD2_DATA3__USDHC2_DATA3		0x4000138e /* SDIO_A_D3 */
-			MX93_PAD_SD2_VSELECT__USDHC2_VSELECT		0x1d0
 		>;
 	};
 
@@ -577,7 +577,6 @@ MX93_PAD_SD2_DATA0__USDHC2_DATA0		0x400013fe /* SDIO_A_D0 */
 			MX93_PAD_SD2_DATA1__USDHC2_DATA1		0x400013fe /* SDIO_A_D1 */
 			MX93_PAD_SD2_DATA2__USDHC2_DATA2		0x400013fe /* SDIO_A_D2 */
 			MX93_PAD_SD2_DATA3__USDHC2_DATA3		0x400013fe /* SDIO_A_D3 */
-			MX93_PAD_SD2_VSELECT__USDHC2_VSELECT		0x1d0
 		>;
 	};
 
-- 
2.46.1

[PATCH v2 11/11] arm64: dts: imx8mp-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This fixes the LDO5 regulator handling of the pca9450 driver by
taking the status of the SD_VSEL into account to determine which
configuration register is used for the voltage setting.

Even without this change there is no functional issue, as the code
for switching the voltage in sdhci.c currently switches both, the
VSELECT/SD_VSEL signal and the regulator voltage at the same time
and doesn't run into an invalid corner case.

We should still make sure, that we always use the correct register
when controlling the regulator. At least in U-Boot this fixes an
actual bug where the wrong IO voltage is used and it makes sure
that the correct voltage can be read from sysfs.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* new patch
---
 arch/arm64/boot/dts/freescale/imx8mp-kontron-osm-s.dtsi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-kontron-osm-s.dtsi
index e0e9f6f7616d9..b97bfeb1c30f8 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-kontron-osm-s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-kontron-osm-s.dtsi
@@ -311,6 +311,7 @@ reg_nvcc_sd: LDO5 {
 				regulator-name = "NVCC_SD (LDO5)";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
+				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
 			};
 		};
 	};
@@ -808,7 +809,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0		0x1d0 /* SDIO_A_D0 */
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1		0x1d0 /* SDIO_A_D1 */
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2		0x1d0 /* SDIO_A_D2 */
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3		0x1d0 /* SDIO_A_D3 */
-			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x1d0
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -820,7 +821,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0		0x1d4 /* SDIO_A_D0 */
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1		0x1d4 /* SDIO_A_D1 */
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2		0x1d4 /* SDIO_A_D2 */
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3		0x1d4 /* SDIO_A_D3 */
-			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x1d0
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -832,7 +833,7 @@ MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0		0x1d6 /* SDIO_A_D0 */
 			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1		0x1d6 /* SDIO_A_D1 */
 			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2		0x1d6 /* SDIO_A_D2 */
 			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3		0x1d6 /* SDIO_A_D3 */
-			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x1d0
+			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
-- 
2.46.1

Re: [PATCH v2 01/11] Revert "regulator: pca9450: Add sd-vsel GPIO"

[Mark Brown]

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

On Wed, Nov 27, 2024 at 05:42:17PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This reverts commit 27866e3e8a7e93494f8374f48061aa73ee46ceb2.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

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

Re: [PATCH v2 03/11] dt-bindings: regulator: pca9450: Document nxp,sd-vsel-fixed-low property for LDO5

[Conor Dooley]

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

On Wed, Nov 27, 2024 at 05:42:19PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This new property can be used for boards which have the SD_VSEL tied
> to a fixed low level. The voltage of LDO5 is therefore only controlled
> by writing to the LDO5CTRL_L register.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * new patch
> ---
>  .../bindings/regulator/nxp,pca9450-regulator.yaml           | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> index 5d0d684186c96..0e19c54aa5f8a 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> @@ -49,6 +49,12 @@ properties:
>            Properties for single LDO5 regulator.
>  
>          properties:
> +          nxp,sd-vsel-fixed-low:
> +            type: boolean
> +            description:
> +              Let the driver know that SD_VSEL is hardwired to low level and
> +              there is no GPIO to get the actual value from.

Does this mean that if you don't provide the property or a GPIO it is tied
high or High-Z? If so, please mention it here. More likely, given the
context of this patch, no gpio and no tied low property means the driver
should handle things as they used to be - but you should call that out
in your commit message to be clear.

> +
>            sd-vsel-gpios:
>              description:
>                GPIO that can be used to read the current status of the SD_VSEL
> -- 
> 2.46.1
> 

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

Re: [PATCH v2 01/11] Revert "regulator: pca9450: Add sd-vsel GPIO"

[Conor Dooley]

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

On Wed, Nov 27, 2024 at 05:42:17PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This reverts commit 27866e3e8a7e93494f8374f48061aa73ee46ceb2.
> 
> It turned out that this feature was implemented based on
> the wrong assumption that the SD_VSEL signal needs to be
> controlled as GPIO in any case.
> 
> In fact the straight-forward approach is to mux the signal
> as USDHC_VSELECT and let the USDHC controller do the job.
> 
> Most users never even used this property and the few who
> did have been or are getting migrated to the alternative
> approach.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * split revert into separate patch
> ---
>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> index f8057bba747a5..79fc0baf5fa2f 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> @@ -77,11 +77,6 @@ properties:
>  
>      additionalProperties: false
>  
> -  sd-vsel-gpios:
> -    description: GPIO that is used to switch LDO5 between being configured by
> -      LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is
> -      connected to a host GPIO.

Your driver side of this, that I wasn't sent and cba downloading an
mbox of is not backwards compatible. The code has been there for a few
years, are you sure that there are no out of tree users or other OSes
that use the property?

tbh, I think all 3 of your dt-binding patches should be squashed rather
than drip-feeding the conversion. It makes more sense as a single
change, rather than splitting the rationales across 3 patches.

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

Re: [PATCH v2 03/11] dt-bindings: regulator: pca9450: Document nxp,sd-vsel-fixed-low property for LDO5

[Frieder Schrempf]

On 28.11.24 6:33 PM, Conor Dooley wrote:
> On Wed, Nov 27, 2024 at 05:42:19PM +0100, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> This new property can be used for boards which have the SD_VSEL tied
>> to a fixed low level. The voltage of LDO5 is therefore only controlled
>> by writing to the LDO5CTRL_L register.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Changes for v2:
>> * new patch
>> ---
>>  .../bindings/regulator/nxp,pca9450-regulator.yaml           | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> index 5d0d684186c96..0e19c54aa5f8a 100644
>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> @@ -49,6 +49,12 @@ properties:
>>            Properties for single LDO5 regulator.
>>  
>>          properties:
>> +          nxp,sd-vsel-fixed-low:
>> +            type: boolean
>> +            description:
>> +              Let the driver know that SD_VSEL is hardwired to low level and
>> +              there is no GPIO to get the actual value from.
> 
> Does this mean that if you don't provide the property or a GPIO it is tied
> high or High-Z? If so, please mention it here. More likely, given the
> context of this patch, no gpio and no tied low property means the driver
> should handle things as they used to be - but you should call that out
> in your commit message to be clear.

Providing neither 'sd-vsel-gpios', nor 'nxp,sd-vsel-fixed-low' means the
driver has to assume that SD_VSEL is tied high and it has to use the
LDO5CTRL_H for voltage control.

I will make this more clear in the commit message.

This is the original/current behavior of the driver, though it doesn't
match the actual hardware as all known boards actually have the SD_VSEL
connected to the USDHC_VSELECT (which changes state whenever the USDHC
controller wants to switch IO voltage).

Getting rid of this mismatch is one of the main motivations for this series.

> 
>> +
>>            sd-vsel-gpios:
>>              description:
>>                GPIO that can be used to read the current status of the SD_VSEL
>> -- 
>> 2.46.1
>>

Re: [PATCH v2 01/11] Revert "regulator: pca9450: Add sd-vsel GPIO"

[Frieder Schrempf]

On 28.11.24 6:37 PM, Conor Dooley wrote:
> On Wed, Nov 27, 2024 at 05:42:17PM +0100, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> This reverts commit 27866e3e8a7e93494f8374f48061aa73ee46ceb2.
>>
>> It turned out that this feature was implemented based on
>> the wrong assumption that the SD_VSEL signal needs to be
>> controlled as GPIO in any case.
>>
>> In fact the straight-forward approach is to mux the signal
>> as USDHC_VSELECT and let the USDHC controller do the job.
>>
>> Most users never even used this property and the few who
>> did have been or are getting migrated to the alternative
>> approach.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Changes for v2:
>> * split revert into separate patch
>> ---
>>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> index f8057bba747a5..79fc0baf5fa2f 100644
>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> @@ -77,11 +77,6 @@ properties:
>>  
>>      additionalProperties: false
>>  
>> -  sd-vsel-gpios:
>> -    description: GPIO that is used to switch LDO5 between being configured by
>> -      LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is
>> -      connected to a host GPIO.
> 
> Your driver side of this, that I wasn't sent and cba downloading an
> mbox of is not backwards compatible. The code has been there for a few
> years, are you sure that there are no out of tree users or other OSes
> that use the property?

Yes, this is not backwards compatible. I introduced the original meaning
for the sd-vsel-gpios property based on some misunderstanding of how the
hardware actually works. Therefore I'm quite sure that except for the
cases where someone copied my erroneous implementation into their
devicetree, nobody has really any reason to actually use this.

In-tree all users have been removed (one fix still included in this
series). Of course we can't be fully sure that there isn't someone out
there having non-standard hardware (SD_VSEL not connected to
USDHC_VSELECT but to GPIO only) and using the old sd-vsel-gpios, but the
probability is very, very low.

IMHO taking the small risk here is better than keeping the misleading
implementation which will likely cause confusion and failures in the
future. But of course that's not up to me to decide.

> 
> tbh, I think all 3 of your dt-binding patches should be squashed rather
> than drip-feeding the conversion. It makes more sense as a single
> change, rather than splitting the rationales across 3 patches.

Ok, if you like this better in one change I can squash these for the
next version.

Thanks!

Re: [PATCH v2 03/11] dt-bindings: regulator: pca9450: Document nxp,sd-vsel-fixed-low property for LDO5

[Conor Dooley]

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

On Tue, Dec 10, 2024 at 04:36:41PM +0100, Frieder Schrempf wrote:
> On 28.11.24 6:33 PM, Conor Dooley wrote:
> > On Wed, Nov 27, 2024 at 05:42:19PM +0100, Frieder Schrempf wrote:
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> This new property can be used for boards which have the SD_VSEL tied
> >> to a fixed low level. The voltage of LDO5 is therefore only controlled
> >> by writing to the LDO5CTRL_L register.
> >>
> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >> ---
> >> Changes for v2:
> >> * new patch
> >> ---
> >>  .../bindings/regulator/nxp,pca9450-regulator.yaml           | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> index 5d0d684186c96..0e19c54aa5f8a 100644
> >> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> @@ -49,6 +49,12 @@ properties:
> >>            Properties for single LDO5 regulator.
> >>  
> >>          properties:
> >> +          nxp,sd-vsel-fixed-low:
> >> +            type: boolean
> >> +            description:
> >> +              Let the driver know that SD_VSEL is hardwired to low level and
> >> +              there is no GPIO to get the actual value from.
> > 
> > Does this mean that if you don't provide the property or a GPIO it is tied
> > high or High-Z? If so, please mention it here. More likely, given the
> > context of this patch, no gpio and no tied low property means the driver
> > should handle things as they used to be - but you should call that out
> > in your commit message to be clear.
> 
> Providing neither 'sd-vsel-gpios', nor 'nxp,sd-vsel-fixed-low' means the
> driver has to assume that SD_VSEL is tied high and it has to use the
> LDO5CTRL_H for voltage control.
> 
> I will make this more clear in the commit message.
> 
> This is the original/current behavior of the driver, though it doesn't
> match the actual hardware as all known boards actually have the SD_VSEL
> connected to the USDHC_VSELECT (which changes state whenever the USDHC
> controller wants to switch IO voltage).

That's fine, as long as it is a match for the old behaviour. Sorry for
the delay responding, I was unexpectedly AFK last week.

> 
> Getting rid of this mismatch is one of the main motivations for this series.
> 
> > 
> >> +
> >>            sd-vsel-gpios:
> >>              description:
> >>                GPIO that can be used to read the current status of the SD_VSEL
> >> -- 
> >> 2.46.1
> >>
> 

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

Re: [PATCH v2 01/11] Revert "regulator: pca9450: Add sd-vsel GPIO"

[Conor Dooley]

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

On Tue, Dec 10, 2024 at 04:59:06PM +0100, Frieder Schrempf wrote:
> On 28.11.24 6:37 PM, Conor Dooley wrote:
> > On Wed, Nov 27, 2024 at 05:42:17PM +0100, Frieder Schrempf wrote:
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> This reverts commit 27866e3e8a7e93494f8374f48061aa73ee46ceb2.
> >>
> >> It turned out that this feature was implemented based on
> >> the wrong assumption that the SD_VSEL signal needs to be
> >> controlled as GPIO in any case.
> >>
> >> In fact the straight-forward approach is to mux the signal
> >> as USDHC_VSELECT and let the USDHC controller do the job.
> >>
> >> Most users never even used this property and the few who
> >> did have been or are getting migrated to the alternative
> >> approach.
> >>
> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >> ---
> >> Changes for v2:
> >> * split revert into separate patch
> >> ---
> >>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml | 5 -----
> >>  1 file changed, 5 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> index f8057bba747a5..79fc0baf5fa2f 100644
> >> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> @@ -77,11 +77,6 @@ properties:
> >>  
> >>      additionalProperties: false
> >>  
> >> -  sd-vsel-gpios:
> >> -    description: GPIO that is used to switch LDO5 between being configured by
> >> -      LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is
> >> -      connected to a host GPIO.
> > 
> > Your driver side of this, that I wasn't sent and cba downloading an
> > mbox of is not backwards compatible. The code has been there for a few
> > years, are you sure that there are no out of tree users or other OSes
> > that use the property?
> 
> Yes, this is not backwards compatible. I introduced the original meaning
> for the sd-vsel-gpios property based on some misunderstanding of how the
> hardware actually works. Therefore I'm quite sure that except for the
> cases where someone copied my erroneous implementation into their
> devicetree, nobody has really any reason to actually use this.
> 
> In-tree all users have been removed (one fix still included in this
> series). Of course we can't be fully sure that there isn't someone out
> there having non-standard hardware (SD_VSEL not connected to
> USDHC_VSELECT but to GPIO only) and using the old sd-vsel-gpios, but the
> probability is very, very low.
> 
> IMHO taking the small risk here is better than keeping the misleading
> implementation which will likely cause confusion and failures in the
> future. But of course that's not up to me to decide.

Given that the !property case retains the behaviour from before, only
those with the property are affected - which means if it does end up
being problematic then it can be rectified at that point in time.

> > tbh, I think all 3 of your dt-binding patches should be squashed rather
> > than drip-feeding the conversion. It makes more sense as a single
> > change, rather than splitting the rationales across 3 patches.
> 
> Ok, if you like this better in one change I can squash these for the
> next version.

Sounds good, sorry again for the delay getting back to you.

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