[PATCH V4 1/2] Phy: Exynos: Add Exynos5250 sata phy driver

[Yuvaraj Kumar]

On Wed, Jan 8, 2014 at 9:16 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie at samsung.com> wrote:
> On Wednesday, January 08, 2014 06:39:52 PM Yuvaraj Kumar wrote:
>> On Tue, Jan 7, 2014 at 7:52 PM, Bartlomiej Zolnierkiewicz
>> <b.zolnierkie at samsung.com> wrote:
>> >
>> > Hi,
>> >
>> > On Thursday, January 02, 2014 07:03:23 PM Yuvaraj Kumar wrote:
>> >> On Tue, Dec 31, 2013 at 9:00 PM, Bartlomiej Zolnierkiewicz
>> >> <b.zolnierkie at samsung.com> wrote:
>> >
>> >> >> @@ -8,3 +8,4 @@ obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)   += phy-exynos-mipi-video.o
>> >> >>  obj-$(CONFIG_PHY_MVEBU_SATA)         += phy-mvebu-sata.o
>> >> >>  obj-$(CONFIG_OMAP_USB2)                      += phy-omap-usb2.o
>> >> >>  obj-$(CONFIG_TWL4030_USB)            += phy-twl4030-usb.o
>> >> >> +obj-$(CONFIG_EXYNOS5250_SATA_PHY)    += sata_phy_exynos5250.o exynos5250_phy_i2c.o
>> >> >
>> >> > Will this compile/work without I2C support?
>> >> No.I missed this.It will not compile without I2C support.
>> >> How about  below change in drivers/phy/Kconfig ?
>> >> config EXYNOS5250_SATA_PHY
>> >> select I2C
>> >> select I2C_S3C2410
>> >
>> > Fine with me.
>> >
>> >> > CONFIG_EXYNOS5250_SATA_PHY doesn't require it currently.
>> >> I didnt get this. what it doesn't require?
>> >
>> > It doesn't require I2C.  If you add above I2C selects it will be OK.
>> >
>> >> >> +struct i2c_driver sataphy_i2c_driver = {
>> >> >
>> >> > Keeping it global together with sataphy_attach_i2c_client() is not very
>> >> > nice.  I've talked with our local device tree guru (a.k.a. Tomasz Figa)
>> >> > about this and it may be that this i2c driver is not even necessary.
>> >> Can you elaborate more on this?
>> >
>> > The usage of the global i2c driver is not a proper solution.
>> >
>> > i2c driver should register itself in the driver init function
>> do you mean i2c-s3c2410.c driver?
>
> No, I mean that drivers/phy/exynos5250_phy_i2c.c should do
>
>         i2c_add_driver(&sataphy_i2c_driver)
>
> instead of drivers/phy/sata_phy_exynos5250.c.
>
> i.e.:
>
> ...
> static int exynos_sata_i2c_probe(struct i2c_client *client,
>                                 const struct i2c_device_id *i2c_id)
> {
>         return 0;
> }
> ...
> static struct i2c_driver sataphy_i2c_driver = {
> ...
> };
> ...
> static int __init exynos5250_phy_i2c_init(void)
> {
>         return i2c_add_driver(&sataphy_i2c_driver);
> }
> ...
> module_init(exynos5250_phy_i2c_init);
> ...
Thanks for the explaination.Will change as above.
>
> BTW For consistency with the new naming it would be good to rename
> exynos5250_phy_i2c.c to phy-exynos5250-sata-i2c.c.
Sure,will rename it.
>
>> > (which is not present currently) instead of being registered by
>> > the SATA PHY driver.
>> >
>> > The SATA PHY driver should find i2c client device using DT.
>> >
>> > sataphy_attach_i2c_client() should then be removed.
>> >
>> >> > If you manage to extract i2c adapter and address data from the device
>> >> > tree (using proper of_* methods) they can be used instead of i2c client
>> >> > data in the SATA PHY driver.
>> >> I think the above is true, if the complete SATA PHY controller sits on
>> >> the i2c adapter.
>> >> But in Exynos5250 case,only the few configurations ( CMU and TRSV
>> >> blocks ) SATA PHY
>> >> are done through I2C(channel 9). Correct me if i am wrong.
>> >
>> > Well, it shouldn't matter if all or only some of the configuration of
>> > the SATA PHY controller is done through i2c.
>> >
>> > Anyway, how about another idea -> adding a new phandle of i2c client
>> > device to SATA PHY DT entry and using DT in the SATA PHY driver to
>> > find i2c client device?
>> >
>> > i.e. "samsung,exynos-sataphy-i2c-phandle" property in SATA PHY
>> > controller DT entry can point at existing "sata-phy at 38" sataphy i2c
>> > client DT entry (by adding new label to it, i.e. "sata_phy_i2c").
>> >
>> > Such new phandle can be used first to find the DT device node of i2c
>> > device (by using of_parse_phandle(), if it returns NULL the error
>> > should be returned) and then later to find proper i2c client device
>> > (by using of_find_i2c_device_by_node(), if it returns NULL deferred
>> > probing should be requested by returning -EPROBE_DEFER).
>> I can get the i2c_client structure,but how the client driver binding
>> happens to registered device?
>> Currently with this approach i2c client device is being registered but
>> cleint driver could not able to bind with the device.
>
> Could you please explain more what the problem is?  What is the new
> code exacly and what is the difference in the kernel logs?
I misunderstood your previous comments.Now its clarified.
I have addressed these comments and posted V5.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> [    0.082680] i2c i2c-9: adapter [s3c2410-i2c] registered
>> [    0.082691] i2c i2c-9: of_i2c: walking child nodes
>> [    0.082696] i2c i2c-9: of_i2c: register /i2c at 121D0000/sata-phy at 38
>> [    0.082794] i2c 9-0038: uevent
>> [    0.082845] i2c i2c-9: client [exynos-sataphy-i2c] registered with
>> bus id 9-0038
>> [    0.082851] s3c-i2c 121d0000.i2c: i2c-9: S3C I2C adapter
>> >
>> > i2c at 121D0000 {
>> >         ...
>> >         sata_phy_i2c: sata-phy at 38 {
>> >                 ...
>> >         };
>> >         ...
>> > };
>> >
>> > sata_phy: sata-phy at 12170000 {
>> >         ...
>> >         samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
>> >         ...
>> > };
>> >
>> > Best regards,
>> > --
>> > Bartlomiej Zolnierkiewicz
>> > Samsung R&D Institute Poland
>> > Samsung Electronics
>