Re: [PATCH v4 1/6] drivers: phy: add generic PHY framework

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

On 04/02/2013 12:38 AM, Stephen Warren wrote:
> On 04/01/2013 04:27 PM, Sylwester Nawrocki wrote:
>> On 03/28/2013 06:43 AM, Kishon Vijay Abraham I wrote:
>>> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt
>
>>> +example2:
>>> +phys: phy {
>>> +    compatible = "xxx";
>>> +    reg =<...>;
>>> +    .
>>> +    .
>>> +    phys =<&phys 1>;
>>> +    .
>>> +    .
>>> +};
>>> +
>>> +This node represents a controller that uses one of the PHYs which is defined
>>> +previously. Note that the phy handle has an additional specifier "1" to
>>> +differentiate between the three PHYs. For this case, the controller driver
>>> +should use of_phy_get_with_args/devm_of_phy_get_with_args.
>>
>> This means a PHY user needs to know indexes at the PHY driver ?
>
> The client node's DT has to specify which of the provider's PHYs it
> references, yes. Presumably the device driver for the client node knows
> absolutely nothing about this.

Ah, right. The device driver for the client node not necessarily need to be
aware about this. I think I got misled by the 'index' argument of
of_phy_get_with_args() which the PHY consumer need to supply. However it is
only an index pointing to a PHY specifier in its 'phys' property, hence it
would be normally 0 if the client needs only one PHY. Hopefully I got it
right this time.

>> I have been thinking of using this for an IP which has 4 video PHYs: 2 MIPI
>> CSI-2 and 2 MIPI DSI. The IP has just 2 registers, each of which is shared
>> between one MIPI CSI-2 DPHY and one MIPI DSI DPHY. So I thought about creating
>> a single device node for this IP and using 4 indexes for the PHYs, e.g. 0...3.
>
> That sounds right.
>
>> Then users of each PHY type would use only indexes 0..1 (to select their
>> corresponding port).
>
> I don't follow that. If the provider exports PHYs 0..3, then the client
> nodes would refer to PHYS 0..3 not 0..1.

Indeed, it seems I just wanted to preserve the CSI/DSI "channel" information
(0, 1), but that is not really needed anywhere.

>> However I fail to see how this could now be represented in the bindings.
>
> Exactly like the example you gave below, I would expect.
>
>> I assume struct phy::type could be used to differentiate between CSI-2 and DSI.
>> And struct phy::port could be used to select specific CSI-2 or DSI channel
>> (0, 1). Ideally the phy users should not care about index of a PHY at the PHY
>> device tree node. E.g. there are 2 MIPI CSI-2 receivers and each has only
>> one PHY assigned to it. I'm just wondering how the binding should look like,
>> so a PHY could be associated with a receiver automatically by the phy-core,
>> e.g.
>
> Details such as phy::type and phy::port sounds like internal driver
> implementation details which would have no effect on the bindings.

Yes, I seem to have mixed the phy-core implementation and the bindings a 
bit.
My intention was to have the PHYs registered with phy::port that would 
reflect
data channel id, as specified in the SoC's datasheet. However I can't 
think of
any use of it at the moment, except for debugging purpose.

>> /* DPHY IP node */
>> video-phy {
>>        reg =<0x10000000 8>;
>> };
>>
>> /* MIPI DSI nodes */
>> dsi_0 {
>>       phys =<&video-phy 0>;
>> };
>>
>> dsi_1 {
>>       phys =<&video-phy 1>;
>> };
>>
>> /* MIPI CSI-2 nodes */
>> csi_0 {
>>       phys =<&video-phy 2>;
>> };
>>
>> csi_1 {
>>       phys =<&video-phy 3>;
>> };
>
> That looks correct to me.
>
>> I'm not sure if it is not an overkill to use this the PHY framework with
>> a device which has only 2 registers. Perhaps something less heavy could
>> be designed for it. However, if the PHY framework is commonly used there
>> should be no issue wrt enabling the whole big infrastructure for a simple
>> device like this.
>
> I don't think the number of registers should really makes any
> difference; the whole point of the PHY framework is to decouple to
> providers and consumers, so doing anything custom for special cases
> would completely destroy the ability of the PHY framework to do that.

Ok, that's a very good argument. Something I have not been focused on
that much, given the architecture of hardware I used to work with.

I'll git it a try and I'll see if any any further questions jump out.