[U-Boot] [PATCH v3 1/2] usb: ulpi: add indicator configuration function

[Lucas Stach]

Am Freitag, den 28.09.2012, 10:15 +0200 schrieb Igor Grinberg:
> On 09/26/12 00:35, 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>
> > Acked-by: Igor Grinberg <grinberg at compulab.co.il>
> > ---
> > v3: Only touch each register once. Now checkpatch clean.
> > ---
> >  drivers/usb/ulpi/ulpi.c | 32 ++++++++++++++++++++++++++++----
> >  include/usb/ulpi.h      | 13 +++++++++++--
> >  2 Dateien geändert, 39 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)
> > 
> > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> > index dde2585..98dd23c 100644
> > --- a/drivers/usb/ulpi/ulpi.c
> > +++ b/drivers/usb/ulpi/ulpi.c
> > @@ -106,20 +106,44 @@ 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)
> > +{
> > +	u32 flags;
> > +	int ret;
> > +
> > +	ret = ulpi_write(ulpi_vp,
> > +			external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear,
> > +			ULPI_OTG_EXTVBUSIND);
> 
> I think the below would be clearer and also will look as the rest of the file does:
> 
> reg = external ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear;
> ret = ulpi_write(ulpi_vp, reg, ULPI_OTG_EXTVBUSIND);
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	flags = passthu ? ULPI_IFACE_PASSTHRU : 0;
> > +	flags |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0;
> > +	ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_set, flags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	flags = passthu ? 0 : ULPI_IFACE_PASSTHRU;
> > +	flags |= complement ? 0 : ULPI_IFACE_EXTVBUS_COMPLEMENT;
> > +	ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl_clear, flags);
> 
> Errr..., that is not what I meant... sorry for confusion.
> What I meant is something like:
> 
> u32 pthrough = passthu ? ULPI_IFACE_PASSTHRU : 0;
> u32 extcompl |= complement ? ULPI_IFACE_EXTVBUS_COMPLEMENT : 0;
> 
> val = ulpi_read(ulpi_vp, &ulpi->iface_ctrl);
> if (val == ULPI_ERROR)
> 	return val;
> 
> val = (val & ~ULPI_IFACE_PASSTHRU) | pthrough;
> val = (val & ~ULPI_IFACE_EXTVBUS_COMPLEMENT) | extcompl;
> ret = ulpi_write(ulpi_vp, &ulpi->iface_ctrl, val);
> 
> That way you write only once to each register and the code also look uniform.
> 
I tend to disagree. The ULPI PHY register set was specifically designed
to not need this use-modify-write dance. Why would we like to ignore
this?

Yes we are possible doing one unneeded register access here, but what
would it buy us to ignore the set/clear registers just to avoid one
register access? 
> > +	if (ret)
> > +		return ret;
> > +
> > +	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.
>