On 09/06/2012 04:46 PM, Felipe Balbi wrote: > Hi, > > We should at least add Grant to the loop, I guess. > > On Thu, Sep 06, 2012 at 10:46:13PM +0800, Richard Zhao wrote: >> On Thu, Sep 06, 2012 at 05:30:02PM +0300, Felipe Balbi wrote: >>> On Tue, Sep 04, 2012 at 09:32:06PM +0200, Marc Kleine-Budde wrote: >>>> On 09/04/2012 07:51 PM, ABRAHAM, KISHON VIJAY wrote: >>>>>>>>>>>>>>> Since it's already a common function, we may give phandler >>>>>>>>>>>>>>> property >>>>>>>>>>>>>>> a common name too. So we will not need phandle argument. >>>>>>>>>>>>>>> Please also don't forget to document the devm_xxx and dt >>>>>>>>>>>>>>> binding. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I don't think this is a good idea. If we hardcode the phandle >>>>>>>>>>>>>> name, we >>>>>>>>>>>>>> introduce a limit of one phy per usb device. The usb3 controllers >>>>>>>>>>>>>> alreadyt use two phys (one for usb2, the othere for usb3) for one >>>>>>>>>>>>>> controller. So I think we should not make the same mistake again. >>>>>>>>>>>> That only means current binding is not good enough. Rather not, >>>>>>>>>>>> means >>>>>>>>>>>> it should not be in common code. >>>>>>>>>>>> Maybe something like: >>>>>>>>>>>> usbport0-phys = <&phy0>; >>>>>>>>>>>> usbport1-phys = <&phy1 &phy2>; /* usb2 & usb3 */ >>>>>>>>>>> >>>>>>>>>>> Granted. Do we need strings that describe the phys, like in pinctrl >>>>>>>>>>> or >>>>>>>>>>> is a index enough? What about this? >>>>>>>>>>> >>>>>>>>>>> struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, >>>>>>>>>>> int index) >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Comments? The phandle_name string will be "usbphy". >>>>>>>>> >>>>>>>>> I don't think phandle_name should be usbphy. Eventually we want to >>>>>>>>> turn >>>>>>>>> this into a kernel-wide phy subsystem and if we use usbphy, we will >>>>>>>>> just >>>>>>>>> have to patch a bunch of dts files once we make the move. >>>>>>> Coud you please give a link of "kernel-wide phy subsystem" discussion? >>>>>>>> >>>>>>>> Is just "phy" better? >>>>>>> If the property name don't include port number, how do we know what >>>>>>> port the phy is attached to? >>>>> >>>>> We can use something like "xxxx-phy" as the phandle name. And the >>>>> users can get the phy by using >>>>> devm_usb_get_phy_by_phandle(dev, "xxxx"). >>>>> (So the frwrk appends *-phy* to the name and searches). Or we don't >>>>> have any restriction on the phandle naming conventions and search for >>>>> the phandle by the name the user passes in devm_usb_get_phy_by_phandle >>>>> directly. >>>> >>>> Maintainer, I need a Maintainer. Can someone please decide what we want >>>> to have here. I can code all that, but please someone has to make a >>>> decision. Now, please. >>> >>> Like I said on another reply: >>> >>> phyN (phy1, phy2, ... phyN) is better since eventually we want to turn >>> this into a kernel-wide PHY layer. >> I think Marc is wondering how to handle the below two case in such way. >> - how to get the port number the phy is attached to >> - how to describe it if a port has two phys. > > Would something like below work ? > > phy = <&phy1 0 &phy2 1>; > > where the first attribute is a phandle to the phy and the second is a > port number ?
We have two (and some brain damaged) use cases:
a) USB2/USB3:
One USB controller has two phys, one for USB2 the other for USB3.
The hardware has only one USB jack.
b) IMHO "poorly abstracted" USB devices, where there are actually two
USB ports per "device", there are two USB jacks on the hardware.
c) mixture of a)+b)
My current code (patch not yet published) implements:
struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
int index)
With a hard coded "phy" phandle string and a index to that phandle. Your
above example looks like this (taking 0 as base):
DT:
phy = <&phy0 &phy1>;
c-code:
phy0 = *devm_usb_get_phy_by_phandle(struct device *dev, 0)
phy1 = *devm_usb_get_phy_by_phandle(struct device *dev, 1)
This solves use case:
a) The driver must know index 0 -> USB2 phy, index 1 -> USB3 phy
b) The driver must know index 0 -> phy of 1st usb port,
index 1 -> phy of 2nd USB port
But c) is not so easy:
c) driver must know how many combined USB2/USB3 jacks and USB2 only
jack the hardware has.
My solution fails for b) and thus c) if one of the ports (e.g. the first
one is/should not be used). In my ideal world of proper abstracted
devices each USB port would have its own device entry in the DT and
would just stay "disabled". Your solution is superior here. But
struggles at c), too.
Both solutions lack to describe what kind of phy it is.
I'm now off for holidays, and back in the office on the 12th of September.
Regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
