| Message ID | 1412237073-1288-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ > 1 file changed, 5 insertions(+) > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts > index 1698591..4badd0a 100644 > --- a/arch/arm/boot/dts/r8a7790-lager.dts > +++ b/arch/arm/boot/dts/r8a7790-lager.dts > @@ -445,3 +445,8 @@ > }; > }; > }; > + > +&hsusb { > + status = "okay"; > + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; It's certainly active-high. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. (2014/10/04 4:50), Sergei Shtylyov wrote: > On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: > >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> --- >> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >> 1 file changed, 5 insertions(+) > >> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >> index 1698591..4badd0a 100644 >> --- a/arch/arm/boot/dts/r8a7790-lager.dts >> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >> @@ -445,3 +445,8 @@ >> }; >> }; >> }; >> + >> +&hsusb { >> + status = "okay"; >> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; > > It's certainly active-high. Since the current code has the following, we have to set the active_low... However, the code is unreadable, I think. So, I will modify the code. /* check GPIO determining if USB function should be enabled */ if (priv->dparam.enable_gpio) { gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); ret = !gpio_get_value(priv->dparam.enable_gpio); gpio_free(priv->dparam.enable_gpio); if (ret) { dev_warn(&pdev->dev, "USB function not selected (GPIO %d)\n", priv->dparam.enable_gpio); ret = -ENOTSUPP; goto probe_end_mod_exit; } } Best regards, Yoshihiro Shimoda > WBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 06, 2014 at 09:59:48AM +0900, Yoshihiro Shimoda wrote: > Hello. > > (2014/10/04 4:50), Sergei Shtylyov wrote: > > On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: > > > >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > >> --- > >> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ > >> 1 file changed, 5 insertions(+) > > > >> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts > >> index 1698591..4badd0a 100644 > >> --- a/arch/arm/boot/dts/r8a7790-lager.dts > >> +++ b/arch/arm/boot/dts/r8a7790-lager.dts > >> @@ -445,3 +445,8 @@ > >> }; > >> }; > >> }; > >> + > >> +&hsusb { > >> + status = "okay"; > >> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; > > > > It's certainly active-high. > > Since the current code has the following, we have to set the active_low... > However, the code is unreadable, I think. So, I will modify the code. It seems to me that would be best. As far as possible the bindings and their use should describe the hardware rather than the software. > /* check GPIO determining if USB function should be enabled */ > if (priv->dparam.enable_gpio) { > gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); > ret = !gpio_get_value(priv->dparam.enable_gpio); > gpio_free(priv->dparam.enable_gpio); > if (ret) { > dev_warn(&pdev->dev, > "USB function not selected (GPIO %d)\n", > priv->dparam.enable_gpio); > ret = -ENOTSUPP; > goto probe_end_mod_exit; > } > } > > Best regards, > Yoshihiro Shimoda > > > WBR, Sergei > > > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/6/2014 4:59 AM, Yoshihiro Shimoda wrote: >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >>> --- >>> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >>> index 1698591..4badd0a 100644 >>> --- a/arch/arm/boot/dts/r8a7790-lager.dts >>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >>> @@ -445,3 +445,8 @@ >>> }; >>> }; >>> }; >>> + >>> +&hsusb { >>> + status = "okay"; >>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >> It's certainly active-high. > Since the current code has the following, we have to set the active_low... Exactly opposite conclusion follows from this code. > However, the code is unreadable, I think. So, I will modify the code. There's no dire need, I think. > /* check GPIO determining if USB function should be enabled */ > if (priv->dparam.enable_gpio) { > gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); > ret = !gpio_get_value(priv->dparam.enable_gpio); > gpio_free(priv->dparam.enable_gpio); > if (ret) { 'ret' is non-zero here if gpio_get_value() returned 0, so 0 means "host" and 1 means "gadget", i.e. GPIO is active-high. > dev_warn(&pdev->dev, > "USB function not selected (GPIO %d)\n", > priv->dparam.enable_gpio); > ret = -ENOTSUPP; > goto probe_end_mod_exit; > } > } > Best regards, > Yoshihiro Shimoda WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ > 1 file changed, 5 insertions(+) > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts > index 1698591..4badd0a 100644 > --- a/arch/arm/boot/dts/r8a7790-lager.dts > +++ b/arch/arm/boot/dts/r8a7790-lager.dts > @@ -445,3 +445,8 @@ > }; > }; > }; > + > +&hsusb { > + status = "okay"; > + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; > +}; Darn, this also misses the PinMux props... I was going to post v2 of these patches but now would need to add that and retest... :-/ WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. (2014/10/07 6:38), Sergei Shtylyov wrote: > Hello. > > On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: > >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >> --- >> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >> 1 file changed, 5 insertions(+) > >> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >> index 1698591..4badd0a 100644 >> --- a/arch/arm/boot/dts/r8a7790-lager.dts >> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >> @@ -445,3 +445,8 @@ >> }; >> }; >> }; >> + >> +&hsusb { >> + status = "okay"; >> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >> +}; > > Darn, this also misses the PinMux props... I was going to post v2 of these > patches but now would need to add that and retest... :-/ I'm sorry for the my delayed work... I will submit v2 patches today. By the way, I cannot understand that "this also misses the PinMux props". According to the original "legacy" code in board-lager.c, it uses gpio5 18: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36be7686caa05334ca8d52df157b373a41d8d9f1 Best regards, Yoshihiro Shimoda > WBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. (2014/10/06 9:59), Yoshihiro Shimoda wrote: > Hello. > > (2014/10/04 4:50), Sergei Shtylyov wrote: >> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: >> >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >>> --- >>> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >>> 1 file changed, 5 insertions(+) >> >>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >>> index 1698591..4badd0a 100644 >>> --- a/arch/arm/boot/dts/r8a7790-lager.dts >>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >>> @@ -445,3 +445,8 @@ >>> }; >>> }; >>> }; >>> + >>> +&hsusb { >>> + status = "okay"; >>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >> >> It's certainly active-high. > > Since the current code has the following, we have to set the active_low... > However, the code is unreadable, I think. So, I will modify the code. > > /* check GPIO determining if USB function should be enabled */ > if (priv->dparam.enable_gpio) { > gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); > ret = !gpio_get_value(priv->dparam.enable_gpio); > gpio_free(priv->dparam.enable_gpio); > if (ret) { > dev_warn(&pdev->dev, > "USB function not selected (GPIO %d)\n", > priv->dparam.enable_gpio); > ret = -ENOTSUPP; > goto probe_end_mod_exit; > } > } I am confusing about the gpio_get_value()... In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally. gpio_get_value() in arch/arm/include/asm/gpio.h --> __gpio_get_value() in include/asm-generic/gpio.h --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}. So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver? Or, Do I misunderstand something? Best regards, Yoshihiro Shimoda > Best regards, > Yoshihiro Shimoda > >> WBR, Sergei >> -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. (2014/10/07 12:25), Yoshihiro Shimoda wrote: > Hello. > > (2014/10/06 9:59), Yoshihiro Shimoda wrote: >> Hello. >> >> (2014/10/04 4:50), Sergei Shtylyov wrote: >>> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote: >>> < snip > >>>> +&hsusb { >>>> + status = "okay"; >>>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >>> >>> It's certainly active-high. >> >> Since the current code has the following, we have to set the active_low... >> However, the code is unreadable, I think. So, I will modify the code. >> >> /* check GPIO determining if USB function should be enabled */ >> if (priv->dparam.enable_gpio) { >> gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); >> ret = !gpio_get_value(priv->dparam.enable_gpio); >> gpio_free(priv->dparam.enable_gpio); >> if (ret) { >> dev_warn(&pdev->dev, >> "USB function not selected (GPIO %d)\n", >> priv->dparam.enable_gpio); >> ret = -ENOTSUPP; >> goto probe_end_mod_exit; >> } >> } > > I am confusing about the gpio_get_value()... > In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally. > > gpio_get_value() in arch/arm/include/asm/gpio.h > --> __gpio_get_value() in include/asm-generic/gpio.h > --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c > > The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}. > So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver? > Or, Do I misunderstand something? I looked at the gpio-keys driver, and then I understand the renesas_usbhs driver should have the gpio flags from of_get_named_gpio_flags(). So, I will try to modify the renesas_usbhs driver. Best regards, Yoshihiro Shimoda > Best regards, > Yoshihiro Shimoda > >> Best regards, >> Yoshihiro Shimoda >> >>> WBR, Sergei >>> -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/7/2014 4:20 AM, Yoshihiro Shimoda wrote: >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >>> --- >>> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >>> index 1698591..4badd0a 100644 >>> --- a/arch/arm/boot/dts/r8a7790-lager.dts >>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >>> @@ -445,3 +445,8 @@ >>> }; >>> }; >>> }; >>> + >>> +&hsusb { >>> + status = "okay"; >>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >>> +}; >> Darn, this also misses the PinMux props... I was going to post v2 of these >> patches but now would need to add that and retest... :-/ > I'm sorry for the my delayed work... I will submit v2 patches today. > By the way, I cannot understand that "this also misses the PinMux props". > According to the original "legacy" code in board-lager.c, it uses gpio5 18: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36be7686caa05334ca8d52df157b373a41d8d9f1 Right. But there are also USB0 pins. > Best regards, > Yoshihiro Shimoda WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/7/2014 7:25 AM, Yoshihiro Shimoda wrote: >>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >>>> --- >>>> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >>>> index 1698591..4badd0a 100644 >>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts >>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >>>> @@ -445,3 +445,8 @@ >>>> }; >>>> }; >>>> }; >>>> + >>>> +&hsusb { >>>> + status = "okay"; >>>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; >>> It's certainly active-high. >> Since the current code has the following, we have to set the active_low... >> However, the code is unreadable, I think. So, I will modify the code. >> >> /* check GPIO determining if USB function should be enabled */ >> if (priv->dparam.enable_gpio) { >> gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); >> ret = !gpio_get_value(priv->dparam.enable_gpio); >> gpio_free(priv->dparam.enable_gpio); >> if (ret) { >> dev_warn(&pdev->dev, >> "USB function not selected (GPIO %d)\n", >> priv->dparam.enable_gpio); >> ret = -ENOTSUPP; >> goto probe_end_mod_exit; >> } >> } > I am confusing about the gpio_get_value()... > In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally. > gpio_get_value() in arch/arm/include/asm/gpio.h > --> __gpio_get_value() in include/asm-generic/gpio.h > --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c > The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}. > So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver? Why do you still think it's active-low?! > Or, Do I misunderstand something? Look at the above code again. > Best regards, > Yoshihiro Shimoda WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. (2014/10/07 18:46), Sergei Shtylyov wrote: > On 10/7/2014 7:25 AM, Yoshihiro Shimoda wrote: > >>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> >>>>> --- >>>>> arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ >>>>> 1 file changed, 5 insertions(+) > >>>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts >>>>> index 1698591..4badd0a 100644 >>>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts >>>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts >>>>> @@ -445,3 +445,8 @@ >>>>> }; >>>>> }; >>>>> }; >>>>> + >>>>> +&hsusb { >>>>> + status = "okay"; >>>>> + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; > >>>> It's certainly active-high. > >>> Since the current code has the following, we have to set the active_low... >>> However, the code is unreadable, I think. So, I will modify the code. >>> >>> /* check GPIO determining if USB function should be enabled */ >>> if (priv->dparam.enable_gpio) { >>> gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL); >>> ret = !gpio_get_value(priv->dparam.enable_gpio); >>> gpio_free(priv->dparam.enable_gpio); >>> if (ret) { >>> dev_warn(&pdev->dev, >>> "USB function not selected (GPIO %d)\n", >>> priv->dparam.enable_gpio); >>> ret = -ENOTSUPP; >>> goto probe_end_mod_exit; >>> } >>> } > >> I am confusing about the gpio_get_value()... >> In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally. > >> gpio_get_value() in arch/arm/include/asm/gpio.h >> --> __gpio_get_value() in include/asm-generic/gpio.h >> --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c > >> The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}. >> So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver? > > Why do you still think it's active-low?! > >> Or, Do I misunderstand something? > > Look at the above code again. Thank you for the comment. Finally, I understood this. I will not modify the driver and I just change the "GPIO_ACTIVE_LOW" in the dts file to "GPIO_ACTIVE_HIGH" because of your previous comment below: >> 'ret' is non-zero here if gpio_get_value() returned 0, so 0 means "host" >> and 1 means "gadget", i.e. GPIO is active-high. Best regards, Yoshihiro Shimoda >> Best regards, >> Yoshihiro Shimoda > > WBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts index 1698591..4badd0a 100644 --- a/arch/arm/boot/dts/r8a7790-lager.dts +++ b/arch/arm/boot/dts/r8a7790-lager.dts @@ -445,3 +445,8 @@ }; }; }; + +&hsusb { + status = "okay"; + renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>; +};
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- arch/arm/boot/dts/r8a7790-lager.dts | 5 +++++ 1 file changed, 5 insertions(+)