| Message ID | 20111024134025.GE1755@S2100-06.ap.freescale.net |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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.
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'.
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.
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. >
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. >
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.
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.
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?
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 = ®ulator_class;