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

[Igor Grinberg <grinberg@compulab.co.il>]

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.


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!

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@lynxeye.de>

After the below comments are fixed:
Acked-by: Igor Grinberg <grinberg@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.

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.

-- 
Regards,
Igor.