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

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

Hi Lucas,

On 09/28/12 16:46, Lucas Stach wrote:
> 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@lynxeye.de>
>>> Acked-by: Igor Grinberg <grinberg@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

[...]

>>> +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?

We don't...
I mean, we use this in cases when only one bit needs to be modified.
When you need to modify more then one bit (or more precisely >2 bits),
it is better be done in r-m-w manner, so you do as few ULPI bus
accesses as possible. Each ULPI transaction takes much more time than
playing with bits in the function.

> 
> 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? 

In this function only two bits are changed, so it is still two
transactions on the ULPI bus, but if for some reason, in the future,
we will need to change more bits in the same function and register,
we will get patches adding more transactions instead of updating
the local variable. Also, the ulpi.c is done having this in mind,
so for it to be uniform, we'd better do this in the same fashion.

Also, I've understood from your patch, that what you have understood
from my comments was not exactly what meant, so I wanted to make my
comments clear and open for discussion. It does not meter now,
as Marek already applied the new version.

Anyway, thanks for the patches and the effort!

[...]

-- 
Regards,
Igor.