[U-Boot] [PATCH] power: extend prefix match to regulator-name property

Wed Oct 25 07:10:19 UTC 2017

On Tue, Oct 24, 2017 at 2:30 PM, Felix Brack <fb at ltec.ch> wrote:
> On 23.10.2017 22:34, Chen-Yu Tsai wrote:
>> On Tue, Oct 24, 2017 at 12:36 AM, Felix Brack <fb at ltec.ch> wrote:
>>> On 23.10.2017 16:36, Chen-Yu Tsai wrote:
>>>> Hi,
>>>>
>>>> On Thu, Oct 12, 2017 at 11:40 PM, Felix Brack <fb at ltec.ch> wrote:
>>>>> On 12.10.2017 14:53, Chen-Yu Tsai wrote:
>>>>>> On Thu, Oct 12, 2017 at 8:24 PM, Felix Brack <fb at ltec.ch> wrote:
>>>>>>> On 12.10.2017 04:46, Chen-Yu Tsai wrote:
>>>>>>>> On Mon, Oct 9, 2017 at 6:04 PM, Felix Brack <fb at ltec.ch> wrote:
>>>>>>>>> This patch extends pmic_bind_children prefix matching. In addition to
>>>>>>>>> the node name the property regulator-name is used while trying to match
>>>>>>>>> prefixes. This allows assigning different drivers to regulator nodes
>>>>>>>>> named regulator at 1 and regulator at 10 for example.
>>>>>>>>
>>>>>>>> No. See the regulator bindings:
>>>>>>>>
>>>>>>>> Optional properties:
>>>>>>>> - regulator-name: A string used as a descriptive name for regulator outputs
>>>>>>>>
>>>>>>> The actual regulator.txt states:
>>>>>>>
>>>>>>> Optional properties:
>>>>>>>  - regulator-name: a string, required by the regulator uclass
>>>>>>>
>>>>>>> This was the reason for choosing the regulator-name property.
>>>>>>
>>>>>> Mine was from the Linux Kernel. Are there two sets of bindings?
>>>>>> Shouldn't there be just one?
>>>>>>
>>>>> Mine was from U-Boot as this is the U-Boot mailing list and things do
>>>>> not seem to be fully synchronized between LINUX and U-Boot.
>>>>
>>>> That seems to be the underlying problem. They really should be the same.
>>>> I'm not sure which platforms really follow through with this though.
>>>>
>>>>>>>> This can vary from board to board. The name should match the power rail
>>>>>>>> name of the board (which may not be the same as the regulator chip's
>>>>>>>> output name).
>>>>>>>>
>>>>>>> Exactly. I totally agree but as stated in an earlier post: I did not
>>>>>>> define the names for the regulators and modifying them is almost
>>>>>>> certainly not the right way to go. Let me explain this briefly. The
>>>>>>> regulator names I'm trying to match are those from tps65910.dtsi, an
>>>>>>> include file. The exact same file is part of the LINUX kernel. Therefore
>>>>>>> I resigned suggesting the modification of the node names.
>>>>>>
>>>>>> First, it is an include file. Board files are free to override the
>>>>>> name. We did that for sunxi at first, have the .dtsi file provide
>>>>>> some default names, then have board .dts files override them with
>>>>>> names that make more sense. Later on we just dropped the default
>>>>>> names altogether.
>>>>>>
>>>>> I am definitely confused now, maybe we are using same terms for
>>>>> different things. When I'm talking about a 'name' I mean the 'node name'
>>>>>  according to DT specification (as for instance returned by
>>>>> ofnode_get_name). I'm not talking about a property or a node label. The
>>>>> following code defines a node name 'regulator at 2' ore more precisely a
>>>>> node named 'regulator' having unit-address 2:
>>>>>
>>>>> vdd1_reg: regulator at 2 {
>>>>>         reg = <2>;
>>>>>         regulator-compatible = "vdd1";
>>>>> };
>>>>>
>>>>> This is found in tps65910.dtsi include file. You say "board files are
>>>>> free to override the name". Hence I could include the tps65910.dtsi into
>>>>> my board dts file and kind of rename node 'regulator at 2' by let's say
>>>>> 'buck_vdd1 at 2'?
>>>>> The only way of "overriding" I can think of is by not including the dtsi
>>>>> file and redefining all nodes.
>>>>
>>>> I'm talking about the "name" as defined in the "regulator-name"
>>>> property, which is what you want to match against in your patch.
>>>>
>>>> So boards are definitely free to override the property by setting
>>>> a new value for it. And indeed boards do that.
>>>>
>>> This is the driving idea behind my patch.
>>>
>>>>>> The same pattern is also seen in tps65910.dtsi and am3517-craneboard.dts.
>>>>>> And also am335x-evm.dts, which the tps65910.dtsi was tested with.
>>>>>>
>>>>> Looking at the am3517-craneboard.dts file I don't see how node names are
>>>>> getting overwritten. What gets set, changed or overwritten is the node
>>>>> property regulator-name.
>>>>
>>>> Yes. That's what I'm referring to. Doesn't this work against your
>>>> attempt to bind pmic-uclass against regulator-name properties?
>>>>
>>> Well, yes, my patch works like charm. I didn't mention that it doesn't
>>> work ;-).
>>>
>>>>>
>>>>>> Now I couldn't find the binding document for tps65910, but looking
>>>>>> at tps65217 instead, it says:
>>>>>>
>>>>>> - regulators: list of regulators provided by this controller, must be named
>>>>>>   after their hardware counterparts: dcdc[1-3] and ldo[1-4]
>>>>>>
>>>>>> And indeed that is what's used in the example.
>>>>>>
>>>>> Yes, exactly and correct. Again, this tps65217.txt does only exist in
>>>>> LINUX and not in U-Boot.
>>>>
>>>> Device tree bindings are a set of rules, contracts, ABIs, whatever
>>>> you call it, between the hardware description that is the device
>>>> tree, and software that implement support for the hardware. Having
>>>> two or more diverging sets is not an improvement. Communities should
>>>> work together to have a common set of bindings.
>>>>
>>> I totally agree.
>>>
>>>> FreeBSD seems to be importing device tree bindings from Linux. There
>>>> was also talk about importing bindings from other projects that have
>>>> already created and implemented support for bindings that do not
>>>> exist in Linux. This has been raised for the Device Tree Workshop
>>>> this Thursday at OSSE/ELCE:
>>>>
>>>>   https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004833.html
>>>>   https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004891.html
>>>>
>>> If that wish from Andrew would become true it would be the ultimate
>>> solution: "I’d like it if there was a common repo for all devicetree
>>> consumers to share".
>>>
>>>> I've also raised the issue of diverging device trees and bindings:
>>>>
>>>>   https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html
>>>>
>>> Thanks! I hope this helps moving things into the right direction. And
>>> yes, there is quite a lot of truth in calling my patch a "dirty
>>> workaround" for broken dts and even worse dtsi files. But for U-Boot
>>> this patch could help to bridge the time gap until the real fix. Sadly I
>>> now that it could also have the exact opposite effect and lead to even
>>> more broken (with respect to binding rules) dts and dtsi files.
>>
>> I'm OK with a temporary fix or workaround. But temporary means someone
>> needs to follow up and get it where it needs to go. It should also be
>> noted in the commit log. And the binding should be marked as a
>> workaround for existing (broken) device trees, so people don't adapt
>> it by mistake.
>>
> This has already been addressed in v2 patch I posted a week ago
> (https://patchwork.ozlabs.org/patch/827507/).

I see that now. Thanks

ChenYu