| Message ID | 20160627143422.GB21398@kuha.fi.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote: > Quoting Heikki Krogerus (2016-06-27 07:34:22) > > Hi, > > > > I'm fine with most of the patch, except.. > > > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > > > @@ -39,7 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > > > struct ulpi *ulpi = to_ulpi_dev(dev); > > > const struct ulpi_device_id *id; > > > > > > - for (id = drv->id_table; id->vendor; id++) > > > + if (of_driver_match_device(dev, driver)) > > > + return 1; > > > > I don't like this part. We should match separately like that only > > if the bus does not support native enumeration, and of course ULPI > > with its vendor and product IDs does. There really should always be > > IDs to match with here. So exceptions have to be solved before we > > attempt matching. > > > > Since we also have to support platforms where the PHY is initially > > powered off and reading the IDs from the registers is not possible > > because of that, I think we should consider getting the product and > > vendor IDs optionally from device properties. Something like this: > > Ok, I'm a little worried about conflating the powered off problem with > this product/vendor ID missing problem. But if you're ok with that I'll > combine the two patches into one using your approach below. > > > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > > index 01c0c04..6228a85 100644 > > --- a/drivers/usb/common/ulpi.c > > +++ b/drivers/usb/common/ulpi.c > > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver); > > > > /* -------------------------------------------------------------------------- */ > > > > -static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > +static int ulpi_read_id(struct ulpi *ulpi) > > { > > int ret; > > > > @@ -174,6 +174,21 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); > > ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; > > > > + return 0; > > +} > > + > > +static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > +{ > > + int ret; > > + > > + ret = device_property_read_u16(dev, "ulpi-vendor", &ulpi->id.vendor); > > + ret |= device_property_read_u16(dev, "ulpi-product", &ulpi->id.product); > > + if (ret) { > > + ret = ulpi_read_id(ulpi); > > + if (ret) > > + return ret; > > + } > > + > > ulpi->dev.parent = dev; > > ulpi->dev.bus = &ulpi_bus; > > ulpi->dev.type = &ulpi_dev_type; > > > > > > That should cover both cases. You would just have to create the IDs > > yourself in this case. > > > > Right, I would have to make up some IDs in this case. I suppose I can > use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy > and HSIC phy? That doesn't make me feel great because it's all made up, > but I guess there's no other option. I hope they don't decide to start > populating these ids in the future though and then we may have > conflicting product ids. If that happens I suppose we can do a > workaround based on compatible strings in the DT node. Fun! > > Nice side effect of all that is I can drop requesting the module by DT > aliases and things become simpler. I'll try this out. I was hoping that we could manage with product id 0 as an exception (I failed to consider that you have multiple PHYs to deal with). I don't think we can just come up with product id > 0. I guess we should have the of_driver_match_device() call after all. Let's just call it conditionally, only in cases where there is no product ID, to make me feel a bit more better. I don't want to make it too easy to use. The properties for the vendor and product ID are still something that we need to introduce in any case. We have the powered off problem on all kinds of platforms, and not all of them use DT. Please feel free to incorporate the diff into the patch you had for the powered off case if you are OK with it. So I think in your case you would just need to addthe correct ulpi-vendor id 0x05c6 and ulpi-product id 0 to the chipidea device node, and I think this would work. Sorry about the hassle. Thanks,
On Tue, Jun 28, 2016 at 02:42:05PM +0300, Heikki Krogerus wrote: > On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote: > > Quoting Heikki Krogerus (2016-06-27 07:34:22) > > > Hi, > > > > > > I'm fine with most of the patch, except.. > > > > > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > > > > @@ -39,7 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > > > > struct ulpi *ulpi = to_ulpi_dev(dev); > > > > const struct ulpi_device_id *id; > > > > > > > > - for (id = drv->id_table; id->vendor; id++) > > > > + if (of_driver_match_device(dev, driver)) > > > > + return 1; > > > > > > I don't like this part. We should match separately like that only > > > if the bus does not support native enumeration, and of course ULPI > > > with its vendor and product IDs does. There really should always be > > > IDs to match with here. So exceptions have to be solved before we > > > attempt matching. > > > > > > Since we also have to support platforms where the PHY is initially > > > powered off and reading the IDs from the registers is not possible > > > because of that, I think we should consider getting the product and > > > vendor IDs optionally from device properties. Something like this: > > > > Ok, I'm a little worried about conflating the powered off problem with > > this product/vendor ID missing problem. But if you're ok with that I'll > > combine the two patches into one using your approach below. > > > > > > > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > > > index 01c0c04..6228a85 100644 > > > --- a/drivers/usb/common/ulpi.c > > > +++ b/drivers/usb/common/ulpi.c > > > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver); > > > > > > /* -------------------------------------------------------------------------- */ > > > > > > -static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > > +static int ulpi_read_id(struct ulpi *ulpi) > > > { > > > int ret; > > > > > > @@ -174,6 +174,21 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > > ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); > > > ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; > > > > > > + return 0; > > > +} > > > + > > > +static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > > +{ > > > + int ret; > > > + > > > + ret = device_property_read_u16(dev, "ulpi-vendor", &ulpi->id.vendor); > > > + ret |= device_property_read_u16(dev, "ulpi-product", &ulpi->id.product); > > > + if (ret) { > > > + ret = ulpi_read_id(ulpi); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > ulpi->dev.parent = dev; > > > ulpi->dev.bus = &ulpi_bus; > > > ulpi->dev.type = &ulpi_dev_type; > > > > > > > > > That should cover both cases. You would just have to create the IDs > > > yourself in this case. > > > > > > > Right, I would have to make up some IDs in this case. I suppose I can > > use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy > > and HSIC phy? That doesn't make me feel great because it's all made up, > > but I guess there's no other option. I hope they don't decide to start > > populating these ids in the future though and then we may have > > conflicting product ids. If that happens I suppose we can do a > > workaround based on compatible strings in the DT node. Fun! > > > > Nice side effect of all that is I can drop requesting the module by DT > > aliases and things become simpler. I'll try this out. > > I was hoping that we could manage with product id 0 as an exception (I > failed to consider that you have multiple PHYs to deal with). I don't > think we can just come up with product id > 0. > > I guess we should have the of_driver_match_device() call after all. > Let's just call it conditionally, only in cases where there is no > product ID, to make me feel a bit more better. I don't want to make it > too easy to use. > > The properties for the vendor and product ID are still something that > we need to introduce in any case. We have the powered off problem on > all kinds of platforms, and not all of them use DT. I am thinking power sequence framework, how power sequence elements (eg, clock, reset-gpios) can get for non-DT platform? Does ACPI does power sequence for x86 platforms?
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index 01c0c04..6228a85 100644 --- a/drivers/usb/common/ulpi.c +++ b/drivers/usb/common/ulpi.c @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver); /* -------------------------------------------------------------------------- */ -static int ulpi_register(struct device *dev, struct ulpi *ulpi) +static int ulpi_read_id(struct ulpi *ulpi) { int ret; @@ -174,6 +174,21 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; + return 0; +} + +static int ulpi_register(struct device *dev, struct ulpi *ulpi) +{ + int ret; + + ret = device_property_read_u16(dev, "ulpi-vendor", &ulpi->id.vendor); + ret |= device_property_read_u16(dev, "ulpi-product", &ulpi->id.product); + if (ret) { + ret = ulpi_read_id(ulpi); + if (ret) + return ret; + } + ulpi->dev.parent = dev; ulpi->dev.bus = &ulpi_bus; ulpi->dev.type = &ulpi_dev_type;