Message-ID: <570429B8.3060002@wwwdotorg.org>
Date:	Tue, 5 Apr 2016 15:10:16 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Kishon Vijay Abraham I <kishon@...com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Andrew Bresticker <abrestic@...omium.org>,
	linux-tegra@...r.kernel.org, devicetree@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>
Subject: Re: [PATCH v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210
 support

On 04/05/2016 08:44 AM, Thierry Reding wrote:
> On Wed, Mar 16, 2016 at 11:59:44AM -0600, Stephen Warren wrote:
>> On 03/04/2016 09:19 AM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@...dia.com>
>>>
>>> Extend the binding to cover the set of feature found in Tegra210.
>>
>> Acked-by: Stephen Warren <swarren@...dia.com>
>>
>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>
>>> +	padctl@0,7009f000 {
>> ...
>>> +		pads {
>> ...
>>> +		};
>>> +
>>> +		ports {
>> ...
>>> +		};
>>
>> As a comment not affecting my ack in any way: At the top-level, we place all
>> the child nodes into "array container" nodes named "pads" and "ports". This
>> is nice since it separates different types of child nodes and allows easily
>> adding more types of child nodes in the future without interference, and in
>> a way that allows us to explicitly know what each node is without having to
>> interpret its name or compatible value to do so. However, we haven't done
>> this with the per-lane child nodes inside each pad. If we were to rev the
>> design, I'd be tempted to suggest:
>>
>> 	padctl@0,7009f000 {
>> 		pads {
>> 			usb2 {
>> 				lanes { // This level is new
>> 					usb2-0 {
>
> I tried to make this work, but it's unfortunately not possible with the
> current code. The reason is that the PHY subsystem assumes that either
> the provider DT node corresponds to that of the device (the usb2 pad in
> the above example) or one of its children. Hence, putting everything
> into one more level further down would require some mechanism to tell
> the subsystem about it so that it can be found.

When the padctl driver registers the PHY objects with the PHY subsystem, 
can it pass the lanes node as the DT node? That woulud mean each lane 
/was/ a child of the node registered with the PHY subsystem.

Perhaps the PHY subsystem requires a struct device rather than a DT node 
registered with it? If so, does it make sense to create a separate 
struct device with the of_node pointing at lanes{}?

> Arguably the current support code isn't a good argument for designing a
> binding, so perhaps it'd be useful to add this mechanism in order to get
> a better binding. On the other hand, I'm not sure it's really worth it,
> since we already have generic bindings that specify the layout of child
> devices, and those have been agreed upon broadly (presumably).
>
> In light of the recent discussion on DPAUX vs. I2C, I see how having the
> extra level would be useful to provide additional context. If you think
> it's worth it I can spend the extra time to get this implemented in the
> core.

Naively, it sounds like it'd be a good idea to fix the PHY core. It 
really shouldn't care about the parent of any object registered with it; 
it should only interact with the specific object it was given, and any 
other data such as "ops" callbacks. Do you have any inkling how much 
work that would be?

Powered by blists - more mailing lists