Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

On Tue, Jun 14, 2016 at 08:37:14PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-06-14 at 10:26 -0700, David Cohen wrote:
> 
> > > Yes. Will rename in this way, re-test and re-send.
> > 
> > I'm curious about that. What pmu prefix would stand for?
> 
> It's indeed widely used as Performance Monitoring Unit

Thanks :)

Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

On Tue, Jun 14, 2016 at 08:37:14PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-06-14 at 10:26 -0700, David Cohen wrote:
> 
> > > Yes. Will rename in this way, re-test and re-send.
> > 
> > I'm curious about that. What pmu prefix would stand for?
> 
> It's indeed widely used as Performance Monitoring Unit

Thanks :)

Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

On Tue, Jun 14, 2016 at 10:26:21AM -0700, David Cohen wrote:
> Hi,
> 
> On Tue, Jun 14, 2016 at 07:07:14PM +0300, Andy Shevchenko wrote:
> > On Tue, 2016-06-14 at 17:58 +0200, Ingo Molnar wrote:
> > > * Andy Shevchenko <andriy.shevche...@linux.intel.com> wrote:
> > > 
> > > > On Tue, 2016-06-14 at 17:29 +0200, Ingo Molnar wrote:
> > > > 
> > > > > * Andy Shevchenko <andriy.shevche...@linux.intel.com> wrote:
> > > > > 
> > > > > > On Tue, 2016-06-14 at 12:43 +0200, Ingo Molnar wrote:
> > > > > > In the TRM it's called Power Management Unit, though once or
> > > > > > twice
> > > > > > in some 
> > > > > > documents as Power Management Controller. I actually woudn't
> > > > > > like to
> > > > > > use PMC 
> > > > > > abbreviation to not be confused with pmc_atom.c and many other
> > > > > > variation of 
> > > > > > existing PMC drivers of other Intel platforms.
> > > > > > 
> > > > > > PM* as a prefix might be too short to conflict with Power
> > > > > > Management
> > > > > > framework 
> > > > > > in the kernel. P-Unit (punit*) is existing part in SoC which
> > > > > > will
> > > > > > have its own 
> > > > > > driver in the future, so, can't use it either.
> > > > > > 
> > > > > > pwr*, pwrmu*, scpmu* (as of South Complex Power Management Unit)
> > > > > > —
> > > > > > one of them?
> > > > > 
> > > > > 'pwr' certainly sounds good to me! PWMU perhaps?
> > > > 
> > > > Wouldn't be a bit confusing with pwm? I would stay at 'pwr'.
> > > 
> > > Yeah, indeed - so pwr it is?
> > 
> > Yes. Will rename in this way, re-test and re-send.
> 
> I'm curious about that. What pmu prefix would stand for?

Nevermind :)
I'd vote for pwmu, as it would make more sense WRT the hw
documentation.

Br, David

> 
> Br, David

Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

On Tue, Jun 14, 2016 at 10:26:21AM -0700, David Cohen wrote:
> Hi,
> 
> On Tue, Jun 14, 2016 at 07:07:14PM +0300, Andy Shevchenko wrote:
> > On Tue, 2016-06-14 at 17:58 +0200, Ingo Molnar wrote:
> > > * Andy Shevchenko  wrote:
> > > 
> > > > On Tue, 2016-06-14 at 17:29 +0200, Ingo Molnar wrote:
> > > > 
> > > > > * Andy Shevchenko  wrote:
> > > > > 
> > > > > > On Tue, 2016-06-14 at 12:43 +0200, Ingo Molnar wrote:
> > > > > > In the TRM it's called Power Management Unit, though once or
> > > > > > twice
> > > > > > in some 
> > > > > > documents as Power Management Controller. I actually woudn't
> > > > > > like to
> > > > > > use PMC 
> > > > > > abbreviation to not be confused with pmc_atom.c and many other
> > > > > > variation of 
> > > > > > existing PMC drivers of other Intel platforms.
> > > > > > 
> > > > > > PM* as a prefix might be too short to conflict with Power
> > > > > > Management
> > > > > > framework 
> > > > > > in the kernel. P-Unit (punit*) is existing part in SoC which
> > > > > > will
> > > > > > have its own 
> > > > > > driver in the future, so, can't use it either.
> > > > > > 
> > > > > > pwr*, pwrmu*, scpmu* (as of South Complex Power Management Unit)
> > > > > > —
> > > > > > one of them?
> > > > > 
> > > > > 'pwr' certainly sounds good to me! PWMU perhaps?
> > > > 
> > > > Wouldn't be a bit confusing with pwm? I would stay at 'pwr'.
> > > 
> > > Yeah, indeed - so pwr it is?
> > 
> > Yes. Will rename in this way, re-test and re-send.
> 
> I'm curious about that. What pmu prefix would stand for?

Nevermind :)
I'd vote for pwmu, as it would make more sense WRT the hw
documentation.

Br, David

> 
> Br, David

Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

Hi,

On Tue, Jun 14, 2016 at 07:07:14PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-06-14 at 17:58 +0200, Ingo Molnar wrote:
> > * Andy Shevchenko  wrote:
> > 
> > > On Tue, 2016-06-14 at 17:29 +0200, Ingo Molnar wrote:
> > > 
> > > > * Andy Shevchenko  wrote:
> > > > 
> > > > > On Tue, 2016-06-14 at 12:43 +0200, Ingo Molnar wrote:
> > > > > In the TRM it's called Power Management Unit, though once or
> > > > > twice
> > > > > in some 
> > > > > documents as Power Management Controller. I actually woudn't
> > > > > like to
> > > > > use PMC 
> > > > > abbreviation to not be confused with pmc_atom.c and many other
> > > > > variation of 
> > > > > existing PMC drivers of other Intel platforms.
> > > > > 
> > > > > PM* as a prefix might be too short to conflict with Power
> > > > > Management
> > > > > framework 
> > > > > in the kernel. P-Unit (punit*) is existing part in SoC which
> > > > > will
> > > > > have its own 
> > > > > driver in the future, so, can't use it either.
> > > > > 
> > > > > pwr*, pwrmu*, scpmu* (as of South Complex Power Management Unit)
> > > > > —
> > > > > one of them?
> > > > 
> > > > 'pwr' certainly sounds good to me! PWMU perhaps?
> > > 
> > > Wouldn't be a bit confusing with pwm? I would stay at 'pwr'.
> > 
> > Yeah, indeed - so pwr it is?
> 
> Yes. Will rename in this way, re-test and re-send.

I'm curious about that. What pmu prefix would stand for?

Br, David

Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[David Cohen]

Hi,

On Tue, Jun 14, 2016 at 07:07:14PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-06-14 at 17:58 +0200, Ingo Molnar wrote:
> > * Andy Shevchenko  wrote:
> > 
> > > On Tue, 2016-06-14 at 17:29 +0200, Ingo Molnar wrote:
> > > 
> > > > * Andy Shevchenko  wrote:
> > > > 
> > > > > On Tue, 2016-06-14 at 12:43 +0200, Ingo Molnar wrote:
> > > > > In the TRM it's called Power Management Unit, though once or
> > > > > twice
> > > > > in some 
> > > > > documents as Power Management Controller. I actually woudn't
> > > > > like to
> > > > > use PMC 
> > > > > abbreviation to not be confused with pmc_atom.c and many other
> > > > > variation of 
> > > > > existing PMC drivers of other Intel platforms.
> > > > > 
> > > > > PM* as a prefix might be too short to conflict with Power
> > > > > Management
> > > > > framework 
> > > > > in the kernel. P-Unit (punit*) is existing part in SoC which
> > > > > will
> > > > > have its own 
> > > > > driver in the future, so, can't use it either.
> > > > > 
> > > > > pwr*, pwrmu*, scpmu* (as of South Complex Power Management Unit)
> > > > > —
> > > > > one of them?
> > > > 
> > > > 'pwr' certainly sounds good to me! PWMU perhaps?
> > > 
> > > Wouldn't be a bit confusing with pwm? I would stay at 'pwr'.
> > 
> > Yeah, indeed - so pwr it is?
> 
> Yes. Will rename in this way, re-test and re-send.

I'm curious about that. What pmu prefix would stand for?

Br, David

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

On Wed, Dec 02, 2015 at 12:27:10PM +0200, Heikki Krogerus wrote:
> Hi David,
> 
> 
> 
> > > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > > +{
> > 
> > There are still 2 pending comments for this unregister function:
> > 
> > 1) How about a protection against unbalanced unregistering? In case an
> > user mistakenly unregisters twice or unregisters without a previous
> > registering.
> 
> True. You already pointed that out in our off-list thread, but I
> forgot. Sorry about that.
> 
> > 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> > set mux to automatic mode again. Or maybe you could save CFG0's initial
> > value and set it again here. Anyway, when unregistering the mux driver
> > you need to make sure the mux goes back to its original configuration.
> 
> This one is already been taken care of..
> 
> > > + extcon_unregister_notifier(>edev, EXTCON_USB_HOST, >nb);
> > > + extcon_dev_unregister(>edev);
> > > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> 
> 

Of course :)
Sorry I missed this part.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

On Wed, Dec 02, 2015 at 12:27:10PM +0200, Heikki Krogerus wrote:
> Hi David,
> 
> 
> 
> > > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > > +{
> > 
> > There are still 2 pending comments for this unregister function:
> > 
> > 1) How about a protection against unbalanced unregistering? In case an
> > user mistakenly unregisters twice or unregisters without a previous
> > registering.
> 
> True. You already pointed that out in our off-list thread, but I
> forgot. Sorry about that.
> 
> > 2) When unregistering, you need to clear the bit CFG0_SW_IDPIN_EN to
> > set mux to automatic mode again. Or maybe you could save CFG0's initial
> > value and set it again here. Anyway, when unregistering the mux driver
> > you need to make sure the mux goes back to its original configuration.
> 
> This one is already been taken care of..
> 
> > > + extcon_unregister_notifier(>edev, EXTCON_USB_HOST, >nb);
> > > + extcon_dev_unregister(>edev);
> > > + writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> 
> 

Of course :)
Sorry I missed this part.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Felipe,

On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:

[snip]

> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> > +
> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
> > +   extcon_unregister_notifier(>edev, EXTCON_USB_HOST, >nb);
> > +   extcon_dev_unregister(>edev);
> > +   writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> > +   iounmap(mux->regs);
> > +   kfree(mux);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> 
> so who's gonna call these two functions ? IMO, this looks like a recipe
> for randbuild breakage.

There are function stubs on header file when the functions aren't
available.
But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
automatically selected when a driver that requires it is selected too.

With the 2 cases above, IMHO it should not bring issues with randbuild
tests.

Br, David

> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

Follow my comments below.

On Tue, Dec 01, 2015 at 03:32:37PM +0200, Heikki Krogerus wrote:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  drivers/extcon/Kconfig   |   5 ++
>  drivers/extcon/Makefile  |   1 +
>  drivers/extcon/extcon-intel-usb.c| 112 
> +++
>  include/linux/extcon/intel_usb_mux.h |  31 ++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/extcon/extcon-intel-usb.c
>  create mode 100644 include/linux/extcon/intel_usb_mux.h
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0cebbf6..0a7ccb1 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
> Used typically if GPIO is used for USB ID pin detection.
>  
>  endif
> +
> +config EXTCON_INTEL_USB
> + bool
> + depends on X86 && USB
> + select EXTCON
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index ba787d0..e6e031a 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)+= extcon-usb-gpio.o
> +obj-$(CONFIG_EXTCON_INTEL_USB)   += extcon-intel-usb.o
> diff --git a/drivers/extcon/extcon-intel-usb.c 
> b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#define INTEL_MUX_CFG0   0x00
> +#define INTEL_MUX_CFG1   0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK0x3
> +#define CFG0_SW_DRD_DYN  0
> +#define CFG0_SW_DRD_STATIC_HOST  1
> +#define CFG0_SW_DRD_STATIC_DEV   2
> +#define CFG0_SW_SYNC_SS_AND_HS   BIT(2)
> +#define CFG0_SW_SWITCH_ENBIT(16)
> +#define CFG0_SW_IDPINBIT(20)
> +#define CFG0_SW_IDPIN_EN BIT(21)
> +#define CFG0_SW_VBUS_VALID   BIT(24)
> +
> +#define CFG1_MODEBIT(29)
> +
> +struct intel_usb_mux {
> + struct notifier_block nb;
> + struct extcon_dev edev;
> + void __iomem *regs;
> + u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> + EXTCON_USB_HOST,
> + EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> +   unsigned long old, void *ptr)
> +{
> + struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> + u32 val;
> +
> + if (mux->edev.state)
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> + else
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +   CFG0_SW_DRD_STATIC_DEV;
> +
> + writel(val, mux->regs);
> + return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +  struct resource *r)
> +{
> + struct intel_usb_mux *mux;
> + int ret;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->regs = ioremap_nocache(r->start, resource_size(r));
> + if (!mux->regs) {
> + kfree(mux);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> + mux->edev.dev.parent = dev;
> + mux->edev.supported_cable = intel_mux_cable;
> +
> + ret = extcon_dev_register(>edev);
> + if (ret)
> + goto err;
> +
> + mux->edev.name = "intel_usb_mux";
> + mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> + /* An external source needs to tell us what to do */
> + mux->nb.notifier_call = intel_usb_mux_notifier;
> + ret = extcon_register_notifier(>edev, EXTCON_USB_HOST, >nb);
> + if (ret) {
> + dev_err(>edev.dev, "failed to register notifier\n");
> + extcon_dev_unregister(>edev);
> + goto err;
> + }
> + return mux;
> +err:
> + iounmap(mux->regs);
> + kfree(mux);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +void intel_usb_mux_unregister(struct 

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Heikki,

Follow my comments below.

On Tue, Dec 01, 2015 at 03:32:37PM +0200, Heikki Krogerus wrote:
> Several Intel PCHs and SOCs have an internal mux that is
> used to share one USB port between USB Device Controller and
> xHCI. The mux is normally handled by System FW/BIOS, but not
> always. For those platforms where the FW does not take care
> of the mux, this driver is needed.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  drivers/extcon/Kconfig   |   5 ++
>  drivers/extcon/Makefile  |   1 +
>  drivers/extcon/extcon-intel-usb.c| 112 
> +++
>  include/linux/extcon/intel_usb_mux.h |  31 ++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/extcon/extcon-intel-usb.c
>  create mode 100644 include/linux/extcon/intel_usb_mux.h
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0cebbf6..0a7ccb1 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -118,3 +118,8 @@ config EXTCON_USB_GPIO
> Used typically if GPIO is used for USB ID pin detection.
>  
>  endif
> +
> +config EXTCON_INTEL_USB
> + bool
> + depends on X86 && USB
> + select EXTCON
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index ba787d0..e6e031a 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)  += extcon-sm5502.o
>  obj-$(CONFIG_EXTCON_USB_GPIO)+= extcon-usb-gpio.o
> +obj-$(CONFIG_EXTCON_INTEL_USB)   += extcon-intel-usb.o
> diff --git a/drivers/extcon/extcon-intel-usb.c 
> b/drivers/extcon/extcon-intel-usb.c
> new file mode 100644
> index 000..5534781
> --- /dev/null
> +++ b/drivers/extcon/extcon-intel-usb.c
> @@ -0,0 +1,112 @@
> +/**
> + * extcon-intel-usb.c - Driver for Intel USB mux
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Heikki Krogerus 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#define INTEL_MUX_CFG0   0x00
> +#define INTEL_MUX_CFG1   0x04
> +
> +#define CFG0_SW_DRD_MODE_MASK0x3
> +#define CFG0_SW_DRD_DYN  0
> +#define CFG0_SW_DRD_STATIC_HOST  1
> +#define CFG0_SW_DRD_STATIC_DEV   2
> +#define CFG0_SW_SYNC_SS_AND_HS   BIT(2)
> +#define CFG0_SW_SWITCH_ENBIT(16)
> +#define CFG0_SW_IDPINBIT(20)
> +#define CFG0_SW_IDPIN_EN BIT(21)
> +#define CFG0_SW_VBUS_VALID   BIT(24)
> +
> +#define CFG1_MODEBIT(29)
> +
> +struct intel_usb_mux {
> + struct notifier_block nb;
> + struct extcon_dev edev;
> + void __iomem *regs;
> + u32 cfg0_ctx;
> +};
> +
> +static const int intel_mux_cable[] = {
> + EXTCON_USB_HOST,
> + EXTCON_NONE,
> +};
> +
> +static int intel_usb_mux_notifier(struct notifier_block *nb,
> +   unsigned long old, void *ptr)
> +{
> + struct intel_usb_mux *mux = container_of(nb, struct intel_usb_mux, nb);
> + u32 val;
> +
> + if (mux->edev.state)
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_DRD_STATIC_HOST;
> + else
> + val = CFG0_SW_IDPIN_EN | CFG0_SW_IDPIN | CFG0_SW_VBUS_VALID |
> +   CFG0_SW_DRD_STATIC_DEV;
> +
> + writel(val, mux->regs);
> + return NOTIFY_OK;
> +}
> +
> +struct intel_usb_mux *intel_usb_mux_register(struct device *dev,
> +  struct resource *r)
> +{
> + struct intel_usb_mux *mux;
> + int ret;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->regs = ioremap_nocache(r->start, resource_size(r));
> + if (!mux->regs) {
> + kfree(mux);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + mux->cfg0_ctx = readl(mux->regs + INTEL_MUX_CFG0);
> +
> + mux->edev.dev.parent = dev;
> + mux->edev.supported_cable = intel_mux_cable;
> +
> + ret = extcon_dev_register(>edev);
> + if (ret)
> + goto err;
> +
> + mux->edev.name = "intel_usb_mux";
> + mux->edev.state = !!(readl(mux->regs + INTEL_MUX_CFG1) & CFG1_MODE);
> +
> + /* An external source needs to tell us what to do */
> + mux->nb.notifier_call = intel_usb_mux_notifier;
> + ret = extcon_register_notifier(>edev, EXTCON_USB_HOST, >nb);
> + if (ret) {
> + dev_err(>edev.dev, "failed to register notifier\n");
> + extcon_dev_unregister(>edev);
> + goto err;
> + }
> + return mux;
> +err:
> + iounmap(mux->regs);
> + kfree(mux);
> + return ERR_PTR(ret);
> +}
> 

Re: [PATCH 1/2] extcon: add driver for Intel USB mux

[David Cohen]

Hi Felipe,

On Tue, Dec 01, 2015 at 02:34:34PM -0600, Felipe Balbi wrote:

[snip]

> > +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> > +
> > +void intel_usb_mux_unregister(struct intel_usb_mux *mux)
> > +{
> > +   extcon_unregister_notifier(>edev, EXTCON_USB_HOST, >nb);
> > +   extcon_dev_unregister(>edev);
> > +   writel(mux->cfg0_ctx, mux->regs + INTEL_MUX_CFG0);
> > +   iounmap(mux->regs);
> > +   kfree(mux);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> 
> so who's gonna call these two functions ? IMO, this looks like a recipe
> for randbuild breakage.

There are function stubs on header file when the functions aren't
available.
But also notice CONFIG_EXTCON_INTEL_USB is not user-selectable. It's
automatically selected when a driver that requires it is selected too.

With the 2 cases above, IMHO it should not bring issues with randbuild
tests.

Br, David

> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] wdt: intel-mid: add Magic Closure flag

[David Cohen]

Adding WDIOF_MAGICCLOSE to Intel MID watchdog driver. Once the watchdog
is opened, it makes sense to disable watchdog only if it was gracefully
released.

Signed-off-by: David Cohen 
---
 drivers/watchdog/intel-mid_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
index 0a436b5d1e84..db36d12e2b52 100644
--- a/drivers/watchdog/intel-mid_wdt.c
+++ b/drivers/watchdog/intel-mid_wdt.c
@@ -101,7 +101,7 @@ static irqreturn_t mid_wdt_irq(int irq, void *dev_id)
 
 static const struct watchdog_info mid_wdt_info = {
.identity = "Intel MID SCU watchdog",
-   .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+   .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
 };
 
 static const struct watchdog_ops mid_wdt_ops = {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] wdt: intel-mid: add Magic Closure flag

[David Cohen]

Adding WDIOF_MAGICCLOSE to Intel MID watchdog driver. Once the watchdog
is opened, it makes sense to disable watchdog only if it was gracefully
released.

Signed-off-by: David Cohen <david.a.co...@linux.intel.com>
---
 drivers/watchdog/intel-mid_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
index 0a436b5d1e84..db36d12e2b52 100644
--- a/drivers/watchdog/intel-mid_wdt.c
+++ b/drivers/watchdog/intel-mid_wdt.c
@@ -101,7 +101,7 @@ static irqreturn_t mid_wdt_irq(int irq, void *dev_id)
 
 static const struct watchdog_info mid_wdt_info = {
.identity = "Intel MID SCU watchdog",
-   .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+   .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
 };
 
 static const struct watchdog_ops mid_wdt_ops = {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference

[David Cohen]

Hi Sudip,

On Fri, Sep 04, 2015 at 05:12:23PM +0530, Sudip Mukherjee wrote:
> We were checking if dev->regs is NULL but it was done after
> dereferencing it. Lets reset the controller and iounmap dev->regs only
> if it is not NULL.
> free_irq() does not need dev->regs, so unmaping it before freeing the
> irq should not matter.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/usb/gadget/udc/amd5536udc.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/amd5536udc.c 
> b/drivers/usb/gadget/udc/amd5536udc.c
> index fdacddb..26066d3 100644
> --- a/drivers/usb/gadget/udc/amd5536udc.c
> +++ b/drivers/usb/gadget/udc/amd5536udc.c
> @@ -3135,11 +3135,12 @@ static void udc_pci_remove(struct pci_dev *pdev)
>   }
>  
>   /* reset controller */
> - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), >regs->cfg);
> + if (dev->regs) {
> + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), >regs->cfg);
> + iounmap(dev->regs);

I'm not familiar with the driver, but you're iounmap'ing before freeing
irq. Looks fishy to me.

Br, David

> + }
>   if (dev->irq_registered)
>   free_irq(pdev->irq, dev);
> - if (dev->regs)
> - iounmap(dev->regs);
>   if (dev->mem_region)
>   release_mem_region(pci_resource_start(pdev, 0),
>   pci_resource_len(pdev, 0));
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: gadget: amd5536udc: fix NULL pointer dereference

[David Cohen]

Hi Sudip,

On Fri, Sep 04, 2015 at 05:12:23PM +0530, Sudip Mukherjee wrote:
> We were checking if dev->regs is NULL but it was done after
> dereferencing it. Lets reset the controller and iounmap dev->regs only
> if it is not NULL.
> free_irq() does not need dev->regs, so unmaping it before freeing the
> irq should not matter.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/usb/gadget/udc/amd5536udc.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/amd5536udc.c 
> b/drivers/usb/gadget/udc/amd5536udc.c
> index fdacddb..26066d3 100644
> --- a/drivers/usb/gadget/udc/amd5536udc.c
> +++ b/drivers/usb/gadget/udc/amd5536udc.c
> @@ -3135,11 +3135,12 @@ static void udc_pci_remove(struct pci_dev *pdev)
>   }
>  
>   /* reset controller */
> - writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), >regs->cfg);
> + if (dev->regs) {
> + writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), >regs->cfg);
> + iounmap(dev->regs);

I'm not familiar with the driver, but you're iounmap'ing before freeing
irq. Looks fishy to me.

Br, David

> + }
>   if (dev->irq_registered)
>   free_irq(pdev->irq, dev);
> - if (dev->regs)
> - iounmap(dev->regs);
>   if (dev->mem_region)
>   release_mem_region(pci_resource_start(pdev, 0),
>   pci_resource_len(pdev, 0));
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Tue, May 26, 2015 at 07:37:02PM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 27, 2015 at 09:45:37AM +0800, Lu Baolu wrote:
> > Phy drivers and the ulpi interface providers depend on the
> > registration of the ulpi bus.  Ulpi registers the bus in
> > module_init(). This could cause unnecessary probe delays.
> 
> What do you mean by "probe delays"?

I believe he meant the bus users' probe delays. i.e. unnecessary
-EPROBE_DEFER happening on ulpi_drivers in case they're registered
before ulpi bus itself.

Is that correct, Baolu?

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi Greg,

On Tue, May 26, 2015 at 07:41:18PM -0700, Greg KH wrote:
> On Tue, May 26, 2015 at 10:54:01AM -0700, David Cohen wrote:
> > Hi,
> > 
> > On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
> > > Greg KH  writes:
> > > 
> > > > If there are other bus drivers that do this, I'll go fix them up,
> > > > pointers to those files would be appreciated.
> > > 
> > > git grep -E 'if .*\.p\W' found a couple of interesting candidates you
> > > might want to check out:
> > > 
> > >  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
> > >  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
> > >  drivers/spmi/spmi.c:if (WARN_ON(!spmi_bus_type.p))
> > > 
> > > And this looks somewhat suspicious too, but could very well be OK for
> > > all I know (very close to nothing :)
> > > 
> > >  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p) {
> > >  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p)
> > 
> > I think we need a clear statement on how to proceed on this case.
> 
> Don't mess with bus->p.  I can rename it to
> "do_not_touch_this_isnt_for_you" if people think that would make it more
> obvious that a private data structure shouldn't be messed with in any
> way.  Outside of the driver core, you have no knowledge that even if it
> is a pointer, what that means with regards to anything.

I get that, I agree it's ugly and I'm not trying to push it further. If
you follow the thread, you'll see I reviewed and commented a macro would
make more sense than checking the private data directly for the same
reaon you mentioned. But since with Linux development the source code is
part of the documentation, we need a clear state about what's the
correct way to handle it before go telling people "do not do what kernel
is already doing because it's wrong".

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Tue, May 26, 2015 at 07:37:02PM -0700, Greg Kroah-Hartman wrote:
 On Wed, May 27, 2015 at 09:45:37AM +0800, Lu Baolu wrote:
  Phy drivers and the ulpi interface providers depend on the
  registration of the ulpi bus.  Ulpi registers the bus in
  module_init(). This could cause unnecessary probe delays.
 
 What do you mean by probe delays?

I believe he meant the bus users' probe delays. i.e. unnecessary
-EPROBE_DEFER happening on ulpi_drivers in case they're registered
before ulpi bus itself.

Is that correct, Baolu?

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi Greg,

On Tue, May 26, 2015 at 07:41:18PM -0700, Greg KH wrote:
 On Tue, May 26, 2015 at 10:54:01AM -0700, David Cohen wrote:
  Hi,
  
  On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
   Greg KH gre...@linuxfoundation.org writes:
   
If there are other bus drivers that do this, I'll go fix them up,
pointers to those files would be appreciated.
   
   git grep -E 'if .*\.p\W' found a couple of interesting candidates you
   might want to check out:
   
drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
drivers/spmi/spmi.c:if (WARN_ON(!spmi_bus_type.p))
   
   And this looks somewhat suspicious too, but could very well be OK for
   all I know (very close to nothing :)
   
drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p) {
drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p)
  
  I think we need a clear statement on how to proceed on this case.
 
 Don't mess with bus-p.  I can rename it to
 do_not_touch_this_isnt_for_you if people think that would make it more
 obvious that a private data structure shouldn't be messed with in any
 way.  Outside of the driver core, you have no knowledge that even if it
 is a pointer, what that means with regards to anything.

I get that, I agree it's ugly and I'm not trying to push it further. If
you follow the thread, you'll see I reviewed and commented a macro would
make more sense than checking the private data directly for the same
reaon you mentioned. But since with Linux development the source code is
part of the documentation, we need a clear state about what's the
correct way to handle it before go telling people do not do what kernel
is already doing because it's wrong.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi,

On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
> Greg KH  writes:
> 
> > If there are other bus drivers that do this, I'll go fix them up,
> > pointers to those files would be appreciated.
> 
> git grep -E 'if .*\.p\W' found a couple of interesting candidates you
> might want to check out:
> 
>  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
>  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
>  drivers/spmi/spmi.c:if (WARN_ON(!spmi_bus_type.p))
> 
> And this looks somewhat suspicious too, but could very well be OK for
> all I know (very close to nothing :)
> 
>  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p) {
>  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p)

I think we need a clear statement on how to proceed on this case. With
Linux, the code is the documentation most of times. I gave a reviewed-by
feedback based on seeing other examples following the same approach.

What would be the correct way?

Thanks,

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi,

On Mon, May 25, 2015 at 07:00:13PM +0200, Bjørn Mork wrote:
 Greg KH gre...@linuxfoundation.org writes:
 
  If there are other bus drivers that do this, I'll go fix them up,
  pointers to those files would be appreciated.
 
 git grep -E 'if .*\.p\W' found a couple of interesting candidates you
 might want to check out:
 
  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p))) {
  drivers/i2c/i2c-core.c: if (unlikely(WARN_ON(!i2c_bus_type.p)))
  drivers/spmi/spmi.c:if (WARN_ON(!spmi_bus_type.p))
 
 And this looks somewhat suspicious too, but could very well be OK for
 all I know (very close to nothing :)
 
  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p) {
  drivers/gpio/gpiolib-sysfs.c:   if (!gpio_class.p)

I think we need a clear statement on how to proceed on this case. With
Linux, the code is the documentation most of times. I gave a reviewed-by
feedback based on seeing other examples following the same approach.

What would be the correct way?

Thanks,

David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

On Fri, May 22, 2015 at 03:50:47PM +0800, Lu, Baolu wrote:
> 
> 
> On 05/22/2015 02:46 PM, Lu, Baolu wrote:
> >
> >
> >On 05/22/2015 11:11 AM, David Cohen wrote:
> >>On Thu, May 21, 2015 at 08:09:54PM -0700, David Cohen wrote:
> >>>Hi,
> >>>
> >>>On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
> >>>>Many drivers and modules depend on ULPI bus registeration to
> >>>>register ULPI interfaces and drivers. It's more appropriate
> >>>>to register ULPI bus in subsys_initcall instead of module_init.
> >>>>
> >>>>Kernel panic has been reported with some kind of kernel config.
> >>>Even though I agree subsys_initcall is better to register ulpi bus,
> >>>it's
> >>>still no excuse to have kernel panic. What about ULPI bus being
> >>>compiled
> >>>as module?
> >
> >No kernel panic if ULPI is built as a module.
> >
> >>>IMHO this is avoiding the proper kernel panic fix which should be
> >>>failing gracefully (or defer probe) from tusb1210 driver.
> >>Maybe I need to express myself better :)
> >>IMHO we should not consider work done with this patch only. It's still
> >>incomplete.
> >
> >I am with you on that we should know the real problem.
> >
> >I could go ahead with further debugging. Do you have any suggestions
> >about which direction should I go?
> 
> I forgot to mention that the panic was found in an Android environment.
> The kernel version is  v4.1-rc3.

FWIW:

The problem with Android environment is the amount of off-tree patches
you may have over there.
For upstream tasks, I'd suggest use a clean tree + patches you want to
test. Usually yocto looks more friendly to test under this environment.

Br, David

> 
> >
> >>
> >>Br, David
> >
> >Thank you,
> >-Baolu
> >
> >>-- 
> >>To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> >>in
> >>the body of a message to majord...@vger.kernel.org
> >>More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>Please read the FAQ at  http://www.tux.org/lkml/
> >>
> >>
> >
> >-- 
> >To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >the body of a message to majord...@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >Please read the FAQ at  http://www.tux.org/lkml/
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Fri, May 22, 2015 at 07:29:15PM +0800, Lu Baolu wrote:
> Phy drivers and the ulpi interface providers depend on the
> registeration of the ulpi bus.  Ulpi registers the bus in
> module_init(). This could result in a load order issue, i.e.

It's still not an issue :(
I'd say "unnecessary probe delays".

But of cource it's Felipe's call :) Description looks better now.

BR, David

> ulpi phy drivers or the ulpi interface providers loading
> before the bus registeration.
> 
> This patch fixes this load order issue by putting ulpi_init
> in subsys_initcall().
> 
> Reported-by: Zhuo Qiuxu 
> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/common/ulpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 0e6f968..01c0c04 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -242,7 +242,7 @@ static int __init ulpi_init(void)
>  {
>   return bus_register(_bus);
>  }
> -module_init(ulpi_init);
> +subsys_initcall(ulpi_init);
>  
>  static void __exit ulpi_exit(void)
>  {
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Fri, May 22, 2015 at 07:29:15PM +0800, Lu Baolu wrote:
 Phy drivers and the ulpi interface providers depend on the
 registeration of the ulpi bus.  Ulpi registers the bus in
 module_init(). This could result in a load order issue, i.e.

It's still not an issue :(
I'd say unnecessary probe delays.

But of cource it's Felipe's call :) Description looks better now.

BR, David

 ulpi phy drivers or the ulpi interface providers loading
 before the bus registeration.
 
 This patch fixes this load order issue by putting ulpi_init
 in subsys_initcall().
 
 Reported-by: Zhuo Qiuxu qiuxu.z...@intel.com
 Signed-off-by: Lu Baolu baolu...@linux.intel.com
 ---
  drivers/usb/common/ulpi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
 index 0e6f968..01c0c04 100644
 --- a/drivers/usb/common/ulpi.c
 +++ b/drivers/usb/common/ulpi.c
 @@ -242,7 +242,7 @@ static int __init ulpi_init(void)
  {
   return bus_register(ulpi_bus);
  }
 -module_init(ulpi_init);
 +subsys_initcall(ulpi_init);
  
  static void __exit ulpi_exit(void)
  {
 -- 
 2.1.4
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

On Fri, May 22, 2015 at 03:50:47PM +0800, Lu, Baolu wrote:
 
 
 On 05/22/2015 02:46 PM, Lu, Baolu wrote:
 
 
 On 05/22/2015 11:11 AM, David Cohen wrote:
 On Thu, May 21, 2015 at 08:09:54PM -0700, David Cohen wrote:
 Hi,
 
 On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
 Many drivers and modules depend on ULPI bus registeration to
 register ULPI interfaces and drivers. It's more appropriate
 to register ULPI bus in subsys_initcall instead of module_init.
 
 Kernel panic has been reported with some kind of kernel config.
 Even though I agree subsys_initcall is better to register ulpi bus,
 it's
 still no excuse to have kernel panic. What about ULPI bus being
 compiled
 as module?
 
 No kernel panic if ULPI is built as a module.
 
 IMHO this is avoiding the proper kernel panic fix which should be
 failing gracefully (or defer probe) from tusb1210 driver.
 Maybe I need to express myself better :)
 IMHO we should not consider work done with this patch only. It's still
 incomplete.
 
 I am with you on that we should know the real problem.
 
 I could go ahead with further debugging. Do you have any suggestions
 about which direction should I go?
 
 I forgot to mention that the panic was found in an Android environment.
 The kernel version is  v4.1-rc3.

FWIW:

The problem with Android environment is the amount of off-tree patches
you may have over there.
For upstream tasks, I'd suggest use a clean tree + patches you want to
test. Usually yocto looks more friendly to test under this environment.

Br, David

 
 
 
 Br, David
 
 Thank you,
 -Baolu
 
 -- 
 To unsubscribe from this list: send the line unsubscribe linux-kernel
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 
 
 
 -- 
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

On Thu, May 21, 2015 at 08:09:54PM -0700, David Cohen wrote:
> Hi,
> 
> On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
> > Many drivers and modules depend on ULPI bus registeration to
> > register ULPI interfaces and drivers. It's more appropriate
> > to register ULPI bus in subsys_initcall instead of module_init.
> > 
> > Kernel panic has been reported with some kind of kernel config.
> 
> Even though I agree subsys_initcall is better to register ulpi bus, it's
> still no excuse to have kernel panic. What about ULPI bus being compiled
> as module?
> IMHO this is avoiding the proper kernel panic fix which should be
> failing gracefully (or defer probe) from tusb1210 driver.

Maybe I need to express myself better :)
IMHO we should not consider work done with this patch only. It's still
incomplete.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
> Many drivers and modules depend on ULPI bus registeration to
> register ULPI interfaces and drivers. It's more appropriate
> to register ULPI bus in subsys_initcall instead of module_init.
> 
> Kernel panic has been reported with some kind of kernel config.

Even though I agree subsys_initcall is better to register ulpi bus, it's
still no excuse to have kernel panic. What about ULPI bus being compiled
as module?
IMHO this is avoiding the proper kernel panic fix which should be
failing gracefully (or defer probe) from tusb1210 driver.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

Hi,

On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
 Many drivers and modules depend on ULPI bus registeration to
 register ULPI interfaces and drivers. It's more appropriate
 to register ULPI bus in subsys_initcall instead of module_init.
 
 Kernel panic has been reported with some kind of kernel config.

Even though I agree subsys_initcall is better to register ulpi bus, it's
still no excuse to have kernel panic. What about ULPI bus being compiled
as module?
IMHO this is avoiding the proper kernel panic fix which should be
failing gracefully (or defer probe) from tusb1210 driver.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/1] usb: ulpi: ulpi_init should be executed in subsys_initcall

[David Cohen]

On Thu, May 21, 2015 at 08:09:54PM -0700, David Cohen wrote:
 Hi,
 
 On Fri, May 22, 2015 at 10:07:05AM +0800, Lu Baolu wrote:
  Many drivers and modules depend on ULPI bus registeration to
  register ULPI interfaces and drivers. It's more appropriate
  to register ULPI bus in subsys_initcall instead of module_init.
  
  Kernel panic has been reported with some kind of kernel config.
 
 Even though I agree subsys_initcall is better to register ulpi bus, it's
 still no excuse to have kernel panic. What about ULPI bus being compiled
 as module?
 IMHO this is avoiding the proper kernel panic fix which should be
 failing gracefully (or defer probe) from tusb1210 driver.

Maybe I need to express myself better :)
IMHO we should not consider work done with this patch only. It's still
incomplete.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi,

On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
> ULPI registers it's bus at module_init so if the bus fails to register, the

A minor comment: s/it's/its/

> module will fail to load and all will be well in the world.
> 
> However, if the ULPI code is built-in rather than a module, the bus
> initialization may fail but we'd still try to register drivers later onto
> a non-existant bus, which will panic the kernel.
> 
> Fix that by checking that the bus was indeed initialized before trying to
> register drivers on top of it.
> 
> Signed-off-by: Sasha Levin 
> ---
>  drivers/usb/common/ulpi.c |4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 0e6f968..0b0a5e7 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
>   if (!drv->probe)
>   return -EINVAL;
>  
> + /* Was the bus registered successfully? */
> + if (!ulpi_bus.p)
> + return -ENODEV;
> +

Good catch. Otherwise it may trigger BUG() on driver_register().
I wonder if it would be nice to have a macro for that checking :)

Anyway,

Reviewed-by: David Cohen 

>   drv->driver.bus = _bus;
>  
>   return driver_register(>driver);
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: ulpi: don't register drivers if bus doesn't exist

[David Cohen]

Hi,

On Wed, May 20, 2015 at 03:33:26PM -0400, Sasha Levin wrote:
 ULPI registers it's bus at module_init so if the bus fails to register, the

A minor comment: s/it's/its/

 module will fail to load and all will be well in the world.
 
 However, if the ULPI code is built-in rather than a module, the bus
 initialization may fail but we'd still try to register drivers later onto
 a non-existant bus, which will panic the kernel.
 
 Fix that by checking that the bus was indeed initialized before trying to
 register drivers on top of it.
 
 Signed-off-by: Sasha Levin sasha.le...@oracle.com
 ---
  drivers/usb/common/ulpi.c |4 
  1 file changed, 4 insertions(+)
 
 diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
 index 0e6f968..0b0a5e7 100644
 --- a/drivers/usb/common/ulpi.c
 +++ b/drivers/usb/common/ulpi.c
 @@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
   if (!drv-probe)
   return -EINVAL;
  
 + /* Was the bus registered successfully? */
 + if (!ulpi_bus.p)
 + return -ENODEV;
 +

Good catch. Otherwise it may trigger BUG() on driver_register().
I wonder if it would be nice to have a macro for that checking :)

Anyway,

Reviewed-by: David Cohen david.a.co...@linux.intel.com

   drv-driver.bus = ulpi_bus;
  
   return driver_register(drv-driver);
 -- 
 1.7.10.4
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

On Sat, Apr 25, 2015 at 10:47:42AM -0500, Felipe Balbi wrote:
> Hi,

Hi Felipe,

> 
> On Fri, Apr 24, 2015 at 01:56:25PM -0700, David Cohen wrote:
> > > > > > When going into bus suspend/resume we _must_
> > > > > > call gadget driver's ->suspend/->resume callbacks
> > > > > > accordingly. This patch implements that very feature
> > > > > > which has been missing forever.
> > > > > > 
> > > > > > Cc:  # 3.14
> > > > > > Signed-off-by: Felipe Balbi 
> > > > > > Signed-off-by: David Cohen 
> > > > > > ---
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > This patch was introduced on v3.15.
> > > > > > But the issue it fixes already existed on v3.14 and v3.14 is a long 
> > > > > > term
> > > > > > support version.
> > > > > 
> > > > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > > > this a bug fix ? What sort of drawbacks did you notice ?
> > > > 
> > > > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > > > the bug report :)
> > > > 
> > > > 1. BC1.2 compliance testing - SDP2.0
> > > > ---
> > > > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> > > >Actual result 100mA to 500mA
> > > > 
> > > > 2. On Host Suspend (ER: Fall back to 0mA):
> > > >not falling back to 0mA, remains at 500mA
> > > > 
> > > > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> > > >cable-props shown as 100mA, which means drawing a current of 100mA 
> > > > from
> > > >Suspended Host
> > > > 
> > > > 4. On making Host active (ER: 500mA):
> > > >500mA
> > > 
> > > But we don't support Battery Charging with dwc3 as of now :-) In fact,
> > > just note that none of the BC registers are even defined in the current
> > > driver anywhere. Seems like you should cherry-pick these to your vendor
> > > tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> > > claimed to be at fault, right ?
> > 
> > We could call it a missing feature that could lead to a potential bug :)
> > By your own comment, he "must" was stressed out:
> 
> sure it was because they really must be called :-) However, the only
> actual problem that arises from not calling them is with something
> that's not supported :-)
> 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > '''
> > 
> > Since v3.14 is a LTS kernel and the changes are safe, it's worth to
> > consider.
> 
> definitely worth to consider, but not as something that fixes BC1.2
> because that's, as said, not supported in any public tree :-)

Thanks for the reply.
The gadget having this issue is really out-of-tree (android gadget).
I could do 2 next steps:
1) I could drop android gadget and try to reproduce this issue using
legacy g_ffs, which supports adbd as well.
2) I could propose this change directly on google's android 3.14 tree
instead.

Would you prefer one of those?

Thanks,

David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's -suspend/-resume

[David Cohen]

On Sat, Apr 25, 2015 at 10:47:42AM -0500, Felipe Balbi wrote:
 Hi,

Hi Felipe,

 
 On Fri, Apr 24, 2015 at 01:56:25PM -0700, David Cohen wrote:
  When going into bus suspend/resume we _must_
  call gadget driver's -suspend/-resume callbacks
  accordingly. This patch implements that very feature
  which has been missing forever.
  
  Cc: sta...@vger.kernel.org # 3.14
  Signed-off-by: Felipe Balbi ba...@ti.com
  Signed-off-by: David Cohen david.a.co...@linux.intel.com
  ---
  
  Hi,
  
  This patch was introduced on v3.15.
  But the issue it fixes already existed on v3.14 and v3.14 is a long 
  term
  support version.
 
 Can you show me a log of this breaking anywhere ? Why do you consider
 this a bug fix ? What sort of drawbacks did you notice ?

We're seeing BC1.2 compliance test failure. I borrowed this info from
the bug report :)

1. BC1.2 compliance testing - SDP2.0
---
1. On Connect to active Host (Expected result: 100mA to 500mA):
   Actual result 100mA to 500mA

2. On Host Suspend (ER: Fall back to 0mA):
   not falling back to 0mA, remains at 500mA

3. On Connect to Suspended Host (ER: 100mA to 0mA):
   cable-props shown as 100mA, which means drawing a current of 100mA 
from
   Suspended Host

4. On making Host active (ER: 500mA):
   500mA
   
   But we don't support Battery Charging with dwc3 as of now :-) In fact,
   just note that none of the BC registers are even defined in the current
   driver anywhere. Seems like you should cherry-pick these to your vendor
   tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
   claimed to be at fault, right ?
  
  We could call it a missing feature that could lead to a potential bug :)
  By your own comment, he must was stressed out:
 
 sure it was because they really must be called :-) However, the only
 actual problem that arises from not calling them is with something
 that's not supported :-)
 
  When going into bus suspend/resume we _must_
  call gadget driver's -suspend/-resume callbacks
  accordingly. This patch implements that very feature
  which has been missing forever.
  '''
  
  Since v3.14 is a LTS kernel and the changes are safe, it's worth to
  consider.
 
 definitely worth to consider, but not as something that fixes BC1.2
 because that's, as said, not supported in any public tree :-)

Thanks for the reply.
The gadget having this issue is really out-of-tree (android gadget).
I could do 2 next steps:
1) I could drop android gadget and try to reproduce this issue using
legacy g_ffs, which supports adbd as well.
2) I could propose this change directly on google's android 3.14 tree
instead.

Would you prefer one of those?

Thanks,

David

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

On Fri, Apr 24, 2015 at 02:48:27PM -0500, Felipe Balbi wrote:
> Hi,

Hi Felipe,

> 
> On Thu, Apr 23, 2015 at 03:37:48PM -0700, David Cohen wrote:
> > On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> > > On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > > > From: Felipe Balbi 
> > > 
> > > missing the required:
> > > 
> > > [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> > > 
> > > > 
> > > > When going into bus suspend/resume we _must_
> > > > call gadget driver's ->suspend/->resume callbacks
> > > > accordingly. This patch implements that very feature
> > > > which has been missing forever.
> > > > 
> > > > Cc:  # 3.14
> > > > Signed-off-by: Felipe Balbi 
> > > > Signed-off-by: David Cohen 
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > This patch was introduced on v3.15.
> > > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > > support version.
> > > 
> > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > this a bug fix ? What sort of drawbacks did you notice ?
> > 
> > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > the bug report :)
> > 
> > 1. BC1.2 compliance testing - SDP2.0
> > ---
> > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> >Actual result 100mA to 500mA
> > 
> > 2. On Host Suspend (ER: Fall back to 0mA):
> >not falling back to 0mA, remains at 500mA
> > 
> > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> >cable-props shown as 100mA, which means drawing a current of 100mA from
> >Suspended Host
> > 
> > 4. On making Host active (ER: 500mA):
> >500mA
> 
> But we don't support Battery Charging with dwc3 as of now :-) In fact,
> just note that none of the BC registers are even defined in the current
> driver anywhere. Seems like you should cherry-pick these to your vendor
> tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> claimed to be at fault, right ?

We could call it a missing feature that could lead to a potential bug :)
By your own comment, he "must" was stressed out:
'''
When going into bus suspend/resume we _must_
call gadget driver's ->suspend/->resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.
'''

Since v3.14 is a LTS kernel and the changes are safe, it's worth to
consider.

> 
> I'll leave the final decision to Greg and I don't really oppose having
> both patches on v3.14-stable, but this is not a bug fix in my view.

Thanks. I appreciate your feedback.

BR, David

PS: FWIW implementing features or fixing bugs aren't much different tasks:
https://geekwhisperin.files.wordpress.com/2009/09/bug-vs-feature.jpg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's -suspend/-resume

[David Cohen]

On Fri, Apr 24, 2015 at 02:48:27PM -0500, Felipe Balbi wrote:
 Hi,

Hi Felipe,

 
 On Thu, Apr 23, 2015 at 03:37:48PM -0700, David Cohen wrote:
  On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
   On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
From: Felipe Balbi ba...@ti.com
   
   missing the required:
   
   [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
   

When going into bus suspend/resume we _must_
call gadget driver's -suspend/-resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.

Cc: sta...@vger.kernel.org # 3.14
Signed-off-by: Felipe Balbi ba...@ti.com
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

This patch was introduced on v3.15.
But the issue it fixes already existed on v3.14 and v3.14 is a long term
support version.
   
   Can you show me a log of this breaking anywhere ? Why do you consider
   this a bug fix ? What sort of drawbacks did you notice ?
  
  We're seeing BC1.2 compliance test failure. I borrowed this info from
  the bug report :)
  
  1. BC1.2 compliance testing - SDP2.0
  ---
  1. On Connect to active Host (Expected result: 100mA to 500mA):
 Actual result 100mA to 500mA
  
  2. On Host Suspend (ER: Fall back to 0mA):
 not falling back to 0mA, remains at 500mA
  
  3. On Connect to Suspended Host (ER: 100mA to 0mA):
 cable-props shown as 100mA, which means drawing a current of 100mA from
 Suspended Host
  
  4. On making Host active (ER: 500mA):
 500mA
 
 But we don't support Battery Charging with dwc3 as of now :-) In fact,
 just note that none of the BC registers are even defined in the current
 driver anywhere. Seems like you should cherry-pick these to your vendor
 tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
 claimed to be at fault, right ?

We could call it a missing feature that could lead to a potential bug :)
By your own comment, he must was stressed out:
'''
When going into bus suspend/resume we _must_
call gadget driver's -suspend/-resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.
'''

Since v3.14 is a LTS kernel and the changes are safe, it's worth to
consider.

 
 I'll leave the final decision to Greg and I don't really oppose having
 both patches on v3.14-stable, but this is not a bug fix in my view.

Thanks. I appreciate your feedback.

BR, David

PS: FWIW implementing features or fixing bugs aren't much different tasks:
https://geekwhisperin.files.wordpress.com/2009/09/bug-vs-feature.jpg
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

Hi Greg,

On Fri, Apr 17, 2015 at 09:42:57PM +0200, Greg KH wrote:
> On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > From: Felipe Balbi 
> > 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > 
> > Cc:  # 3.14
> > Signed-off-by: Felipe Balbi 
> > Signed-off-by: David Cohen 
> > ---
> > 
> > Hi,
> > 
> > This patch was introduced on v3.15.
> > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > support version.
> > I propose to backport it over there as well.
> 
> What is the git commit id of the patch in Linus's tree?

Sorry for the missing info. As Felipe replied, this is the commit:
Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9

I'm resending it to fix the commit message and add a second patch as
well.

Br, David

> 
> thanks,
> 
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

Hi Felipe,

On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > From: Felipe Balbi 
> 
> missing the required:
> 
> [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> 
> > 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > 
> > Cc:  # 3.14
> > Signed-off-by: Felipe Balbi 
> > Signed-off-by: David Cohen 
> > ---
> > 
> > Hi,
> > 
> > This patch was introduced on v3.15.
> > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > support version.
> 
> Can you show me a log of this breaking anywhere ? Why do you consider
> this a bug fix ? What sort of drawbacks did you notice ?

We're seeing BC1.2 compliance test failure. I borrowed this info from
the bug report :)

1. BC1.2 compliance testing - SDP2.0
---
1. On Connect to active Host (Expected result: 100mA to 500mA):
   Actual result 100mA to 500mA

2. On Host Suspend (ER: Fall back to 0mA):
   not falling back to 0mA, remains at 500mA

3. On Connect to Suspended Host (ER: 100mA to 0mA):
   cable-props shown as 100mA, which means drawing a current of 100mA from
   Suspended Host

4. On making Host active (ER: 500mA):
   500mA

> 
> > I propose to backport it over there as well.
> > 
> > BR, David
> > ---
> > 
> >  drivers/usb/dwc3/gadget.c | 35 +++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8f6738d46b14..1bb752736c32 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> > }
> >  }
> >  
> > +static void dwc3_suspend_gadget(struct dwc3 *dwc)
> > +{
> > +   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> 
> you also need Dan Carperter's commit which fixes this cut & paste error.
> That's commit 73a30bfc0d526db899033165db6f95c427e70505

Thanks. I'll add that to my next try.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's -suspend/-resume

[David Cohen]

Hi Greg,

On Fri, Apr 17, 2015 at 09:42:57PM +0200, Greg KH wrote:
 On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
  From: Felipe Balbi ba...@ti.com
  
  When going into bus suspend/resume we _must_
  call gadget driver's -suspend/-resume callbacks
  accordingly. This patch implements that very feature
  which has been missing forever.
  
  Cc: sta...@vger.kernel.org # 3.14
  Signed-off-by: Felipe Balbi ba...@ti.com
  Signed-off-by: David Cohen david.a.co...@linux.intel.com
  ---
  
  Hi,
  
  This patch was introduced on v3.15.
  But the issue it fixes already existed on v3.14 and v3.14 is a long term
  support version.
  I propose to backport it over there as well.
 
 What is the git commit id of the patch in Linus's tree?

Sorry for the missing info. As Felipe replied, this is the commit:
Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9

I'm resending it to fix the commit message and add a second patch as
well.

Br, David

 
 thanks,
 
 greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: dwc3: gadget: call gadget driver's -suspend/-resume

[David Cohen]

Hi Felipe,

On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
 On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
  From: Felipe Balbi ba...@ti.com
 
 missing the required:
 
 [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
 
  
  When going into bus suspend/resume we _must_
  call gadget driver's -suspend/-resume callbacks
  accordingly. This patch implements that very feature
  which has been missing forever.
  
  Cc: sta...@vger.kernel.org # 3.14
  Signed-off-by: Felipe Balbi ba...@ti.com
  Signed-off-by: David Cohen david.a.co...@linux.intel.com
  ---
  
  Hi,
  
  This patch was introduced on v3.15.
  But the issue it fixes already existed on v3.14 and v3.14 is a long term
  support version.
 
 Can you show me a log of this breaking anywhere ? Why do you consider
 this a bug fix ? What sort of drawbacks did you notice ?

We're seeing BC1.2 compliance test failure. I borrowed this info from
the bug report :)

1. BC1.2 compliance testing - SDP2.0
---
1. On Connect to active Host (Expected result: 100mA to 500mA):
   Actual result 100mA to 500mA

2. On Host Suspend (ER: Fall back to 0mA):
   not falling back to 0mA, remains at 500mA

3. On Connect to Suspended Host (ER: 100mA to 0mA):
   cable-props shown as 100mA, which means drawing a current of 100mA from
   Suspended Host

4. On making Host active (ER: 500mA):
   500mA

 
  I propose to backport it over there as well.
  
  BR, David
  ---
  
   drivers/usb/dwc3/gadget.c | 35 +++
   1 file changed, 35 insertions(+)
  
  diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
  index 8f6738d46b14..1bb752736c32 100644
  --- a/drivers/usb/dwc3/gadget.c
  +++ b/drivers/usb/dwc3/gadget.c
  @@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
  }
   }
   
  +static void dwc3_suspend_gadget(struct dwc3 *dwc)
  +{
  +   if (dwc-gadget_driver  dwc-gadget_driver-disconnect) {
 
 you also need Dan Carperter's commit which fixes this cut  paste error.
 That's commit 73a30bfc0d526db899033165db6f95c427e70505

Thanks. I'll add that to my next try.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

From: Felipe Balbi 

When going into bus suspend/resume we _must_
call gadget driver's ->suspend/->resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.

Cc:  # 3.14
Signed-off-by: Felipe Balbi 
Signed-off-by: David Cohen 
---

Hi,

This patch was introduced on v3.15.
But the issue it fixes already existed on v3.14 and v3.14 is a long term
support version.
I propose to backport it over there as well.

BR, David
---

 drivers/usb/dwc3/gadget.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8f6738d46b14..1bb752736c32 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
}
 }
 
+static void dwc3_suspend_gadget(struct dwc3 *dwc)
+{
+   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+   spin_unlock(>lock);
+   dwc->gadget_driver->suspend(>gadget);
+   spin_lock(>lock);
+   }
+}
+
+static void dwc3_resume_gadget(struct dwc3 *dwc)
+{
+   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+   spin_unlock(>lock);
+   dwc->gadget_driver->resume(>gadget);
+   spin_lock(>lock);
+   }
+}
+
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
 {
struct dwc3_ep *dep;
@@ -2391,6 +2409,23 @@ static void dwc3_gadget_linksts_change_interrupt(struct 
dwc3 *dwc,
 
dwc->link_state = next;
 
+   switch (next) {
+   case DWC3_LINK_STATE_U1:
+   if (dwc->speed == USB_SPEED_SUPER)
+   dwc3_suspend_gadget(dwc);
+   break;
+   case DWC3_LINK_STATE_U2:
+   case DWC3_LINK_STATE_U3:
+   dwc3_suspend_gadget(dwc);
+   break;
+   case DWC3_LINK_STATE_RESUME:
+   dwc3_resume_gadget(dwc);
+   break;
+   default:
+   /* do nothing */
+   break;
+   }
+
dev_vdbg(dwc->dev, "%s link %d\n", __func__, dwc->link_state);
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] usb: dwc3: gadget: call gadget driver's -suspend/-resume

[David Cohen]

From: Felipe Balbi ba...@ti.com

When going into bus suspend/resume we _must_
call gadget driver's -suspend/-resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.

Cc: sta...@vger.kernel.org # 3.14
Signed-off-by: Felipe Balbi ba...@ti.com
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

This patch was introduced on v3.15.
But the issue it fixes already existed on v3.14 and v3.14 is a long term
support version.
I propose to backport it over there as well.

BR, David
---

 drivers/usb/dwc3/gadget.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8f6738d46b14..1bb752736c32 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
}
 }
 
+static void dwc3_suspend_gadget(struct dwc3 *dwc)
+{
+   if (dwc-gadget_driver  dwc-gadget_driver-disconnect) {
+   spin_unlock(dwc-lock);
+   dwc-gadget_driver-suspend(dwc-gadget);
+   spin_lock(dwc-lock);
+   }
+}
+
+static void dwc3_resume_gadget(struct dwc3 *dwc)
+{
+   if (dwc-gadget_driver  dwc-gadget_driver-disconnect) {
+   spin_unlock(dwc-lock);
+   dwc-gadget_driver-resume(dwc-gadget);
+   spin_lock(dwc-lock);
+   }
+}
+
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
 {
struct dwc3_ep *dep;
@@ -2391,6 +2409,23 @@ static void dwc3_gadget_linksts_change_interrupt(struct 
dwc3 *dwc,
 
dwc-link_state = next;
 
+   switch (next) {
+   case DWC3_LINK_STATE_U1:
+   if (dwc-speed == USB_SPEED_SUPER)
+   dwc3_suspend_gadget(dwc);
+   break;
+   case DWC3_LINK_STATE_U2:
+   case DWC3_LINK_STATE_U3:
+   dwc3_suspend_gadget(dwc);
+   break;
+   case DWC3_LINK_STATE_RESUME:
+   dwc3_resume_gadget(dwc);
+   break;
+   default:
+   /* do nothing */
+   break;
+   }
+
dev_vdbg(dwc-dev, %s link %d\n, __func__, dwc-link_state);
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] gpio: add set_active_low() function to gpio_chip

[David Cohen]

Some gpio controllers are capable of programming its pins' active-low
state. Let's add this new gpio_chip function for such cases and use it
in gpiolib.

When set_active_low() is implemented, we no longer need to do soft flips
on values from non-raw get functions.

Signed-off-by: David Cohen 
---

Hi,

This is a RFC, not meant for integration (yet).

We have a GPIO controller that is capable of inverting the GPIO logical value
on hardware using a register (and GPIO voltage level if configured for output):
- If GPIO pin is configured for input, only the logical value is affected: the
  GPIO level stays the same but the read values are inverted (it affects even
  interrupt event triggers).
- If GPIO pin is configured for output, the GPIO level is also inverted.

Is it acceptable to expose this functionality via new gpio chip operation
set_active_low()?

Comments are welcome.

Br, David
---

 drivers/gpio/gpiolib-sysfs.c | 4 
 drivers/gpio/gpiolib.c   | 6 --
 include/linux/gpio/driver.h  | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 7722ed53bd65..316a1d008cad 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -289,11 +289,15 @@ static DEVICE_ATTR(edge, 0644, gpio_edge_show, 
gpio_edge_store);
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
int value)
 {
+   struct gpio_chip*chip = desc->chip;
int status = 0;
 
if (!!test_bit(FLAG_ACTIVE_LOW, >flags) == !!value)
return 0;
 
+   if (chip->set_active_low)
+   chip->set_active_low(chip, gpio_chip_hwgpio(desc), value);
+
if (value)
set_bit(FLAG_ACTIVE_LOW, >flags);
else
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1ca9295b2c10..028a0d34ddab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1196,7 +1196,8 @@ int gpiod_get_value(const struct gpio_desc *desc)
WARN_ON(desc->chip->can_sleep);
 
value = _gpiod_get_raw_value(desc);
-   if (test_bit(FLAG_ACTIVE_LOW, >flags))
+   if (!desc->chip->set_active_low &&
+   test_bit(FLAG_ACTIVE_LOW, >flags))
value = !value;
 
return value;
@@ -1550,7 +1551,8 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
return 0;
 
value = _gpiod_get_raw_value(desc);
-   if (test_bit(FLAG_ACTIVE_LOW, >flags))
+   if (!desc->chip->set_active_low &&
+   test_bit(FLAG_ACTIVE_LOW, >flags))
value = !value;
 
return value;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c497c62889d1..2d665352fd69 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -90,6 +90,8 @@ struct gpio_chip {
unsigned offset);
void(*set)(struct gpio_chip *chip,
unsigned offset, int value);
+   void(*set_active_low)(struct gpio_chip *chip,
+   unsigned offset, int value);
void(*set_multiple)(struct gpio_chip *chip,
unsigned long *mask,
unsigned long *bits);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 00/12] usb: ulpi bus

[David Cohen]

On Wed, Mar 18, 2015 at 10:12:13AM -0700, David Cohen wrote:
> Hi,
> 
> On Wed, Mar 18, 2015 at 02:40:21PM +0200, Heikki Krogerus wrote:
> > 
> > Major changes since v1:
> > - calling dwc3_phy_setup earlier and registering the ULPI interface there.
> > - new property to dwc3 for selecting the interface in case of UTMI+ and 
> > ULPI as
> >   suggested by Felipe
> > - dwc3 soft reset before registration of the ULPI interface to sync the 
> > clocks
> >   as suggested by David
> > - Including support for the BYT boards that have the GPIOs controlling the 
> > reset
> >   and cs signals.
> > 
> > 
> > Heikki Krogerus (12):
> >   usb: add bus type for USB ULPI
> >   usb: dwc3: USB2 PHY register access bits
> >   usb: dwc3: ULPI or UTMI+ select
> >   usb: dwc3: store driver data earlier
> >   usb: dwc3: cache hwparams earlier
> >   usb: dwc3: soft reset to it's own function
> >   usb: dwc3: setup phys earlier
> >   usb: dwc3: add hsphy_interface property
> >   usb: dwc3: pci: add quirk for Baytrails
> >   usb: dwc3: add ULPI interface support
> >   phy: helpers for USB ULPI PHY registering
> >   phy: add driver for TI TUSB1210 ULPI PHY
> 
> Except for my comment on patch "9/12", this version looks fine on my
> side.

Making things official. For the whole series (except 9/12):
Acked-by: David Cohen 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 00/12] usb: ulpi bus

[David Cohen]

Hi,

On Wed, Mar 18, 2015 at 02:40:21PM +0200, Heikki Krogerus wrote:
> 
> Major changes since v1:
> - calling dwc3_phy_setup earlier and registering the ULPI interface there.
> - new property to dwc3 for selecting the interface in case of UTMI+ and ULPI 
> as
>   suggested by Felipe
> - dwc3 soft reset before registration of the ULPI interface to sync the clocks
>   as suggested by David
> - Including support for the BYT boards that have the GPIOs controlling the 
> reset
>   and cs signals.
> 
> 
> Heikki Krogerus (12):
>   usb: add bus type for USB ULPI
>   usb: dwc3: USB2 PHY register access bits
>   usb: dwc3: ULPI or UTMI+ select
>   usb: dwc3: store driver data earlier
>   usb: dwc3: cache hwparams earlier
>   usb: dwc3: soft reset to it's own function
>   usb: dwc3: setup phys earlier
>   usb: dwc3: add hsphy_interface property
>   usb: dwc3: pci: add quirk for Baytrails
>   usb: dwc3: add ULPI interface support
>   phy: helpers for USB ULPI PHY registering
>   phy: add driver for TI TUSB1210 ULPI PHY

Except for my comment on patch "9/12", this version looks fine on my
side.

BR, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 09/12] usb: dwc3: pci: add quirk for Baytrails

[David Cohen]

Hi Heikki,

On Wed, Mar 18, 2015 at 02:40:30PM +0200, Heikki Krogerus wrote:
> On some BYT platforms the USB2 PHY needs to be put into
> operational mode by the controller driver with GPIOs
> controlling the PHYs reset and cs signals.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  drivers/usb/dwc3/dwc3-pci.c | 36 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index b773fb5..01cfdbd 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #include "platform_data.h"
>  
> @@ -31,6 +33,15 @@
>  #define PCI_DEVICE_ID_INTEL_SPTLP0x9d30
>  #define PCI_DEVICE_ID_INTEL_SPTH 0xa130
>  
> +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params cs_gpios = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_dwc3_byt_gpios[] = {
> + { "reset-gpios", _gpios, 1 },
> + { "cs-gpios", _gpios, 1 },
> + { },
> +};
> +
>  static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
>   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> @@ -65,6 +76,31 @@ static int dwc3_pci_quirks(struct pci_dev *pdev)
>   sizeof(pdata));
>   }
>  
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> + struct gpio_desc *gpio;
> +
> + acpi_dev_add_driver_gpios(ACPI_COMPANION(>dev),
> +   acpi_dwc3_byt_gpios);
> +
> + gpio = gpiod_get(>dev, "reset");
> + if (IS_ERR(gpio))
> + return 0;
> +
> + /* These GPIOs will turn on the USB2 PHY */
> + gpiod_direction_output(gpio, 0);
> + gpiod_set_value_cansleep(gpio, 1);
> + gpiod_put(gpio);
> +
> + gpio = gpiod_get(>dev, "cs");

CS GPIO should be handled before RESET.

BR, David

> + if (!IS_ERR(gpio)) {
> + gpiod_direction_output(gpio, 0);
> + gpiod_set_value_cansleep(gpio, 1);
> + gpiod_put(gpio);
> + }
> + msleep(10);
> + }
> +
>   return 0;
>  }
>  
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 11/12] phy: helpers for USB ULPI PHY registering

[David Cohen]

On Wed, Mar 18, 2015 at 05:46:44PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 3/18/2015 3:40 PM, Heikki Krogerus wrote:
> 
> >ULPI PHYs need to be bound to their controllers with a
> >lookup. This adds helpers that the ULPI drivers can use to
> >do both, the registration of the PHY and the lookup, at the
> >same time.
> 
> >Signed-off-by: Heikki Krogerus 
> >Cc: Kishon Vijay Abraham I 
> >---
> >  drivers/phy/ulpi_phy.h | 31 +++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 drivers/phy/ulpi_phy.h
> 
> >diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> >new file mode 100644
> >index 000..ac49fb6
> >--- /dev/null
> >+++ b/drivers/phy/ulpi_phy.h
> >@@ -0,0 +1,31 @@
> >+#include 
> >+
> >+/**
> >+ * Helper that registers PHY for a ULPI device and adds a lookup for 
> >binding it
> >+ * and it's controller, which is always the parent.
> >+ */
> >+static inline struct phy
> >+*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> 
>Please either keep the prototype on a single line or don't break it like
> this. The result type should be all on the 1st line.

IMHO this is quite fine and readable. We can find this style in many
places of the kernel.
Maybe it's a matter of taste :)

BR, David

> 
> [...]
> 
> WBR, Sergei
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 11/12] phy: helpers for USB ULPI PHY registering

[David Cohen]

On Wed, Mar 18, 2015 at 05:46:44PM +0300, Sergei Shtylyov wrote:
 Hello.
 
 On 3/18/2015 3:40 PM, Heikki Krogerus wrote:
 
 ULPI PHYs need to be bound to their controllers with a
 lookup. This adds helpers that the ULPI drivers can use to
 do both, the registration of the PHY and the lookup, at the
 same time.
 
 Signed-off-by: Heikki Krogerus heikki.kroge...@linux.intel.com
 Cc: Kishon Vijay Abraham I kis...@ti.com
 ---
   drivers/phy/ulpi_phy.h | 31 +++
   1 file changed, 31 insertions(+)
   create mode 100644 drivers/phy/ulpi_phy.h
 
 diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
 new file mode 100644
 index 000..ac49fb6
 --- /dev/null
 +++ b/drivers/phy/ulpi_phy.h
 @@ -0,0 +1,31 @@
 +#include linux/phy/phy.h
 +
 +/**
 + * Helper that registers PHY for a ULPI device and adds a lookup for 
 binding it
 + * and it's controller, which is always the parent.
 + */
 +static inline struct phy
 +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
 
Please either keep the prototype on a single line or don't break it like
 this. The result type should be all on the 1st line.

IMHO this is quite fine and readable. We can find this style in many
places of the kernel.
Maybe it's a matter of taste :)

BR, David

 
 [...]
 
 WBR, Sergei
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 09/12] usb: dwc3: pci: add quirk for Baytrails

[David Cohen]

Hi Heikki,

On Wed, Mar 18, 2015 at 02:40:30PM +0200, Heikki Krogerus wrote:
 On some BYT platforms the USB2 PHY needs to be put into
 operational mode by the controller driver with GPIOs
 controlling the PHYs reset and cs signals.
 
 Signed-off-by: Heikki Krogerus heikki.kroge...@linux.intel.com
 ---
  drivers/usb/dwc3/dwc3-pci.c | 36 
  1 file changed, 36 insertions(+)
 
 diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
 index b773fb5..01cfdbd 100644
 --- a/drivers/usb/dwc3/dwc3-pci.c
 +++ b/drivers/usb/dwc3/dwc3-pci.c
 @@ -21,6 +21,8 @@
  #include linux/slab.h
  #include linux/pci.h
  #include linux/platform_device.h
 +#include linux/gpio/consumer.h
 +#include linux/acpi.h
  
  #include platform_data.h
  
 @@ -31,6 +33,15 @@
  #define PCI_DEVICE_ID_INTEL_SPTLP0x9d30
  #define PCI_DEVICE_ID_INTEL_SPTH 0xa130
  
 +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
 +static const struct acpi_gpio_params cs_gpios = { 1, 0, false };
 +
 +static const struct acpi_gpio_mapping acpi_dwc3_byt_gpios[] = {
 + { reset-gpios, reset_gpios, 1 },
 + { cs-gpios, cs_gpios, 1 },
 + { },
 +};
 +
  static int dwc3_pci_quirks(struct pci_dev *pdev)
  {
   if (pdev-vendor == PCI_VENDOR_ID_AMD 
 @@ -65,6 +76,31 @@ static int dwc3_pci_quirks(struct pci_dev *pdev)
   sizeof(pdata));
   }
  
 + if (pdev-vendor == PCI_VENDOR_ID_INTEL 
 + pdev-device == PCI_DEVICE_ID_INTEL_BYT) {
 + struct gpio_desc *gpio;
 +
 + acpi_dev_add_driver_gpios(ACPI_COMPANION(pdev-dev),
 +   acpi_dwc3_byt_gpios);
 +
 + gpio = gpiod_get(pdev-dev, reset);
 + if (IS_ERR(gpio))
 + return 0;
 +
 + /* These GPIOs will turn on the USB2 PHY */
 + gpiod_direction_output(gpio, 0);
 + gpiod_set_value_cansleep(gpio, 1);
 + gpiod_put(gpio);
 +
 + gpio = gpiod_get(pdev-dev, cs);

CS GPIO should be handled before RESET.

BR, David

 + if (!IS_ERR(gpio)) {
 + gpiod_direction_output(gpio, 0);
 + gpiod_set_value_cansleep(gpio, 1);
 + gpiod_put(gpio);
 + }
 + msleep(10);
 + }
 +
   return 0;
  }
  
 -- 
 2.1.4
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 00/12] usb: ulpi bus

[David Cohen]

Hi,

On Wed, Mar 18, 2015 at 02:40:21PM +0200, Heikki Krogerus wrote:
 
 Major changes since v1:
 - calling dwc3_phy_setup earlier and registering the ULPI interface there.
 - new property to dwc3 for selecting the interface in case of UTMI+ and ULPI 
 as
   suggested by Felipe
 - dwc3 soft reset before registration of the ULPI interface to sync the clocks
   as suggested by David
 - Including support for the BYT boards that have the GPIOs controlling the 
 reset
   and cs signals.
 
 
 Heikki Krogerus (12):
   usb: add bus type for USB ULPI
   usb: dwc3: USB2 PHY register access bits
   usb: dwc3: ULPI or UTMI+ select
   usb: dwc3: store driver data earlier
   usb: dwc3: cache hwparams earlier
   usb: dwc3: soft reset to it's own function
   usb: dwc3: setup phys earlier
   usb: dwc3: add hsphy_interface property
   usb: dwc3: pci: add quirk for Baytrails
   usb: dwc3: add ULPI interface support
   phy: helpers for USB ULPI PHY registering
   phy: add driver for TI TUSB1210 ULPI PHY

Except for my comment on patch 9/12, this version looks fine on my
side.

BR, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 00/12] usb: ulpi bus

[David Cohen]

On Wed, Mar 18, 2015 at 10:12:13AM -0700, David Cohen wrote:
 Hi,
 
 On Wed, Mar 18, 2015 at 02:40:21PM +0200, Heikki Krogerus wrote:
  
  Major changes since v1:
  - calling dwc3_phy_setup earlier and registering the ULPI interface there.
  - new property to dwc3 for selecting the interface in case of UTMI+ and 
  ULPI as
suggested by Felipe
  - dwc3 soft reset before registration of the ULPI interface to sync the 
  clocks
as suggested by David
  - Including support for the BYT boards that have the GPIOs controlling the 
  reset
and cs signals.
  
  
  Heikki Krogerus (12):
usb: add bus type for USB ULPI
usb: dwc3: USB2 PHY register access bits
usb: dwc3: ULPI or UTMI+ select
usb: dwc3: store driver data earlier
usb: dwc3: cache hwparams earlier
usb: dwc3: soft reset to it's own function
usb: dwc3: setup phys earlier
usb: dwc3: add hsphy_interface property
usb: dwc3: pci: add quirk for Baytrails
usb: dwc3: add ULPI interface support
phy: helpers for USB ULPI PHY registering
phy: add driver for TI TUSB1210 ULPI PHY
 
 Except for my comment on patch 9/12, this version looks fine on my
 side.

Making things official. For the whole series (except 9/12):
Acked-by: David Cohen david.a.co...@linux.intel.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] gpio: add set_active_low() function to gpio_chip

[David Cohen]

Some gpio controllers are capable of programming its pins' active-low
state. Let's add this new gpio_chip function for such cases and use it
in gpiolib.

When set_active_low() is implemented, we no longer need to do soft flips
on values from non-raw get functions.

Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

This is a RFC, not meant for integration (yet).

We have a GPIO controller that is capable of inverting the GPIO logical value
on hardware using a register (and GPIO voltage level if configured for output):
- If GPIO pin is configured for input, only the logical value is affected: the
  GPIO level stays the same but the read values are inverted (it affects even
  interrupt event triggers).
- If GPIO pin is configured for output, the GPIO level is also inverted.

Is it acceptable to expose this functionality via new gpio chip operation
set_active_low()?

Comments are welcome.

Br, David
---

 drivers/gpio/gpiolib-sysfs.c | 4 
 drivers/gpio/gpiolib.c   | 6 --
 include/linux/gpio/driver.h  | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 7722ed53bd65..316a1d008cad 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -289,11 +289,15 @@ static DEVICE_ATTR(edge, 0644, gpio_edge_show, 
gpio_edge_store);
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
int value)
 {
+   struct gpio_chip*chip = desc-chip;
int status = 0;
 
if (!!test_bit(FLAG_ACTIVE_LOW, desc-flags) == !!value)
return 0;
 
+   if (chip-set_active_low)
+   chip-set_active_low(chip, gpio_chip_hwgpio(desc), value);
+
if (value)
set_bit(FLAG_ACTIVE_LOW, desc-flags);
else
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1ca9295b2c10..028a0d34ddab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1196,7 +1196,8 @@ int gpiod_get_value(const struct gpio_desc *desc)
WARN_ON(desc-chip-can_sleep);
 
value = _gpiod_get_raw_value(desc);
-   if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
+   if (!desc-chip-set_active_low 
+   test_bit(FLAG_ACTIVE_LOW, desc-flags))
value = !value;
 
return value;
@@ -1550,7 +1551,8 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
return 0;
 
value = _gpiod_get_raw_value(desc);
-   if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
+   if (!desc-chip-set_active_low 
+   test_bit(FLAG_ACTIVE_LOW, desc-flags))
value = !value;
 
return value;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c497c62889d1..2d665352fd69 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -90,6 +90,8 @@ struct gpio_chip {
unsigned offset);
void(*set)(struct gpio_chip *chip,
unsigned offset, int value);
+   void(*set_active_low)(struct gpio_chip *chip,
+   unsigned offset, int value);
void(*set_multiple)(struct gpio_chip *chip,
unsigned long *mask,
unsigned long *bits);
-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Mon, Mar 16, 2015 at 09:46:00AM -0700, David Cohen wrote:
> Adding Mika to CC list.

Grrr :(
Adding for real now.

> 
> Br, David
> 
> On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
> > Hi Linus,
> > 
> > On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> > > On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > > > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> > > >  wrote:
> > > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > > 
> > > > >> I would put this adjacent to the phy driver somewhere in 
> > > > >> drivers/usb/*
> > > > >> and make the actual USB-driver thing handle its GPIOs directly.
> > > > >> But I guess David and Felipe have already discussed that as we're
> > > > >> seeing this patch?
> > > > >
> > > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > > driver (Linus, your input here would be nice :)
> > > > 
> > > > I don't understand what this means, does it mean a pin control function
> > > > somewhere else controlled by a GPIO pin?
> > > > 
> > > > Or do you mean a new combined pin control and GPIO driver (we have
> > > > plenty of these).
> > > > 
> > > > If you elaborate on what you need to do in that driver I might
> > > > understand it better.
> > 
> > This is a "virtual" ACPI device. Long story short we've 3 GPIOs
> > controlling the state of an USB OTG port. Neither of the hw components
> > controlled by these GPIOs are connected to a bus.
> > The ACPI table was implemented in such way that it considers this USB
> > port as a single "device" giving all 3 GPIOs to it.
> > 
> > When upstreaming this driver, the feedback I got is to split this driver
> > into simpler and more generic ones. FWIW ACPI tables are constructed
> > considering a valid Linux API during its implementation and are quite
> > difficult to change after product is released. The solution would be to
> > use MFD subsystem: this driver would create simpler children platform
> > devices.
> > 
> > But at least on ACPI case, GPIO API blocks the ability to create
> > children platform devices that require GPIO as platform data, despite
> > we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
> > 
> > Are we considering those driver deprecated from the GPIO point of view?
> > If yes, we need to provide an update for them (we can't let upstreamed
> > drivers broken). If no, we need to provide a way to move forward the
> > GPIO to children devices.
> > 
> > BR, David
> > 
> > > 
> > > there's a discrete mux (not something integrated in the SoC) whose
> > > select signal is tied to a GPIO (in some cases, more than one, but
> > > usually people use 2-state muxes).
> > > 
> > > -- 
> > > balbi
> > 
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Adding Mika to CC list.

Br, David

On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
> Hi Linus,
> 
> On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> > On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> > >  wrote:
> > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > 
> > > >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > >> and make the actual USB-driver thing handle its GPIOs directly.
> > > >> But I guess David and Felipe have already discussed that as we're
> > > >> seeing this patch?
> > > >
> > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > driver (Linus, your input here would be nice :)
> > > 
> > > I don't understand what this means, does it mean a pin control function
> > > somewhere else controlled by a GPIO pin?
> > > 
> > > Or do you mean a new combined pin control and GPIO driver (we have
> > > plenty of these).
> > > 
> > > If you elaborate on what you need to do in that driver I might
> > > understand it better.
> 
> This is a "virtual" ACPI device. Long story short we've 3 GPIOs
> controlling the state of an USB OTG port. Neither of the hw components
> controlled by these GPIOs are connected to a bus.
> The ACPI table was implemented in such way that it considers this USB
> port as a single "device" giving all 3 GPIOs to it.
> 
> When upstreaming this driver, the feedback I got is to split this driver
> into simpler and more generic ones. FWIW ACPI tables are constructed
> considering a valid Linux API during its implementation and are quite
> difficult to change after product is released. The solution would be to
> use MFD subsystem: this driver would create simpler children platform
> devices.
> 
> But at least on ACPI case, GPIO API blocks the ability to create
> children platform devices that require GPIO as platform data, despite
> we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
> 
> Are we considering those driver deprecated from the GPIO point of view?
> If yes, we need to provide an update for them (we can't let upstreamed
> drivers broken). If no, we need to provide a way to move forward the
> GPIO to children devices.
> 
> BR, David
> 
> > 
> > there's a discrete mux (not something integrated in the SoC) whose
> > select signal is tied to a GPIO (in some cases, more than one, but
> > usually people use 2-state muxes).
> > 
> > -- 
> > balbi
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Adding Mika to CC list.

Br, David

On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
 Hi Linus,
 
 On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
  On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
   On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
   david.a.co...@linux.intel.com wrote:
On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
   
I would put this adjacent to the phy driver somewhere in drivers/usb/*
and make the actual USB-driver thing handle its GPIOs directly.
But I guess David and Felipe have already discussed that as we're
seeing this patch?
   
- The mux functions would be controlled by a possible new pinctrl-gpio
driver (Linus, your input here would be nice :)
   
   I don't understand what this means, does it mean a pin control function
   somewhere else controlled by a GPIO pin?
   
   Or do you mean a new combined pin control and GPIO driver (we have
   plenty of these).
   
   If you elaborate on what you need to do in that driver I might
   understand it better.
 
 This is a virtual ACPI device. Long story short we've 3 GPIOs
 controlling the state of an USB OTG port. Neither of the hw components
 controlled by these GPIOs are connected to a bus.
 The ACPI table was implemented in such way that it considers this USB
 port as a single device giving all 3 GPIOs to it.
 
 When upstreaming this driver, the feedback I got is to split this driver
 into simpler and more generic ones. FWIW ACPI tables are constructed
 considering a valid Linux API during its implementation and are quite
 difficult to change after product is released. The solution would be to
 use MFD subsystem: this driver would create simpler children platform
 devices.
 
 But at least on ACPI case, GPIO API blocks the ability to create
 children platform devices that require GPIO as platform data, despite
 we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
 
 Are we considering those driver deprecated from the GPIO point of view?
 If yes, we need to provide an update for them (we can't let upstreamed
 drivers broken). If no, we need to provide a way to move forward the
 GPIO to children devices.
 
 BR, David
 
  
  there's a discrete mux (not something integrated in the SoC) whose
  select signal is tied to a GPIO (in some cases, more than one, but
  usually people use 2-state muxes).
  
  -- 
  balbi
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Mon, Mar 16, 2015 at 09:46:00AM -0700, David Cohen wrote:
 Adding Mika to CC list.

Grrr :(
Adding for real now.

 
 Br, David
 
 On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
  Hi Linus,
  
  On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
   On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
david.a.co...@linux.intel.com wrote:
 On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:

 I would put this adjacent to the phy driver somewhere in 
 drivers/usb/*
 and make the actual USB-driver thing handle its GPIOs directly.
 But I guess David and Felipe have already discussed that as we're
 seeing this patch?

 - The mux functions would be controlled by a possible new pinctrl-gpio
 driver (Linus, your input here would be nice :)

I don't understand what this means, does it mean a pin control function
somewhere else controlled by a GPIO pin?

Or do you mean a new combined pin control and GPIO driver (we have
plenty of these).

If you elaborate on what you need to do in that driver I might
understand it better.
  
  This is a virtual ACPI device. Long story short we've 3 GPIOs
  controlling the state of an USB OTG port. Neither of the hw components
  controlled by these GPIOs are connected to a bus.
  The ACPI table was implemented in such way that it considers this USB
  port as a single device giving all 3 GPIOs to it.
  
  When upstreaming this driver, the feedback I got is to split this driver
  into simpler and more generic ones. FWIW ACPI tables are constructed
  considering a valid Linux API during its implementation and are quite
  difficult to change after product is released. The solution would be to
  use MFD subsystem: this driver would create simpler children platform
  devices.
  
  But at least on ACPI case, GPIO API blocks the ability to create
  children platform devices that require GPIO as platform data, despite
  we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
  
  Are we considering those driver deprecated from the GPIO point of view?
  If yes, we need to provide an update for them (we can't let upstreamed
  drivers broken). If no, we need to provide a way to move forward the
  GPIO to children devices.
  
  BR, David
  
   
   there's a discrete mux (not something integrated in the SoC) whose
   select signal is tied to a GPIO (in some cases, more than one, but
   usually people use 2-state muxes).
   
   -- 
   balbi
  
  
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> >  wrote:
> > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > 
> > >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > >> and make the actual USB-driver thing handle its GPIOs directly.
> > >> But I guess David and Felipe have already discussed that as we're
> > >> seeing this patch?
> > >
> > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > driver (Linus, your input here would be nice :)
> > 
> > I don't understand what this means, does it mean a pin control function
> > somewhere else controlled by a GPIO pin?
> > 
> > Or do you mean a new combined pin control and GPIO driver (we have
> > plenty of these).
> > 
> > If you elaborate on what you need to do in that driver I might
> > understand it better.

This is a "virtual" ACPI device. Long story short we've 3 GPIOs
controlling the state of an USB OTG port. Neither of the hw components
controlled by these GPIOs are connected to a bus.
The ACPI table was implemented in such way that it considers this USB
port as a single "device" giving all 3 GPIOs to it.

When upstreaming this driver, the feedback I got is to split this driver
into simpler and more generic ones. FWIW ACPI tables are constructed
considering a valid Linux API during its implementation and are quite
difficult to change after product is released. The solution would be to
use MFD subsystem: this driver would create simpler children platform
devices.

But at least on ACPI case, GPIO API blocks the ability to create
children platform devices that require GPIO as platform data, despite
we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c

Are we considering those driver deprecated from the GPIO point of view?
If yes, we need to provide an update for them (we can't let upstreamed
drivers broken). If no, we need to provide a way to move forward the
GPIO to children devices.

BR, David

> 
> there's a discrete mux (not something integrated in the SoC) whose
> select signal is tied to a GPIO (in some cases, more than one, but
> usually people use 2-state muxes).
> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
 On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
  On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
  david.a.co...@linux.intel.com wrote:
   On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
  
   I would put this adjacent to the phy driver somewhere in drivers/usb/*
   and make the actual USB-driver thing handle its GPIOs directly.
   But I guess David and Felipe have already discussed that as we're
   seeing this patch?
  
   - The mux functions would be controlled by a possible new pinctrl-gpio
   driver (Linus, your input here would be nice :)
  
  I don't understand what this means, does it mean a pin control function
  somewhere else controlled by a GPIO pin?
  
  Or do you mean a new combined pin control and GPIO driver (we have
  plenty of these).
  
  If you elaborate on what you need to do in that driver I might
  understand it better.

This is a virtual ACPI device. Long story short we've 3 GPIOs
controlling the state of an USB OTG port. Neither of the hw components
controlled by these GPIOs are connected to a bus.
The ACPI table was implemented in such way that it considers this USB
port as a single device giving all 3 GPIOs to it.

When upstreaming this driver, the feedback I got is to split this driver
into simpler and more generic ones. FWIW ACPI tables are constructed
considering a valid Linux API during its implementation and are quite
difficult to change after product is released. The solution would be to
use MFD subsystem: this driver would create simpler children platform
devices.

But at least on ACPI case, GPIO API blocks the ability to create
children platform devices that require GPIO as platform data, despite
we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c

Are we considering those driver deprecated from the GPIO point of view?
If yes, we need to provide an update for them (we can't let upstreamed
drivers broken). If no, we need to provide a way to move forward the
GPIO to children devices.

BR, David

 
 there's a discrete mux (not something integrated in the SoC) whose
 select signal is tied to a GPIO (in some cases, more than one, but
 usually people use 2-state muxes).
 
 -- 
 balbi


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

On Wed, Feb 25, 2015 at 10:34:45AM +0900, Alexandre Courbot wrote:
> On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
>  wrote:
> > Hi,
> >
> >> If we decide to go ahead with the solution proposed by this patch for
> >> practical reasons (which are good reasons indeed), I still have one
> >> problem with its current form.
> >>
> >> As the discussion highlighted, this is an ACPI problem, so I'd very
> >> much like it to be confined to the ACPI GPIO code, to be enabled only
> >> when ACPI is, and to use function names that start with acpi_gpio. The
> >> current implementation leverages platform lookup, making said lookup
> >> less efficient in the process and bringing confusion about its
> >> purpose. Although the two processes are indeed similar, they are
> >> separate things: one is a legitimate way to map GPIOs, the other is a
> >> fixup for broken firmware.
> >>
> >> I suppose we all agree this is a hackish fix, so let's confine it as
> >> much as we can.
> >
> > Are we considering MFD cases hackish as well?
> > i.e. if we have a driver that needs to register children devices and this
> > driver needs to pass GPIO to a child.
> 
> In that case wouldn't the GPIO be best defined in the child node
> itself, for the child device's driver to directly probe?

In my case [1] I need 2 "virtual devices" (and more in future) to be
part of an USB OTG port control. I call it virtual because they are too
simple components connected to no bus and controlled by GPIOs:
- a fixed regulator controlled by GPIO
- a generic mux controlled by GPIO

I'd need to request official ACPI HID for them in order to make them
self-sufficient.

I can go ahead with this approach, but we have many examples of drivers
on upstream that are platform driver expecting to receive gpio via
platform data (e.g. extcon-gpio). The ACPI table of some products on
market were defined following this concept and won't change anymore.

Br, David

[1] https://lkml.org/lkml/2015/2/19/411
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

On Wed, Feb 25, 2015 at 10:34:45AM +0900, Alexandre Courbot wrote:
 On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
 david.a.co...@linux.intel.com wrote:
  Hi,
 
  If we decide to go ahead with the solution proposed by this patch for
  practical reasons (which are good reasons indeed), I still have one
  problem with its current form.
 
  As the discussion highlighted, this is an ACPI problem, so I'd very
  much like it to be confined to the ACPI GPIO code, to be enabled only
  when ACPI is, and to use function names that start with acpi_gpio. The
  current implementation leverages platform lookup, making said lookup
  less efficient in the process and bringing confusion about its
  purpose. Although the two processes are indeed similar, they are
  separate things: one is a legitimate way to map GPIOs, the other is a
  fixup for broken firmware.
 
  I suppose we all agree this is a hackish fix, so let's confine it as
  much as we can.
 
  Are we considering MFD cases hackish as well?
  i.e. if we have a driver that needs to register children devices and this
  driver needs to pass GPIO to a child.
 
 In that case wouldn't the GPIO be best defined in the child node
 itself, for the child device's driver to directly probe?

In my case [1] I need 2 virtual devices (and more in future) to be
part of an USB OTG port control. I call it virtual because they are too
simple components connected to no bus and controlled by GPIOs:
- a fixed regulator controlled by GPIO
- a generic mux controlled by GPIO

I'd need to request official ACPI HID for them in order to make them
self-sufficient.

I can go ahead with this approach, but we have many examples of drivers
on upstream that are platform driver expecting to receive gpio via
platform data (e.g. extcon-gpio). The ACPI table of some products on
market were defined following this concept and won't change anymore.

Br, David

[1] https://lkml.org/lkml/2015/2/19/411
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

Hi,

> If we decide to go ahead with the solution proposed by this patch for
> practical reasons (which are good reasons indeed), I still have one
> problem with its current form.
> 
> As the discussion highlighted, this is an ACPI problem, so I'd very
> much like it to be confined to the ACPI GPIO code, to be enabled only
> when ACPI is, and to use function names that start with acpi_gpio. The
> current implementation leverages platform lookup, making said lookup
> less efficient in the process and bringing confusion about its
> purpose. Although the two processes are indeed similar, they are
> separate things: one is a legitimate way to map GPIOs, the other is a
> fixup for broken firmware.
> 
> I suppose we all agree this is a hackish fix, so let's confine it as
> much as we can.

Are we considering MFD cases hackish as well?
i.e. if we have a driver that needs to register children devices and this
driver needs to pass GPIO to a child.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

[snip]

> Felipe suggested to "divide to conquer" instead of having a single
> extcon driver to handle all these functions:
> 
> - The mux functions would be controlled by a possible new pinctrl-gpio
> driver (Linus, your input here would be nice :)
> - The VBUS would be a fixed regulator
> - The USB ID would make usage of existent extcon-gpio
> 
> But the on fw side, this is a single ACPI device representing a virtual
> device for USB OTG port, which is nothing but a bunch of independent
> GPIOs.
> 
> I could make a mfd driver to register devices for those simpler and more
> generic drivers, but according to [1] community recognized it as a hack
> with ACPI since I'd need to give them the GPIO without requesting on
> mfd.

I believe this case could be resumed in:

Would be [1] acceptable for mfd drivers?
- If yes, I can split this driver into more generic ones
- If no, I see no other option but having this driver fully controlling
  the USB OTG port.

BR, David

[1] https://lkml.org/lkml/2014/12/18/82
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

Hi,

 If we decide to go ahead with the solution proposed by this patch for
 practical reasons (which are good reasons indeed), I still have one
 problem with its current form.
 
 As the discussion highlighted, this is an ACPI problem, so I'd very
 much like it to be confined to the ACPI GPIO code, to be enabled only
 when ACPI is, and to use function names that start with acpi_gpio. The
 current implementation leverages platform lookup, making said lookup
 less efficient in the process and bringing confusion about its
 purpose. Although the two processes are indeed similar, they are
 separate things: one is a legitimate way to map GPIOs, the other is a
 fixup for broken firmware.
 
 I suppose we all agree this is a hackish fix, so let's confine it as
 much as we can.

Are we considering MFD cases hackish as well?
i.e. if we have a driver that needs to register children devices and this
driver needs to pass GPIO to a child.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

[snip]

 Felipe suggested to divide to conquer instead of having a single
 extcon driver to handle all these functions:
 
 - The mux functions would be controlled by a possible new pinctrl-gpio
 driver (Linus, your input here would be nice :)
 - The VBUS would be a fixed regulator
 - The USB ID would make usage of existent extcon-gpio
 
 But the on fw side, this is a single ACPI device representing a virtual
 device for USB OTG port, which is nothing but a bunch of independent
 GPIOs.
 
 I could make a mfd driver to register devices for those simpler and more
 generic drivers, but according to [1] community recognized it as a hack
 with ACPI since I'd need to give them the GPIO without requesting on
 mfd.

I believe this case could be resumed in:

Would be [1] acceptable for mfd drivers?
- If yes, I can split this driver into more generic ones
- If no, I see no other option but having this driver fully controlling
  the USB OTG port.

BR, David

[1] https://lkml.org/lkml/2014/12/18/82
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 02:00:26PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:59:27AM -0800, David Cohen wrote:
> > On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
> > > On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> > > > Hi Linus and Robert,
> > > > 
> > > > CC'ing Heikki as it involves a RFC from him.
> > > > 
> > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > > > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga 
> > > > >  wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > > > > >> Some Intel platforms have an USB OTG port fully (or partially)
> > > > > >> controlled by GPIOs:
> > > > > >>
> > > > > >> (1) USB ID is connected directly to a pulled up GPIO.
> > > > > >>
> > > > > >> Optionally:
> > > > > >> (2) VBUS is enabled/disabled by a GPIO
> > > > > >> (3) Platform has 2 USB controllers connected to same port: one for
> > > > > >> device and one for host role. D+/- are switched between phys.
> > > > > >> according to this GPIO level.
> > > > > >>
> > > > > >> This driver configures USB OTG port for device or host role 
> > > > > >> according to
> > > > > >> USB ID value.
> > > > > >>  - If USB ID's GPIO level is low, OTG port is configured for host 
> > > > > >> role
> > > > > >>by sourcing VBUS and switching D+/- to host phy.
> > > > > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > > > > >>configured for device role by not sourcing VBUS and switching 
> > > > > >> D+/- to
> > > > > >>device controller.
> > > > > >
> > > > > > IMO it's not very elegant to handle VBUS power on/off in extcon 
> > > > > > driver.
> > > > > > Creating fixed regulator would allow to make VBUS handling more 
> > > > > > generic.
> > > > 
> > > > I agree. But please, see below.
> > > > 
> > > > > 
> > > > > IMHO it's just layers of abstraction piled on top of each other here.
> > > > > 
> > > > > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > > > and make the actual USB-driver thing handle its GPIOs directly.
> > > > > But I guess David and Felipe have already discussed that as we're
> > > > > seeing this patch?
> > > > 
> > > > Felipe suggested to "divide to conquer" instead of having a single
> > > > extcon driver to handle all these functions:
> > > > 
> > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > driver (Linus, your input here would be nice :)
> > > > - The VBUS would be a fixed regulator
> > > > - The USB ID would make usage of existent extcon-gpio
> > > > 
> > > > But the on fw side, this is a single ACPI device representing a virtual
> > > > device for USB OTG port, which is nothing but a bunch of independent
> > > > GPIOs.
> > > > 
> > > > I could make a mfd driver to register devices for those simpler and more
> > > > generic drivers, but according to [1] community recognized it as a hack
> > > > with ACPI since I'd need to give them the GPIO without requesting on
> > > > mfd.
> > > > 
> > > > I'm open for suggestions :)
> > > 
> > > use MFD to create children devices and pass the required data to each
> > > one ?
> > 
> > I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
> > and then give them to children devices.
> > Heikki proposed a way to do that on [1], but it got nack'ed by community.
> 
> you missed [1] :-)

Oops. Looks like it went away during your past reply :)

[1] https://lkml.org/lkml/2014/12/18/82

Br, David

> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> > Hi Linus and Robert,
> > 
> > CC'ing Heikki as it involves a RFC from him.
> > 
> > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga  
> > > wrote:
> > > > Hi David,
> > > >
> > > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > > >> Some Intel platforms have an USB OTG port fully (or partially)
> > > >> controlled by GPIOs:
> > > >>
> > > >> (1) USB ID is connected directly to a pulled up GPIO.
> > > >>
> > > >> Optionally:
> > > >> (2) VBUS is enabled/disabled by a GPIO
> > > >> (3) Platform has 2 USB controllers connected to same port: one for
> > > >> device and one for host role. D+/- are switched between phys.
> > > >> according to this GPIO level.
> > > >>
> > > >> This driver configures USB OTG port for device or host role according 
> > > >> to
> > > >> USB ID value.
> > > >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> > > >>by sourcing VBUS and switching D+/- to host phy.
> > > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > > >>configured for device role by not sourcing VBUS and switching D+/- 
> > > >> to
> > > >>device controller.
> > > >
> > > > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > > > Creating fixed regulator would allow to make VBUS handling more generic.
> > 
> > I agree. But please, see below.
> > 
> > > 
> > > IMHO it's just layers of abstraction piled on top of each other here.
> > > 
> > > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > and make the actual USB-driver thing handle its GPIOs directly.
> > > But I guess David and Felipe have already discussed that as we're
> > > seeing this patch?
> > 
> > Felipe suggested to "divide to conquer" instead of having a single
> > extcon driver to handle all these functions:
> > 
> > - The mux functions would be controlled by a possible new pinctrl-gpio
> > driver (Linus, your input here would be nice :)
> > - The VBUS would be a fixed regulator
> > - The USB ID would make usage of existent extcon-gpio
> > 
> > But the on fw side, this is a single ACPI device representing a virtual
> > device for USB OTG port, which is nothing but a bunch of independent
> > GPIOs.
> > 
> > I could make a mfd driver to register devices for those simpler and more
> > generic drivers, but according to [1] community recognized it as a hack
> > with ACPI since I'd need to give them the GPIO without requesting on
> > mfd.
> > 
> > I'm open for suggestions :)
> 
> use MFD to create children devices and pass the required data to each
> one ?

I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
and then give them to children devices.
Heikki proposed a way to do that on [1], but it got nack'ed by community.

Br, David

> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 08:10:34PM +0100, Paul Bolle wrote:
> On Fri, 2015-02-20 at 11:02 -0800, David Cohen wrote:
> > On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > > As always, comments are welcome.
> > > 
> > > Are nits welcome too?
> > > 
> > > > +MODULE_LICENSE("GPLv2");
> > > 
> > > You probably meant
> > > MODULE_LICENSE("GPL v2")
> > > 
> > > Didn't that trigger a warning or error?
> > 
> > checkpatch showed no warning about that, not even with --strict option.
> > I believe both ways are fine. But I can add the space.
> 
> "GPLv2" is not mentioned in include/linux/license.h so, as I remember,
> it would taint the kernel, and a warning will be printed when you load
> that module.

Ack :)
I'll add a space on next version.

Br, David

> 
> Thanks,
> 
> 
> Paul Bolle
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:09:00PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:02:26AM -0800, David Cohen wrote:
> > Hi,
> > 
> > On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > > As always, comments are welcome.
> > > 
> > > Are nits welcome too?
> > > 
> > > > +MODULE_LICENSE("GPLv2");
> > > 
> > > You probably meant
> > > MODULE_LICENSE("GPL v2")
> > > 
> > > Didn't that trigger a warning or error?
> > 
> > checkpatch showed no warning about that, not even with --strict option.
> > I believe both ways are fine. But I can add the space.
> 
> Documentation says it should be with space:
> 
> /*
>  * The following license idents are currently accepted as indicating free
>  * software modules
>  *
>  *"GPL"   [GNU Public License v2 or later]
>  *"GPL v2"[GNU Public License v2]

Thanks :)
I'll add it.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus and Robert,

CC'ing Heikki as it involves a RFC from him.

On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga  wrote:
> > Hi David,
> >
> > On 02/19/2015 08:59 PM, David Cohen wrote:
> >> Some Intel platforms have an USB OTG port fully (or partially)
> >> controlled by GPIOs:
> >>
> >> (1) USB ID is connected directly to a pulled up GPIO.
> >>
> >> Optionally:
> >> (2) VBUS is enabled/disabled by a GPIO
> >> (3) Platform has 2 USB controllers connected to same port: one for
> >> device and one for host role. D+/- are switched between phys.
> >> according to this GPIO level.
> >>
> >> This driver configures USB OTG port for device or host role according to
> >> USB ID value.
> >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> >>by sourcing VBUS and switching D+/- to host phy.
> >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> >>configured for device role by not sourcing VBUS and switching D+/- to
> >>device controller.
> >
> > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > Creating fixed regulator would allow to make VBUS handling more generic.

I agree. But please, see below.

> 
> IMHO it's just layers of abstraction piled on top of each other here.
> 
> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> and make the actual USB-driver thing handle its GPIOs directly.
> But I guess David and Felipe have already discussed that as we're
> seeing this patch?

Felipe suggested to "divide to conquer" instead of having a single
extcon driver to handle all these functions:

- The mux functions would be controlled by a possible new pinctrl-gpio
driver (Linus, your input here would be nice :)
- The VBUS would be a fixed regulator
- The USB ID would make usage of existent extcon-gpio

But the on fw side, this is a single ACPI device representing a virtual
device for USB OTG port, which is nothing but a bunch of independent
GPIOs.

I could make a mfd driver to register devices for those simpler and more
generic drivers, but according to [1] community recognized it as a hack
with ACPI since I'd need to give them the GPIO without requesting on
mfd.

I'm open for suggestions :)

Br, David

[1] https://lkml.org/lkml/2014/12/18/82

> 
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > As always, comments are welcome.
> 
> Are nits welcome too?
> 
> > +MODULE_LICENSE("GPLv2");
> 
> You probably meant
> MODULE_LICENSE("GPL v2")
> 
> Didn't that trigger a warning or error?

checkpatch showed no warning about that, not even with --strict option.
I believe both ways are fine. But I can add the space.

Br, David

> 
> 
> Paul Bolle
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 02:00:26PM -0600, Felipe Balbi wrote:
 On Fri, Feb 20, 2015 at 11:59:27AM -0800, David Cohen wrote:
  On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
   On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
Hi Linus and Robert,

CC'ing Heikki as it involves a RFC from him.

On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
 On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga 
 r.bald...@samsung.com wrote:
  Hi David,
 
  On 02/19/2015 08:59 PM, David Cohen wrote:
  Some Intel platforms have an USB OTG port fully (or partially)
  controlled by GPIOs:
 
  (1) USB ID is connected directly to a pulled up GPIO.
 
  Optionally:
  (2) VBUS is enabled/disabled by a GPIO
  (3) Platform has 2 USB controllers connected to same port: one for
  device and one for host role. D+/- are switched between phys.
  according to this GPIO level.
 
  This driver configures USB OTG port for device or host role 
  according to
  USB ID value.
   - If USB ID's GPIO level is low, OTG port is configured for host 
  role
 by sourcing VBUS and switching D+/- to host phy.
   - If USB ID's GPIO level is high, by standard, the OTG port is
 configured for device role by not sourcing VBUS and switching 
  D+/- to
 device controller.
 
  IMO it's not very elegant to handle VBUS power on/off in extcon 
  driver.
  Creating fixed regulator would allow to make VBUS handling more 
  generic.

I agree. But please, see below.

 
 IMHO it's just layers of abstraction piled on top of each other here.
 
 I would put this adjacent to the phy driver somewhere in drivers/usb/*
 and make the actual USB-driver thing handle its GPIOs directly.
 But I guess David and Felipe have already discussed that as we're
 seeing this patch?

Felipe suggested to divide to conquer instead of having a single
extcon driver to handle all these functions:

- The mux functions would be controlled by a possible new pinctrl-gpio
driver (Linus, your input here would be nice :)
- The VBUS would be a fixed regulator
- The USB ID would make usage of existent extcon-gpio

But the on fw side, this is a single ACPI device representing a virtual
device for USB OTG port, which is nothing but a bunch of independent
GPIOs.

I could make a mfd driver to register devices for those simpler and more
generic drivers, but according to [1] community recognized it as a hack
with ACPI since I'd need to give them the GPIO without requesting on
mfd.

I'm open for suggestions :)
   
   use MFD to create children devices and pass the required data to each
   one ?
  
  I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
  and then give them to children devices.
  Heikki proposed a way to do that on [1], but it got nack'ed by community.
 
 you missed [1] :-)

Oops. Looks like it went away during your past reply :)

[1] https://lkml.org/lkml/2014/12/18/82

Br, David

 
 -- 
 balbi


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
 On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
  As always, comments are welcome.
 
 Are nits welcome too?
 
  +MODULE_LICENSE(GPLv2);
 
 You probably meant
 MODULE_LICENSE(GPL v2)
 
 Didn't that trigger a warning or error?

checkpatch showed no warning about that, not even with --strict option.
I believe both ways are fine. But I can add the space.

Br, David

 
 
 Paul Bolle
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 08:10:34PM +0100, Paul Bolle wrote:
 On Fri, 2015-02-20 at 11:02 -0800, David Cohen wrote:
  On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
   On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
As always, comments are welcome.
   
   Are nits welcome too?
   
+MODULE_LICENSE(GPLv2);
   
   You probably meant
   MODULE_LICENSE(GPL v2)
   
   Didn't that trigger a warning or error?
  
  checkpatch showed no warning about that, not even with --strict option.
  I believe both ways are fine. But I can add the space.
 
 GPLv2 is not mentioned in include/linux/license.h so, as I remember,
 it would taint the kernel, and a warning will be printed when you load
 that module.

Ack :)
I'll add a space on next version.

Br, David

 
 Thanks,
 
 
 Paul Bolle
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:09:00PM -0600, Felipe Balbi wrote:
 On Fri, Feb 20, 2015 at 11:02:26AM -0800, David Cohen wrote:
  Hi,
  
  On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
   On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
As always, comments are welcome.
   
   Are nits welcome too?
   
+MODULE_LICENSE(GPLv2);
   
   You probably meant
   MODULE_LICENSE(GPL v2)
   
   Didn't that trigger a warning or error?
  
  checkpatch showed no warning about that, not even with --strict option.
  I believe both ways are fine. But I can add the space.
 
 Documentation says it should be with space:
 
 /*
  * The following license idents are currently accepted as indicating free
  * software modules
  *
  *GPL   [GNU Public License v2 or later]
  *GPL v2[GNU Public License v2]

Thanks :)
I'll add it.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus and Robert,

CC'ing Heikki as it involves a RFC from him.

On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
 On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga r.bald...@samsung.com wrote:
  Hi David,
 
  On 02/19/2015 08:59 PM, David Cohen wrote:
  Some Intel platforms have an USB OTG port fully (or partially)
  controlled by GPIOs:
 
  (1) USB ID is connected directly to a pulled up GPIO.
 
  Optionally:
  (2) VBUS is enabled/disabled by a GPIO
  (3) Platform has 2 USB controllers connected to same port: one for
  device and one for host role. D+/- are switched between phys.
  according to this GPIO level.
 
  This driver configures USB OTG port for device or host role according to
  USB ID value.
   - If USB ID's GPIO level is low, OTG port is configured for host role
 by sourcing VBUS and switching D+/- to host phy.
   - If USB ID's GPIO level is high, by standard, the OTG port is
 configured for device role by not sourcing VBUS and switching D+/- to
 device controller.
 
  IMO it's not very elegant to handle VBUS power on/off in extcon driver.
  Creating fixed regulator would allow to make VBUS handling more generic.

I agree. But please, see below.

 
 IMHO it's just layers of abstraction piled on top of each other here.
 
 I would put this adjacent to the phy driver somewhere in drivers/usb/*
 and make the actual USB-driver thing handle its GPIOs directly.
 But I guess David and Felipe have already discussed that as we're
 seeing this patch?

Felipe suggested to divide to conquer instead of having a single
extcon driver to handle all these functions:

- The mux functions would be controlled by a possible new pinctrl-gpio
driver (Linus, your input here would be nice :)
- The VBUS would be a fixed regulator
- The USB ID would make usage of existent extcon-gpio

But the on fw side, this is a single ACPI device representing a virtual
device for USB OTG port, which is nothing but a bunch of independent
GPIOs.

I could make a mfd driver to register devices for those simpler and more
generic drivers, but according to [1] community recognized it as a hack
with ACPI since I'd need to give them the GPIO without requesting on
mfd.

I'm open for suggestions :)

Br, David

[1] https://lkml.org/lkml/2014/12/18/82

 
 Yours,
 Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
 On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
  Hi Linus and Robert,
  
  CC'ing Heikki as it involves a RFC from him.
  
  On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
   On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga r.bald...@samsung.com 
   wrote:
Hi David,
   
On 02/19/2015 08:59 PM, David Cohen wrote:
Some Intel platforms have an USB OTG port fully (or partially)
controlled by GPIOs:
   
(1) USB ID is connected directly to a pulled up GPIO.
   
Optionally:
(2) VBUS is enabled/disabled by a GPIO
(3) Platform has 2 USB controllers connected to same port: one for
device and one for host role. D+/- are switched between phys.
according to this GPIO level.
   
This driver configures USB OTG port for device or host role according 
to
USB ID value.
 - If USB ID's GPIO level is low, OTG port is configured for host role
   by sourcing VBUS and switching D+/- to host phy.
 - If USB ID's GPIO level is high, by standard, the OTG port is
   configured for device role by not sourcing VBUS and switching D+/- 
to
   device controller.
   
IMO it's not very elegant to handle VBUS power on/off in extcon driver.
Creating fixed regulator would allow to make VBUS handling more generic.
  
  I agree. But please, see below.
  
   
   IMHO it's just layers of abstraction piled on top of each other here.
   
   I would put this adjacent to the phy driver somewhere in drivers/usb/*
   and make the actual USB-driver thing handle its GPIOs directly.
   But I guess David and Felipe have already discussed that as we're
   seeing this patch?
  
  Felipe suggested to divide to conquer instead of having a single
  extcon driver to handle all these functions:
  
  - The mux functions would be controlled by a possible new pinctrl-gpio
  driver (Linus, your input here would be nice :)
  - The VBUS would be a fixed regulator
  - The USB ID would make usage of existent extcon-gpio
  
  But the on fw side, this is a single ACPI device representing a virtual
  device for USB OTG port, which is nothing but a bunch of independent
  GPIOs.
  
  I could make a mfd driver to register devices for those simpler and more
  generic drivers, but according to [1] community recognized it as a hack
  with ACPI since I'd need to give them the GPIO without requesting on
  mfd.
  
  I'm open for suggestions :)
 
 use MFD to create children devices and pass the required data to each
 one ?

I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
and then give them to children devices.
Heikki proposed a way to do that on [1], but it got nack'ed by community.

Br, David

 
 -- 
 balbi


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Some Intel platforms have an USB OTG port fully (or partially)
controlled by GPIOs:

(1) USB ID is connected directly to a pulled up GPIO.

Optionally:
(2) VBUS is enabled/disabled by a GPIO
(3) Platform has 2 USB controllers connected to same port: one for
device and one for host role. D+/- are switched between phys.
according to this GPIO level.

This driver configures USB OTG port for device or host role according to
USB ID value.
 - If USB ID's GPIO level is low, OTG port is configured for host role
   by sourcing VBUS and switching D+/- to host phy.
 - If USB ID's GPIO level is high, by standard, the OTG port is
   configured for device role by not sourcing VBUS and switching D+/- to
   device controller.

Signed-off-by: David Cohen 
---

Hi,

Since splitting this driver into smaller pieces would result in ugly fixes WRT
ACPI, I'm resending the same approach.
This time I addressed all comments I got from RFC version.

As always, comments are welcome.

Br, David
---

 drivers/extcon/Kconfig   |   8 ++
 drivers/extcon/Makefile  |   1 +
 drivers/extcon/extcon-otg_gpio.c | 186 +++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/extcon/extcon-otg_gpio.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de6fa54..986ca7da9c1b 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -63,6 +63,14 @@ config EXTCON_MAX8997
  Maxim MAX8997 PMIC. The MAX8997 MUIC is a USB port accessory
  detector and switch.
 
+config EXTCON_OTG_GPIO
+   tristate "VIRTUAL USB OTG PORT support"
+   depends on GPIOLIB && ACPI
+   help
+ Say Y here to enable support for virtual USB OTG port device
+ controlled by GPIOs. This driver can be used when at least USB ID pin
+ is connected directly to GPIO.
+
 config EXTCON_PALMAS
tristate "Palmas USB EXTCON support"
depends on MFD_PALMAS
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42e5a27..0d5b152b51ea 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_EXTCON_GPIO)   += extcon-gpio.o
 obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX8997)   += extcon-max8997.o
+obj-$(CONFIG_EXTCON_OTG_GPIO)  += extcon-otg_gpio.o
 obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
new file mode 100644
index ..7dfa2c15b562
--- /dev/null
+++ b/drivers/extcon/extcon-otg_gpio.c
@@ -0,0 +1,186 @@
+/*
+ * Virtual USB OTG Port driver controlled by gpios
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: David Cohen 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME   "usb_otg_port"
+
+struct vuport {
+   struct device *dev;
+   struct gpio_desc *gpio_vbus_en;
+   struct gpio_desc *gpio_usb_id;
+   struct gpio_desc *gpio_usb_mux;
+
+   struct extcon_dev edev;
+};
+
+static const char *vuport_extcon_cable[] = {
+   [0] = "USB-Host",
+   NULL,
+};
+
+/*
+ * If id == 1, USB port should be set to peripheral
+ * if id == 0, USB port should be set to host
+ *
+ * Peripheral: set USB mux to peripheral and disable VBUS
+ * Host: set USB mux to host and enable VBUS
+ */
+static void vuport_set_port(struct vuport *vup, int id)
+{
+   int mux_val = id;
+   int vbus_val = !id;
+
+   if (!IS_ERR(vup->gpio_usb_mux))
+   gpiod_direction_output(vup->gpio_usb_mux, mux_val);
+
+   if (!IS_ERR(vup->gpio_vbus_en))
+   gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
+}
+
+static void vuport_do_usb_id(struct vuport *vup)
+{
+   int id = gpiod_get_value(vup->gpio_usb_id);
+
+   dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
+
+   /*
+* id == 1: PERIPHERAL
+* id == 0: HOST
+*/
+   vuport_set_port(vup, id);
+
+   /*
+* id == 0: HOST connected
+* id == 1: Host disconnected
+*/
+   extcon_set_cable_state(>edev, "USB-Host", !id);
+}
+
+static irqreturn_t vuport_thread_isr(int irq, void *priv)
+{
+   struct vuport *vup = priv;
+
+   vuport_do_usb_

[PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Some Intel platforms have an USB OTG port fully (or partially)
controlled by GPIOs:

(1) USB ID is connected directly to a pulled up GPIO.

Optionally:
(2) VBUS is enabled/disabled by a GPIO
(3) Platform has 2 USB controllers connected to same port: one for
device and one for host role. D+/- are switched between phys.
according to this GPIO level.

This driver configures USB OTG port for device or host role according to
USB ID value.
 - If USB ID's GPIO level is low, OTG port is configured for host role
   by sourcing VBUS and switching D+/- to host phy.
 - If USB ID's GPIO level is high, by standard, the OTG port is
   configured for device role by not sourcing VBUS and switching D+/- to
   device controller.

Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

Since splitting this driver into smaller pieces would result in ugly fixes WRT
ACPI, I'm resending the same approach.
This time I addressed all comments I got from RFC version.

As always, comments are welcome.

Br, David
---

 drivers/extcon/Kconfig   |   8 ++
 drivers/extcon/Makefile  |   1 +
 drivers/extcon/extcon-otg_gpio.c | 186 +++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/extcon/extcon-otg_gpio.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de6fa54..986ca7da9c1b 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -63,6 +63,14 @@ config EXTCON_MAX8997
  Maxim MAX8997 PMIC. The MAX8997 MUIC is a USB port accessory
  detector and switch.
 
+config EXTCON_OTG_GPIO
+   tristate VIRTUAL USB OTG PORT support
+   depends on GPIOLIB  ACPI
+   help
+ Say Y here to enable support for virtual USB OTG port device
+ controlled by GPIOs. This driver can be used when at least USB ID pin
+ is connected directly to GPIO.
+
 config EXTCON_PALMAS
tristate Palmas USB EXTCON support
depends on MFD_PALMAS
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42e5a27..0d5b152b51ea 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_EXTCON_GPIO)   += extcon-gpio.o
 obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX8997)   += extcon-max8997.o
+obj-$(CONFIG_EXTCON_OTG_GPIO)  += extcon-otg_gpio.o
 obj-$(CONFIG_EXTCON_PALMAS)+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)   += extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
new file mode 100644
index ..7dfa2c15b562
--- /dev/null
+++ b/drivers/extcon/extcon-otg_gpio.c
@@ -0,0 +1,186 @@
+/*
+ * Virtual USB OTG Port driver controlled by gpios
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: David Cohen david.a.co...@linux.intel.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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/acpi.h
+#include linux/extcon.h
+#include linux/gpio/consumer.h
+#include linux/interrupt.h
+#include linux/module.h
+#include linux/platform_device.h
+
+#define DRV_NAME   usb_otg_port
+
+struct vuport {
+   struct device *dev;
+   struct gpio_desc *gpio_vbus_en;
+   struct gpio_desc *gpio_usb_id;
+   struct gpio_desc *gpio_usb_mux;
+
+   struct extcon_dev edev;
+};
+
+static const char *vuport_extcon_cable[] = {
+   [0] = USB-Host,
+   NULL,
+};
+
+/*
+ * If id == 1, USB port should be set to peripheral
+ * if id == 0, USB port should be set to host
+ *
+ * Peripheral: set USB mux to peripheral and disable VBUS
+ * Host: set USB mux to host and enable VBUS
+ */
+static void vuport_set_port(struct vuport *vup, int id)
+{
+   int mux_val = id;
+   int vbus_val = !id;
+
+   if (!IS_ERR(vup-gpio_usb_mux))
+   gpiod_direction_output(vup-gpio_usb_mux, mux_val);
+
+   if (!IS_ERR(vup-gpio_vbus_en))
+   gpiod_direction_output(vup-gpio_vbus_en, vbus_val);
+}
+
+static void vuport_do_usb_id(struct vuport *vup)
+{
+   int id = gpiod_get_value(vup-gpio_usb_id);
+
+   dev_info(vup-dev, USB PORT ID: %s\n, id ? PERIPHERAL : HOST);
+
+   /*
+* id == 1: PERIPHERAL
+* id == 0: HOST
+*/
+   vuport_set_port(vup, id);
+
+   /*
+* id == 0: HOST connected
+* id == 1: Host disconnected
+*/
+   extcon_set_cable_state(vup-edev, USB-Host, !id);
+}
+
+static irqreturn_t vuport_thread_isr(int irq, void *priv)
+{
+   struct vuport *vup

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Wed, Feb 18, 2015 at 12:17:06PM +0200, Mika Westerberg wrote:
> On Tue, Feb 17, 2015 at 11:35:23AM -0800, David Cohen wrote:
> > Hi,
> > 
> > Adding Mika.
> > 
> > On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > > > > (3) Platform has 2 USB controllers connected to same port: one 
> > > > > > > > for
> > > > > > > > device and one for host role. D+/- are switched between phys
> > > > > > > > by GPIO.
> > > > > > > 
> > > > > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > > > > 
> > > > > > > 
> > > > > > >  .---..  ..
> > > > > > >  |   ||  ||  D+
> > > > > > >  |   ||  ||<-.
> > > > > > >  |   ||  ||  |
> > > > > > >  |   |USB Host-->|P   |  |
> > > > > > >  |   ||  |H   |  |
> > > > > > >  |   ||  |Y   |D-|
> > > > > > >  |   ''  |0   |<.|
> > > > > > >  |   |   || ||
> > > > > > >  |   |   ''  ..  D+
> > > > > > >  |   |   ||-->
> > > > > > >  |   SOCGPIO | ->||
> > > > > > >  |   |   |   MUX  |
> > > > > > >  |   |   ||-->
> > > > > > >  |   |   ..  ''  D-
> > > > > > >  |   ..  ||   D-  |  |
> > > > > > >  |   ||  |P   |<--'  |
> > > > > > >  |   ||  |H   |  |
> > > > > > >  |   ||  |Y   |  |
> > > > > > >  |   |   USB Device   -->|1   |  |
> > > > > > >  |   ||  ||  D+  |
> > > > > > >  |   ||  ||<-'
> > > > > > >  |   ||  ||
> > > > > > >  '---''  ''
> > > > > > 
> > > > > > Nice ASCII pic :)
> > > > > 
> > > > > asciio ftw \o/
> > > > > 
> > > > > > Yes, that's the case.
> > > > > 
> > > > > alright
> > > > > 
> > > > > > > I have been on and off about writing a pinctrl-gpio.c driver 
> > > > > > > which would
> > > > > > > allow us to hide this detail from users. It shouldn't really 
> > > > > > > matter
> > > > > > > which modes are available behind the mux, but we should be able 
> > > > > > > to tell
> > > > > > > the mux to go into mode 0 or mode 1 by toggling its select 
> > > > > > > signal. And
> > > > > > > it shouldn't really matter that we have a GPIO pin. The problem 
> > > > > > > is: I
> > > > > > > don't really know if pinctrl would be able to handle discrete 
> > > > > > > muxes.
> > > > > > > 
> > > > > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few 
> > > > > > > boards
> > > > > > > which different modes hidden behind discrete muxes.
> > > > > > 
> > > > > > An input from Linus would fine in this case.
> > > > > > 
> > > > > > > 
> > > > > > > > As per initial version, this driver has the duty to control 
> > > > > > > > whether
> > > > > > > > USB-Host cable is plugged in or not:
> > > >

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Wed, Feb 18, 2015 at 12:17:06PM +0200, Mika Westerberg wrote:
 On Tue, Feb 17, 2015 at 11:35:23AM -0800, David Cohen wrote:
  Hi,
  
  Adding Mika.
  
  On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
   Hi,
   
   On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
(3) Platform has 2 USB controllers connected to same port: one 
for
device and one for host role. D+/- are switched between phys
by GPIO.
   
   so you have discrete mux with a GPIO select signal, like below ?
   
   
.---..  ..
|   ||  ||  D+
|   ||  ||-.
|   ||  ||  |
|   |USB Host--|P   |  |
|   ||  |H   |  |
|   ||  |Y   |D-|
|   ''  |0   |.|
|   |   || ||
|   |   ''  ..  D+
|   |   ||--
|   SOCGPIO | -||
|   |   |   MUX  |
|   |   ||--
|   |   ..  ''  D-
|   ..  ||   D-  |  |
|   ||  |P   |--'  |
|   ||  |H   |  |
|   ||  |Y   |  |
|   |   USB Device   --|1   |  |
|   ||  ||  D+  |
|   ||  ||-'
|   ||  ||
'---''  ''
  
  Nice ASCII pic :)
 
 asciio ftw \o/
 
  Yes, that's the case.
 
 alright
 
   I have been on and off about writing a pinctrl-gpio.c driver 
   which would
   allow us to hide this detail from users. It shouldn't really 
   matter
   which modes are available behind the mux, but we should be able 
   to tell
   the mux to go into mode 0 or mode 1 by toggling its select 
   signal. And
   it shouldn't really matter that we have a GPIO pin. The problem 
   is: I
   don't really know if pinctrl would be able to handle discrete 
   muxes.
   
   Adding Linus W to ask. Linus, what do you think ? Should we have a
   pinctrl-gpio.c for such cases ? In TI we too have quite a few 
   boards
   which different modes hidden behind discrete muxes.
  
  An input from Linus would fine in this case.
  
   
As per initial version, this driver has the duty to control 
whether
USB-Host cable is plugged in or not:
 - If yes, OTG port is configured for host role
 - If no, by standard, the OTG port is configured for device 
role
   
   correct, this default-B is mandated by OTG spec anyway.
   
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

Some Intel Bay Trail boards have an unusual way to handle the 
USB OTG port:
 - The USB ID pin is connected directly to GPIO on SoC
 - When in host role, VBUS is provided by enabling a GPIO
 - Device and host roles are supported by 2 independent 
controllers with D+/-
   pins from port switched between different phys according a 
GPIO level.

The ACPI table describes this USB port as a (virtual) device 
with all the
necessary GPIOs. This driver implements support to this virtual 
device as an
extcon class driver. All drivers that depend on the USB OTG 
port state (USB phy,
PMIC, charge detection) will listen to extcon events.
   
   Right I think you're almost there, but I still think that this 
   needs to
   be a bit broken down. First, we need some generic DRD library 
   under
   drivers/usb/common, and that should be the one listening to 
   extcon cable
   events. But your extcon driver should be doing only that: 
   checking which
   cable was attached, it shouldn't be doing the switch by itself. 
   That
   should be part of the DRD library.
   
   That DRD library would also be the one enabling the (optional) 
   VBUS
   regulator.
   
   George Cherian tried to implement a generic DRD library but I 
   think
   there are still too many changes happening on usbcore and 
   udc-core. Most
   of the pieces are already there (usb_del_hcd

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

Adding Mika.

On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > > > device and one for host role. D+/- are switched between phys
> > > > > > by GPIO.
> > > > > 
> > > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > > 
> > > > > 
> > > > >  .---..  ..
> > > > >  |   ||  ||  D+
> > > > >  |   ||  ||<-.
> > > > >  |   ||  ||  |
> > > > >  |   |USB Host-->|P   |  |
> > > > >  |   ||  |H   |  |
> > > > >  |   ||  |Y   |D-|
> > > > >  |   ''  |0   |<.|
> > > > >  |   |   || ||
> > > > >  |   |   ''  ..  D+
> > > > >  |   |   ||-->
> > > > >  |   SOCGPIO | ->||
> > > > >  |   |   |   MUX  |
> > > > >  |   |   ||-->
> > > > >  |   |   ..  ''  D-
> > > > >  |   ..  ||   D-  |  |
> > > > >  |   ||  |P   |<--'  |
> > > > >  |   ||  |H   |  |
> > > > >  |   ||  |Y   |  |
> > > > >  |   |   USB Device   -->|1   |  |
> > > > >  |   ||  ||  D+  |
> > > > >  |   ||  ||<-'
> > > > >  |   ||  ||
> > > > >  '---''  ''
> > > > 
> > > > Nice ASCII pic :)
> > > 
> > > asciio ftw \o/
> > > 
> > > > Yes, that's the case.
> > > 
> > > alright
> > > 
> > > > > I have been on and off about writing a pinctrl-gpio.c driver which 
> > > > > would
> > > > > allow us to hide this detail from users. It shouldn't really matter
> > > > > which modes are available behind the mux, but we should be able to 
> > > > > tell
> > > > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > > > don't really know if pinctrl would be able to handle discrete muxes.
> > > > > 
> > > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > > > which different modes hidden behind discrete muxes.
> > > > 
> > > > An input from Linus would fine in this case.
> > > > 
> > > > > 
> > > > > > As per initial version, this driver has the duty to control whether
> > > > > > USB-Host cable is plugged in or not:
> > > > > >  - If yes, OTG port is configured for host role
> > > > > >  - If no, by standard, the OTG port is configured for device role
> > > > > 
> > > > > correct, this default-B is mandated by OTG spec anyway.
> > > > > 
> > > > > > Signed-off-by: David Cohen 
> > > > > > ---
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > Some Intel Bay Trail boards have an unusual way to handle the USB 
> > > > > > OTG port:
> > > > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > > > >  - Device and host roles are supported by 2 independent controllers 
> > > > > > with D+/-
> > > > > >pins from port switched between different phys according a GPIO 
> > > > > &g

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

Thanks for reviewing.

On Thu, Jan 08, 2015 at 08:23:03PM +0100, Linus Walleij wrote:
> On Mon, Dec 22, 2014 at 11:43 PM, David Cohen
>  wrote:
> 
> > Some platforms have an USB OTG port fully (or partially) controlled by
> > GPIOs:
> >
> > (1) USB ID is connected directly to GPIO
> >
> > Optionally:
> > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > (3) Platform has 2 USB controllers connected to same port: one for
> > device and one for host role. D+/- are switched between phys
> > by GPIO.
> >
> > As per initial version, this driver has the duty to control whether
> > USB-Host cable is plugged in or not:
> >  - If yes, OTG port is configured for host role
> >  - If no, by standard, the OTG port is configured for device role
> >
> > Signed-off-by: David Cohen 
> 
> Pretty interesting! I don't understand the USB stuff so commenting
> from a GPIO side of things only.
> 
> > +config EXTCON_OTG_GPIO
> > +   tristate "VIRTUAL USB OTG PORT support"
> > +   depends on GPIOLIB
> 
> Isn't it dependent on ACPI? This was mentioned in the commit message.

Yep, I'll add it :)

> 
> > +/*
> > + * Virtual USB OTG Port driver controlled by gpios
> > + *
> > + * Copyright (c) 2014, Intel Corporation.
> > + * Author: David Cohen 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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 
> > +#include 
> > +#include 
> 
> You should include 
> 
> And nothing else. (I think it'll just work.)

It should work. I'll fix it.

> 
> > +static int __init vuport_init(void)
> > +{
> > +   return platform_driver_register(_driver);
> > +}
> > +subsys_initcall(vuport_init);
> 
> Usually we try to avoid this kind of early initcalls.
> Doesn't deferred probe work as intended?

Yeah, deferred probe is a better thing to try here.

Br, David

> 
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Felipe and Linus,

On Thu, Dec 25, 2014 at 10:49:29PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 24, 2014 at 02:43:27PM -0800, David Cohen wrote:
> > Hi Felipe,
> > 
> > Thanks replying.
> > 
> > On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > > GPIOs:
> > > > 
> > > > (1) USB ID is connected directly to GPIO
> > > > 
> > > > Optionally:
> > > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > > 
> > > ok, so a fixed regulator with a GPIO enable pin.
> > 
> > Pretty much yes.
> 
> ok
> 
> > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > device and one for host role. D+/- are switched between phys
> > > > by GPIO.
> > > 
> > > so you have discrete mux with a GPIO select signal, like below ?
> > > 
> > > 
> > >  .---..  ..
> > >  |   ||  ||  D+
> > >  |   ||  ||<-.
> > >  |   ||  ||  |
> > >  |   |USB Host-->|P   |  |
> > >  |   ||  |H   |  |
> > >  |   ||  |Y   |D-|
> > >  |   ''  |0   |<.|
> > >  |   |   || ||
> > >  |   |   ''  ..  D+
> > >  |   |   ||-->
> > >  |   SOCGPIO | ->||
> > >  |   |   |   MUX  |
> > >  |   |   ||-->
> > >  |   |   ..  ''  D-
> > >  |   ..  ||   D-  |  |
> > >  |   ||  |P   |<--'  |
> > >  |   ||  |H   |  |
> > >  |   ||  |Y   |  |
> > >  |   |   USB Device   -->|1   |  |
> > >  |   ||  ||  D+  |
> > >  |   ||  ||<-'
> > >  |   ||  ||
> > >  '---''  ''
> > 
> > Nice ASCII pic :)
> 
> asciio ftw \o/
> 
> > Yes, that's the case.
> 
> alright
> 
> > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > allow us to hide this detail from users. It shouldn't really matter
> > > which modes are available behind the mux, but we should be able to tell
> > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > don't really know if pinctrl would be able to handle discrete muxes.
> > > 
> > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > which different modes hidden behind discrete muxes.
> > 
> > An input from Linus would fine in this case.
> > 
> > > 
> > > > As per initial version, this driver has the duty to control whether
> > > > USB-Host cable is plugged in or not:
> > > >  - If yes, OTG port is configured for host role
> > > >  - If no, by standard, the OTG port is configured for device role
> > > 
> > > correct, this default-B is mandated by OTG spec anyway.
> > > 
> > > > Signed-off-by: David Cohen 
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG 
> > > > port:
> > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > >  - Device and host roles are supported by 2 independent controllers 
> > > > with D+/-
> > > >pins from port switched between different phys according a GPIO 
> > > > level.
> > > > 
> > > > The ACPI table describes this 

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

Thanks for reviewing.

On Thu, Jan 08, 2015 at 08:23:03PM +0100, Linus Walleij wrote:
 On Mon, Dec 22, 2014 at 11:43 PM, David Cohen
 david.a.co...@linux.intel.com wrote:
 
  Some platforms have an USB OTG port fully (or partially) controlled by
  GPIOs:
 
  (1) USB ID is connected directly to GPIO
 
  Optionally:
  (2) VBUS is enabled by a GPIO (when ID is grounded)
  (3) Platform has 2 USB controllers connected to same port: one for
  device and one for host role. D+/- are switched between phys
  by GPIO.
 
  As per initial version, this driver has the duty to control whether
  USB-Host cable is plugged in or not:
   - If yes, OTG port is configured for host role
   - If no, by standard, the OTG port is configured for device role
 
  Signed-off-by: David Cohen david.a.co...@linux.intel.com
 
 Pretty interesting! I don't understand the USB stuff so commenting
 from a GPIO side of things only.
 
  +config EXTCON_OTG_GPIO
  +   tristate VIRTUAL USB OTG PORT support
  +   depends on GPIOLIB
 
 Isn't it dependent on ACPI? This was mentioned in the commit message.

Yep, I'll add it :)

 
  +/*
  + * Virtual USB OTG Port driver controlled by gpios
  + *
  + * Copyright (c) 2014, Intel Corporation.
  + * Author: David Cohen david.a.co...@linux.intel.com
  + *
  + * This program is free software; you can redistribute it and/or modify
  + * it under the terms of the GNU General Public License version 2 as
  + * published by the Free Software Foundation.
  + *
  + * 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/acpi.h
  +#include linux/extcon.h
  +#include linux/gpio.h
 
 You should include linux/gpio/consumer.h
 
 And nothing else. (I think it'll just work.)

It should work. I'll fix it.

 
  +static int __init vuport_init(void)
  +{
  +   return platform_driver_register(vuport_driver);
  +}
  +subsys_initcall(vuport_init);
 
 Usually we try to avoid this kind of early initcalls.
 Doesn't deferred probe work as intended?

Yeah, deferred probe is a better thing to try here.

Br, David

 
 Yours,
 Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Felipe and Linus,

On Thu, Dec 25, 2014 at 10:49:29PM -0600, Felipe Balbi wrote:
 Hi,
 
 On Wed, Dec 24, 2014 at 02:43:27PM -0800, David Cohen wrote:
  Hi Felipe,
  
  Thanks replying.
  
  On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
   Hi,
   
   On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
Some platforms have an USB OTG port fully (or partially) controlled by
GPIOs:

(1) USB ID is connected directly to GPIO

Optionally:
(2) VBUS is enabled by a GPIO (when ID is grounded)
   
   ok, so a fixed regulator with a GPIO enable pin.
  
  Pretty much yes.
 
 ok
 
(3) Platform has 2 USB controllers connected to same port: one for
device and one for host role. D+/- are switched between phys
by GPIO.
   
   so you have discrete mux with a GPIO select signal, like below ?
   
   
.---..  ..
|   ||  ||  D+
|   ||  ||-.
|   ||  ||  |
|   |USB Host--|P   |  |
|   ||  |H   |  |
|   ||  |Y   |D-|
|   ''  |0   |.|
|   |   || ||
|   |   ''  ..  D+
|   |   ||--
|   SOCGPIO | -||
|   |   |   MUX  |
|   |   ||--
|   |   ..  ''  D-
|   ..  ||   D-  |  |
|   ||  |P   |--'  |
|   ||  |H   |  |
|   ||  |Y   |  |
|   |   USB Device   --|1   |  |
|   ||  ||  D+  |
|   ||  ||-'
|   ||  ||
'---''  ''
  
  Nice ASCII pic :)
 
 asciio ftw \o/
 
  Yes, that's the case.
 
 alright
 
   I have been on and off about writing a pinctrl-gpio.c driver which would
   allow us to hide this detail from users. It shouldn't really matter
   which modes are available behind the mux, but we should be able to tell
   the mux to go into mode 0 or mode 1 by toggling its select signal. And
   it shouldn't really matter that we have a GPIO pin. The problem is: I
   don't really know if pinctrl would be able to handle discrete muxes.
   
   Adding Linus W to ask. Linus, what do you think ? Should we have a
   pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
   which different modes hidden behind discrete muxes.
  
  An input from Linus would fine in this case.
  
   
As per initial version, this driver has the duty to control whether
USB-Host cable is plugged in or not:
 - If yes, OTG port is configured for host role
 - If no, by standard, the OTG port is configured for device role
   
   correct, this default-B is mandated by OTG spec anyway.
   
Signed-off-by: David Cohen david.a.co...@linux.intel.com
---

Hi,

Some Intel Bay Trail boards have an unusual way to handle the USB OTG 
port:
 - The USB ID pin is connected directly to GPIO on SoC
 - When in host role, VBUS is provided by enabling a GPIO
 - Device and host roles are supported by 2 independent controllers 
with D+/-
   pins from port switched between different phys according a GPIO 
level.

The ACPI table describes this USB port as a (virtual) device with all 
the
necessary GPIOs. This driver implements support to this virtual device 
as an
extcon class driver. All drivers that depend on the USB OTG port state 
(USB phy,
PMIC, charge detection) will listen to extcon events.
   
   Right I think you're almost there, but I still think that this needs to
   be a bit broken down. First, we need some generic DRD library under
   drivers/usb/common, and that should be the one listening to extcon cable
   events. But your extcon driver should be doing only that: checking which
   cable was attached, it shouldn't be doing the switch by itself. That
   should be part of the DRD library.
   
   That DRD library would also be the one enabling the (optional) VBUS
   regulator.
   
   George Cherian tried to implement a generic DRD library but I think
   there are still too many changes happening on usbcore and udc-core. Most
   of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
   know how to properly unload an hcd/udc), but there are details missing,
   no doubt.
   
   If we can find a way to broadcast (probably not the best term, but
   whatever) a Hey

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

Adding Mika.

On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
 Hi,
 
 On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
  (3) Platform has 2 USB controllers connected to same port: one for
  device and one for host role. D+/- are switched between phys
  by GPIO.
 
 so you have discrete mux with a GPIO select signal, like below ?
 
 
  .---..  ..
  |   ||  ||  D+
  |   ||  ||-.
  |   ||  ||  |
  |   |USB Host--|P   |  |
  |   ||  |H   |  |
  |   ||  |Y   |D-|
  |   ''  |0   |.|
  |   |   || ||
  |   |   ''  ..  D+
  |   |   ||--
  |   SOCGPIO | -||
  |   |   |   MUX  |
  |   |   ||--
  |   |   ..  ''  D-
  |   ..  ||   D-  |  |
  |   ||  |P   |--'  |
  |   ||  |H   |  |
  |   ||  |Y   |  |
  |   |   USB Device   --|1   |  |
  |   ||  ||  D+  |
  |   ||  ||-'
  |   ||  ||
  '---''  ''

Nice ASCII pic :)
   
   asciio ftw \o/
   
Yes, that's the case.
   
   alright
   
 I have been on and off about writing a pinctrl-gpio.c driver which 
 would
 allow us to hide this detail from users. It shouldn't really matter
 which modes are available behind the mux, but we should be able to 
 tell
 the mux to go into mode 0 or mode 1 by toggling its select signal. And
 it shouldn't really matter that we have a GPIO pin. The problem is: I
 don't really know if pinctrl would be able to handle discrete muxes.
 
 Adding Linus W to ask. Linus, what do you think ? Should we have a
 pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
 which different modes hidden behind discrete muxes.

An input from Linus would fine in this case.

 
  As per initial version, this driver has the duty to control whether
  USB-Host cable is plugged in or not:
   - If yes, OTG port is configured for host role
   - If no, by standard, the OTG port is configured for device role
 
 correct, this default-B is mandated by OTG spec anyway.
 
  Signed-off-by: David Cohen david.a.co...@linux.intel.com
  ---
  
  Hi,
  
  Some Intel Bay Trail boards have an unusual way to handle the USB 
  OTG port:
   - The USB ID pin is connected directly to GPIO on SoC
   - When in host role, VBUS is provided by enabling a GPIO
   - Device and host roles are supported by 2 independent controllers 
  with D+/-
 pins from port switched between different phys according a GPIO 
  level.
  
  The ACPI table describes this USB port as a (virtual) device with 
  all the
  necessary GPIOs. This driver implements support to this virtual 
  device as an
  extcon class driver. All drivers that depend on the USB OTG port 
  state (USB phy,
  PMIC, charge detection) will listen to extcon events.
 
 Right I think you're almost there, but I still think that this needs 
 to
 be a bit broken down. First, we need some generic DRD library under
 drivers/usb/common, and that should be the one listening to extcon 
 cable
 events. But your extcon driver should be doing only that: checking 
 which
 cable was attached, it shouldn't be doing the switch by itself. That
 should be part of the DRD library.
 
 That DRD library would also be the one enabling the (optional) VBUS
 regulator.
 
 George Cherian tried to implement a generic DRD library but I think
 there are still too many changes happening on usbcore and udc-core. 
 Most
 of the pieces are already there (usb_del_hcd() and 
 usb_del_gadget_udc()
 know how to properly unload an hcd/udc), but there are details 
 missing,
 no doubt.
 
 If we can find a way to broadcast (probably not the best term, but
 whatever) a Hey ID pin was just grounded message, we can get things
 working.
 
 That message, btw, shouldn't really depend on extcon-gpio alone 
 because
 other platforms might use non-gpio methods to verify ID pin level

Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen]

On Fri, Feb 13, 2015 at 04:03:57PM -0600, Felipe Balbi wrote:
> On Fri, Feb 13, 2015 at 02:02:11PM -0800, David Cohen wrote:
> > Hi Felipe,
> > 
> > [snip]
> > 
> > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > > index 8d95056..53902ea 100644
> > > --- a/drivers/usb/dwc3/dwc3-pci.c
> > > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > > @@ -21,6 +21,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include "platform_data.h"
> > >  
> > > @@ -35,6 +36,24 @@
> > >  
> > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > >  {
> > > + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > + struct gpio_desc *gpio;
> > > +
> > > + gpio = gpiod_get_index(>dev, "reset", 0);
> > > + if (!IS_ERR(gpio)) {
> > > + gpiod_direction_output(gpio, 0);
> > > + gpiod_set_value_cansleep(gpio, 1);
> > > + gpiod_put(gpio);
> > > + }
> > > + gpio = gpiod_get_index(>dev, "cs", 1);
> > > + if (!IS_ERR(gpio)) {
> > > + gpiod_direction_output(gpio, 0);
> > > + gpiod_set_value_cansleep(gpio, 1);
> > > + gpiod_put(gpio);
> > > + }
> > > + }
> > > +
> > 
> > A lot has been discussed in other branches of this thread.
> > 
> > But in resume, this is the last open point to make Heikki's proposal
> > good on my side. If you accept this ugly quirk (but necessary for
> > current BYT-CR products when ULPI bus enumerates phy), everything seems
> > good to me. If you don't accept, we need to figure out a way to get the
> > platform driver code back to give gpio to phy as platform data in a way
> > that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).
> 
> Is this needed to *all* Baytrail devices or could we have devices with
> updated firmware which won't need this ? I wonder if we should apply the
> quirk for each known product that actually needs this.

The products that need this quirk will have the gpios on DSDT, otherwise
it won't be there. Except for the order of gpios (CS needs to be enabled
first and it's index 0 AFAIR), the quirk should follow Heikki's logic
here: if gpio isn't found we silently ignore it.

> 
> Also, I will only accept the series, after I'm shown logs that it works
> with your known-to-be-broken device.

I can provide that when Heikki resends his patch set.

Br, David

> 
> cheers
> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

Hi Heikki,

On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote:
> On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index a8c9062..66cbf38 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device 
> > > > > *pdev)
> > > > >   platform_set_drvdata(pdev, dwc);
> > > > >   dwc3_cache_hwparams(dwc);
> > > > >  
> > > > > + ret = dwc3_ulpi_init(dwc);
> > > > 
> > > > If I understood correctly, this call will result in enumerating the phy
> > > > via ULPI bus, then registering the correct ULPI device.
> > > > Can you guarantee ULPI will be always accessible at this point if we
> > > > remove dwc3 module and load it again?
> > > 
> > > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > > this point. If we are in a state where we could soft reset dwc3, we
> > > know we can access ULPI. The fact that dwc3 itself expects to be able
> > > to write to the ULPI registers at that point guarantees it for us.
> > 
> > I just double checked DWC3 TRM.
> > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> > bus is already accessible. But the TRM also specifies an ULPI phy would
> > be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> > to an ULPI phy register during reset, but it also mentions the clock
> > sync happens during that time.
> > 
> > That makes no even OK, but more correct IMO to power on phy before
> > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> > whether ULPI is reliably accessible before core's soft reset.
> > 
> > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> > core's soft reset we've got no more grey area.
> 
> Well, we have already requested the phys in dwc3_probe before that so
> some refactoring will be needed. It's probable no a problem.

Sounds good :)

> 
> Btw, I'm sorry about telling this so late, but I'm going to be on
> vacation for the next two weeks.

Thanks for the notice.

Br, David

> 
> 
> Cheers,
> 
> -- 
> heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen]

Hi Felipe,

[snip]

> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 8d95056..53902ea 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "platform_data.h"
>  
> @@ -35,6 +36,24 @@
>  
>  static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> + pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> + struct gpio_desc *gpio;
> +
> + gpio = gpiod_get_index(>dev, "reset", 0);
> + if (!IS_ERR(gpio)) {
> + gpiod_direction_output(gpio, 0);
> + gpiod_set_value_cansleep(gpio, 1);
> + gpiod_put(gpio);
> + }
> + gpio = gpiod_get_index(>dev, "cs", 1);
> + if (!IS_ERR(gpio)) {
> + gpiod_direction_output(gpio, 0);
> + gpiod_set_value_cansleep(gpio, 1);
> + gpiod_put(gpio);
> + }
> + }
> +

A lot has been discussed in other branches of this thread.

But in resume, this is the last open point to make Heikki's proposal
good on my side. If you accept this ugly quirk (but necessary for
current BYT-CR products when ULPI bus enumerates phy), everything seems
good to me. If you don't accept, we need to figure out a way to get the
platform driver code back to give gpio to phy as platform data in a way
that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).

Br, David

>   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
>   pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
>   struct dwc3_platform_data pdata;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen]

Hi Felipe,

[snip]

 diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
 index 8d95056..53902ea 100644
 --- a/drivers/usb/dwc3/dwc3-pci.c
 +++ b/drivers/usb/dwc3/dwc3-pci.c
 @@ -21,6 +21,7 @@
  #include linux/slab.h
  #include linux/pci.h
  #include linux/platform_device.h
 +#include linux/gpio/consumer.h
  
  #include platform_data.h
  
 @@ -35,6 +36,24 @@
  
  static int dwc3_pci_quirks(struct pci_dev *pdev)
  {
 + if (pdev-vendor == PCI_VENDOR_ID_INTEL 
 + pdev-device == PCI_DEVICE_ID_INTEL_BYT) {
 + struct gpio_desc *gpio;
 +
 + gpio = gpiod_get_index(pdev-dev, reset, 0);
 + if (!IS_ERR(gpio)) {
 + gpiod_direction_output(gpio, 0);
 + gpiod_set_value_cansleep(gpio, 1);
 + gpiod_put(gpio);
 + }
 + gpio = gpiod_get_index(pdev-dev, cs, 1);
 + if (!IS_ERR(gpio)) {
 + gpiod_direction_output(gpio, 0);
 + gpiod_set_value_cansleep(gpio, 1);
 + gpiod_put(gpio);
 + }
 + }
 +

A lot has been discussed in other branches of this thread.

But in resume, this is the last open point to make Heikki's proposal
good on my side. If you accept this ugly quirk (but necessary for
current BYT-CR products when ULPI bus enumerates phy), everything seems
good to me. If you don't accept, we need to figure out a way to get the
platform driver code back to give gpio to phy as platform data in a way
that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).

Br, David

   if (pdev-vendor == PCI_VENDOR_ID_AMD 
   pdev-device == PCI_DEVICE_ID_AMD_NL_USB) {
   struct dwc3_platform_data pdata;

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

Hi Heikki,

On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote:
 On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
  On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index a8c9062..66cbf38 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device 
 *pdev)
   platform_set_drvdata(pdev, dwc);
   dwc3_cache_hwparams(dwc);
  
 + ret = dwc3_ulpi_init(dwc);

If I understood correctly, this call will result in enumerating the phy
via ULPI bus, then registering the correct ULPI device.
Can you guarantee ULPI will be always accessible at this point if we
remove dwc3 module and load it again?
   
   OK, got it. So yes, I can guarantee that ULPI will be acessible at
   this point. If we are in a state where we could soft reset dwc3, we
   know we can access ULPI. The fact that dwc3 itself expects to be able
   to write to the ULPI registers at that point guarantees it for us.
  
  I just double checked DWC3 TRM.
  You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
  bus is already accessible. But the TRM also specifies an ULPI phy would
  be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
  before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
  to an ULPI phy register during reset, but it also mentions the clock
  sync happens during that time.
  
  That makes no even OK, but more correct IMO to power on phy before
  core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
  whether ULPI is reliably accessible before core's soft reset.
  
  I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
  core's soft reset we've got no more grey area.
 
 Well, we have already requested the phys in dwc3_probe before that so
 some refactoring will be needed. It's probable no a problem.

Sounds good :)

 
 Btw, I'm sorry about telling this so late, but I'm going to be on
 vacation for the next two weeks.

Thanks for the notice.

Br, David

 
 
 Cheers,
 
 -- 
 heikki
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen]

On Fri, Feb 13, 2015 at 04:03:57PM -0600, Felipe Balbi wrote:
 On Fri, Feb 13, 2015 at 02:02:11PM -0800, David Cohen wrote:
  Hi Felipe,
  
  [snip]
  
   diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
   index 8d95056..53902ea 100644
   --- a/drivers/usb/dwc3/dwc3-pci.c
   +++ b/drivers/usb/dwc3/dwc3-pci.c
   @@ -21,6 +21,7 @@
#include linux/slab.h
#include linux/pci.h
#include linux/platform_device.h
   +#include linux/gpio/consumer.h

#include platform_data.h

   @@ -35,6 +36,24 @@

static int dwc3_pci_quirks(struct pci_dev *pdev)
{
   + if (pdev-vendor == PCI_VENDOR_ID_INTEL 
   + pdev-device == PCI_DEVICE_ID_INTEL_BYT) {
   + struct gpio_desc *gpio;
   +
   + gpio = gpiod_get_index(pdev-dev, reset, 0);
   + if (!IS_ERR(gpio)) {
   + gpiod_direction_output(gpio, 0);
   + gpiod_set_value_cansleep(gpio, 1);
   + gpiod_put(gpio);
   + }
   + gpio = gpiod_get_index(pdev-dev, cs, 1);
   + if (!IS_ERR(gpio)) {
   + gpiod_direction_output(gpio, 0);
   + gpiod_set_value_cansleep(gpio, 1);
   + gpiod_put(gpio);
   + }
   + }
   +
  
  A lot has been discussed in other branches of this thread.
  
  But in resume, this is the last open point to make Heikki's proposal
  good on my side. If you accept this ugly quirk (but necessary for
  current BYT-CR products when ULPI bus enumerates phy), everything seems
  good to me. If you don't accept, we need to figure out a way to get the
  platform driver code back to give gpio to phy as platform data in a way
  that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).
 
 Is this needed to *all* Baytrail devices or could we have devices with
 updated firmware which won't need this ? I wonder if we should apply the
 quirk for each known product that actually needs this.

The products that need this quirk will have the gpios on DSDT, otherwise
it won't be there. Except for the order of gpios (CS needs to be enabled
first and it's index 0 AFAIR), the quirk should follow Heikki's logic
here: if gpio isn't found we silently ignore it.

 
 Also, I will only accept the series, after I'm shown logs that it works
 with your known-to-be-broken device.

I can provide that when Heikki resends his patch set.

Br, David

 
 cheers
 
 -- 
 balbi


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index a8c9062..66cbf38 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > > > platform_set_drvdata(pdev, dwc);
> > > > dwc3_cache_hwparams(dwc);
> > > >  
> > > > +   ret = dwc3_ulpi_init(dwc);
> > > 
> > > If I understood correctly, this call will result in enumerating the phy
> > > via ULPI bus, then registering the correct ULPI device.
> > > Can you guarantee ULPI will be always accessible at this point if we
> > > remove dwc3 module and load it again?
> > 
> > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > this point. If we are in a state where we could soft reset dwc3, we
> > know we can access ULPI. The fact that dwc3 itself expects to be able
> > to write to the ULPI registers at that point guarantees it for us.
> 
> I just double checked DWC3 TRM.
> You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> bus is already accessible. But the TRM also specifies an ULPI phy would
> be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> to an ULPI phy register during reset, but it also mentions the clock
> sync happens during that time.
> 
> That makes no even OK, but more correct IMO to power on phy before

Sorry for the typo. I meant:
That makes not only OK, but more correct...

> core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> whether ULPI is reliably accessible before core's soft reset.
> 
> I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> core's soft reset we've got no more grey area.
> 
> Br, David
> 
> > 
> > So in practice when ever dwc3 is powered we will be able to access
> > ULPI for as long as the USB2 PHY interface is not suspended separately
> > with GUSB2PHYCFG SusPHY bit. And even then we would only need to
> > resume it with the same bit and ULPI is accessible again.
> > 
> > 
> > Cheers,
> > 
> > -- 
> > heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen]

Hi Heikki,

Sorry I am starting a new branch on this thread.
I need to go back to another topic on this same patch.

On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> TUSB1210 ULPI PHY has vendor specific register for eye
> diagram tuning. On some platforms the system firmware has
> set optimized value to it. In order to not loose the
> optimized value, the driver stores it during probe and
> restores it every time the PHY is powered back on.
> 
> Signed-off-by: Heikki Krogerus 
> Cc: Kishon Vijay Abraham I 
> ---

[snip]

> + /* Store initial eye diagram optimisation value */
> + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);

We can't rely on eye diagram optimization value here. It's possible the
phy went through reset (e.g. module unloading/loading).
We need a more consistent method. Could we use _DSD instead? That would
be more compatible with DT too.

Br, David

> + if (ret < 0)
> + return ret;
> +
> + tusb->eye_diagram_tuning = ret;
> +
> + tusb->phy = ulpi_phy_create(ulpi, _ops);
> + if (IS_ERR(tusb->phy))
> + return PTR_ERR(tusb->phy);
> +
> + tusb->ulpi = ulpi;
> +
> + phy_set_drvdata(tusb->phy, tusb);
> + ulpi_set_drvdata(ulpi, tusb);
> + return 0;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index a8c9062..66cbf38 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > >   platform_set_drvdata(pdev, dwc);
> > >   dwc3_cache_hwparams(dwc);
> > >  
> > > + ret = dwc3_ulpi_init(dwc);
> > 
> > If I understood correctly, this call will result in enumerating the phy
> > via ULPI bus, then registering the correct ULPI device.
> > Can you guarantee ULPI will be always accessible at this point if we
> > remove dwc3 module and load it again?
> 
> OK, got it. So yes, I can guarantee that ULPI will be acessible at
> this point. If we are in a state where we could soft reset dwc3, we
> know we can access ULPI. The fact that dwc3 itself expects to be able
> to write to the ULPI registers at that point guarantees it for us.

I just double checked DWC3 TRM.
You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
bus is already accessible. But the TRM also specifies an ULPI phy would
be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
to an ULPI phy register during reset, but it also mentions the clock
sync happens during that time.

That makes no even OK, but more correct IMO to power on phy before
core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
whether ULPI is reliably accessible before core's soft reset.

I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
core's soft reset we've got no more grey area.

Br, David

> 
> So in practice when ever dwc3 is powered we will be able to access
> ULPI for as long as the USB2 PHY interface is not suspended separately
> with GUSB2PHYCFG SusPHY bit. And even then we would only need to
> resume it with the same bit and ULPI is accessible again.
> 
> 
> Cheers,
> 
> -- 
> heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] USB phy type C

[David Cohen]

On Thu, Feb 12, 2015 at 12:45:53PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Feb 12, 2015 at 10:41:37AM -0800, David Cohen wrote:
> > Hi Felipe, et al,
> > 
> > Is there any on going discussion regarding to USB phy for type C connectors?
> > 
> > We'd like to know the community's preferable direction for handling
> > (still unsupported) cases like USB3.1 PD, Accessories and Alternate
> > modes as a new layer or an extension of current USB phy layer.
> > 
> > Comments are welcome :)
> 
> Not the USB phy layer, but the generic phy layer. All you need to do is
> add a new phy provider and add support for it from within dwc3.
> Something like below:
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9f0e209b8f6c..55a85b40e43e 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -653,6 +653,19 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>   }
>   }
>  
> + dwc->usb3_typec_phy = devm_phy_get(dev, "usb3-typec-phy");
> + if (IS_ERR(dwc->usb3_typec_phy)) {
> + ret = PTR_ERR(dwc->usb3_typec_phy);
> + if (ret == -ENOSYS || ret == -ENODEV) {
> + dwc->usb3_typec_phy = NULL;
> + } else if (ret == -EPROBE_DEFER) {
> + return ret;
> + } else {
> + dev_err(dev, "no usb3 typeC phy, continuing without\n");
> + dwc->usb3_typec_phy = NULL;
> + }
> + }
> +
>   return 0;
>  }

Thanks. That's a nice direction.

>  
> 
> The only interesting part will be dealing with the other modes of
> operation, I haven't wrapped my head around it yet.

We'll probably get back to here with more questions/proposals about
that.

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] USB phy type C

[David Cohen]

Hi Felipe, et al,

Is there any on going discussion regarding to USB phy for type C connectors?

We'd like to know the community's preferable direction for handling
(still unsupported) cases like USB3.1 PD, Accessories and Alternate
modes as a new layer or an extension of current USB phy layer.

Comments are welcome :)

Br, David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] USB phy type C

[David Cohen]

Hi Felipe, et al,

Is there any on going discussion regarding to USB phy for type C connectors?

We'd like to know the community's preferable direction for handling
(still unsupported) cases like USB3.1 PD, Accessories and Alternate
modes as a new layer or an extension of current USB phy layer.

Comments are welcome :)

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] USB phy type C

[David Cohen]

On Thu, Feb 12, 2015 at 12:45:53PM -0600, Felipe Balbi wrote:
 Hi,
 
 On Thu, Feb 12, 2015 at 10:41:37AM -0800, David Cohen wrote:
  Hi Felipe, et al,
  
  Is there any on going discussion regarding to USB phy for type C connectors?
  
  We'd like to know the community's preferable direction for handling
  (still unsupported) cases like USB3.1 PD, Accessories and Alternate
  modes as a new layer or an extension of current USB phy layer.
  
  Comments are welcome :)
 
 Not the USB phy layer, but the generic phy layer. All you need to do is
 add a new phy provider and add support for it from within dwc3.
 Something like below:
 
 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index 9f0e209b8f6c..55a85b40e43e 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -653,6 +653,19 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
   }
   }
  
 + dwc-usb3_typec_phy = devm_phy_get(dev, usb3-typec-phy);
 + if (IS_ERR(dwc-usb3_typec_phy)) {
 + ret = PTR_ERR(dwc-usb3_typec_phy);
 + if (ret == -ENOSYS || ret == -ENODEV) {
 + dwc-usb3_typec_phy = NULL;
 + } else if (ret == -EPROBE_DEFER) {
 + return ret;
 + } else {
 + dev_err(dev, no usb3 typeC phy, continuing without\n);
 + dwc-usb3_typec_phy = NULL;
 + }
 + }
 +
   return 0;
  }

Thanks. That's a nice direction.

  
 
 The only interesting part will be dealing with the other modes of
 operation, I haven't wrapped my head around it yet.

We'll probably get back to here with more questions/proposals about
that.

Br, David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
   index a8c9062..66cbf38 100644
   --- a/drivers/usb/dwc3/core.c
   +++ b/drivers/usb/dwc3/core.c
   @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
 platform_set_drvdata(pdev, dwc);
 dwc3_cache_hwparams(dwc);

   + ret = dwc3_ulpi_init(dwc);
  
  If I understood correctly, this call will result in enumerating the phy
  via ULPI bus, then registering the correct ULPI device.
  Can you guarantee ULPI will be always accessible at this point if we
  remove dwc3 module and load it again?
 
 OK, got it. So yes, I can guarantee that ULPI will be acessible at
 this point. If we are in a state where we could soft reset dwc3, we
 know we can access ULPI. The fact that dwc3 itself expects to be able
 to write to the ULPI registers at that point guarantees it for us.

I just double checked DWC3 TRM.
You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
bus is already accessible. But the TRM also specifies an ULPI phy would
be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
to an ULPI phy register during reset, but it also mentions the clock
sync happens during that time.

That makes no even OK, but more correct IMO to power on phy before
core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
whether ULPI is reliably accessible before core's soft reset.

I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
core's soft reset we've got no more grey area.

Br, David

 
 So in practice when ever dwc3 is powered we will be able to access
 ULPI for as long as the USB2 PHY interface is not suspended separately
 with GUSB2PHYCFG SusPHY bit. And even then we would only need to
 resume it with the same bit and ULPI is accessible again.
 
 
 Cheers,
 
 -- 
 heikki
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/