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

Message ID 20111024134025.GE1755@S2100-06.ap.freescale.net
State New
Headers show

Commit Message

Shawn Guo Oct. 24, 2011, 1:40 p.m.
On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> 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.
> 
The only thing different from what I attached last time is the
compatible string added to 'regulators' node.

        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 {
                        	compatible = "fsl,mc13892-regulator";

                                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 {
                        ......
                };
        };

> > 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.
> 
Fair point.  Actually, I also hate to have the finding of node
'regulators' plugged into regulator driver.  What about following
change to address Grant's concern on global device tree search?

Comments

Mark Brown Oct. 24, 2011, 1:49 p.m. | #1
On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:

> +++ b/drivers/regulator/core.c
> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
>         /* find device_node and attach it */
> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> +                                                regulator_desc->name);
> 

Is that going to do the right thing if you've got a MFD which does
register each regulator as a separate device?  Might be best to just
search within dev and get drivers to pass the "real" device in when
registering the regulator rather than the virtual device.
Grant Likely Oct. 24, 2011, 1:59 p.m. | #2
On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> > 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.
> > 
> The only thing different from what I attached last time is the
> compatible string added to 'regulators' node.
> 
>         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 {
>                         	compatible = "fsl,mc13892-regulator";
> 
>                                 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 {
>                         ......
>                 };
>         };
> 
> > > 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.
> > 
> Fair point.  Actually, I also hate to have the finding of node
> 'regulators' plugged into regulator driver.  What about following
> change to address Grant's concern on global device tree search?
>  
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 8fe132d..29dcf90 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> 
>         /* find device_node and attach it */
> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> +                                                regulator_desc->name);

of_find_node_by_name() doesn't work that way.  The first argument is a
starting point, but it doesn't restrict the search to children of a
node.

for_each_child_of_node() is what you want to use when iterating over
the children which unfortunately changes the structure of this
function.

g.
Shawn Guo Oct. 24, 2011, 2:47 p.m. | #3
On Mon, Oct 24, 2011 at 03:49:30PM +0200, Mark Brown wrote:
> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> 
> > +++ b/drivers/regulator/core.c
> > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> >         /* find device_node and attach it */
> > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > +                                                regulator_desc->name);
> > 
> 
> Is that going to do the right thing if you've got a MFD which does
> register each regulator as a separate device?

Based on my understanding, 1:1 is just a special case of N:1.  I
failed to see any problem having it work with registering each
regulator as a separate device.

> Might be best to just
> search within dev and get drivers to pass the "real" device in when
> registering the regulator rather than the virtual device.
> 
It should also work, but it will also change the API slightly for
non-dt users.  I'm not sure it's something we want.
Shawn Guo Oct. 24, 2011, 2:51 p.m. | #4
On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > On Mon, Oct 24, 2011 at 03:06:37PM +0200, Mark Brown wrote:
> > > 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.
> > > 
> > The only thing different from what I attached last time is the
> > compatible string added to 'regulators' node.
> > 
> >         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 {
> >                         	compatible = "fsl,mc13892-regulator";
> > 
> >                                 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 {
> >                         ......
> >                 };
> >         };
> > 
> > > > 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.
> > > 
> > Fair point.  Actually, I also hate to have the finding of node
> > 'regulators' plugged into regulator driver.  What about following
> > change to address Grant's concern on global device tree search?
> >  
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 8fe132d..29dcf90 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > 
> >         /* find device_node and attach it */
> > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > +                                                regulator_desc->name);
> 
> of_find_node_by_name() doesn't work that way.  The first argument is a
> starting point, but it doesn't restrict the search to children of a
> node.
> 
> for_each_child_of_node() is what you want to use when iterating over
> the children which unfortunately changes the structure of this
> function.
> 
The dev->parent->of_node is meant to point to node 'pmic: mc13892@0'.
And the intention here is not to iterate over the children, but to
start a search from a reasonable point rather than the top root node.
Grant Likely Oct. 24, 2011, 2:56 p.m. | #5
On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > index 8fe132d..29dcf90 100644
> > > --- a/drivers/regulator/core.c
> > > +++ b/drivers/regulator/core.c
> > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > 
> > >         /* find device_node and attach it */
> > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > +                                                regulator_desc->name);
> > 
> > of_find_node_by_name() doesn't work that way.  The first argument is a
> > starting point, but it doesn't restrict the search to children of a
> > node.
> > 
> > for_each_child_of_node() is what you want to use when iterating over
> > the children which unfortunately changes the structure of this
> > function.
> > 
> The dev->parent->of_node is meant to point to node 'pmic: mc13892@0'.
> And the intention here is not to iterate over the children, but to
> start a search from a reasonable point rather than the top root node.

It is always better to attach the of_node at struct device
registration time instead of searching the tree in common code.  The
of_node should already be assigned by the time regulator_register() is
called.  The caller should already have access to all that information
before the call anyway, especially since it is not strictly manditory
for all regulators to use the common binding.  It is entirely
conceivable that the proposed binding won't work for some regulators.

g.
Shawn Guo Oct. 24, 2011, 3:51 p.m. | #6
On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> > On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > > index 8fe132d..29dcf90 100644
> > > > --- a/drivers/regulator/core.c
> > > > +++ b/drivers/regulator/core.c
> > > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > > 
> > > >         /* find device_node and attach it */
> > > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > > +                                                regulator_desc->name);
> > > 
> > > of_find_node_by_name() doesn't work that way.  The first argument is a
> > > starting point, but it doesn't restrict the search to children of a
> > > node.
> > > 
> > > for_each_child_of_node() is what you want to use when iterating over
> > > the children which unfortunately changes the structure of this
> > > function.
> > > 
> > The dev->parent->of_node is meant to point to node 'pmic: mc13892@0'.
> > And the intention here is not to iterate over the children, but to
> > start a search from a reasonable point rather than the top root node.
> 
> It is always better to attach the of_node at struct device
> registration time instead of searching the tree in common code.  The
> of_node should already be assigned by the time regulator_register() is
> called.

That's the problem we have.  There is no 'struct dev' to attach of_node
for each regulator by the time regulator_register() is called, because
the 'struct dev' for each regulator is created inside
regulator_register() as wrapped by 'struct regulator_dev'.
Grant Likely Oct. 24, 2011, 10:21 p.m. | #7
On Mon, Oct 24, 2011 at 11:51:33PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> > On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
> > > On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
> > > > On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
> > > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > > > > index 8fe132d..29dcf90 100644
> > > > > --- a/drivers/regulator/core.c
> > > > > +++ b/drivers/regulator/core.c
> > > > > @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> > > > >         BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> > > > > 
> > > > >         /* find device_node and attach it */
> > > > > -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
> > > > > +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
> > > > > +                                                regulator_desc->name);
> > > > 
> > > > of_find_node_by_name() doesn't work that way.  The first argument is a
> > > > starting point, but it doesn't restrict the search to children of a
> > > > node.
> > > > 
> > > > for_each_child_of_node() is what you want to use when iterating over
> > > > the children which unfortunately changes the structure of this
> > > > function.
> > > > 
> > > The dev->parent->of_node is meant to point to node 'pmic: mc13892@0'.
> > > And the intention here is not to iterate over the children, but to
> > > start a search from a reasonable point rather than the top root node.
> > 
> > It is always better to attach the of_node at struct device
> > registration time instead of searching the tree in common code.  The
> > of_node should already be assigned by the time regulator_register() is
> > called.
> 
> That's the problem we have.  There is no 'struct dev' to attach of_node
> for each regulator by the time regulator_register() is called, because
> the 'struct dev' for each regulator is created inside
> regulator_register() as wrapped by 'struct regulator_dev'.

Then regulator_register must be split in half.  This is a common use
case, and other libraries have had to do the same.  One half to
allocate the structure, and then a second half for registration, and
regulator_register() becomes a simpler helper that calls both.  That
way a caller can insert extra initialization between the two steps.

g.
Rajendra Nayak Oct. 25, 2011, 6:10 a.m. | #8
On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 10:51:40PM +0800, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 03:59:50PM +0200, Grant Likely wrote:
>>>> On Mon, Oct 24, 2011 at 09:40:26PM +0800, Shawn Guo wrote:
>>>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>>>> index 8fe132d..29dcf90 100644
>>>>> --- a/drivers/regulator/core.c
>>>>> +++ b/drivers/regulator/core.c
>>>>> @@ -2673,7 +2673,8 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
>>>>>          BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
>>>>>
>>>>>          /* find device_node and attach it */
>>>>> -       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
>>>>> +       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
>>>>> +                                                regulator_desc->name);
>>>>
>>>> of_find_node_by_name() doesn't work that way.  The first argument is a
>>>> starting point, but it doesn't restrict the search to children of a
>>>> node.
>>>>
>>>> for_each_child_of_node() is what you want to use when iterating over
>>>> the children which unfortunately changes the structure of this
>>>> function.
>>>>
>>> The dev->parent->of_node is meant to point to node 'pmic: mc13892@0'.
>>> And the intention here is not to iterate over the children, but to
>>> start a search from a reasonable point rather than the top root node.
>>
>> It is always better to attach the of_node at struct device
>> registration time instead of searching the tree in common code.  The
>> of_node should already be assigned by the time regulator_register() is
>> called.
>
> That's the problem we have.  There is no 'struct dev' to attach of_node
> for each regulator by the time regulator_register() is called, because
> the 'struct dev' for each regulator is created inside
> regulator_register() as wrapped by 'struct regulator_dev'.

The root of your problem seems to be that your pmic driver isn't
registering regulator devices from DT, and if it did, you wouldn't
need to do a search in dev->parent->of_node and instead the driver
would have the right dev->of_node populated.

>
Rajendra Nayak Oct. 25, 2011, 7:01 a.m. | #9
On Tuesday 25 October 2011 12:38 PM, Shawn Guo wrote:
> On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
>> On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
>>> On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> [...]
>>>> It is always better to attach the of_node at struct device
>>>> registration time instead of searching the tree in common code.  The
>>>> of_node should already be assigned by the time regulator_register() is
>>>> called.
>>>
>>> That's the problem we have.  There is no 'struct dev' to attach of_node
>>> for each regulator by the time regulator_register() is called, because
>>> the 'struct dev' for each regulator is created inside
>>> regulator_register() as wrapped by 'struct regulator_dev'.
>>
>> The root of your problem seems to be that your pmic driver isn't
>> registering regulator devices from DT, and if it did, you wouldn't
>> need to do a search in dev->parent->of_node and instead the driver
>> would have the right dev->of_node populated.
>>
> No, it's not the root of my problem.  Again, we are talking about
> 'Case 2', where multiple regulator devices are registered to
> regulator core with regulator driver being probed once, where each
> regulator node is taken as the child of 'regulators' node.  Having
> device_node of 'regulators' attached to dev->of_node does not help
> at all.  What we need is to have each child node attached to
> regulator_dev->dev.of_node.

It certainly helps if dev->of_node has the 'regulators' node attached.
The driver can very easily then do a for_each_child_of_node() to extract
and register individual regulators passing an additional of_node param.

>
Shawn Guo Oct. 25, 2011, 7:08 a.m. | #10
On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
> On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> >On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
[...]
> >>It is always better to attach the of_node at struct device
> >>registration time instead of searching the tree in common code.  The
> >>of_node should already be assigned by the time regulator_register() is
> >>called.
> >
> >That's the problem we have.  There is no 'struct dev' to attach of_node
> >for each regulator by the time regulator_register() is called, because
> >the 'struct dev' for each regulator is created inside
> >regulator_register() as wrapped by 'struct regulator_dev'.
> 
> The root of your problem seems to be that your pmic driver isn't
> registering regulator devices from DT, and if it did, you wouldn't
> need to do a search in dev->parent->of_node and instead the driver
> would have the right dev->of_node populated.
> 
No, it's not the root of my problem.  Again, we are talking about
'Case 2', where multiple regulator devices are registered to
regulator core with regulator driver being probed once, where each
regulator node is taken as the child of 'regulators' node.  Having
device_node of 'regulators' attached to dev->of_node does not help
at all.  What we need is to have each child node attached to
regulator_dev->dev.of_node.
Mark Brown Oct. 25, 2011, 7:11 a.m. | #11
On Mon, Oct 24, 2011 at 10:47:17PM +0800, Shawn Guo wrote:
> On Mon, Oct 24, 2011 at 03:49:30PM +0200, Mark Brown wrote:

> > Might be best to just
> > search within dev and get drivers to pass the "real" device in when
> > registering the regulator rather than the virtual device.

> It should also work, but it will also change the API slightly for
> non-dt users.  I'm not sure it's something we want.

I don't think that's an issue at all.
Shawn Guo Oct. 25, 2011, 7:28 a.m. | #12
On Tue, Oct 25, 2011 at 12:31:51PM +0530, Rajendra Nayak wrote:
> On Tuesday 25 October 2011 12:38 PM, Shawn Guo wrote:
> >On Tue, Oct 25, 2011 at 11:40:34AM +0530, Rajendra Nayak wrote:
> >>On Monday 24 October 2011 09:21 PM, Shawn Guo wrote:
> >>>On Mon, Oct 24, 2011 at 04:56:31PM +0200, Grant Likely wrote:
> >[...]
> >>>>It is always better to attach the of_node at struct device
> >>>>registration time instead of searching the tree in common code.  The
> >>>>of_node should already be assigned by the time regulator_register() is
> >>>>called.
> >>>
> >>>That's the problem we have.  There is no 'struct dev' to attach of_node
> >>>for each regulator by the time regulator_register() is called, because
> >>>the 'struct dev' for each regulator is created inside
> >>>regulator_register() as wrapped by 'struct regulator_dev'.
> >>
> >>The root of your problem seems to be that your pmic driver isn't
> >>registering regulator devices from DT, and if it did, you wouldn't
> >>need to do a search in dev->parent->of_node and instead the driver
> >>would have the right dev->of_node populated.
> >>
> >No, it's not the root of my problem.  Again, we are talking about
> >'Case 2', where multiple regulator devices are registered to
> >regulator core with regulator driver being probed once, where each
> >regulator node is taken as the child of 'regulators' node.  Having
> >device_node of 'regulators' attached to dev->of_node does not help
> >at all.  What we need is to have each child node attached to
> >regulator_dev->dev.of_node.
> 
> It certainly helps if dev->of_node has the 'regulators' node attached.
> The driver can very easily then do a for_each_child_of_node() to extract
> and register individual regulators passing an additional of_node param.
> 
Well, let's look at what Grant is asking for.  He is asking for that
each child node should be attached to regulator_dev->dev.of_node by
the time regulator_register() is called.  How can this be helping that?

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8fe132d..29dcf90 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2673,7 +2673,8 @@  struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
        BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);

        /* find device_node and attach it */
-       rdev->dev.of_node = of_find_node_by_name(NULL, regulator_desc->name);
+       rdev->dev.of_node = of_find_node_by_name(dev->parent->of_node,
+                                                regulator_desc->name);

        /* register with sysfs */
        rdev->dev.class = &regulator_class;