Messages in this thread | | | Date | Tue, 9 Jul 2013 11:10:10 +0530 | From | George Cherian <> | Subject | Re: [PATCH 0/5] Add phy support for AM335X platform using Generic PHy framework |
| |
On 7/9/2013 1:14 AM, Sebastian Andrzej Siewior wrote: > On 07/08/2013 12:43 PM, George Cherian wrote: >> This patch series adds phy support for AM335X platform. >> This patch series is based on Generic PHY framework [1]. >> >> >> This series has >> - adds dual musb instances support for am335x platform (just for testing) >> - adds phy-amxxxx-usb driver used in AMxxxx platforms >> - adds dt bindings for the phys >> - removes usb-phy and replaced with generic phy apis in glue layer > No, I don't like this all. You did the one thing I tried to avoid while > posting my quick-and-dirty phy driver recently: You duplicated a lot of > code which can be served by the nop driver and added only power > on/power off callbacks. I wanted to add phy wakeup control also, but currently phy_ops dont have an op for wkup_ctrl Kishon, Can we add one? > In numbers: >> 7 files changed, 316 insertions(+), 70 deletions(-) > vs > 2 files changed, 117 insertions, 12 deletions > > I assumed you had also the OTG callbacks (set host/device mode) and > wake up but I don't see it there. > Adding a power regulator would do the same job, wouldn't it? If the phy > driver remains just doing power on/off I suggest simply adding a power > regulator. If it will do more I would move the am35xx specific bits > into a separate file and glue it to the nop driver. > > What else? The abstraction in device tree is wrong. It remains wrong if > add stuff on top to it. Yes definitely , I liked the way you split the device node. Probably I should base on your dt patch and add phy nodes. > We need two nodes each one with a glue layer and a musb child node. The > instances crap in kernel has to vanish. Same as above I just did it for quick testing (mentioned in the commit log). > Also that means your phy nodes > are wrong. This is not musb with two ports but two musb instances each > with one port. yes true. > Sebastian
| |