[PATCH 1/2] regulator: fixed: use devm_* for gpio request

[PATCH 1/2] regulator: fixed: use devm_* for gpio request

[Laxman Dewangan]

Use devm_* version of gpio APIs gpio_request_one() for
requesting gpios.
This avoid extra code for freeing gpios.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/fixed.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 8bda365..0a0baf3 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -235,7 +235,7 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 		if (config->gpio_is_open_drain)
 			gpio_flag |= GPIOF_OPEN_DRAIN;
 
-		ret = gpio_request_one(config->gpio, gpio_flag,
+		ret = devm_gpio_request_one(&pdev->dev, config->gpio, gpio_flag,
 						config->supply_name);
 		if (ret) {
 			dev_err(&pdev->dev,
@@ -259,7 +259,7 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	if (IS_ERR(drvdata->dev)) {
 		ret = PTR_ERR(drvdata->dev);
 		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
-		goto err_gpio;
+		goto err_name;
 	}
 
 	platform_set_drvdata(pdev, drvdata);
@@ -269,9 +269,6 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_gpio:
-	if (gpio_is_valid(config->gpio))
-		gpio_free(config->gpio);
 err_name:
 	kfree(drvdata->desc.name);
 err:
@@ -283,8 +280,6 @@ static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
 	struct fixed_voltage_data *drvdata = platform_get_drvdata(pdev);
 
 	regulator_unregister(drvdata->dev);
-	if (gpio_is_valid(drvdata->gpio))
-		gpio_free(drvdata->gpio);
 	kfree(drvdata->desc.name);
 
 	return 0;
-- 
1.7.1.1

[PATCH 2/2] regulator: fixed: dt: support for input supply

[Laxman Dewangan]

Add support for input supply in DT parsing of node.
The input supply will be provided by the property
"vin-supply" in the regulator node.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 .../bindings/regulator/fixed-regulator.txt         |    2 ++
 drivers/regulator/fixed.c                          |   12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 2f5b6b1..4fae41d 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -10,6 +10,7 @@ Optional properties:
 If this property is missing, the default assumed is Active low.
 - gpio-open-drain: GPIO is open drain type.
   If this property is missing then default assumption is false.
+-vin-supply: Input supply name.
 
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
@@ -29,4 +30,5 @@ Example:
 		enable-active-high;
 		regulator-boot-on;
 		gpio-open-drain;
+		vin-supply = <&parent_reg>;
 	};
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 0a0baf3..8633e21 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -45,13 +45,14 @@ struct fixed_voltage_data {
 /**
  * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
  * @dev: device requesting for fixed_voltage_config
+ * @vin_supply: pointer to store whether input supply is available or not.
  *
  * Populates fixed_voltage_config structure by extracting data from device
  * tree node, returns a pointer to the populated structure of NULL if memory
  * alloc fails.
  */
 static struct fixed_voltage_config *
-of_get_fixed_voltage_config(struct device *dev)
+of_get_fixed_voltage_config(struct device *dev, bool *vin_supply)
 {
 	struct fixed_voltage_config *config;
 	struct device_node *np = dev->of_node;
@@ -93,6 +94,9 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (of_find_property(np, "gpio-open-drain", NULL))
 		config->gpio_is_open_drain = true;
 
+	if (of_find_property(np, "vin-supply", NULL))
+		*vin_supply = true;
+
 	return config;
 }
 
@@ -171,9 +175,10 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	struct fixed_voltage_data *drvdata;
 	struct regulator_config cfg = { };
 	int ret;
+	bool vin_supply = false;
 
 	if (pdev->dev.of_node)
-		config = of_get_fixed_voltage_config(&pdev->dev);
+		config = of_get_fixed_voltage_config(&pdev->dev , &vin_supply);
 	else
 		config = pdev->dev.platform_data;
 
@@ -197,6 +202,9 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	drvdata->desc.type = REGULATOR_VOLTAGE;
 	drvdata->desc.owner = THIS_MODULE;
 
+	if (vin_supply)
+		drvdata->desc.supply_name = "vin";
+
 	if (config->microvolts)
 		drvdata->desc.n_voltages = 1;
 
-- 
1.7.1.1

Re: [PATCH 1/2] regulator: fixed: use devm_* for gpio request

[Mark Brown]

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

On Mon, Jul 02, 2012 at 03:37:24PM +0530, Laxman Dewangan wrote:
> Use devm_* version of gpio APIs gpio_request_one() for
> requesting gpios.
> This avoid extra code for freeing gpios.

This can't be applied yet since the export for the devm version hasn't
been applied so it won't build and also conflicts with the factoring out
of the GPIO code to the core.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] regulator: fixed: use devm_* for gpio request

[Laxman Dewangan]

On Monday 02 July 2012 10:59 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 02, 2012 at 03:37:24PM +0530, Laxman Dewangan wrote:
>> Use devm_* version of gpio APIs gpio_request_one() for
>> requesting gpios.
>> This avoid extra code for freeing gpios.
> This can't be applied yet since the export for the devm version hasn't
> been applied so it won't build and also conflicts with the factoring out
> of the GPIO code to the core.
>
Oops, then lets ignore the 1/2 for now. If require, we will revisit later.

But what about 2/2? Does it look good? I can send that as separate patch 
if it is fine.

Re: [PATCH 2/2] regulator: fixed: dt: support for input supply

[Mark Brown]

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

On Mon, Jul 02, 2012 at 03:37:25PM +0530, Laxman Dewangan wrote:

> +	if (vin_supply)
> +		drvdata->desc.supply_name = "vin";
> +

This isn't great both in terms of it being conditional and the fact that
it's only usable on DT systems as there's no non-DT way to do this.  In
general unless we're working around some device tree issue (which should
be *very* rare) we shouldn't be adding any DT only stuff to generic
code, we need to be able to run Linux on non-DT systems.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regulator: fixed: dt: support for input supply

[Laxman Dewangan]

On Wednesday 04 July 2012 12:47 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 02, 2012 at 03:37:25PM +0530, Laxman Dewangan wrote:
>
>> +	if (vin_supply)
>> +		drvdata->desc.supply_name = "vin";
>> +
> This isn't great both in terms of it being conditional and the fact that
> it's only usable on DT systems as there's no non-DT way to do this.  In
> general unless we're working around some device tree issue (which should
> be *very* rare) we shouldn't be adding any DT only stuff to generic
> code, we need to be able to run Linux on non-DT systems.
>

In non-DT, the input supply is passed with the regulator_init_data 
through the fixed voltage config and it is filled in board files.
In one of discussion on input supply in my old patch, I understand that  
we should not provide the supply_regulator through the init_data as it 
is legacy and should provide with desc->supply_name.

so matching with this I have this change and put th supply regulator in 
desc->supply_name.

If we want to keep the same as DT and non-DT and not the extra care for 
this new policy then we can put the parsing code in 
of_get_fixed_voltage_config() and just fill the 
config->init_data->supply_regulator in this function.
The property checking for vin-supply can be done in this function only 
and set the config->init_data->supply_regulator = "vin" accordingly.

In this way there is no code for DT/Non-DT case.

Let me know your opinion.

Re: [PATCH 2/2] regulator: fixed: dt: support for input supply

[Mark Brown]

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

On Wed, Jul 04, 2012 at 12:32:08PM +0530, Laxman Dewangan wrote:

> In non-DT, the input supply is passed with the regulator_init_data
> through the fixed voltage config and it is filled in board files.

No, this is a legacy way of doing things.  All regulators should be
moving to specifying the name in their descriptor except for supplies
that are power roots (which are often fixed voltage regulators).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regulator: fixed: dt: support for input supply

[Laxman Dewangan]

On Wednesday 04 July 2012 03:51 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jul 04, 2012 at 12:32:08PM +0530, Laxman Dewangan wrote:
>
>> In non-DT, the input supply is passed with the regulator_init_data
>> through the fixed voltage config and it is filled in board files.
> No, this is a legacy way of doing things.  All regulators should be
> moving to specifying the name in their descriptor except for supplies
> that are power roots (which are often fixed voltage regulators).

Then the input_supply should be set through the regulator init data 
"init_data->supply_regulator" rather than the desc? If yes then also I 
need to change the forthcoming patch for tps65910 and tps6586x to pass 
the supply_regulator through init_data.

Re: [PATCH 2/2] regulator: fixed: dt: support for input supply

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]

On Wed, Jul 04, 2012 at 04:12:53PM +0530, Laxman Dewangan wrote:
> On Wednesday 04 July 2012 03:51 PM, Mark Brown wrote:
> >On Wed, Jul 04, 2012 at 12:32:08PM +0530, Laxman Dewangan wrote:

> >>In non-DT, the input supply is passed with the regulator_init_data
> >>through the fixed voltage config and it is filled in board files.

> >No, this is a legacy way of doing things.  All regulators should be
> >moving to specifying the name in their descriptor except for supplies
> >that are power roots (which are often fixed voltage regulators).

> Then the input_supply should be set through the regulator init data
> "init_data->supply_regulator" rather than the desc? If yes then also
> I need to change the forthcoming patch for tps65910 and tps6586x to
> pass the supply_regulator through init_data.

No, exactly the opposite way round - it should be being set in the
descriptor.  The issue with your patch was that it only allowed DT
systems to do this.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss