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

[Chen-Yu Tsai]

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.

>> 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?

>
>> 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.

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

I've also raised the issue of diverging device trees and bindings:

  https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-October/004849.html

>> So clearly the dts files aren't following the binding.
>>
> And what about the dtsi files? Are they correct in defining node names
> as 'regulator@[unit-address]'?

They are broken. Someone needs to fix them.

>>>
>>>> If you have multiple regulator nodes which need to be differentiated,
>>>> you need to use the deprecated "regulator-compatible" property, or just
>>>> use the standard compatible property.
>>>>
>>> Good point. I would not use a deprecated property but the compatible
>>> property seems reasonable to me. So you agree that the patch's concept
>>> could be retained while substituting the node-name property by the
>>> compatible property?
>>
>> If you're going to modify the binding and/or device tree files, please
>> do it in both places. Ideally the platform should have one canonical
>> source for them.
>>
>> Linux's standard regulator DT matching code only matches against node
>> names and the deprecated "regulator-compatible" property. Not even the
>> standard "compatible" is used, though I see a few PMICs using that.
>> So even if you do get them working this way in U-boot, it's still not
>> going to work in Linux, and you might get other comments when pushing
>> your changes.
>>
> I guess if LINUX would not match against the regulator-compatible
> property but only against the node name things would not work properly.
> Again looking the file tps65217.dtsi shows that every regulator node
> (named regulator at 0 to regulator at 6) defines the regulator-compatible
> property. Only matching against this property therefore makes things
> working.

The dtsi file is not following the binding. It should be fixed. But
I suppose that is an issue for the platform maintainer, and any
concerned users.

Regards
ChenYu

>
> Felix