Re: [PATCH v4 1/4] usb: otg: Add ulpi viewport access ops

[Igor Grinberg <grinberg-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>]

Hi,

On 02/18/11 05:42, Benoit Goby wrote:
> Add generic access ops for controllers with a ulpi viewport register
> (e.g. Chipidea/ARC based controllers).

I can see mxc, msm and langwell are using the same bits
(or at least some of them).
Is there some kind of specification for this ulpi viewport register?
Where can we find it?
Or else, why do you call it generic?

Some minor neats below.

> Signed-off-by: Benoit Goby <benoit-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/otg/Kconfig         |    7 ++++
>  drivers/usb/otg/Makefile        |    1 +
>  drivers/usb/otg/ulpi_viewport.c |   77 +++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/ulpi.h        |    5 +++
>  4 files changed, 90 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/usb/otg/ulpi_viewport.c
>
> diff --git a/drivers/usb/otg/Kconfig b/drivers/usb/otg/Kconfig
> index 9ffc823..7702ee6 100644
> --- a/drivers/usb/otg/Kconfig
> +++ b/drivers/usb/otg/Kconfig
> @@ -49,6 +49,13 @@ config USB_ULPI
>  	  Enable this to support ULPI connected USB OTG transceivers which
>  	  are likely found on embedded boards.
>  
> +config USB_ULPI_VIEWPORT
> +	bool
> +	depends on USB_ULPI
> +	help
> +	  Provides read/write operations to the ULPI phy register set for
> +	  controllers with a viewport register (e.g. Chipidea/ARC controllers).
> +
>  config TWL4030_USB
>  	tristate "TWL4030 USB Transceiver Driver"
>  	depends on TWL4030_CORE && REGULATOR_TWL4030
> diff --git a/drivers/usb/otg/Makefile b/drivers/usb/otg/Makefile
> index a520e71..44581a9 100644
> --- a/drivers/usb/otg/Makefile
> +++ b/drivers/usb/otg/Makefile
> @@ -16,5 +16,6 @@ obj-$(CONFIG_TWL6030_USB)	+= twl6030-usb.o
>  obj-$(CONFIG_USB_LANGWELL_OTG)	+= langwell_otg.o
>  obj-$(CONFIG_NOP_USB_XCEIV)	+= nop-usb-xceiv.o
>  obj-$(CONFIG_USB_ULPI)		+= ulpi.o
> +obj-$(CONFIG_USB_ULPI_VIEWPORT)	+= ulpi_viewport.o
>  obj-$(CONFIG_USB_MSM_OTG_72K)	+= msm72k_otg.o
>  obj-$(CONFIG_AB8500_USB)	+= ab8500-usb.o
> diff --git a/drivers/usb/otg/ulpi_viewport.c b/drivers/usb/otg/ulpi_viewport.c
> new file mode 100644
> index 0000000..dd5dfae2
> --- /dev/null
> +++ b/drivers/usb/otg/ulpi_viewport.c
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/usb.h>
> +#include <linux/io.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/ulpi.h>
> +
> +#define ULPI_VIEW_WAKEUP	(1 << 31)
> +#define ULPI_VIEW_RUN		(1 << 30)
> +#define ULPI_VIEW_WRITE		(1 << 29)
> +#define ULPI_VIEW_READ		(0 << 29)
> +#define ULPI_VIEW_ADDR(x)	(((x) & 0xff) << 16)
> +#define ULPI_VIEW_DATA_READ(x)	(((x) >> 8) & 0xff)
> +#define ULPI_VIEW_DATA_WRITE(x)	(((x) & 0xff) << 0)

Please, remove this "zero shift", I don't think it serves any good cause.

> +
> +static int ulpi_viewport_wait(void __iomem *view, u32 mask, u32 result)
> +{
> +	unsigned long usec = 2000;
> +
> +	while (usec--) {
> +		if ((readl(view) & mask) == result)

You always send 0 in the result variable.
Can't the result variable be dropped by:

if (!(readl(view) & mask))
        return 0;

or:

if (readl(view) & mask != mask)
        return 0;

> +			return 0;

Empty line here would be nice.

> +		udelay(1);
> +	};

same here

> +	return -ETIMEDOUT;
> +}
> +
> +static int ulpi_viewport_read(struct otg_transceiver *otg, u32 reg)
> +{
> +	int ret;
> +	void __iomem *view = otg->io_priv;
> +
> +	writel(ULPI_VIEW_WAKEUP | ULPI_VIEW_WRITE, view);
> +	ret = ulpi_viewport_wait(view, ULPI_VIEW_WAKEUP, 0);
> +	if (ret)
> +		return ret;
> +
> +	writel(ULPI_VIEW_RUN | ULPI_VIEW_READ | ULPI_VIEW_ADDR(reg), view);
> +	ret = ulpi_viewport_wait(view, ULPI_VIEW_RUN, 0);
> +	if (ret)
> +		return ret;
> +
> +	return ULPI_VIEW_DATA_READ(readl(view));
> +}
> +
> +static int ulpi_viewport_write(struct otg_transceiver *otg, u32 val, u32 reg)
> +{
> +	int ret;
> +	void __iomem *view = otg->io_priv;
> +
> +	writel(ULPI_VIEW_WAKEUP | ULPI_VIEW_WRITE, view);
> +	ret = ulpi_viewport_wait(view, ULPI_VIEW_WAKEUP, 0);
> +	if (ret)
> +		return ret;
> +
> +	writel(ULPI_VIEW_RUN | ULPI_VIEW_WRITE | ULPI_VIEW_DATA_WRITE(val) |
> +						 ULPI_VIEW_ADDR(reg), view);

and here

> +	return ulpi_viewport_wait(view, ULPI_VIEW_RUN, 0);
> +}
> +
> +struct otg_io_access_ops ulpi_viewport_access_ops = {
> +	.read	= ulpi_viewport_read,
> +	.write	= ulpi_viewport_write,
> +};
> diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
> index 82b1507..9595796 100644
> --- a/include/linux/usb/ulpi.h
> +++ b/include/linux/usb/ulpi.h
> @@ -184,4 +184,9 @@
>  struct otg_transceiver *otg_ulpi_create(struct otg_io_access_ops *ops,
>  					unsigned int flags);
>  
> +#ifdef CONFIG_USB_ULPI_VIEWPORT
> +/* access ops for controllers with a viewport register */
> +extern struct otg_io_access_ops ulpi_viewport_access_ops;
> +#endif
> +
>  #endif /* __LINUX_USB_ULPI_H */

-- 
Regards,
Igor.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html