Hi,
On Mon, Dec 02, 2013 at 04:20:51PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 28 November 2013 09:29 PM, Heikki Krogerus wrote:
> > ULPI PHY is an USB2 PHY that is accessed from the USB
> > controller. ULPI PHYs allow discovery based on vendor and
> > product ids which allows binding the PHY to a driver.
> >
> > For USB controllers that are enumerated from buses such as
> > PCI, that do not provide any information about the PHY, ULPI
> > abstraction layer allows runtime detection. This makes it
> > possible to take advantage of vendor specific functions of
> > the PHYs with product specific drivers without the need for
> > platform or device specific quirks.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/phy/Kconfig | 2 +
> > drivers/phy/Makefile | 1 +
> > drivers/phy/ulpi/Kconfig | 11 ++
> > drivers/phy/ulpi/Makefile | 1 +
> > drivers/phy/ulpi/ulpi.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/phy/ulpi.h | 105 ++++++++++++++++++
> > 6 files changed, 393 insertions(+)
> > create mode 100644 drivers/phy/ulpi/Kconfig
> > create mode 100644 drivers/phy/ulpi/Makefile
> > create mode 100644 drivers/phy/ulpi/ulpi.c
> > create mode 100644 include/linux/phy/ulpi.h
> >
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index a344f3d..6c03824 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -51,4 +51,6 @@ config PHY_EXYNOS_DP_VIDEO
> > help
> > Support for Display Port PHY found on Samsung EXYNOS SoCs.
> >
> > +source "drivers/phy/ulpi/Kconfig"
> > +
> > endmenu
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index d0caae9..d6af605 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o
> > obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
> > obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
> > obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
> > +obj-$(CONFIG_ULPI_PHY) += ulpi/
> > diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
> > new file mode 100644
> > index 0000000..3211aaa
> > --- /dev/null
> > +++ b/drivers/phy/ulpi/Kconfig
> > @@ -0,0 +1,11 @@
> > +
> > +config ULPI_PHY
> > + tristate "USB ULPI PHY interface support"
> > + depends on USB || USB_GADGET
> > + select GENERIC_PHY
> > + help
> > + Say yes if you have ULPI PHY attached to your USB controller. If no
> > + vendor modules are selected, the driver will act as NOP PHY driver.
> > +
> > + If unsure, say Y.
> > +
> > diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
> > new file mode 100644
> > index 0000000..7ed0895
> > --- /dev/null
> > +++ b/drivers/phy/ulpi/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ULPI_PHY) += ulpi.o
> > diff --git a/drivers/phy/ulpi/ulpi.c b/drivers/phy/ulpi/ulpi.c
> > new file mode 100644
> > index 0000000..7aa2f5d
> > --- /dev/null
> > +++ b/drivers/phy/ulpi/ulpi.c
> > @@ -0,0 +1,273 @@
> > +/**
> > + * ulpi.c - USB ULPI PHY abstraction module
> > + *
> > + * Copyright (C) 2013 Intel Corporation
> > + *
> > + * Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include <linux/phy/ulpi.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +static struct phy_consumer phy_consumer[] = {
> > + { .port = ULPI_PORT_NAME, },
>
> We had to introduce the entire phy_consumer stuff to support legacy pdata. Not
> sure if it is a good idea to use it here.
Well, maybe we can improve it at least a bit. I don't see how it's
possible to get rid of the device name matching, but at least the port
name dependency could be replaced with possibility to use index
instead. Check how they made the gpiod lookup to work in gpiolib.
Let's get back to this one.
> > +};
> > +
> > +static struct phy_init_data phy_data = {
> > + .num_consumers = 1,
> > + .consumers = phy_consumer,
> > +};
> > +
> > +static int ulpi_power_on(struct phy *phy)
> > +{
> > + struct ulpi_dev *ulpi = phy_get_drvdata(phy);
> > + struct ulpi_driver *drv = to_ulpi_driver(ulpi->dev.driver);
> > +
> > + if (drv && drv->phy_ops && drv->phy_ops->power_on)
> > + return drv->phy_ops->power_on(phy);
> > +
> > + return 0;
> > +}
> > +
> > +static int ulpi_power_off(struct phy *phy)
> > +{
> > + struct ulpi_dev *ulpi = phy_get_drvdata(phy);
> > + struct ulpi_driver *drv = to_ulpi_driver(ulpi->dev.driver);
> > +
> > + if (drv && drv->phy_ops && drv->phy_ops->power_off)
> > + return drv->phy_ops->power_off(phy);
> > +
> > + return 0;
> > +}
> > +
> > +static struct phy_ops phy_ops = {
> > + .owner = THIS_MODULE,
> > + .power_on = ulpi_power_on,
> > + .power_off = ulpi_power_off,
> > +};
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +static int ulpi_match(struct device *dev, struct device_driver *driver)
> > +{
> > + struct ulpi_driver *drv = to_ulpi_driver(driver);
> > + struct ulpi_dev *ulpi = to_ulpi_dev(dev);
> > + const struct ulpi_device_id *id;
> > +
> > + for (id = drv->id_table; id->vendor; id++)
> > + if (id->vendor == ulpi->id.vendor &&
> > + id->product == ulpi->id.product)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int ulpi_probe(struct device *dev)
> > +{
> > + struct ulpi_driver *drv = to_ulpi_driver(dev->driver);
> > +
> > + return drv->probe(to_ulpi_dev(dev));
> > +}
> > +
> > +static int ulpi_remove(struct device *dev)
> > +{
> > + struct ulpi_driver *drv = to_ulpi_driver(dev->driver);
> > +
> > + if (drv->remove)
> > + drv->remove(to_ulpi_dev(dev));
> > +
> > + return 0;
> > +}
> > +
> > +static struct bus_type ulpi_bus = {
> > + .name = "ulpi",
> > + .match = ulpi_match,
> > + .probe = ulpi_probe,
> > + .remove = ulpi_remove,
> > +};
> > +
> > +/**
> > + * ulpi_register_driver - register driver with the bus
> > + * @drv: the driver
> > + */
> > +int ulpi_register_driver(struct ulpi_driver *drv)
> > +{
> > + if (!drv->probe)
> > + return -EINVAL;
> > +
> > + drv->driver.bus = &ulpi_bus;
> > +
> > + return driver_register(&drv->driver);
> > +}
> > +EXPORT_SYMBOL_GPL(ulpi_register_driver);
> > +
> > +void ulpi_unregister_driver(struct ulpi_driver *drv)
> > +{
> > + driver_unregister(&drv->driver);
> > +}
> > +EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +/**
> > + * ulpi_alloc_interface - allocate new ULPI interface
> > + *
> > + * Allows USB controller to create struct ulpi_interface without
> > + * registering the interface immediately.
> > + */
> > +struct ulpi_interface *ulpi_alloc_interface(void)
> > +{
> > + struct ulpi_dev *ulpi;
> > +
> > + ulpi = kzalloc(sizeof(*ulpi), GFP_KERNEL);
> > + if (!ulpi)
> > + return NULL;
> > +
> > + return &ulpi->interface;
> > +}
> > +EXPORT_SYMBOL_GPL(ulpi_alloc_interface);
>
> Should this be exported?
What's wrong with it? It allows, what ever is providing the ULPI
interface, to get the handle to the interface structure in advance and
registering it later when it's ready.
This is the way many bus drivers handle adapters/masters/whatever.
If you feel strongly about this, we can simply only provide
ulpi_new_interface() as the only way to register an interface.
> > +
> > +#define interface_to_ulpi(c) container_of(c, struct ulpi_dev, interface)
> > +
> > +/**
> > + * ulpi_register - register new ULPI device
> > + * @dev: USB controller's device interface
> > + * @interface: Controller which the device is connected
> > + *
> > + * Companion function to ulpi_alloc_interface. Creates device for the
> > + * ULPI PHY attached to the interface, binds it to a driver and
> > + * creates phy instance for it.
> > + */
> > +int ulpi_register(struct device *dev, struct ulpi_interface *interface)
> > +{
> > + struct ulpi_dev *ulpi = interface_to_ulpi(interface);
> > + int ret;
> > +
> > + /* Test the interface */
> > + ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
> > + if (ret < 0)
> > + goto err;
> > +
> > + ret = ulpi_read(ulpi, ULPI_SCRATCH);
> > + if (ret < 0)
> > + goto err;
> > +
> > + if (ret != 0xaa) {
> > + ret = -ENODEV;
> > + goto err;
> > + }
> > +
> > + ulpi->id.vendor = ulpi_read(ulpi, ULPI_VENDOR_ID_LOW);
> > + ulpi->id.vendor |= ulpi_read(ulpi, ULPI_VENDOR_ID_HIGH) << 8;
> > +
> > + ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW);
> > + ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8;
> > +
> > + ulpi->dev.parent = dev;
> > + ulpi->dev.bus = &ulpi_bus;
> > + dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev));
> > +
> > + ret = device_register(&ulpi->dev);
> > + if (ret)
> > + goto err;
> > +
> > + /**
> > + * Create phy for the device.
> > + * The controller is always the consumer
> > + */
> > + phy_consumer[0].dev_name = dev_name(dev);
> > +
> > + ulpi->phy = phy_create(&ulpi->dev, &phy_ops, &phy_data);
>
> Instead of creating ulpi device and phy device here, phy device (phy_create)
> should be created when the driver is probed (in this case when NXP ISP170X is
> probed).
No. The phy should always be tied to the device and not to the
driver. The controller driver can not depend on a particular ULPI
driver. You want to allow the controller driver to probe and get the
phy before any ULPI drivers are loaded. Thats why it's called
abstraction.
> > + if (IS_ERR(ulpi->phy)) {
> > + ret = PTR_ERR(ulpi->phy);
> > + device_unregister(&ulpi->dev);
> > + goto err;
> > + }
> > +
> > + phy_set_drvdata(ulpi->phy, ulpi);
> > +
> > + dev_dbg(&ulpi->dev, "registered vendor %04x, product %04x\n",
> > + ulpi->id.vendor, ulpi->id.product);
> > +
> > + return 0;
> > +err:
> > + kfree(ulpi);
>
> It's difficult to follow if you have allocation in one function and free it in
> the error path of some other function. This can be improved.
So what do you have in mind?
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
