[U-Boot] [PATCH v3 6/7] usb: ulpi: Extend the existing ulpi framework.

[Igor Grinberg]

On 02/06/12 11:38, Govindraj wrote:
> Hi Igor,
> 
> On Mon, Feb 6, 2012 at 2:25 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>> Hi Govindraj,
>>
>> I was about to ask Tom to reorder the patches while applying, but
>> there are still several things to fix.
>> I'm also fine with fixing these by a follow up patch (after merging this).
>>
> 
> [...]
> 
>>
>> [...]
>>
>>> diff --git a/drivers/usb/ulpi/ulpi-viewport.c b/drivers/usb/ulpi/ulpi-viewport.c
>>> index 490fb0e..6f03f08 100644
>>> --- a/drivers/usb/ulpi/ulpi-viewport.c
>>> +++ b/drivers/usb/ulpi/ulpi-viewport.c
>>
>> [...]
>>
>>> -int ulpi_write(u32 ulpi_viewport, u8 *reg, u32 value)
>>> +int ulpi_write(struct ulpi_viewport *ulpi_vp, u8 *reg, u32 value)
>>>  {
>>>       u32 val = ULPI_RWRUN | ULPI_RWCTRL | ((u32)reg << 16) | (value & 0xff);
>>
>> You should utilize the port_num variable here, right?
>> something like:
>> val |= (ulpi_vp->port_num & 0x7) << 24;
>>
> 
> Yes correct, Shouldn't we add something like this
> 
> [...]
> 
> #ifndef CONFIG_ULPI_PORT_SHIFT && CONFIG_ULPI_PORT_MASK
> #define CONFIG_ULPI_PORT_SHIFT 24
> #define CONFIG_ULPI_PORT_MASK 0x7
> #endif
> 
> [...]
> 
> val |= (ulpi_vp->port_num & CONFIG_ULPI_PORT_MASK) <<
>                     CONFIG_ULPI_PORT_SHIFT;

We have already discussed this in the previous session [1]
And the conclusion was to use plain values instead,
because otherwise, you should replace all other masks and shifts...
and that will be a mess...
IMO,
(ulpi_vp->port_num & 0x7) << 24;
is really self explanatory and intuitive, so replacing it with defines
just make the code look bad and split into multiple lines.
Also the values are viewport specific and will not change, so
there is no reason to make them a config option.

[1] http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/124222

> 
> 
>>>
>>> -     return ulpi_request(ulpi_viewport, val);
>>> +     return ulpi_request(ulpi_vp, val);
>>>  }
>>>
>>> -u32 ulpi_read(u32 ulpi_viewport, u8 *reg)
>>> +u32 ulpi_read(struct ulpi_viewport *ulpi_vp, u8 *reg)
>>>  {
>>>       int err;
>>>       u32 val = ULPI_RWRUN | ((u32)reg << 16);
>>
>> same here
>>
>>>
>>> -     err = ulpi_request(ulpi_viewport, val);
>>> +     err = ulpi_request(ulpi_vp, val);
>>>       if (err)
>>>               return err;
>>>
>>> -     return (readl(ulpi_viewport) >> 8) & 0xff;
>>> +     return (readl(ulpi_vp->viewport_addr) >> 8) & 0xff;
>>>  }

[...]


-- 
Regards,
Igor.