[U-Boot] [PATCH v2 4/5] usb: ulpi: add indicator configuration function

[Marek Vasut]

Dear Igor Grinberg,

> Hi Lucas, Tom,
> 
> I'm sorry for the late reply.
> I understand, that Tom has already applied this to tegra/next,
> but as the changes/follow up patches are required,
> may be we can do this in another fashion...
> 
> 1) Thanks for the patch and working on extending the generic framework!
> 2) This patch has no dependencies on tegra specific patches, so
>    I think, it should go through Marex usb tree, but doing this will
>    require the right merge order, so bisectability will not suffer.
>    So, Marek, Tom, you should decide which way is fine with you both.

_ALWAYS_ CC the right custodians. That is, me. Seeing this patch bypassed me 
completely, I'm really unhappy.

> 
> Tom,
> Yesterday, I was wondering if the patch was already applied, and
> I had no clue what's its status. Also, the patchwork says "New".
> So, if it is not hard for you in the future, I'd like a short reply
> to the list, saying something like: "Applied, thanks.", like most
> custodians do. Thanks!

+1

> On 08/21/12 23:18, Lucas Stach wrote:
> > Allows for easy configuration of the VBUS indicator related ULPI
> > config bits.
> > 
> > Also move the external indicator setup from ulpi_set_vbus() to
> > the new function.
> > 
> > Signed-off-by: Lucas Stach <dev at lynxeye.de>
> 
> After the below comments are fixed:
> Acked-by: Igor Grinberg <grinberg at compulab.co.il>
> 
> > ---
> > 
> >  drivers/usb/ulpi/ulpi.c | 26 ++++++++++++++++++++++----
> >  include/usb/ulpi.h      | 13 +++++++++++--
> >  2 Dateien geändert, 33 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)
> > 
> > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> > index dde2585..f358bde 100644
> > --- a/drivers/usb/ulpi/ulpi.c
> > +++ b/drivers/usb/ulpi/ulpi.c
> > @@ -106,20 +106,38 @@ int ulpi_select_transceiver(struct ulpi_viewport
> > *ulpi_vp, unsigned speed)
> > 
> >  	return ulpi_write(ulpi_vp, &ulpi->function_ctrl, val);
> >  
> >  }
> > 
> > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power,
> > -			int ext_ind)
> > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power)
> > 
> >  {
> >  
> >  	u32 flags = ULPI_OTG_DRVVBUS;
> >  	u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> >  	
> >  	if (ext_power)
> >  	
> >  		flags |= ULPI_OTG_DRVVBUS_EXT;
> > 
> > -	if (ext_ind)
> > -		flags |= ULPI_OTG_EXTVBUSIND;
> > 
> >  	return ulpi_write(ulpi_vp, reg, flags);
> >  
> >  }
> > 
> > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> > +			int passthu, int complement)
> > +{
> > +	u8 *reg;
> > +	int ret;
> > +
> > +	reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> > +		return ret;
> > +
> > +	reg = passthu ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> > +		return ret;
> > +
> > +	reg = complement ? &ulpi->iface_ctrl_set : &ulpi->iface_ctrl_clear;
> > +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> > +		return ret;
> 
> These are fine, two requests though:
> 1) As Tom already pointed in the private email:
> ERROR: do not use assignment in if condition
> #361: FILE: drivers/usb/ulpi/ulpi.c:127:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND)))
> 
> ERROR: do not use assignment in if condition
> #365: FILE: drivers/usb/ulpi/ulpi.c:131:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_PASSTHRU)))
> 
> ERROR: do not use assignment in if condition
> #369: FILE: drivers/usb/ulpi/ulpi.c:135:
> +	if ((ret = ulpi_write(ulpi_vp, reg, ULPI_IFACE_EXTVBUS_COMPLEMENT)))
> 
> those must be fixed.

Agreed, _ALWAYS_ run checkpatch.pl before submitting. It's even better idea to 
add a git precommit hook for that.

> 2) Can you make only one access for each register?
>    Use flags/val variable (like in other places) and
>    do only one access per register. Can you?
> 
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  int ulpi_set_pd(struct ulpi_viewport *ulpi_vp, int enable)
> >  {
> >  
> >  	u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN;
> > 
> > diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
> > index 9a75c24..99166c4 100644
> > --- a/include/usb/ulpi.h
> > +++ b/include/usb/ulpi.h
> > @@ -61,8 +61,17 @@ int ulpi_select_transceiver(struct ulpi_viewport
> > *ulpi_vp, unsigned speed);
> > 
> >   *
> >   * returns 0 on success, ULPI_ERROR on failure.
> >   */
> > 
> > -int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp,
> > -			int on, int ext_power, int ext_ind);
> > +int ulpi_set_vbus(struct ulpi_viewport *ulpi_vp, int on, int ext_power);
> > +
> > +/*
> > + * Configure VBUS indicator
> > + * @external		- external VBUS over-current indicator is used
> > + * @passthru		- disables ANDing of internal VBUS comparator
> > + *                    with external VBUS input
> > + * @complement		- inverts the external VBUS input
> > + */
> > +int ulpi_set_vbus_indicator(struct ulpi_viewport *ulpi_vp, int external,
> > +			int passthru, int complement);
> > 
> >  /*
> >  
> >   * Enable/disable pull-down resistors on D+ and D- USB lines.