linux-kernel - Re: [PATCH V4 1/2] Phy: Exynos: Add Exynos5250 sata phy driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAKuRcOJMihCoZk3+fvsRPZTiCDPqh2q3UaGxckNG30W_T5_O0g@mail.gmail.com>
Date:	Thu, 9 Jan 2014 15:36:28 +0530
From:	Yuvaraj Kumar <yuvaraj.cd@...il.com>
To:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.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 Wed, Jan 8, 2014 at 9:16 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@...sung.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@...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);
> ...
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@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@...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/