[PATCH 2/6] ARM: EXYNOS5: DT Support for SATA and SATA PHY

[Vasanth Ananthan]

Hi,

On Wed, Oct 10, 2012 at 10:01 PM, Olof Johansson <olof at lixom.net> wrote:
> Hi,
>
> On Wed, Oct 10, 2012 at 02:08:31PM +0530, Vasanth Ananthan wrote:
>
>> > > +
>> > > +             sataphy at 70 {
>> >
>> > sata-phy would be a more conventional name.
>> >
>> > > +                     compatible = "samsung,i2c-phy";
>> >
>> > i2c-phy? Seems like an odd choice of name. What is this device?
>> >
>>
>> The SATA physical layer controller is both a platform device and a i2c
>> slave device.
>> This compatible string is for the i2c client driver. Hence I have used
>> i2c-phy here.
>
> Ok, but samsung,i2c-phy is much too generic. Maybe something like
> samsung,exynos5-sata-phy  (and rename the MMIO-controlled phy controls
> to ...sata-phy-controller below).
>

I'll do so..

>
>> > > +        };
>> > > +
>> > > +        sata-phy at 12170000 {
>> > > +                compatible = "samsung,exynos-sata-phy";
>> > > +                reg = <0x12170000 0x1ff>;
>> > > +        };
>> >
>> > Should have binding too. How does this relate to the i2c device above.
>> >
>>
>> As mentioned earlier SATA physical layer controller is both a platform
>> device and also an i2c slave device.
>> This Node is for the SATA physical layer platform device, the previous node
>> is for i2c slave device.
>> Certain initialization settings done directly and other settings has to be
>> done through i2c.
>
> Wow, that's quite awkward. What needs to be done over i2c? I think I have
> seen use of SATA without touching the i2c side but it might have been for
> a simple setup.

40 bit Interface setting is done through i2c. Internal setting address
0x3A and data 0x0B.

>
>> > > +
>> > >       i2c at 12C60000 {
>> > >               compatible = "samsung,s3c2440-i2c";
>> > >               reg = <0x12C60000 0x100>;
>> > > @@ -152,6 +164,13 @@
>> > >               #size-cells = <0>;
>> > >       };
>> > >
>> > > +     i2c at 121D0000 {
>> > > +                compatible = "samsung,s3c2440-sataphy-i2c";
>> >
>> > Is this a unique i2c controller, or is it just another one like the others
>> > on
>> > the chip? If it's the latter, it should use the regular compatible string.
>> >
>>
>> Yes, its a unique i2c controller which lacks an interrupt line while others
>> are interrupt driven.
>> Hence I have used a distinct compatible string for the driver to
>> distinguish the controller.
>
> It would be better to just make the i2c bus driver handle the case where there
> is no interrupt specifier and just use polling in those cases, especially if
> the rest of the device is identical and doesn't need special handling.
>
> As a matter of fact, if that had been done for the hdmi phy, then you could
> have done this patch without modifying the driver at all, just device tree
> contents. And the same would go for the next time down the road when
> a "special" i2c bus is added.
>


Yes, It is the i2c bus driver thats handles this case. There is a
subsequent patch
that provides the polling support to the driver. As far as I know, the
i2c controller for
HDMI PHY is also interrupt driven.

>
> -Olof



-- 
Regards,

Vasanth K A