Message-id: <2147414.aVD9xOxI7n@amdc1032>
Date:	Wed, 08 Jan 2014 16:46:52 +0100
From:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To:	Yuvaraj Kumar <yuvaraj.cd@...il.com>
Cc:	Kishon Vijay Abraham I <kishon@...com>,
	"kgene.kim@...sung.com" <kgene.kim@...sung.com>,
	linux-kernel@...r.kernel.org,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-doc@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
	Jingoo Han <jg1.han@...sung.com>,
	sunil joshi <joshi@...sung.com>,
	Tomasz Figa <t.figa@...sung.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Grant Likely <grant.likely@...aro.org>,
	Christoffer Dall <christoffer.dall@...aro.org>,
	Yuvaraj Kumar C D <yuvaraj.cd@...sung.com>,
	Girish K S <ks.giri@...sung.com>,
	Vasanth Ananthan <vasanth.a@...sung.com>,
	linux-ide@...r.kernel.org
Subject: Re: [PATCH V4 1/2] Phy: Exynos: Add Exynos5250 sata phy driver

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@...sung.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@...sung.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);
...

BTW For consistency with the new naming it would be good to rename
exynos5250_phy_i2c.c to phy-exynos5250-sata-i2c.c.

> > (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@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?

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@...D0000/sata-phy@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@...D0000 {
> >         ...
> >         sata_phy_i2c: sata-phy@38 {
> >                 ...
> >         };
> >         ...
> > };
> >
> > sata_phy: sata-phy@...70000 {
> >         ...
> >         samsung,exynos-sataphy-i2c-phandle = <&sata_phy_i2c>;
> >         ...
> > };
> >
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics

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