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

[Govindraj]

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;


>>
>> -     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;
>>  }
>
> [...]
>
>> diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
>> index 802f077..a036bab 100644
>> --- a/include/usb/ulpi.h
>> +++ b/include/usb/ulpi.h
>> @@ -28,12 +28,23 @@
>>  #endif
>>
>>  /*
>> + * ulpi view port address and
>> + * Port_number that can be passed.
>> + * Any additional data to be passed can
>> + * be extended from this structure
>> + */
>> +struct ulpi_viewport {
>> +     u32 viewport_addr;
>> +     u8 port_num;
>> +};
>
> haven't we aggreed, that this will be:
> u32 port_num;

My bad I missed this will correct now.

> ?
>
>> +
>> +/*
>>   * Initialize the ULPI transciever and check the interface integrity.
>> - * @ulpi_viewport -  the address of the ULPI viewport register.
>> + * @ulpi_viewport -  structure containing ULPI viewport data
>
> This is ulpi_vp now.

will correct this.

--
Thanks,
Govindraj.R