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

[Olof Johansson]

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


> > > +        };
> > > +
> > > +        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.

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


-Olof