On 07/31/2012 07:45 AM, Stephen Warren wrote:
> Oh I see. That's a little confusing. Why not just reference the relevant
> resources directly in each step; something more like:
>
> gpio@1 {
> action = "enable-gpio";
> gpio = <&gpio 1 0>;
> };
>
> I guess that might make parsing/building a little harder, since you'd
> have to detect when you'd already done gpio_request() on a given GPIO
> and not repeat it or something like that, but to me this makes the DT a
> lot easier to comprehend.
I tried to move towards having the phandles directly in the sequences
themselves - that reminded me why I did not do that in the first place.
Let's say we have a sequence like this (reg property omitted on purpose):
power-on-sequence {
step@0 {
regulator = <&backlight_reg>;
enable;
};
step@1 {
delay = <10000>;
};
step@2 {
pwm = <&pwm 2 5000000>;
enable;
};
step@3 {
gpio = <&gpio 28 0>;
enable;
};
};
The problem is, how do we turn these phandles into the resource of
interest. The type of the resource can be infered by the name of the
property. The hard part is resolving the resource from the phandle - it
seems like the API just does not allow to do this. GPIO has
of_get_named_gpio, but AFAIK there are no equivalent for regulator
consumer and PWM: the only way to use the DT with them is through
get_regulator and get_pwm which work at the device level.
Or is there a way that I overlooked?
Thanks,
Alex.
On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >Oh I see. That's a little confusing. Why not just reference the relevant
> >resources directly in each step; something more like:
> >
> > gpio@1 {
> > action = "enable-gpio";
> > gpio = <&gpio 1 0>;
> > };
> >
> >I guess that might make parsing/building a little harder, since you'd
> >have to detect when you'd already done gpio_request() on a given GPIO
> >and not repeat it or something like that, but to me this makes the DT a
> >lot easier to comprehend.
>
> I tried to move towards having the phandles directly in the
> sequences themselves - that reminded me why I did not do that in the
> first place. Let's say we have a sequence like this (reg property
> omitted on purpose):
>
> power-on-sequence {
> step@0 {
> regulator = <&backlight_reg>;
> enable;
> };
> step@1 {
> delay = <10000>;
> };
> step@2 {
> pwm = <&pwm 2 5000000>;
> enable;
> };
> step@3 {
> gpio = <&gpio 28 0>;
> enable;
> };
> };
>
> The problem is, how do we turn these phandles into the resource of
> interest. The type of the resource can be infered by the name of the
> property. The hard part is resolving the resource from the phandle -
> it seems like the API just does not allow to do this. GPIO has
> of_get_named_gpio, but AFAIK there are no equivalent for regulator
> consumer and PWM: the only way to use the DT with them is through
> get_regulator and get_pwm which work at the device level.
>
> Or is there a way that I overlooked?
No, you are right. Perhaps we should add exported functions that do the
equivalent of of_pwm_request() or the regulator_dev_lookup() and
of_get_regulator() pair.
Thierry
On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>> resources directly in each step; something more like:
>>>
>>> gpio@1 {
>>> action = "enable-gpio";
>>> gpio = <&gpio 1 0>;
>>> };
>>>
>>> I guess that might make parsing/building a little harder, since you'd
>>> have to detect when you'd already done gpio_request() on a given GPIO
>>> and not repeat it or something like that, but to me this makes the DT a
>>> lot easier to comprehend.
>>
>> I tried to move towards having the phandles directly in the
>> sequences themselves - that reminded me why I did not do that in the
>> first place. Let's say we have a sequence like this (reg property
>> omitted on purpose):
>>
>> power-on-sequence {
>> step@0 {
>> regulator = <&backlight_reg>;
>> enable;
>> };
>> step@1 {
>> delay = <10000>;
>> };
>> step@2 {
>> pwm = <&pwm 2 5000000>;
>> enable;
>> };
>> step@3 {
>> gpio = <&gpio 28 0>;
>> enable;
>> };
>> };
>>
>> The problem is, how do we turn these phandles into the resource of
>> interest. The type of the resource can be infered by the name of the
>> property. The hard part is resolving the resource from the phandle -
>> it seems like the API just does not allow to do this. GPIO has
>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>> consumer and PWM: the only way to use the DT with them is through
>> get_regulator and get_pwm which work at the device level.
>>
>> Or is there a way that I overlooked?
>
> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.
How would that be looked with respect to "good DT practices"? I can
somehow understand the wish to restrain DT access to these functions
that integrate well with current workflows. Aren't we going to be
frowned upon if we make more low-level functions public?
Alex.
On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> >>On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >>>Oh I see. That's a little confusing. Why not just reference the relevant
> >>>resources directly in each step; something more like:
> >>>
> >>> gpio@1 {
> >>> action = "enable-gpio";
> >>> gpio = <&gpio 1 0>;
> >>> };
> >>>
> >>>I guess that might make parsing/building a little harder, since you'd
> >>>have to detect when you'd already done gpio_request() on a given GPIO
> >>>and not repeat it or something like that, but to me this makes the DT a
> >>>lot easier to comprehend.
> >>
> >>I tried to move towards having the phandles directly in the
> >>sequences themselves - that reminded me why I did not do that in the
> >>first place. Let's say we have a sequence like this (reg property
> >>omitted on purpose):
> >>
> >> power-on-sequence {
> >> step@0 {
> >> regulator = <&backlight_reg>;
> >> enable;
> >> };
> >> step@1 {
> >> delay = <10000>;
> >> };
> >> step@2 {
> >> pwm = <&pwm 2 5000000>;
> >> enable;
> >> };
> >> step@3 {
> >> gpio = <&gpio 28 0>;
> >> enable;
> >> };
> >> };
> >>
> >>The problem is, how do we turn these phandles into the resource of
> >>interest. The type of the resource can be infered by the name of the
> >>property. The hard part is resolving the resource from the phandle -
> >>it seems like the API just does not allow to do this. GPIO has
> >>of_get_named_gpio, but AFAIK there are no equivalent for regulator
> >>consumer and PWM: the only way to use the DT with them is through
> >>get_regulator and get_pwm which work at the device level.
> >>
> >>Or is there a way that I overlooked?
> >
> >No, you are right. Perhaps we should add exported functions that do the
> >equivalent of of_pwm_request() or the regulator_dev_lookup() and
> >of_get_regulator() pair.
>
> How would that be looked with respect to "good DT practices"? I can
> somehow understand the wish to restrain DT access to these functions
> that integrate well with current workflows. Aren't we going to be
> frowned upon if we make more low-level functions public?
Yes, I think that's exactly what will happen.
Thierry
On Thu 02 Aug 2012 05:45:41 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
>> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>>>> resources directly in each step; something more like:
>>>>>
>>>>> gpio@1 {
>>>>> action = "enable-gpio";
>>>>> gpio = <&gpio 1 0>;
>>>>> };
>>>>>
>>>>> I guess that might make parsing/building a little harder, since you'd
>>>>> have to detect when you'd already done gpio_request() on a given GPIO
>>>>> and not repeat it or something like that, but to me this makes the DT a
>>>>> lot easier to comprehend.
>>>>
>>>> I tried to move towards having the phandles directly in the
>>>> sequences themselves - that reminded me why I did not do that in the
>>>> first place. Let's say we have a sequence like this (reg property
>>>> omitted on purpose):
>>>>
>>>> power-on-sequence {
>>>> step@0 {
>>>> regulator = <&backlight_reg>;
>>>> enable;
>>>> };
>>>> step@1 {
>>>> delay = <10000>;
>>>> };
>>>> step@2 {
>>>> pwm = <&pwm 2 5000000>;
>>>> enable;
>>>> };
>>>> step@3 {
>>>> gpio = <&gpio 28 0>;
>>>> enable;
>>>> };
>>>> };
>>>>
>>>> The problem is, how do we turn these phandles into the resource of
>>>> interest. The type of the resource can be infered by the name of the
>>>> property. The hard part is resolving the resource from the phandle -
>>>> it seems like the API just does not allow to do this. GPIO has
>>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>>> consumer and PWM: the only way to use the DT with them is through
>>>> get_regulator and get_pwm which work at the device level.
>>>>
>>>> Or is there a way that I overlooked?
>>>
>>> No, you are right. Perhaps we should add exported functions that do the
>>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>>> of_get_regulator() pair.
>>
>> How would that be looked with respect to "good DT practices"? I can
>> somehow understand the wish to restrain DT access to these functions
>> that integrate well with current workflows. Aren't we going to be
>> frowned upon if we make more low-level functions public?
>
> Yes, I think that's exactly what will happen.
Probably not worth to try this then?
Alex.
On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> > The problem is, how do we turn these phandles into the resource of
> > interest. The type of the resource can be infered by the name of the
> > property. The hard part is resolving the resource from the phandle -
> > it seems like the API just does not allow to do this. GPIO has
> > of_get_named_gpio, but AFAIK there are no equivalent for regulator
> > consumer and PWM: the only way to use the DT with them is through
> > get_regulator and get_pwm which work at the device level.
> > Or is there a way that I overlooked?
> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.
I missed some of the earlier bits of the thread here but why can't we do
device based lookups?
On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>
>>> The problem is, how do we turn these phandles into the resource of
>>> interest. The type of the resource can be infered by the name of the
>>> property. The hard part is resolving the resource from the phandle -
>>> it seems like the API just does not allow to do this. GPIO has
>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>> consumer and PWM: the only way to use the DT with them is through
>>> get_regulator and get_pwm which work at the device level.
>
>>> Or is there a way that I overlooked?
>
>> No, you are right. Perhaps we should add exported functions that do the
>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>> of_get_regulator() pair.
>
> I missed some of the earlier bits of the thread here but why can't we do
> device based lookups?
That is because the phandles would not be properties of the device node
but rather of its sub-nodes:
backlight {
compatible = "pwm-backlight";
...
power-on-sequence {
step@0 {
regulator = <&backlight_reg>;
enable;
};
So here simply using regulator_get on the backlight device would not work.
Alex.
On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
> >I missed some of the earlier bits of the thread here but why can't we do
> >device based lookups?
> That is because the phandles would not be properties of the device
> node but rather of its sub-nodes:
> backlight {
> compatible = "pwm-backlight";
> ...
> power-on-sequence {
> step@0 {
> regulator = <&backlight_reg>;
> enable;
> };
>
> So here simply using regulator_get on the backlight device would not work.
Ah, right. DT isn't being terribly helpful here... I think the thing
I'd expect to work here is that you have a reference back to the supply
property of the backlight device rather than direct to the regulator so
you end up writing "enable supply X" rather than "enable regulator X".
Not quite sure how exactly you'd accomplish that - I guess if
regulator_get() would recursively follow phandles until it hits a node
that'd do the trick?
On 08/04/2012 11:12 PM, Mark Brown wrote:
> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>
>>> I missed some of the earlier bits of the thread here but why can't we do
>>> device based lookups?
>
>> That is because the phandles would not be properties of the device
>> node but rather of its sub-nodes:
>
>> backlight {
>> compatible = "pwm-backlight";
>> ...
>> power-on-sequence {
>> step@0 {
>> regulator = <&backlight_reg>;
>> enable;
>> };
>>
>
>> So here simply using regulator_get on the backlight device would not work.
>
> Ah, right. DT isn't being terribly helpful here... I think the thing
> I'd expect to work here is that you have a reference back to the supply
> property of the backlight device rather than direct to the regulator so
> you end up writing "enable supply X" rather than "enable regulator X".
>
> Not quite sure how exactly you'd accomplish that - I guess if
> regulator_get() would recursively follow phandles until it hits a node
> that'd do the trick?
Do you mean that regulator_get() would parse sub-nodes looking for a
match? That seems rather dangerous and error-prone, especially if one
has actual devices within the sub-nodes - their regulators could be
stolen by the parent device.
I think we only have two choices for this:
1) Stick to the scheme where resources are declared at the device level,
such as they can be referenced by name in the sub-nodes (basically what
I did in this patch):
backlight {
compatible = "pwm-backlight";
...
backlight-supply = <&backlight_reg>;
power-on-sequence {
step@0 {
regulator = "backlight";
enable;
};
This would translate by a get_regulator(dev, "backlight") in the code
which would be properly resolved.
2) Export a lower-level DT API for resolving phandles directly from a
property, similar to of_get_named_gpio. We would then have
of_get_named_regulator and of_get_named_pwm.
If 2) is deemed acceptable, then I think we should go for it as it would
provide the most compact and readable DT syntax. Otherwise 1) is still
acceptable IMHO, as it should at least make sense to people already
familiar with how the DT works.
Opinions from DT experts?
Alex.
On 08/05/2012 08:27 PM, Alex Courbot wrote:
> On 08/04/2012 11:12 PM, Mark Brown wrote:
>> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>>
>>>> I missed some of the earlier bits of the thread here but why can't
>>>> we do
>>>> device based lookups?
...
> I think we only have two choices for this:
>
> 1) Stick to the scheme where resources are declared at the device level,
> such as they can be referenced by name in the sub-nodes (basically what
> I did in this patch):
>
> backlight {
> compatible = "pwm-backlight";
> ...
> backlight-supply = <&backlight_reg>;
>
> power-on-sequence {
> step@0 {
> regulator = "backlight";
> enable;
> };
>
> This would translate by a get_regulator(dev, "backlight") in the code
> which would be properly resolved.
Yes, upon reflection, that scheme does make sense. I withdraw the
comments I made re: whether it'd be better to just stick the phandles
into the steps.
On 08/07/2012 01:16 AM, Stephen Warren wrote:
> On 08/05/2012 08:27 PM, Alex Courbot wrote:
>> On 08/04/2012 11:12 PM, Mark Brown wrote:
>>> On Fri, Aug 03, 2012 at 10:15:46AM +0900, Alex Courbot wrote:
>>>> On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
>>>
>>>>> I missed some of the earlier bits of the thread here but why can't
>>>>> we do
>>>>> device based lookups?
> ...
>> I think we only have two choices for this:
>>
>> 1) Stick to the scheme where resources are declared at the device level,
>> such as they can be referenced by name in the sub-nodes (basically what
>> I did in this patch):
>>
>> backlight {
>> compatible = "pwm-backlight";
>> ...
>> backlight-supply = <&backlight_reg>;
>>
>> power-on-sequence {
>> step@0 {
>> regulator = "backlight";
>> enable;
>> };
>>
>> This would translate by a get_regulator(dev, "backlight") in the code
>> which would be properly resolved.
>
> Yes, upon reflection, that scheme does make sense. I withdraw the
> comments I made re: whether it'd be better to just stick the phandles
> into the steps.
Right - having the phandles directly in the sequences has its merits,
but logically speaking resources are related to a device, so this
declarative approach is probably closer to reality anyway.
I will revise the patch according to all the feedback received and
submit a new version soon.
Thanks,
Alex.