Message-ID: <54B12E54.7000704@redhat.com>
Date:	Sat, 10 Jan 2015 14:51:16 +0100
From:	Hans de Goede <hdegoede@...hat.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Chen-Yu Tsai <wens@...e.org>,
	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Jason Cooper <jason@...edaemon.net>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Andrew Lunn <andrew@...n.ch>,
	devicetree <devicetree@...r.kernel.org>,
	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>,
	Mark Rutland <mark.rutland@....com>,
	Nadav Haklai <nadavh@...vell.com>, linux-ide@...r.kernel.org,
	Lior Amsalem <alior@...vell.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Tejun Heo <tj@...nel.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH v2 2/4] Documentation: bindings: Add the regulator property
 to the sub-nodes AHCI bindings

Hi,

On 10-01-15 12:17, Mark Brown wrote:
> On Sat, Jan 10, 2015 at 11:20:13AM +0100, Hans de Goede wrote:
>> On 09-01-15 18:11, Mark Brown wrote:
>
>>> Or if the supply is for the device at the other end of the link (which
>>> is what it sounded like) then use that.  This just sounds like the same
>>> problem we have for all the enumerable buses in embedded systems where
>>> we need to be able to understand that the device exists prior to it
>>> being fully ready to appear in the system.  Having the link/slot be a
>>> device in Linux does indeed seem to be a common way people think about
>>> doing this, it sounds like for this one it might be the most direct.
>
>> I think we should be careful to not think too much from an implementation
>> pov here, the purpose of the devicetree description is to describe the hardware,
>> as is.
>
> I don't think anyone is talking about changing the DT here, only the way
> it's represented inside Linux.
>
>>                  sata0: sata-port@0 {
>>                          reg = <0>;
>>                          phys = <&sata_phy 0>;
>> 			target-supply = <&reg_sata0>;
>>                  };
>>
>>                  sata1: sata-port@1 {
>>                          reg = <1>;
>>                          phys = <&sata_phy 1>;
>> 			target-supply = <&reg_sata1>;
>>                  };
>>          };
>
>> Seems to match the hardware pretty exactly, and also matches how we've
>> been describing similar devices with only one sata port + power plug sofar,
>> so from a consistency pov it also is a good model.
>
> Right, I think that makes sense

Good, as said I think getting the dt bit rights is the most important
thing here. So if we agree that the above dt example looks sane, lets see
how we can best implement that.

 > it also looks to me like these things
> should be representable as devices within Linux.

I guess we could manually instantiate platform devices for each of the
subnodes which represent an sata port here, yes that should not be
hard to do, it feels like a bit overkill though.

So for our this should likely look something like this:

         if (IS_ENABLED(CONFIG_OF_ADDRESS) && dev->of_node) {
                 for_each_child_of_node(dev->of_node, np) {
                         pdev[i++] = of_platform_device_create(np, NULL, NULL);
                 }
         }

and then loop over the pdev[] array and get the regulator, and phy for each.

I wonder if of_platform_device_create can deal with nodes with
#size-cells = <0>; is 0 though, if it cannot maybe we need to teach it.

>> So supporting this model requires having a regulator_get API which allows
>> specifying which of_node to get the regulator from, e.g. the proposed
>
> It requires nothing of the sort.

You're proposed solution of instantiating devices does more or less exactly
what I say is required, it is a way to pass an of_node into regulator_get,
but you're hiding the node inside dev->of_node. I can see the appeal in
that.

>> of_regulator_get function. I know you (Mark) do not like this, but all
>> other subsystems have an of_foo_get function taking an of_node as argument,
>> I do not see how the regulator subsys is so special that it cannot have one,
>> and also notice that this is only a kernel internal API which we can always
>> change later.
>
> Two things there.  One is that mostly those APIs are legacy APIs from
> before we made DT a first class citizen and generally they shouldn't be
> used these days.  The other is that at least for regulators we have
> constant problems with people abusing the API in various ways, as a
> result the API has a goal of not helping undesirable usage patterns in
> order to help people spot problems.  Having an API which makes it easy
> create broken bindings means that it is much more likely that people
> will do just that.

Hmm I can see where you're coming from, and your proposed solution may
also be useful for when we get similar boards requiring explicit regulator
handling with the sata ports described in ACPI tables.

Gregory, can you give the setup using per sata port devices a try, and
see how that works out code wise ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists