Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
Hi Felipe, On Sun, Feb 1, 2015 at 3:37 PM, Felipe Balbi ba...@ti.com wrote: I like this, very much. Two comments though. We requested the gpio with _optional(), and NULL is a valid gpio_desc, this if (nop-gpiod_reset) is unnecessary. And also, since we don't have anymore the assert But if the reset-gpios property is not present in the dts, then nop-gpiod_reset is NULL, and it is better not to call 'gpiod_direction_output(nop-gpiod_reset, 1)' in this case, right? argument, we should rename nop_reset_set() to nop_reset. Agreed, will change it in v2. Regards, Fabio Estevam -- 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
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
On Tue, Feb 03, 2015 at 06:21:24PM -0200, Fabio Estevam wrote:
Hi Felipe,
On Sun, Feb 1, 2015 at 3:37 PM, Felipe Balbi ba...@ti.com wrote:
I like this, very much. Two comments though. We requested the gpio with
_optional(), and NULL is a valid gpio_desc, this if (nop-gpiod_reset)
is unnecessary. And also, since we don't have anymore the assert
But if the reset-gpios property is not present in the dts, then
nop-gpiod_reset is NULL, and it is better not to call
'gpiod_direction_output(nop-gpiod_reset, 1)' in this case, right?
it doesn't make a difference though, right ?
gpiod_direction_output(NULL, 1) won't do anything.
/me goes look at code
Man this is messed up. If you follow gpiod_get_optional, you'll end up
at gpiod_get_index_optional() which I quote below:
1989 struct gpio_desc *__must_check __gpiod_get_index_optional(struct device
*dev,
1990 const char *con_id,
1991 unsigned int index,
1992 enum gpiod_flags
flags)
1993 {
1994 struct gpio_desc *desc;
1995
1996 desc = gpiod_get_index(dev, con_id, index, flags);
1997 if (IS_ERR(desc)) {
1998 if (PTR_ERR(desc) == -ENOENT)
1999 return NULL;
2000 }
2001
2002 return desc;
2003 }
2004 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
So, if the error code is -ENOENT it returns NULL, that tells me NULL is
a valid gpio descriptor. If you follow gpiod_direction_output() however,
what you get is:
1072 int gpiod_direction_output(struct gpio_desc *desc, int value)
1073 {
1074 if (!desc || !desc-chip) {
1075 pr_warn(%s: invalid GPIO\n, __func__);
1076 return -EINVAL;
1077 }
1078 if (test_bit(FLAG_ACTIVE_LOW, desc-flags))
1079 value = !value;
1080 return _gpiod_direction_output_raw(desc, value);
1081 }
1082 EXPORT_SYMBOL_GPL(gpiod_direction_output);
That pr_warn() tells me NULL is *not* a valid gpio descriptor. Linus, is
that just something that was missed until now ?
argument, we should rename nop_reset_set() to nop_reset.
Agreed, will change it in v2.
Thanks
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
On Tue, Feb 3, 2015 at 6:52 PM, Felipe Balbi ba...@ti.com wrote:
it doesn't make a difference though, right ?
gpiod_direction_output(NULL, 1) won't do anything.
Yes, I will send a v3 without the NULL check.
gpiod_set_value returns immediately if desc is NULL:
void gpiod_set_value(struct gpio_desc *desc, int value)
{
if (!desc)
return;
--
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
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
HI,
On Fri, Jan 30, 2015 at 11:06:17PM -0200, Fabio Estevam wrote:
On Fri, Jan 30, 2015 at 8:13 PM, Fabio Estevam feste...@gmail.com wrote:
Hi Felipe,
On Fri, Jan 30, 2015 at 7:59 PM, Felipe Balbi ba...@ti.com wrote:
- gpiod_direction_output(nop-gpiod_reset, !asserted);
+ if (asserted)
+ goto skip_delay;
why skip it ?
Let's suppose we have an active-low GPIO USB phy reset pin.
In usb_gen_phy_init() we call nop_reset_set(nop, 0); and 'assert' is zero.
Then we do:
- Put the GPIO into 0
- Wait a bit
- Put it back to 1.
In usb_gen_phy_shutdown, we call nop_reset_set(nop, 1) and assert is one.
Then we should simply do:
- Put GPIO reset into 0.
,as we the PHY is no more in usage.
That's the reason we don't need the delay when assert is one.
Also, the code prior to the gpiod introduction also skips the delay
for the assert == 1 case, so this patch restores the original
behaviour.
Or if you prefer we can make nop_reset_set() to only handle the reset case:
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -61,14 +61,13 @@ static int nop_set_suspend(struct usb_phy *x, int suspend)
return 0;
}
-static void nop_reset_set(struct usb_phy_generic *nop, int asserted)
+static void nop_reset_set(struct usb_phy_generic *nop)
{
if (!nop-gpiod_reset)
return;
-
- gpiod_direction_output(nop-gpiod_reset, !asserted);
+ gpiod_set_value(nop-gpiod_reset, 1);
usleep_range(1, 2);
- gpiod_set_value(nop-gpiod_reset, asserted);
+ gpiod_set_value(nop-gpiod_reset, 0);
}
/* interface to regulator framework */
@@ -150,8 +149,7 @@ int usb_gen_phy_init(struct usb_phy *phy)
if (!IS_ERR(nop-clk))
clk_prepare_enable(nop-clk);
- /* De-assert RESET */
- nop_reset_set(nop, 0);
+ nop_reset_set(nop);
return 0;
}
@@ -161,8 +159,8 @@ void usb_gen_phy_shutdown(struct usb_phy *phy)
{
struct usb_phy_generic *nop = dev_get_drvdata(phy-dev);
- /* Assert RESET */
- nop_reset_set(nop, 1);
+ if (nop-gpiod_reset)
+ gpiod_direction_output(nop-gpiod_reset, 1);
if (!IS_ERR(nop-clk))
clk_disable_unprepare(nop-clk);
I like this, very much. Two comments though. We requested the gpio with
_optional(), and NULL is a valid gpio_desc, this if (nop-gpiod_reset)
is unnecessary. And also, since we don't have anymore the assert
argument, we should rename nop_reset_set() to nop_reset.
Other than that, I very much like this other patch of yours.
Thanks
--
balbi
signature.asc
Description: Digital signature
[PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
From: Fabio Estevam fabio.este...@freescale.com Commit 9eb0797722895f4309b4 (sb: phy: generic: fix the gpios to be optional) calls gpiod_direction_output() in the probe function, so there is no need to call it again, as we can simply call gpiod_set_value() directly. Also, in usb_gen_phy_shutdown() we call 'nop_reset_set(nop, 1)' and in this case we should put the GPIO directly in its active level state, so skip the GPIO toggle and delay. Signed-off-by: Fabio Estevam fabio.este...@freescale.com --- drivers/usb/phy/phy-generic.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c index 54697a0..ceb8ac9 100644 --- a/drivers/usb/phy/phy-generic.c +++ b/drivers/usb/phy/phy-generic.c @@ -66,8 +66,11 @@ static void nop_reset_set(struct usb_phy_generic *nop, int asserted) if (!nop-gpiod_reset) return; - gpiod_direction_output(nop-gpiod_reset, !asserted); + if (asserted) + goto skip_delay; + gpiod_set_value(nop-gpiod_reset, !asserted); usleep_range(1, 2); +skip_delay: gpiod_set_value(nop-gpiod_reset, asserted); } -- 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
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
Hi Felipe, On Fri, Jan 30, 2015 at 7:59 PM, Felipe Balbi ba...@ti.com wrote: - gpiod_direction_output(nop-gpiod_reset, !asserted); + if (asserted) + goto skip_delay; why skip it ? Let's suppose we have an active-low GPIO USB phy reset pin. In usb_gen_phy_init() we call nop_reset_set(nop, 0); and 'assert' is zero. Then we do: - Put the GPIO into 0 - Wait a bit - Put it back to 1. In usb_gen_phy_shutdown, we call nop_reset_set(nop, 1) and assert is one. Then we should simply do: - Put GPIO reset into 0. ,as we the PHY is no more in usage. That's the reason we don't need the delay when assert is one. Also, the code prior to the gpiod introduction also skips the delay for the assert == 1 case, so this patch restores the original behaviour. -- 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
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
On Fri, Jan 30, 2015 at 07:27:16PM -0200, Fabio Estevam wrote: From: Fabio Estevam fabio.este...@freescale.com Commit 9eb0797722895f4309b4 (sb: phy: generic: fix the gpios to be optional) calls gpiod_direction_output() in the probe function, so there is no need to call it again, as we can simply call gpiod_set_value() directly. Also, in usb_gen_phy_shutdown() we call 'nop_reset_set(nop, 1)' and in this case we should put the GPIO directly in its active level state, so skip the GPIO toggle and delay. Signed-off-by: Fabio Estevam fabio.este...@freescale.com --- drivers/usb/phy/phy-generic.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c index 54697a0..ceb8ac9 100644 --- a/drivers/usb/phy/phy-generic.c +++ b/drivers/usb/phy/phy-generic.c @@ -66,8 +66,11 @@ static void nop_reset_set(struct usb_phy_generic *nop, int asserted) if (!nop-gpiod_reset) return; - gpiod_direction_output(nop-gpiod_reset, !asserted); + if (asserted) + goto skip_delay; why skip it ? + gpiod_set_value(nop-gpiod_reset, !asserted); usleep_range(1, 2); +skip_delay: gpiod_set_value(nop-gpiod_reset, asserted); } -- 1.9.1 -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: phy: phy-generic: No need to call gpiod_direction_output() twice
On Fri, Jan 30, 2015 at 8:13 PM, Fabio Estevam feste...@gmail.com wrote:
Hi Felipe,
On Fri, Jan 30, 2015 at 7:59 PM, Felipe Balbi ba...@ti.com wrote:
- gpiod_direction_output(nop-gpiod_reset, !asserted);
+ if (asserted)
+ goto skip_delay;
why skip it ?
Let's suppose we have an active-low GPIO USB phy reset pin.
In usb_gen_phy_init() we call nop_reset_set(nop, 0); and 'assert' is zero.
Then we do:
- Put the GPIO into 0
- Wait a bit
- Put it back to 1.
In usb_gen_phy_shutdown, we call nop_reset_set(nop, 1) and assert is one.
Then we should simply do:
- Put GPIO reset into 0.
,as we the PHY is no more in usage.
That's the reason we don't need the delay when assert is one.
Also, the code prior to the gpiod introduction also skips the delay
for the assert == 1 case, so this patch restores the original
behaviour.
Or if you prefer we can make nop_reset_set() to only handle the reset case:
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -61,14 +61,13 @@ static int nop_set_suspend(struct usb_phy *x, int suspend)
return 0;
}
-static void nop_reset_set(struct usb_phy_generic *nop, int asserted)
+static void nop_reset_set(struct usb_phy_generic *nop)
{
if (!nop-gpiod_reset)
return;
-
- gpiod_direction_output(nop-gpiod_reset, !asserted);
+ gpiod_set_value(nop-gpiod_reset, 1);
usleep_range(1, 2);
- gpiod_set_value(nop-gpiod_reset, asserted);
+ gpiod_set_value(nop-gpiod_reset, 0);
}
/* interface to regulator framework */
@@ -150,8 +149,7 @@ int usb_gen_phy_init(struct usb_phy *phy)
if (!IS_ERR(nop-clk))
clk_prepare_enable(nop-clk);
- /* De-assert RESET */
- nop_reset_set(nop, 0);
+ nop_reset_set(nop);
return 0;
}
@@ -161,8 +159,8 @@ void usb_gen_phy_shutdown(struct usb_phy *phy)
{
struct usb_phy_generic *nop = dev_get_drvdata(phy-dev);
- /* Assert RESET */
- nop_reset_set(nop, 1);
+ if (nop-gpiod_reset)
+ gpiod_direction_output(nop-gpiod_reset, 1);
if (!IS_ERR(nop-clk))
clk_disable_unprepare(nop-clk);
--
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
