[v2,3/5] regulator: helper routine to extract regulator_init_data

Message ID 20111021082309.GA337@S2100-06.ap.freescale.net
State New
Headers show

Commit Message

Shawn Guo Oct. 21, 2011, 8:23 a.m.
On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> >>>mc13892-regulator driver.  And when getting probed, this driver will
> >>>call regulator_register() to register those 23 regulators individually.
> >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> >>>These extra 23 'dev' is totally new with DT.
> >>>
> >>
> >>but thats only because the mc13892-regulator driver is implemeted in
> >>such a way that all the regulators on the platform are bundled in as
> >>*one* device.
> >
> >I did not look into too many regulator drivers, but I expect this is
> >way that most regulator drivers are implemented in.  Having
> >mc13892-regulator being probed 23 times to register these 23 regulators
> >just makes less sense to me.
> >
> >>It would again depend on how you would pass these from
> >>the DT, if you indeed stick to the same way of bundling all regulators
> >>as one device from DT, the mc13892-regulator probe would just get called
> >>once and there would be one device associated, no?
> >>
> >Yes, I indeed would stick to the same way of bundling the registration
> >of all regulators with mc13892-regulator being probed once.  The problem
> >I have with the current regulator core DT implementation is that it
> >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> >being attached to rdev->dev.parent rather than itself.  Back to
> >mc13892-regulator example, that said, it requires the dev of
> >mc13892-regulator have the device_node of individual regulator attached
> >to.  IOW, the current implementation forces mc13892-regulator to be
> >probed 23 times to register those 23 regulators.  This is wrong to me.
> 
> I think I now understand to some extent the problem that you seem to be
> reporting. It is mainly with drivers which bundle all regulators and
> pass them as one device and would want to do so with DT too.
> 
> however I am still not clear on how what you seem to suggest would
> solve this problem. Note that not all drivers do it this way, and
> there are drivers where each regulator is considered as one device
> and I suspect they would remain that way with DT too. And hence we
> need to support both.
> 
> Do you have any RFC patch/code which could explain better what you are
> suggesting we do here?
> >
Here is what I changed based on your patches.  It only changes
drivers/regulator/core.c.

---8<-------
------->8---

And my dts file looks like something below.

	ecspi@70010000 { /* ECSPI1 */
		fsl,spi-num-chipselects = <2>;
		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
			   <&gpio3 25 0>; /* GPIO4_25 */
		status = "okay";

		pmic: mc13892@0 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mc13892";
			spi-max-frequency = <6000000>;
			reg = <0>;
			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */

			regulators {
				sw1reg: mc13892_sw1 {
					regulator-min-uV = <600000>;
					regulator-max-uV = <1375000>;
					regulator-change-voltage;
					regulator-boot-on;
					regulator-always-on;
				};

				sw2reg: mc13892_sw2 {
					regulator-min-uV = <900000>;
					regulator-max-uV = <1850000>;
					regulator-change-voltage;
					regulator-boot-on;
					regulator-always-on;
				};

				......
			};

			leds {
				......
			};

			buttons {
				......
			};
		};

		flash: at45db321d@1 {
			......
		};
	};
};

Comments

Rajendra Nayak Oct. 21, 2011, 8:41 a.m. | #1
>>
>> Do you have any RFC patch/code which could explain better what you are
>> suggesting we do here?
>>>
> Here is what I changed based on your patches.  It only changes
> drivers/regulator/core.c.
>
> ---8<-------
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9a5ebbe..8fe132d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>                  node = of_get_regulator(dev, id);
>                  if (node)
>                          list_for_each_entry(rdev,&regulator_list, list)
> -                               if (node == rdev->dev.parent->of_node)
> +                               if (node == rdev->dev.of_node)
>                                          goto found;
>          }
>          list_for_each_entry(map,&regulator_map_list, list) {
> @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>              regulator_desc->type != REGULATOR_CURRENT)
>                  return ERR_PTR(-EINVAL);
>
> -       if (!init_data)
> -               return ERR_PTR(-EINVAL);
> -
>          /* Only one of each should be implemented */
>          WARN_ON(regulator_desc->ops->get_voltage&&
>                  regulator_desc->ops->get_voltage_sel);
> @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>          INIT_LIST_HEAD(&rdev->list);
>          BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
>
> -       /* preform any regulator specific init */
> -       if (init_data->regulator_init) {
> -               ret = init_data->regulator_init(rdev->reg_data);
> -               if (ret<  0)
> -                       goto clean;
> -       }
> +       /* find device_node and attach it */
> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

so would this do a complete dt search for every regulator?
we would also need the driver names and dt names to match for this to
work, right?

The approach otherwise looks fine to me and should work for both cases
of one device per regulator and one device for all regulators.

>
>          /* register with sysfs */
>          rdev->dev.class =&regulator_class;
> @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                  goto clean;
>          }
>
> +       if (!init_data) {
> +               /* try to get init_data from device tree */
> +               init_data = of_get_regulator_init_data(&rdev->dev);
> +               if (!init_data)
> +                       return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* preform any regulator specific init */
> +       if (init_data->regulator_init) {
> +               ret = init_data->regulator_init(rdev->reg_data);
> +               if (ret<  0)
> +                       goto clean;
> +       }
> +
>          dev_set_drvdata(&rdev->dev, rdev);
>
>          /* set regulator constraints */
> @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                          node = of_get_regulator(dev, supply);
>                          if (node)
>                                  list_for_each_entry(r,&regulator_list, list)
> -                                       if (node == r->dev.parent->of_node)
> +                                       if (node == r->dev.of_node)
>                                                  goto found;
>                  }
>
> ------->8---
>
> And my dts file looks like something below.
>
> 	ecspi@70010000 { /* ECSPI1 */
> 		fsl,spi-num-chipselects =<2>;
> 		cs-gpios =<&gpio3 24 0>, /* GPIO4_24 */
> 			<&gpio3 25 0>; /* GPIO4_25 */
> 		status = "okay";
>
> 		pmic: mc13892@0 {
> 			#address-cells =<1>;
> 			#size-cells =<0>;
> 			compatible = "fsl,mc13892";
> 			spi-max-frequency =<6000000>;
> 			reg =<0>;
> 			mc13xxx-irq-gpios =<&gpio0 8 0>; /* GPIO1_8 */
>
> 			regulators {
> 				sw1reg: mc13892_sw1 {
> 					regulator-min-uV =<600000>;
> 					regulator-max-uV =<1375000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
>
> 				sw2reg: mc13892_sw2 {
> 					regulator-min-uV =<900000>;
> 					regulator-max-uV =<1850000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
>
> 				......
> 			};
>
> 			leds {
> 				......
> 			};
>
> 			buttons {
> 				......
> 			};
> 		};
>
> 		flash: at45db321d@1 {
> 			......
> 		};
> 	};
> };
>
Shawn Guo Oct. 21, 2011, 11:58 a.m. | #2
On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
[...]
> >+       /* find device_node and attach it */
> >+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
> so would this do a complete dt search for every regulator?

Yes, with the first param being NULL, tthe entire device tree will be
searched.

> we would also need the driver names and dt names to match for this to
> work, right?
> 
Driver name does not matter.  The key for this search to work is having
regulator's name (regulator_desc->name) match device tree node's name,
case ignored.

> The approach otherwise looks fine to me and should work for both cases
> of one device per regulator and one device for all regulators.
>
Rajendra Nayak Oct. 24, 2011, 6:02 a.m. | #3
On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> [...]
>>> +       /* find device_node and attach it */
>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>
>> so would this do a complete dt search for every regulator?
>
> Yes, with the first param being NULL, tthe entire device tree will be
> searched.
>
>> we would also need the driver names and dt names to match for this to
>> work, right?
>>
> Driver name does not matter.  The key for this search to work is having
> regulator's name (regulator_desc->name) match device tree node's name,
> case ignored.

Mark, whats your take on this? I am somehow not quite sure if we should
have this limitation put in to match DT node names with whats in the
driver structs (regulator_desc).

>
>> The approach otherwise looks fine to me and should work for both cases
>> of one device per regulator and one device for all regulators.
>>
>
Mark Brown Oct. 24, 2011, 7:34 a.m. | #4
On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:

> >Driver name does not matter.  The key for this search to work is having
> >regulator's name (regulator_desc->name) match device tree node's name,
> >case ignored.

> Mark, whats your take on this? I am somehow not quite sure if we should
> have this limitation put in to match DT node names with whats in the
> driver structs (regulator_desc).

If we're matching the string in the regulator_desc I'd expect we're
going to run into trouble with systems that have two of the same type of
regulator in the system.  Is that the case?  Since my primary
development system is such a system I care deeply about them :)
Grant Likely Oct. 24, 2011, 8:17 a.m. | #5
On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> >On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> >[...]
> >>>+       /* find device_node and attach it */
> >>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> >>
> >>so would this do a complete dt search for every regulator?
> >
> >Yes, with the first param being NULL, tthe entire device tree will be
> >searched.
> >
> >>we would also need the driver names and dt names to match for this to
> >>work, right?
> >>
> >Driver name does not matter.  The key for this search to work is having
> >regulator's name (regulator_desc->name) match device tree node's name,
> >case ignored.
> 
> Mark, whats your take on this? I am somehow not quite sure if we should
> have this limitation put in to match DT node names with whats in the
> driver structs (regulator_desc).

This looks wrong to me.  Matching based on node /name/, particularly
when searching the entire tree, will cause problems.

g.
Rajendra Nayak Oct. 24, 2011, 8:53 a.m. | #6
On Monday 24 October 2011 01:47 PM, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
>> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
>>> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
>>> [...]
>>>>> +       /* find device_node and attach it */
>>>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

Shawn/Mark,

How about the driver passing the of_node to be associated with the
regulator, as part of regulator_desc, to the regulator core,
instead of this complete DT search and the restriction of having
to match DT node names with names in regulator_desc.

regards,
Rajendra

>>>>
>>>> so would this do a complete dt search for every regulator?
>>>
>>> Yes, with the first param being NULL, tthe entire device tree will be
>>> searched.
>>>
>>>> we would also need the driver names and dt names to match for this to
>>>> work, right?
>>>>
>>> Driver name does not matter.  The key for this search to work is having
>>> regulator's name (regulator_desc->name) match device tree node's name,
>>> case ignored.
>>
>> Mark, whats your take on this? I am somehow not quite sure if we should
>> have this limitation put in to match DT node names with whats in the
>> driver structs (regulator_desc).
>
> This looks wrong to me.  Matching based on node /name/, particularly
> when searching the entire tree, will cause problems.
>
> g.
Rajendra Nayak Oct. 24, 2011, 8:56 a.m. | #7
On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 10:17:06AM +0200, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
>>> On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
>>>> On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
>>>> [...]
>>>>>> +       /* find device_node and attach it */
>>>>>> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>>>>
>>>>> so would this do a complete dt search for every regulator?
>>>>
>>>> Yes, with the first param being NULL, tthe entire device tree will be
>>>> searched.
>>>>
>>>>> we would also need the driver names and dt names to match for this to
>>>>> work, right?
>>>>>
>>>> Driver name does not matter.  The key for this search to work is having
>>>> regulator's name (regulator_desc->name) match device tree node's name,
>>>> case ignored.
>>>
>>> Mark, whats your take on this? I am somehow not quite sure if we should
>>> have this limitation put in to match DT node names with whats in the
>>> driver structs (regulator_desc).
>>
>> This looks wrong to me.  Matching based on node /name/, particularly
>> when searching the entire tree, will cause problems.
>>
> Okay, it's wrong then since so many people say it's wrong :)  I guess
> a quick fix would be adding one property in device tree node for
> matching some unique field in regulator_desc, id, maybe?  Mark, any
> suggestion?

Thats basically what the DT compatible property is for :)
>
Shawn Guo Oct. 24, 2011, 9:02 a.m. | #8
On Mon, Oct 24, 2011 at 10:17:06AM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> > On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> > >On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> > >[...]
> > >>>+       /* find device_node and attach it */
> > >>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > >>
> > >>so would this do a complete dt search for every regulator?
> > >
> > >Yes, with the first param being NULL, tthe entire device tree will be
> > >searched.
> > >
> > >>we would also need the driver names and dt names to match for this to
> > >>work, right?
> > >>
> > >Driver name does not matter.  The key for this search to work is having
> > >regulator's name (regulator_desc->name) match device tree node's name,
> > >case ignored.
> > 
> > Mark, whats your take on this? I am somehow not quite sure if we should
> > have this limitation put in to match DT node names with whats in the
> > driver structs (regulator_desc).
> 
> This looks wrong to me.  Matching based on node /name/, particularly
> when searching the entire tree, will cause problems.
> 
Okay, it's wrong then since so many people say it's wrong :)  I guess
a quick fix would be adding one property in device tree node for
matching some unique field in regulator_desc, id, maybe?  Mark, any
suggestion?
Shawn Guo Oct. 24, 2011, 9:11 a.m. | #9
On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> >Okay, it's wrong then since so many people say it's wrong :)  I guess
> >a quick fix would be adding one property in device tree node for
> >matching some unique field in regulator_desc, id, maybe?  Mark, any
> >suggestion?
> 
> Thats basically what the DT compatible property is for :)

But adding compatible property will get DT core create 'struct dev'
for each regulator node, while we are trying to avoid that since
regulator core has been doing this.
Rajendra Nayak Oct. 24, 2011, 9:13 a.m. | #10
On Monday 24 October 2011 02:41 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
>>> Okay, it's wrong then since so many people say it's wrong :)  I guess
>>> a quick fix would be adding one property in device tree node for
>>> matching some unique field in regulator_desc, id, maybe?  Mark, any
>>> suggestion?
>>
>> Thats basically what the DT compatible property is for :)
>
> But adding compatible property will get DT core create 'struct dev'
> for each regulator node, while we are trying to avoid that since
> regulator core has been doing this.
>
No it won't.

So here's what I am suggesting.

Case 1:
One device per regulator:

DT nodes look something like this

reg1: reg@1 {
		...
		...
};

reg2: reg@2 {
		...
		...
};

The regulator driver probes 2 devices and each time the
dev passed has a of_node associated which the regulator
driver can then pass to the core.

Case 2:
One device for all regulators:

DT nodes look something like this

regulators {
	reg1: reg@1 {
			...
			...
	};

	reg2: reg@2 {
			...
			...
	};
};

The regulator driver probes only one device and the dev->of_node
points to the "regulators" node above.
The regulator driver then based on compatible property extracts
and registers all the child nodes of "regulators" (for ex: reg1, reg2
above) with each call to regulator_register and passes the child nodes
as of_node to be associated with the regulator device.
Mark Brown Oct. 24, 2011, 9:19 a.m. | #11
On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:

> How about the driver passing the of_node to be associated with the
> regulator, as part of regulator_desc, to the regulator core,
> instead of this complete DT search and the restriction of having
> to match DT node names with names in regulator_desc.

regulator_desc should be const so that we handle multiple instances of
the same device nicely.  We could pass the information in as an argument
I guess.
Shawn Guo Oct. 24, 2011, 9:23 a.m. | #12
On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 01:47 PM, Grant Likely wrote:
> >On Mon, Oct 24, 2011 at 11:32:19AM +0530, Rajendra Nayak wrote:
> >>On Friday 21 October 2011 05:28 PM, Shawn Guo wrote:
> >>>On Fri, Oct 21, 2011 at 02:11:55PM +0530, Rajendra Nayak wrote:
> >>>[...]
> >>>>>+       /* find device_node and attach it */
> >>>>>+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
> Shawn/Mark,
> 
> How about the driver passing the of_node to be associated with the
> regulator, as part of regulator_desc, to the regulator core,
> instead of this complete DT search and the restriction of having
> to match DT node names with names in regulator_desc.
> 
If you agree that we should try to get DT core from creating
'struct dev' for each regulator, the of_node is not being attached
to any 'struct dev' in regulator driver either, so there is no
difference between finding the of_node in regulator driver and in
regulator core.
Grant Likely Oct. 24, 2011, 9:24 a.m. | #13
On Fri, Oct 21, 2011 at 04:23:12PM +0800, Shawn Guo wrote:
> On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> > On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> > >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> > >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> > >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> > >>>mc13892-regulator driver.  And when getting probed, this driver will
> > >>>call regulator_register() to register those 23 regulators individually.
> > >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> > >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> > >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> > >>>These extra 23 'dev' is totally new with DT.
> > >>>
> > >>
> > >>but thats only because the mc13892-regulator driver is implemeted in
> > >>such a way that all the regulators on the platform are bundled in as
> > >>*one* device.
> > >
> > >I did not look into too many regulator drivers, but I expect this is
> > >way that most regulator drivers are implemented in.  Having
> > >mc13892-regulator being probed 23 times to register these 23 regulators
> > >just makes less sense to me.
> > >
> > >>It would again depend on how you would pass these from
> > >>the DT, if you indeed stick to the same way of bundling all regulators
> > >>as one device from DT, the mc13892-regulator probe would just get called
> > >>once and there would be one device associated, no?
> > >>
> > >Yes, I indeed would stick to the same way of bundling the registration
> > >of all regulators with mc13892-regulator being probed once.  The problem
> > >I have with the current regulator core DT implementation is that it
> > >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> > >being attached to rdev->dev.parent rather than itself.  Back to
> > >mc13892-regulator example, that said, it requires the dev of
> > >mc13892-regulator have the device_node of individual regulator attached
> > >to.  IOW, the current implementation forces mc13892-regulator to be
> > >probed 23 times to register those 23 regulators.  This is wrong to me.
> > 
> > I think I now understand to some extent the problem that you seem to be
> > reporting. It is mainly with drivers which bundle all regulators and
> > pass them as one device and would want to do so with DT too.
> > 
> > however I am still not clear on how what you seem to suggest would
> > solve this problem. Note that not all drivers do it this way, and
> > there are drivers where each regulator is considered as one device
> > and I suspect they would remain that way with DT too. And hence we
> > need to support both.
> > 
> > Do you have any RFC patch/code which could explain better what you are
> > suggesting we do here?
> > >
> Here is what I changed based on your patches.  It only changes
> drivers/regulator/core.c.
> 
> ---8<-------
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9a5ebbe..8fe132d 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
>                 node = of_get_regulator(dev, id);
>                 if (node)
>                         list_for_each_entry(rdev, &regulator_list, list)
> -                               if (node == rdev->dev.parent->of_node)
> +                               if (node == rdev->dev.of_node)
>                                         goto found;
>         }
>         list_for_each_entry(map, &regulator_map_list, list) {
> @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>             regulator_desc->type != REGULATOR_CURRENT)
>                 return ERR_PTR(-EINVAL);
> 
> -       if (!init_data)
> -               return ERR_PTR(-EINVAL);
> -
>         /* Only one of each should be implemented */
>         WARN_ON(regulator_desc->ops->get_voltage &&
>                 regulator_desc->ops->get_voltage_sel);
> @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         INIT_LIST_HEAD(&rdev->list);
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
> -       /* preform any regulator specific init */
> -       if (init_data->regulator_init) {
> -               ret = init_data->regulator_init(rdev->reg_data);
> -               if (ret < 0)
> -                       goto clean;
> -       }
> +       /* find device_node and attach it */
> +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> 
>         /* register with sysfs */
>         rdev->dev.class = &regulator_class;
> @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                 goto clean;
>         }
> 
> +       if (!init_data) {
> +               /* try to get init_data from device tree */
> +               init_data = of_get_regulator_init_data(&rdev->dev);
> +               if (!init_data)
> +                       return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* preform any regulator specific init */
> +       if (init_data->regulator_init) {
> +               ret = init_data->regulator_init(rdev->reg_data);
> +               if (ret < 0)
> +                       goto clean;
> +       }
> +
>         dev_set_drvdata(&rdev->dev, rdev);
> 
>         /* set regulator constraints */
> @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>                         node = of_get_regulator(dev, supply);
>                         if (node)
>                                 list_for_each_entry(r, &regulator_list, list)
> -                                       if (node == r->dev.parent->of_node)
> +                                       if (node == r->dev.of_node)
>                                                 goto found;
>                 }
> 
> ------->8---
> 
> And my dts file looks like something below.
> 
> 	ecspi@70010000 { /* ECSPI1 */
> 		fsl,spi-num-chipselects = <2>;
> 		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
> 			   <&gpio3 25 0>; /* GPIO4_25 */
> 		status = "okay";
> 
> 		pmic: mc13892@0 {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			compatible = "fsl,mc13892";
> 			spi-max-frequency = <6000000>;
> 			reg = <0>;
> 			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> 
> 			regulators {
> 				sw1reg: mc13892_sw1 {
> 					regulator-min-uV = <600000>;
> 					regulator-max-uV = <1375000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
> 
> 				sw2reg: mc13892_sw2 {
> 					regulator-min-uV = <900000>;
> 					regulator-max-uV = <1850000>;
> 					regulator-change-voltage;
> 					regulator-boot-on;
> 					regulator-always-on;
> 				};
> 
> 				......
> 			};

To follow up from my earlier comment, this .dts structure looks fine
and reasonable to me, and it would also be fine for the mc13892 driver
to use for_each_child_of_node() to find all the children of the
regulators node.  Even finding the 'regulators' node by name from the
mc13892 driver is perfectly fine provided for_each_child_of_node is
used to find it.  All of this is okay because it is under the umbrella
of the "fsl,mc13892" binding.

What isn't okay is doing a whole tree search for nodes by name because
there is a high likelyhood of getting a conflict (and yes, I realized
that this example has the device name encoded into the node name, but
that doesn't change the fact that deep searches by name are bad
practice).

g.
Mark Brown Oct. 24, 2011, 9:39 a.m. | #14
On Mon, Oct 24, 2011 at 11:24:11AM +0200, Grant Likely wrote:

> To follow up from my earlier comment, this .dts structure looks fine
> and reasonable to me, and it would also be fine for the mc13892 driver
> to use for_each_child_of_node() to find all the children of the
> regulators node.  Even finding the 'regulators' node by name from the
> mc13892 driver is perfectly fine provided for_each_child_of_node is
> used to find it.  All of this is okay because it is under the umbrella
> of the "fsl,mc13892" binding.

Yes, a search of children of the device node that the driver is probing
off seems like a sensible approach.
Rajendra Nayak Oct. 24, 2011, 10:05 a.m. | #15
On Monday 24 October 2011 02:49 PM, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 02:23:50PM +0530, Rajendra Nayak wrote:
>
>> How about the driver passing the of_node to be associated with the
>> regulator, as part of regulator_desc, to the regulator core,
>> instead of this complete DT search and the restriction of having
>> to match DT node names with names in regulator_desc.
>
> regulator_desc should be const so that we handle multiple instances of
> the same device nicely.  We could pass the information in as an argument
> I guess.

Ok, will do. The only downside being that all existing users would
need to be changed.
Grant Likely Oct. 24, 2011, 11:35 a.m. | #16
On Mon, Oct 24, 2011 at 05:11:58PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:26:49PM +0530, Rajendra Nayak wrote:
> > On Monday 24 October 2011 02:32 PM, Shawn Guo wrote:
> > >Okay, it's wrong then since so many people say it's wrong :)  I guess
> > >a quick fix would be adding one property in device tree node for
> > >matching some unique field in regulator_desc, id, maybe?  Mark, any
> > >suggestion?
> > 
> > Thats basically what the DT compatible property is for :)
> 
> But adding compatible property will get DT core create 'struct dev'
> for each regulator node, while we are trying to avoid that since
> regulator core has been doing this.

By design, of_platform_populate() will only go one level deep except
if it matches the passed in list of bus compatible properties.  Having
a compatible property does not necessarily create a struct dev.

That said, I agree with you that a compatible property isn't really
needed in the child nodes **if it is defined as part of the parent
node's binding**.

g.
Shawn Guo Oct. 24, 2011, 1:04 p.m. | #17
On Mon, Oct 24, 2011 at 11:24:11AM +0200, Grant Likely wrote:
> On Fri, Oct 21, 2011 at 04:23:12PM +0800, Shawn Guo wrote:
> > On Thu, Oct 20, 2011 at 05:39:32PM +0530, Rajendra Nayak wrote:
> > > On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote:
> > > >On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote:
> > > >>>Let's look at mc13892-regulator driver.  There are 23 regulators defined
> > > >>>in array mc13892_regulators.  Needless to say, there is a dev behind
> > > >>>mc13892-regulator driver.  And when getting probed, this driver will
> > > >>>call regulator_register() to register those 23 regulators individually.
> > > >>>That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the
> > > >>>parent of all other 23 'dev' (wrapped by regulator_dev).  But with the
> > > >>>current DT implementation, we will have at least 1 + 23 * 2 'dev'.
> > > >>>These extra 23 'dev' is totally new with DT.
> > > >>>
> > > >>
> > > >>but thats only because the mc13892-regulator driver is implemeted in
> > > >>such a way that all the regulators on the platform are bundled in as
> > > >>*one* device.
> > > >
> > > >I did not look into too many regulator drivers, but I expect this is
> > > >way that most regulator drivers are implemented in.  Having
> > > >mc13892-regulator being probed 23 times to register these 23 regulators
> > > >just makes less sense to me.
> > > >
> > > >>It would again depend on how you would pass these from
> > > >>the DT, if you indeed stick to the same way of bundling all regulators
> > > >>as one device from DT, the mc13892-regulator probe would just get called
> > > >>once and there would be one device associated, no?
> > > >>
> > > >Yes, I indeed would stick to the same way of bundling the registration
> > > >of all regulators with mc13892-regulator being probed once.  The problem
> > > >I have with the current regulator core DT implementation is that it
> > > >assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is
> > > >being attached to rdev->dev.parent rather than itself.  Back to
> > > >mc13892-regulator example, that said, it requires the dev of
> > > >mc13892-regulator have the device_node of individual regulator attached
> > > >to.  IOW, the current implementation forces mc13892-regulator to be
> > > >probed 23 times to register those 23 regulators.  This is wrong to me.
> > > 
> > > I think I now understand to some extent the problem that you seem to be
> > > reporting. It is mainly with drivers which bundle all regulators and
> > > pass them as one device and would want to do so with DT too.
> > > 
> > > however I am still not clear on how what you seem to suggest would
> > > solve this problem. Note that not all drivers do it this way, and
> > > there are drivers where each regulator is considered as one device
> > > and I suspect they would remain that way with DT too. And hence we
> > > need to support both.
> > > 
> > > Do you have any RFC patch/code which could explain better what you are
> > > suggesting we do here?
> > > >
> > Here is what I changed based on your patches.  It only changes
> > drivers/regulator/core.c.
> > 
> > ---8<-------
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 9a5ebbe..8fe132d 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -1211,7 +1211,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
> >                 node = of_get_regulator(dev, id);
> >                 if (node)
> >                         list_for_each_entry(rdev, &regulator_list, list)
> > -                               if (node == rdev->dev.parent->of_node)
> > +                               if (node == rdev->dev.of_node)
> >                                         goto found;
> >         }
> >         list_for_each_entry(map, &regulator_map_list, list) {
> > @@ -2642,9 +2642,6 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >             regulator_desc->type != REGULATOR_CURRENT)
> >                 return ERR_PTR(-EINVAL);
> > 
> > -       if (!init_data)
> > -               return ERR_PTR(-EINVAL);
> > -
> >         /* Only one of each should be implemented */
> >         WARN_ON(regulator_desc->ops->get_voltage &&
> >                 regulator_desc->ops->get_voltage_sel);
> > @@ -2675,12 +2672,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         INIT_LIST_HEAD(&rdev->list);
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> > -       /* preform any regulator specific init */
> > -       if (init_data->regulator_init) {
> > -               ret = init_data->regulator_init(rdev->reg_data);
> > -               if (ret < 0)
> > -                       goto clean;
> > -       }
> > +       /* find device_node and attach it */
> > +       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > 
> >         /* register with sysfs */
> >         rdev->dev.class = &regulator_class;
> > @@ -2693,6 +2686,20 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >                 goto clean;
> >         }
> > 
> > +       if (!init_data) {
> > +               /* try to get init_data from device tree */
> > +               init_data = of_get_regulator_init_data(&rdev->dev);
> > +               if (!init_data)
> > +                       return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       /* preform any regulator specific init */
> > +       if (init_data->regulator_init) {
> > +               ret = init_data->regulator_init(rdev->reg_data);
> > +               if (ret < 0)
> > +                       goto clean;
> > +       }
> > +
> >         dev_set_drvdata(&rdev->dev, rdev);
> > 
> >         /* set regulator constraints */
> > @@ -2719,7 +2726,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >                         node = of_get_regulator(dev, supply);
> >                         if (node)
> >                                 list_for_each_entry(r, &regulator_list, list)
> > -                                       if (node == r->dev.parent->of_node)
> > +                                       if (node == r->dev.of_node)
> >                                                 goto found;
> >                 }
> > 
> > ------->8---
> > 
> > And my dts file looks like something below.
> > 
> > 	ecspi@70010000 { /* ECSPI1 */
> > 		fsl,spi-num-chipselects = <2>;
> > 		cs-gpios = <&gpio3 24 0>, /* GPIO4_24 */
> > 			   <&gpio3 25 0>; /* GPIO4_25 */
> > 		status = "okay";
> > 
> > 		pmic: mc13892@0 {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			compatible = "fsl,mc13892";
> > 			spi-max-frequency = <6000000>;
> > 			reg = <0>;
> > 			mc13xxx-irq-gpios = <&gpio0 8 0>; /* GPIO1_8 */
> > 
> > 			regulators {
> > 				sw1reg: mc13892_sw1 {
> > 					regulator-min-uV = <600000>;
> > 					regulator-max-uV = <1375000>;
> > 					regulator-change-voltage;
> > 					regulator-boot-on;
> > 					regulator-always-on;
> > 				};
> > 
> > 				sw2reg: mc13892_sw2 {
> > 					regulator-min-uV = <900000>;
> > 					regulator-max-uV = <1850000>;
> > 					regulator-change-voltage;
> > 					regulator-boot-on;
> > 					regulator-always-on;
> > 				};
> > 
> > 				......
> > 			};
> 
> To follow up from my earlier comment, this .dts structure looks fine
> and reasonable to me, and it would also be fine for the mc13892 driver
> to use for_each_child_of_node() to find all the children of the
> regulators node.  Even finding the 'regulators' node by name from the
> mc13892 driver is perfectly fine provided for_each_child_of_node is
> used to find it.  All of this is okay because it is under the umbrella
> of the "fsl,mc13892" binding.
> 
For mc13892 regulator example, there are 3 levels 'struct dev'.

1. drivers/mfd/mc13xxx-core.c
   The "fsl,mc13892" binding is used in this mfd driver to get
   mc13xxx-core device probed.  And this mfd driver will in turn
   call mfd_add_devices() to add the second level device below.

2. drivers/regulator/mc13892-regulator.c
   As this device is created by mfd_add_devices() above, there is no
   device_node attached to its of_node, though we would hope that node
   'regulators' is attached there.  And the driver will call
   regulator_register() to have each regulator device created and
   registered in regulator core below.

3. drivers/regulator/core.c
   regulator_register() called above will create device (rdev->dev)
   for each regulator.

I am thinking about hiding the device_node discovering for each
regulator from the second level (regulator driver) and make it
internal to the third level (regulator core), as it seems to me
that regulator driver does not need to necessarily know about this.

If we can attach the device_node of 'regulators' node to dev->of_node
when calling regulator_register(regulator_desc, dev, ...) from
regulator driver, the regulator core will be able to find all nodes under
'regulators' using for_each_child_of_node(dev->of_node, child).

Then the question would be how we attach the device_node of 'regulators'
to dev->of_node where 'dev' is the second level device above.  I somehow
hesitate to hack this into mfd_add_devices(), so I would like to add
compatible string "fsl,mc13892-regulators" to node 'regulators' and
find the node using of_find_compatible_node(dev->parent, NULL,
"fsl,mc13892-regulators").

I'm not sure this is exactly same as what your comment suggests above.
So please let me know if it's appropriate.
Mark Brown Oct. 24, 2011, 1:06 p.m. | #18
On Mon, Oct 24, 2011 at 09:04:31PM +0800, Shawn Guo wrote:

> If we can attach the device_node of 'regulators' node to dev->of_node
> when calling regulator_register(regulator_desc, dev, ...) from
> regulator driver, the regulator core will be able to find all nodes under
> 'regulators' using for_each_child_of_node(dev->of_node, child).

Please provide concrete examples of the bindings you're talking about,
the really important thing here is how sane the bindings look and I've
really got no idea what any of what you're talking about will look like
or if they make sense.

> hesitate to hack this into mfd_add_devices(), so I would like to add
> compatible string "fsl,mc13892-regulators" to node 'regulators' and
> find the node using of_find_compatible_node(dev->parent, NULL,
> "fsl,mc13892-regulators").

It's not immediately obvious to me that having a binding for the
regulators separately makes sense, it's not a usefully distinct device.
Shawn Guo Oct. 24, 2011, 1:47 p.m. | #19
On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
> Case 2:
> One device for all regulators:
> 
> DT nodes look something like this
> 
> regulators {
> 	reg1: reg@1 {
> 			...
> 			...
> 	};
> 
> 	reg2: reg@2 {
> 			...
> 			...
> 	};
> };
> 
> The regulator driver probes only one device and the dev->of_node
> points to the "regulators" node above.

The mc13892 example I put in the reply to Grant demonstrates that
for some case, dev->of_node is NULL (devices are created by mfd core).

> The regulator driver then based on compatible property extracts
> and registers all the child nodes of "regulators" (for ex: reg1, reg2
> above) with each call to regulator_register and passes the child nodes
> as of_node to be associated with the regulator device.
> 
I still think the discovery of all the child nodes of "regulators" does
not necessarily need to be done in regulator driver.  Instead, it can
be done in regulator core.
Rajendra Nayak Oct. 25, 2011, 6 a.m. | #20
On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
>> Case 2:
>> One device for all regulators:
>>
>> DT nodes look something like this
>>
>> regulators {
>> 	reg1: reg@1 {
>> 			...
>> 			...
>> 	};
>>
>> 	reg2: reg@2 {
>> 			...
>> 			...
>> 	};
>> };
>>
>> The regulator driver probes only one device and the dev->of_node
>> points to the "regulators" node above.
>
> The mc13892 example I put in the reply to Grant demonstrates that
> for some case, dev->of_node is NULL (devices are created by mfd core).

In that case should you not be first converting the mfd driver to
register regulator devices using DT?
Thats what we did for OMAP, and hence we always have the of_node
populated when the regulator devices are probed.
See this patch from Benoit on how thats done for twl devices..
http://marc.info/?l=linux-omap&m=131489864814428&w=2

>
>> The regulator driver then based on compatible property extracts
>> and registers all the child nodes of "regulators" (for ex: reg1, reg2
>> above) with each call to regulator_register and passes the child nodes
>> as of_node to be associated with the regulator device.
>>
> I still think the discovery of all the child nodes of "regulators" does
> not necessarily need to be done in regulator driver.  Instead, it can
> be done in regulator core.
>
Rajendra Nayak Oct. 25, 2011, 6:26 a.m. | #21
On Tuesday 25 October 2011 11:30 AM, Rajendra Nayak wrote:
>
> In that case should you not be first converting the mfd driver to
> register regulator devices using DT?
> Thats what we did for OMAP, and hence we always have the of_node
> populated when the regulator devices are probed.
> See this patch from Benoit on how thats done for twl devices..
> http://marc.info/?l=linux-omap&m=131489864814428&w=2

Sorry, that was a link to an older version of the patch.
Here's the latest one from Benoit which he did based
on Arnd's suggestion of using of_platform_populate() directly.

http://marc.info/?l=linux-omap&m=131705623822369&w=2
Shawn Guo Oct. 25, 2011, 6:52 a.m. | #22
On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
> >On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
> >>Case 2:
> >>One device for all regulators:
> >>
> >>DT nodes look something like this
> >>
> >>regulators {
> >>	reg1: reg@1 {
> >>			...
> >>			...
> >>	};
> >>
> >>	reg2: reg@2 {
> >>			...
> >>			...
> >>	};
> >>};
> >>
> >>The regulator driver probes only one device and the dev->of_node
> >>points to the "regulators" node above.
> >
> >The mc13892 example I put in the reply to Grant demonstrates that
> >for some case, dev->of_node is NULL (devices are created by mfd core).
> 
> In that case should you not be first converting the mfd driver to
> register regulator devices using DT?

The mc13892 mfd driver calls mfd_add_devices() to add device for
mc13892 regulator driver.  Are you suggesting that I should hack
mfd_add_devices() to have device_node of 'regulators' attached?
The mfd is not a bus like i2c and spi, so I'm not sure this is the
right thing to do.

> Thats what we did for OMAP, and hence we always have the of_node
> populated when the regulator devices are probed.
> See this patch from Benoit on how thats done for twl devices..
> http://marc.info/?l=linux-omap&m=131489864814428&w=2
> 
OMAP is "Case 1", and we are talking about "Case 2".
Rajendra Nayak Oct. 25, 2011, 6:56 a.m. | #23
On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 07:17 PM, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 02:43:58PM +0530, Rajendra Nayak wrote:
>>>> Case 2:
>>>> One device for all regulators:
>>>>
>>>> DT nodes look something like this
>>>>
>>>> regulators {
>>>> 	reg1: reg@1 {
>>>> 			...
>>>> 			...
>>>> 	};
>>>>
>>>> 	reg2: reg@2 {
>>>> 			...
>>>> 			...
>>>> 	};
>>>> };
>>>>
>>>> The regulator driver probes only one device and the dev->of_node
>>>> points to the "regulators" node above.
>>>
>>> The mc13892 example I put in the reply to Grant demonstrates that
>>> for some case, dev->of_node is NULL (devices are created by mfd core).
>>
>> In that case should you not be first converting the mfd driver to
>> register regulator devices using DT?
>
> The mc13892 mfd driver calls mfd_add_devices() to add device for
> mc13892 regulator driver.  Are you suggesting that I should hack
> mfd_add_devices() to have device_node of 'regulators' attached?
> The mfd is not a bus like i2c and spi, so I'm not sure this is the
> right thing to do.
>
>> Thats what we did for OMAP, and hence we always have the of_node
>> populated when the regulator devices are probed.
>> See this patch from Benoit on how thats done for twl devices..
>> http://marc.info/?l=linux-omap&m=131489864814428&w=2
>>
> OMAP is "Case 1", and we are talking about "Case 2".

I don't see why it wouldn't work for "Case 2". The only difference
is in case of "Case 1", the dev->of_node would already point to
the right regulator node, like 'reg1', 'reg2' above.
In case of "Case 2", the dev->of_node would point to the 'regulators'
node instead, and the driver could then do a for_each_child_of_node()
to iterate over all its children to get 'reg1', 'reg2' etc.

>
Rajendra Nayak Oct. 25, 2011, 7:13 a.m. | #24
On Tuesday 25 October 2011 12:50 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
>> On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
>>> On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> [...]
>>>> Thats what we did for OMAP, and hence we always have the of_node
>>>> populated when the regulator devices are probed.
>>>> See this patch from Benoit on how thats done for twl devices..
>>>> http://marc.info/?l=linux-omap&m=131489864814428&w=2
>>>>
>>> OMAP is "Case 1", and we are talking about "Case 2".
>>
>> I don't see why it wouldn't work for "Case 2".
>
> I did not say it wouldn't work for "Case 2".  I meant they work in
> different way.
>
>> The only difference
>> is in case of "Case 1", the dev->of_node would already point to
>> the right regulator node, like 'reg1', 'reg2' above.
>> In case of "Case 2", the dev->of_node would point to the 'regulators'
>> node instead, and the driver could then do a for_each_child_of_node()
>> to iterate over all its children to get 'reg1', 'reg2' etc.
>>
> Yes, that's the difference.  So you will need to distinguish these
> two cases in regulator_register()?  How?

You don't need to distinguish these cases in regulator_register().
The driver knows if its handling "Case 1" devices or "Case 2" devices
and it needs to handle things accordingly and pass the right of_node
which regulator_register() can then attach to regulator_dev->dev.

>
Shawn Guo Oct. 25, 2011, 7:20 a.m. | #25
On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
[...]
> >>Thats what we did for OMAP, and hence we always have the of_node
> >>populated when the regulator devices are probed.
> >>See this patch from Benoit on how thats done for twl devices..
> >>http://marc.info/?l=linux-omap&m=131489864814428&w=2
> >>
> >OMAP is "Case 1", and we are talking about "Case 2".
> 
> I don't see why it wouldn't work for "Case 2".

I did not say it wouldn't work for "Case 2".  I meant they work in
different way.

> The only difference
> is in case of "Case 1", the dev->of_node would already point to
> the right regulator node, like 'reg1', 'reg2' above.
> In case of "Case 2", the dev->of_node would point to the 'regulators'
> node instead, and the driver could then do a for_each_child_of_node()
> to iterate over all its children to get 'reg1', 'reg2' etc.
> 
Yes, that's the difference.  So you will need to distinguish these
two cases in regulator_register()?  How?
Shawn Guo Oct. 25, 2011, 7:42 a.m. | #26
On Tue, Oct 25, 2011 at 12:43:55PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:50 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 12:26:01PM +0530, Rajendra Nayak wrote:
> >>On Tuesday 25 October 2011 12:22 PM, Shawn Guo wrote:
> >>>On Tue, Oct 25, 2011 at 11:30:19AM +0530, Rajendra Nayak wrote:
> >[...]
> >>>>Thats what we did for OMAP, and hence we always have the of_node
> >>>>populated when the regulator devices are probed.
> >>>>See this patch from Benoit on how thats done for twl devices..
> >>>>http://marc.info/?l=linux-omap&m=131489864814428&w=2
> >>>>
> >>>OMAP is "Case 1", and we are talking about "Case 2".
> >>
> >>I don't see why it wouldn't work for "Case 2".
> >
> >I did not say it wouldn't work for "Case 2".  I meant they work in
> >different way.
> >
> >>The only difference
> >>is in case of "Case 1", the dev->of_node would already point to
> >>the right regulator node, like 'reg1', 'reg2' above.
> >>In case of "Case 2", the dev->of_node would point to the 'regulators'
> >>node instead, and the driver could then do a for_each_child_of_node()
> >>to iterate over all its children to get 'reg1', 'reg2' etc.
> >>
> >Yes, that's the difference.  So you will need to distinguish these
> >two cases in regulator_register()?  How?
> 
> You don't need to distinguish these cases in regulator_register().
> The driver knows if its handling "Case 1" devices or "Case 2" devices
> and it needs to handle things accordingly and pass the right of_node
> which regulator_register() can then attach to regulator_dev->dev.
> 
Ah, you are thinking about (for Case 2) calling for_each_child_of_node()
in regulator driver and passing the of_node found for each regulator to
regulator core as a new parameter of regulator_register().  Yes, that
would work, though my thought is to put the device_node discovery in
regulator core and hide it from regulator driver.

Okay, I'm good with this solution.  Grant, Mark, what about you?

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9a5ebbe..8fe132d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1211,7 +1211,7 @@  static struct regulator *_regulator_get(struct device *dev, const char *id,
                node = of_get_regulator(dev, id);
                if (node)
                        list_for_each_entry(rdev, &regulator_list, list)
-                               if (node == rdev->dev.parent->of_node)
+                               if (node == rdev->dev.of_node)
                                        goto found;
        }
        list_for_each_entry(map, &regulator_map_list, list) {
@@ -2642,9 +2642,6 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
            regulator_desc->type != REGULATOR_CURRENT)
                return ERR_PTR(-EINVAL);

-       if (!init_data)
-               return ERR_PTR(-EINVAL);
-
        /* Only one of each should be implemented */
        WARN_ON(regulator_desc->ops->get_voltage &&
                regulator_desc->ops->get_voltage_sel);
@@ -2675,12 +2672,8 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
        INIT_LIST_HEAD(&rdev->list);
        BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

-       /* preform any regulator specific init */
-       if (init_data->regulator_init) {
-               ret = init_data->regulator_init(rdev->reg_data);
-               if (ret < 0)
-                       goto clean;
-       }
+       /* find device_node and attach it */
+       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);

        /* register with sysfs */
        rdev->dev.class = &regulator_class;
@@ -2693,6 +2686,20 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
                goto clean;
        }

+       if (!init_data) {
+               /* try to get init_data from device tree */
+               init_data = of_get_regulator_init_data(&rdev->dev);
+               if (!init_data)
+                       return ERR_PTR(-EINVAL);
+       }
+
+       /* preform any regulator specific init */
+       if (init_data->regulator_init) {
+               ret = init_data->regulator_init(rdev->reg_data);
+               if (ret < 0)
+                       goto clean;
+       }
+
        dev_set_drvdata(&rdev->dev, rdev);

        /* set regulator constraints */
@@ -2719,7 +2726,7 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
                        node = of_get_regulator(dev, supply);
                        if (node)
                                list_for_each_entry(r, &regulator_list, list)
-                                       if (node == r->dev.parent->of_node)
+                                       if (node == r->dev.of_node)
                                                goto found;
                }