Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1337496 > unrolled thread
| Started by | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| First post | 2016-02-18 17:20 +0100 |
| Last post | 2016-02-18 22:50 +0100 |
| Articles | 5 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter Alan Stern <stern@rowland.harvard.edu> - 2016-02-18 17:20 +0100
Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter Marek Vasut <marex@denx.de> - 2016-02-18 17:40 +0100
Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter Antony Pavlov <antonynpavlov@gmail.com> - 2016-02-18 18:50 +0100
Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-18 19:40 +0100
Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter Antony Pavlov <antonynpavlov@gmail.com> - 2016-02-18 22:50 +0100
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-02-18 17:20 +0100 |
| Subject | Re: [RFC v5 07/15] usb: ehci: add vbus-gpio parameter |
| Message-ID | <r3zt9-88d-13@gated-at.bofh.it> |
On Tue, 9 Feb 2016, Antony Pavlov wrote:
> This patch retrieves and configures the vbus control gpio via
> the device tree.
>
> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
>
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index bd7082f2..0d95ced 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -28,6 +28,7 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_gpio.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/reset.h>
> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
> .power_off = ehci_platform_power_off,
> };
>
> +static void setup_vbus_gpio(struct device *dev)
> +{
> + int err;
> + int gpio;
> +
> + if (!dev->of_node)
> + return;
> +
> + gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
> + if (!gpio_is_valid(gpio))
> + return;
> +
> + err = devm_gpio_request_one(dev, gpio,
> + GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
> + "ehci_vbus_gpio");
> + if (err)
> + dev_err(dev, "can't request ehci vbus gpio %d", gpio);
I don't understand this. If you get an error here, what's the point of
allowing the probe to continue? Shouldn't you return an error code so
the probe will fail?
Alan Stern
> +}
> +
> static int ehci_platform_probe(struct platform_device *dev)
> {
> struct usb_hcd *hcd;
> @@ -174,6 +194,8 @@ static int ehci_platform_probe(struct platform_device *dev)
> return irq;
> }
>
> + setup_vbus_gpio(&dev->dev);
> +
> hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
> dev_name(&dev->dev));
> if (!hcd)
>
[toc] | [next] | [standalone]
| From | Marek Vasut <marex@denx.de> |
|---|---|
| Date | 2016-02-18 17:40 +0100 |
| Message-ID | <r3zMt-8l2-3@gated-at.bofh.it> |
| In reply to | #1337496 |
On 02/18/2016 05:12 PM, Alan Stern wrote:
> On Tue, 9 Feb 2016, Antony Pavlov wrote:
>
>> This patch retrieves and configures the vbus control gpio via
>> the device tree.
>>
>> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
>> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
>>
>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-usb@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index bd7082f2..0d95ced 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -28,6 +28,7 @@
>> #include <linux/io.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> #include <linux/phy/phy.h>
>> #include <linux/platform_device.h>
>> #include <linux/reset.h>
>> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
>> .power_off = ehci_platform_power_off,
>> };
>>
>> +static void setup_vbus_gpio(struct device *dev)
>> +{
>> + int err;
>> + int gpio;
>> +
>> + if (!dev->of_node)
>> + return;
>> +
>> + gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
>> + if (!gpio_is_valid(gpio))
>> + return;
>> +
>> + err = devm_gpio_request_one(dev, gpio,
>> + GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
>> + "ehci_vbus_gpio");
>> + if (err)
>> + dev_err(dev, "can't request ehci vbus gpio %d", gpio);
>
> I don't understand this. If you get an error here, what's the point of
> allowing the probe to continue? Shouldn't you return an error code so
> the probe will fail?
The idea is I believe that if there is no vbus gpio specified, the port
might just not have vbus control, so the probe can continue. But this
patch is irrelevant anyway, since Alexey will switch to CI HDRC driver
and use standard regulator, as it should be done.
[toc] | [prev] | [next] | [standalone]
| From | Antony Pavlov <antonynpavlov@gmail.com> |
|---|---|
| Date | 2016-02-18 18:50 +0100 |
| Message-ID | <r3ASe-CA-5@gated-at.bofh.it> |
| In reply to | #1337496 |
On Thu, 18 Feb 2016 11:12:43 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 9 Feb 2016, Antony Pavlov wrote:
>
> > This patch retrieves and configures the vbus control gpio via
> > the device tree.
> >
> > This patch is based on a ehci-s5p.c commit fd81d59c90d38661
> > ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
> >
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> > index bd7082f2..0d95ced 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -28,6 +28,7 @@
> > #include <linux/io.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/reset.h>
> > @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
> > .power_off = ehci_platform_power_off,
> > };
> >
> > +static void setup_vbus_gpio(struct device *dev)
> > +{
> > + int err;
> > + int gpio;
> > +
> > + if (!dev->of_node)
> > + return;
> > +
> > + gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
> > + if (!gpio_is_valid(gpio))
> > + return;
> > +
> > + err = devm_gpio_request_one(dev, gpio,
> > + GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
> > + "ehci_vbus_gpio");
> > + if (err)
> > + dev_err(dev, "can't request ehci vbus gpio %d", gpio);
>
>
> I don't understand this. If you get an error here, what's the point of
> allowing the probe to continue? Shouldn't you return an error code so
> the probe will fail?
Please ignore the 'usb: ehci: add vbus-gpio parameter' patch!
In the new AR9331 patchseries I use chipidea USB driver (thanks to Marek for the suggestion)
in the AR9331 dtsi-file:
usb: usb@1b000100 {
compatible = "chipidea,usb2";
reg = <0x1b000000 0x200>;
interrupt-parent = <&cpuintc>;
interrupts = <3>;
resets = <&rst 5>;
phy-names = "usb-phy";
phys = <&usb_phy>;
status = "disabled";
};
so I use regulator in the TL-MR3020 board dts file:
reg_usb_vbus: reg_usb_vbus {
compatible = "regulator-fixed";
regulator-name = "usb_vbus";
regulator-min-microvolt = <5000000>;
regulator-max-microvolt = <5000000>;
gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
&usb {
dr_mode = "host";
vbus-supply = <®_usb_vbus>;
status = "okay";
};
As a result there is no need in adding vbus-gpio parameter to ehci anymore!
> > +}
> > +
> > static int ehci_platform_probe(struct platform_device *dev)
> > {
> > struct usb_hcd *hcd;
> > @@ -174,6 +194,8 @@ static int ehci_platform_probe(struct platform_device *dev)
> > return irq;
> > }
> >
> > + setup_vbus_gpio(&dev->dev);
> > +
> > hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
> > dev_name(&dev->dev));
> > if (!hcd)
> >
>
--
--
Best regards,
Antony Pavlov
[toc] | [prev] | [next] | [standalone]
| From | Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> |
|---|---|
| Date | 2016-02-18 19:40 +0100 |
| Message-ID | <r3BEC-1ga-7@gated-at.bofh.it> |
| In reply to | #1337580 |
On 02/18/2016 09:06 PM, Antony Pavlov wrote:
>>> This patch retrieves and configures the vbus control gpio via
>>> the device tree.
>>>
>>> This patch is based on a ehci-s5p.c commit fd81d59c90d38661
>>> ("USB: ehci-s5p: Add vbus setup function to the s5p ehci glue layer").
>>>
>>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>> drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++++
>>> 1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index bd7082f2..0d95ced 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -28,6 +28,7 @@
>>> #include <linux/io.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>> #include <linux/phy/phy.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/reset.h>
>>> @@ -142,6 +143,25 @@ static struct usb_ehci_pdata ehci_platform_defaults = {
>>> .power_off = ehci_platform_power_off,
>>> };
>>>
>>> +static void setup_vbus_gpio(struct device *dev)
>>> +{
>>> + int err;
>>> + int gpio;
>>> +
>>> + if (!dev->of_node)
>>> + return;
>>> +
>>> + gpio = of_get_named_gpio(dev->of_node, "vbus-gpio", 0);
>>> + if (!gpio_is_valid(gpio))
>>> + return;
>>> +
>>> + err = devm_gpio_request_one(dev, gpio,
>>> + GPIOF_OUT_INIT_HIGH | GPIOF_EXPORT_DIR_FIXED,
>>> + "ehci_vbus_gpio");
>>> + if (err)
>>> + dev_err(dev, "can't request ehci vbus gpio %d", gpio);
>>
>>
>> I don't understand this. If you get an error here, what's the point of
>> allowing the probe to continue? Shouldn't you return an error code so
>> the probe will fail?
>
> Please ignore the 'usb: ehci: add vbus-gpio parameter' patch!
>
> In the new AR9331 patchseries I use chipidea USB driver (thanks to Marek for the suggestion)
> in the AR9331 dtsi-file:
>
> usb: usb@1b000100 {
> compatible = "chipidea,usb2";
> reg = <0x1b000000 0x200>;
>
> interrupt-parent = <&cpuintc>;
> interrupts = <3>;
> resets = <&rst 5>;
>
> phy-names = "usb-phy";
> phys = <&usb_phy>;
>
> status = "disabled";
> };
>
>
> so I use regulator in the TL-MR3020 board dts file:
>
> reg_usb_vbus: reg_usb_vbus {
> compatible = "regulator-fixed";
> regulator-name = "usb_vbus";
> regulator-min-microvolt = <5000000>;
Not 0?
> regulator-max-microvolt = <5000000>;
> gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
Where's the switch if both voltages are equal?
> enable-active-high;
> };
>
> &usb {
> dr_mode = "host";
> vbus-supply = <®_usb_vbus>;
> status = "okay";
> };
>
> As a result there is no need in adding vbus-gpio parameter to ehci anymore!
[...]
MBR, Sergei
[toc] | [prev] | [next] | [standalone]
| From | Antony Pavlov <antonynpavlov@gmail.com> |
|---|---|
| Date | 2016-02-18 22:50 +0100 |
| Message-ID | <r3ECu-3kv-9@gated-at.bofh.it> |
| In reply to | #1337616 |
On Thu, 18 Feb 2016 21:31:20 +0300
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> On 02/18/2016 09:06 PM, Antony Pavlov wrote:
[...]
> > so I use regulator in the TL-MR3020 board dts file:
> >
> > reg_usb_vbus: reg_usb_vbus {
> > compatible = "regulator-fixed";
> > regulator-name = "usb_vbus";
> > regulator-min-microvolt = <5000000>;
>
> Not 0?
>
> > regulator-max-microvolt = <5000000>;
> > gpio = <&gpio 8 GPIO_ACTIVE_HIGH>;
>
> Where's the switch if both voltages are equal?
Here is a quote from linux/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
Any property defined as part of the core regulator
binding, defined in regulator.txt, can also be used.
However a fixed voltage regulator is expected to have the
regulator-min-microvolt and regulator-max-microvolt
to be the same.
Moreover please see this of_get_fixed_voltage_config() code fragment
(please see linux/drivers/regulator/fixed.c for details):
if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
config->microvolts = init_data->constraints.min_uV;
} else {
dev_err(dev,
"Fixed regulator specified with variable voltages\n");
return ERR_PTR(-EINVAL);
}
--
Best regards,
Antony Pavlov
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web