[PATCH v3 0/2] regulator: richtek,rt4801: parse GPIOs per regulator

[PATCH v3 0/2] regulator: richtek,rt4801: parse GPIOs per regulator

[Krzysztof Kozlowski]

Hi,

Not tested.

Follow up of discussion:
https://lore.kernel.org/all/2d124d53-1f36-5315-3877-af8f8ad5f824@linaro.org/#t

Changes since v2
================
Patch 2: Do not use core's ena_gpiod feature because of lost voltage issue
         (ChiYuan Huang).

Changes since v1
================
Patch 1: add review tag.
Patch 2: Check for ena_gpio_state before accessing I2C (issue pointed out by
         ChiYuan Huang).

Best regards,
Krzysztof

Krzysztof Kozlowski (2):
  regulator: dt-bindings: richtek,rt4801: use existing ena_gpiod feature
  regulator: richtek,rt4801: parse GPIOs per regulator

 .../regulator/richtek,rt4801-regulator.yaml   | 14 +++++-
 drivers/regulator/rt4801-regulator.c          | 49 ++++++++++++++-----
 2 files changed, 51 insertions(+), 12 deletions(-)

-- 
2.32.0

[PATCH v3 1/2] regulator: dt-bindings: richtek,rt4801: use existing ena_gpiod feature

[Krzysztof Kozlowski]

The binding and driver duplicated regulator core feature of controlling
regulators with GPIOs (of_parse_cb + ena_gpiod) and created its own
enable-gpios property with multiple GPIOs.

This is a less preferred way, because enable-gpios should enable only one
element, not multiple. It also duplicates existing solution.

Deprecate the original 'enable-gpios' and add per-regulator property.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../regulator/richtek,rt4801-regulator.yaml        | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.yaml
index 4a4dfa7d9d09..091150c4e579 100644
--- a/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt4801-regulator.yaml
@@ -32,8 +32,11 @@ properties:
       If only one gpio is specified, only one gpio used to control ENP/ENM.
       Else if both are specified, DSVP/DSVN could be controlled individually.
       If this property not specified, treat both as always-on regulators.
+
+      Property is deprecated. Use enable-gpios in each regulator.
     minItems: 1
     maxItems: 2
+    deprecated: true
 
 patternProperties:
   "^DSV(P|N)$":
@@ -42,6 +45,14 @@ patternProperties:
     description:
       Properties for single display bias regulator.
 
+    properties:
+      enable-gpios:
+        description:
+          GPIO to use to enable DSVP/DSVN regulator. One GPIO can be configured
+          for controlling both regulators.  If this property not specified for
+          any regulator, treat both as always-on regulators.
+        maxItems: 1
+
 required:
   - compatible
   - reg
@@ -57,19 +68,20 @@ examples:
         rt4801@73 {
             compatible = "richtek,rt4801";
             reg = <0x73>;
-            enable-gpios = <&gpio26 2 0>, <&gpio26 3 0>;
 
             dsvp: DSVP {
                 regulator-name = "rt4801,dsvp";
                 regulator-min-microvolt = <4000000>;
                 regulator-max-microvolt = <6000000>;
                 regulator-boot-on;
+                enable-gpios = <&gpio26 2 0>;
             };
             dsvn: DSVN {
                 regulator-name = "rt4801,dsvn";
                 regulator-min-microvolt = <4000000>;
                 regulator-max-microvolt = <6000000>;
                 regulator-boot-on;
+                enable-gpios = <&gpio26 3 0>;
             };
 
         };
-- 
2.32.0

[PATCH v3 2/2] regulator: richtek,rt4801: parse GPIOs per regulator

[Krzysztof Kozlowski]

Having one enable-gpios property for all regulators is discouraged and
instead, similarly to regulator core ena_gpiod feature, each GPIO should
be present in each regulator node.  Add support for parsing such GPIOs,
keeping backwards compatibility.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/regulator/rt4801-regulator.c | 49 +++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/rt4801-regulator.c b/drivers/regulator/rt4801-regulator.c
index 7a87788d3f09..563d79196fdd 100644
--- a/drivers/regulator/rt4801-regulator.c
+++ b/drivers/regulator/rt4801-regulator.c
@@ -29,11 +29,33 @@
 
 struct rt4801_priv {
 	struct device *dev;
-	struct gpio_descs *enable_gpios;
+	struct gpio_desc *enable_gpios[DSV_OUT_MAX];
 	unsigned int enable_flag;
 	unsigned int volt_sel[DSV_OUT_MAX];
 };
 
+static int rt4801_of_parse_cb(struct device_node *np,
+			      const struct regulator_desc *desc,
+			      struct regulator_config *config)
+{
+	struct rt4801_priv *priv = config->driver_data;
+	int id = desc->id;
+
+	if (priv->enable_gpios[id]) {
+		dev_warn(priv->dev, "duplicated enable-gpios property\n");
+		return 0;
+	}
+	priv->enable_gpios[id] = devm_fwnode_gpiod_get_index(priv->dev,
+							     of_fwnode_handle(np),
+							     "enable", 0,
+							     GPIOD_OUT_HIGH,
+							     "rt4801");
+	if (IS_ERR(priv->enable_gpios[id]))
+		priv->enable_gpios[id] = NULL;
+
+	return 0;
+}
+
 static int rt4801_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
 {
 	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
@@ -63,15 +85,14 @@ static int rt4801_get_voltage_sel(struct regulator_dev *rdev)
 static int rt4801_enable(struct regulator_dev *rdev)
 {
 	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
-	struct gpio_descs *gpios = priv->enable_gpios;
 	int id = rdev_get_id(rdev), ret;
 
-	if (!gpios || gpios->ndescs <= id) {
+	if (!priv->enable_gpios[id]) {
 		dev_warn(&rdev->dev, "no dedicated gpio can control\n");
 		goto bypass_gpio;
 	}
 
-	gpiod_set_value(gpios->desc[id], 1);
+	gpiod_set_value(priv->enable_gpios[id], 1);
 
 bypass_gpio:
 	ret = regmap_write(rdev->regmap, rdev->desc->vsel_reg, priv->volt_sel[id]);
@@ -85,15 +106,14 @@ static int rt4801_enable(struct regulator_dev *rdev)
 static int rt4801_disable(struct regulator_dev *rdev)
 {
 	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
-	struct gpio_descs *gpios = priv->enable_gpios;
 	int id = rdev_get_id(rdev);
 
-	if (!gpios || gpios->ndescs <= id) {
+	if (!priv->enable_gpios[id]) {
 		dev_warn(&rdev->dev, "no dedicated gpio can control\n");
 		goto bypass_gpio;
 	}
 
-	gpiod_set_value(gpios->desc[id], 0);
+	gpiod_set_value(priv->enable_gpios[id], 0);
 
 bypass_gpio:
 	priv->enable_flag &= ~BIT(id);
@@ -122,6 +142,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
 		.name = "DSVP",
 		.ops = &rt4801_regulator_ops,
 		.of_match = of_match_ptr("DSVP"),
+		.of_parse_cb = rt4801_of_parse_cb,
 		.type = REGULATOR_VOLTAGE,
 		.id = DSV_OUT_POS,
 		.min_uV = MIN_UV,
@@ -135,6 +156,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
 		.name = "DSVN",
 		.ops = &rt4801_regulator_ops,
 		.of_match = of_match_ptr("DSVN"),
+		.of_parse_cb = rt4801_of_parse_cb,
 		.type = REGULATOR_VOLTAGE,
 		.id = DSV_OUT_NEG,
 		.min_uV = MIN_UV,
@@ -172,10 +194,15 @@ static int rt4801_probe(struct i2c_client *i2c)
 		return PTR_ERR(regmap);
 	}
 
-	priv->enable_gpios = devm_gpiod_get_array_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
-	if (IS_ERR(priv->enable_gpios)) {
-		dev_err(&i2c->dev, "Failed to get gpios\n");
-		return PTR_ERR(priv->enable_gpios);
+	for (i = 0; i < DSV_OUT_MAX; i++) {
+		priv->enable_gpios[i] = devm_gpiod_get_index_optional(&i2c->dev,
+								      "enable",
+								      i,
+								      GPIOD_OUT_HIGH);
+		if (IS_ERR(priv->enable_gpios[i])) {
+			dev_err(&i2c->dev, "Failed to get gpios\n");
+			return PTR_ERR(priv->enable_gpios[i]);
+		}
 	}
 
 	for (i = 0; i < DSV_OUT_MAX; i++) {
-- 
2.32.0

Re: [PATCH v3 2/2] regulator: richtek,rt4801: parse GPIOs per regulator

[ChiYuan Huang]

On Mon, Apr 25, 2022 at 09:24:55AM +0200, Krzysztof Kozlowski wrote:
> Having one enable-gpios property for all regulators is discouraged and
> instead, similarly to regulator core ena_gpiod feature, each GPIO should
> be present in each regulator node.  Add support for parsing such GPIOs,
> keeping backwards compatibility.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/regulator/rt4801-regulator.c | 49 +++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/rt4801-regulator.c b/drivers/regulator/rt4801-regulator.c
> index 7a87788d3f09..563d79196fdd 100644
> --- a/drivers/regulator/rt4801-regulator.c
> +++ b/drivers/regulator/rt4801-regulator.c
> @@ -29,11 +29,33 @@
>  
>  struct rt4801_priv {
>  	struct device *dev;
> -	struct gpio_descs *enable_gpios;
> +	struct gpio_desc *enable_gpios[DSV_OUT_MAX];
>  	unsigned int enable_flag;
>  	unsigned int volt_sel[DSV_OUT_MAX];
>  };
>  
> +static int rt4801_of_parse_cb(struct device_node *np,
> +			      const struct regulator_desc *desc,
> +			      struct regulator_config *config)
> +{
> +	struct rt4801_priv *priv = config->driver_data;
> +	int id = desc->id;
> +
> +	if (priv->enable_gpios[id]) {
> +		dev_warn(priv->dev, "duplicated enable-gpios property\n");
> +		return 0;
> +	}
> +	priv->enable_gpios[id] = devm_fwnode_gpiod_get_index(priv->dev,
> +							     of_fwnode_handle(np),
> +							     "enable", 0,
> +							     GPIOD_OUT_HIGH,
> +							     "rt4801");
> +	if (IS_ERR(priv->enable_gpios[id]))
> +		priv->enable_gpios[id] = NULL;
> +
> +	return 0;
> +}
> +
>  static int rt4801_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
>  {
>  	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
> @@ -63,15 +85,14 @@ static int rt4801_get_voltage_sel(struct regulator_dev *rdev)
>  static int rt4801_enable(struct regulator_dev *rdev)
>  {
>  	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
> -	struct gpio_descs *gpios = priv->enable_gpios;
>  	int id = rdev_get_id(rdev), ret;
>  
> -	if (!gpios || gpios->ndescs <= id) {
> +	if (!priv->enable_gpios[id]) {
>  		dev_warn(&rdev->dev, "no dedicated gpio can control\n");
>  		goto bypass_gpio;
>  	}
>  
> -	gpiod_set_value(gpios->desc[id], 1);
> +	gpiod_set_value(priv->enable_gpios[id], 1);
>  
>  bypass_gpio:
>  	ret = regmap_write(rdev->regmap, rdev->desc->vsel_reg, priv->volt_sel[id]);
> @@ -85,15 +106,14 @@ static int rt4801_enable(struct regulator_dev *rdev)
>  static int rt4801_disable(struct regulator_dev *rdev)
>  {
>  	struct rt4801_priv *priv = rdev_get_drvdata(rdev);
> -	struct gpio_descs *gpios = priv->enable_gpios;
>  	int id = rdev_get_id(rdev);
>  
> -	if (!gpios || gpios->ndescs <= id) {
> +	if (!priv->enable_gpios[id]) {
>  		dev_warn(&rdev->dev, "no dedicated gpio can control\n");
>  		goto bypass_gpio;
>  	}
>  
> -	gpiod_set_value(gpios->desc[id], 0);
> +	gpiod_set_value(priv->enable_gpios[id], 0);
>  
>  bypass_gpio:
>  	priv->enable_flag &= ~BIT(id);
> @@ -122,6 +142,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
>  		.name = "DSVP",
>  		.ops = &rt4801_regulator_ops,
>  		.of_match = of_match_ptr("DSVP"),
> +		.of_parse_cb = rt4801_of_parse_cb,
>  		.type = REGULATOR_VOLTAGE,
>  		.id = DSV_OUT_POS,
>  		.min_uV = MIN_UV,
> @@ -135,6 +156,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
>  		.name = "DSVN",
>  		.ops = &rt4801_regulator_ops,
>  		.of_match = of_match_ptr("DSVN"),
> +		.of_parse_cb = rt4801_of_parse_cb,
>  		.type = REGULATOR_VOLTAGE,
>  		.id = DSV_OUT_NEG,
>  		.min_uV = MIN_UV,
> @@ -172,10 +194,15 @@ static int rt4801_probe(struct i2c_client *i2c)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	priv->enable_gpios = devm_gpiod_get_array_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> -	if (IS_ERR(priv->enable_gpios)) {
> -		dev_err(&i2c->dev, "Failed to get gpios\n");
> -		return PTR_ERR(priv->enable_gpios);
> +	for (i = 0; i < DSV_OUT_MAX; i++) {
> +		priv->enable_gpios[i] = devm_gpiod_get_index_optional(&i2c->dev,
> +								      "enable",
> +								      i,
> +								      GPIOD_OUT_HIGH);
> +		if (IS_ERR(priv->enable_gpios[i])) {
> +			dev_err(&i2c->dev, "Failed to get gpios\n");
> +			return PTR_ERR(priv->enable_gpios[i]);
> +		}
>  	}
You can directly removed all about 'enable-gpios' in probe phase.
Just keep of_parse_cb to get per regulator 'enable' gpio. 

The others looks fine.

Best regards,
ChiYuan Huang.
>  
>  	for (i = 0; i < DSV_OUT_MAX; i++) {
> -- 
> 2.32.0
> 

Re: [PATCH v3 2/2] regulator: richtek,rt4801: parse GPIOs per regulator

[Krzysztof Kozlowski]

On 25/04/2022 09:58, ChiYuan Huang wrote:
>>  
>> -	priv->enable_gpios = devm_gpiod_get_array_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
>> -	if (IS_ERR(priv->enable_gpios)) {
>> -		dev_err(&i2c->dev, "Failed to get gpios\n");
>> -		return PTR_ERR(priv->enable_gpios);
>> +	for (i = 0; i < DSV_OUT_MAX; i++) {
>> +		priv->enable_gpios[i] = devm_gpiod_get_index_optional(&i2c->dev,
>> +								      "enable",
>> +								      i,
>> +								      GPIOD_OUT_HIGH);
>> +		if (IS_ERR(priv->enable_gpios[i])) {
>> +			dev_err(&i2c->dev, "Failed to get gpios\n");
>> +			return PTR_ERR(priv->enable_gpios[i]);
>> +		}
>>  	}
> You can directly removed all about 'enable-gpios' in probe phase.
> Just keep of_parse_cb to get per regulator 'enable' gpio. 

We cannot, it would break the ABI and make the change backwards
incompatible.


Best regards,
Krzysztof

Re: [PATCH v3 2/2] regulator: richtek,rt4801: parse GPIOs per regulator

[ChiYuan Huang]

On Mon, Apr 25, 2022 at 10:01:24AM +0200, Krzysztof Kozlowski wrote:
> On 25/04/2022 09:58, ChiYuan Huang wrote:
> >>  
> >> -	priv->enable_gpios = devm_gpiod_get_array_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> >> -	if (IS_ERR(priv->enable_gpios)) {
> >> -		dev_err(&i2c->dev, "Failed to get gpios\n");
> >> -		return PTR_ERR(priv->enable_gpios);
> >> +	for (i = 0; i < DSV_OUT_MAX; i++) {
> >> +		priv->enable_gpios[i] = devm_gpiod_get_index_optional(&i2c->dev,
> >> +								      "enable",
> >> +								      i,
> >> +								      GPIOD_OUT_HIGH);
> >> +		if (IS_ERR(priv->enable_gpios[i])) {
> >> +			dev_err(&i2c->dev, "Failed to get gpios\n");
> >> +			return PTR_ERR(priv->enable_gpios[i]);
> >> +		}
> >>  	}
> > You can directly removed all about 'enable-gpios' in probe phase.
> > Just keep of_parse_cb to get per regulator 'enable' gpio. 
> 
> We cannot, it would break the ABI and make the change backwards
> incompatible.
> 

Almost forget the backward compatibility.

I'm testing it right now. After that I'll add 'Tested-by' tag.

Thanks.
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 2/2] regulator: richtek,rt4801: parse GPIOs per regulator

[ChiYuan Huang]

On Mon, Apr 25, 2022 at 04:08:31PM +0800, ChiYuan Huang wrote:
> On Mon, Apr 25, 2022 at 10:01:24AM +0200, Krzysztof Kozlowski wrote:
> > On 25/04/2022 09:58, ChiYuan Huang wrote:
> > >>  
> > >> -	priv->enable_gpios = devm_gpiod_get_array_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> > >> -	if (IS_ERR(priv->enable_gpios)) {
> > >> -		dev_err(&i2c->dev, "Failed to get gpios\n");
> > >> -		return PTR_ERR(priv->enable_gpios);
> > >> +	for (i = 0; i < DSV_OUT_MAX; i++) {
> > >> +		priv->enable_gpios[i] = devm_gpiod_get_index_optional(&i2c->dev,
> > >> +								      "enable",
> > >> +								      i,
> > >> +								      GPIOD_OUT_HIGH);
> > >> +		if (IS_ERR(priv->enable_gpios[i])) {
> > >> +			dev_err(&i2c->dev, "Failed to get gpios\n");
> > >> +			return PTR_ERR(priv->enable_gpios[i]);
> > >> +		}
> > >>  	}
> > > You can directly removed all about 'enable-gpios' in probe phase.
> > > Just keep of_parse_cb to get per regulator 'enable' gpio. 
> > 
> > We cannot, it would break the ABI and make the change backwards
> > incompatible.
> > 
> 
> Almost forget the backward compatibility.
> 
> I'm testing it right now. After that I'll add 'Tested-by' tag.
> 
> Thanks.

Tested-by: ChiYuan Huang <cy_huang@richtek.com>

Best regards,
ChiYuan Huang.
> > 
> > Best regards,
> > Krzysztof

Re: [PATCH v3 0/2] regulator: richtek,rt4801: parse GPIOs per regulator

[Mark Brown]

On Mon, 25 Apr 2022 09:24:53 +0200, Krzysztof Kozlowski wrote:
> Not tested.
> 
> Follow up of discussion:
> https://lore.kernel.org/all/2d124d53-1f36-5315-3877-af8f8ad5f824@linaro.org/#t
> 
> Changes since v2
> ================
> Patch 2: Do not use core's ena_gpiod feature because of lost voltage issue
>          (ChiYuan Huang).
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] regulator: dt-bindings: richtek,rt4801: use existing ena_gpiod feature
      commit: a498db69dc6d7511d76a4f04ec19c5e378db1c3b
[2/2] regulator: richtek,rt4801: parse GPIOs per regulator
      commit: 6963569103c02b22076da245b19bd081d8da8f9e

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark