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 <festevam@xxxxxxxxx> wrote:
> > Hi Felipe,
> >
> > On Fri, Jan 30, 2015 at 7:59 PM, Felipe Balbi <balbi@xxxxxx> 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(10000, 20000);
> - 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
Attachment:
signature.asc
Description: Digital signature
