[PATCH v9 6/6] davinci: USB1.1 support for Omapl138-Hawkboard

[Victor Rodriguez]

On Mon, Dec 6, 2010 at 9:53 AM, Ben Gardiner <bengardiner at nanometrics.ca> wrote:
> On Mon, Dec 6, 2010 at 10:47 AM, Victor Rodriguez <vm.rod25 at gmail.com> wrote:
>> On Mon, Dec 6, 2010 at 5:43 AM, Sergei Shtylyov <sshtylyov at mvista.com> wrote:
>>> Hello.
>>>
>>> On 06-12-2010 14:12, Nori, Sekhar wrote:
>>>
>>>>>  arch/arm/mach-davinci/board-omapl138-hawk.c |   38
>>>>> ++++++++++++++++++++------
>>>>>  1 files changed, 29 insertions(+), 9 deletions(-)
>>>
>>>>> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>>> b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>>> index da51136..8fc78f2 100644
>>>>> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>>> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>
>>> [...]
>>>>>
>>>>> @@ -165,13 +165,23 @@ static __init void omapl138_hawk_mmc_init(void)
>>>>>       if (ret<  0) {
>>>>>               pr_warning("%s: can not open GPIO %d\n",
>>>>>                       __func__, DA850_HAWK_MMCSD_WP_PIN);
>>>>> -             return;
>>>>> +             goto exp_setup_wp_fail;
>>>>>       }
>>>>>
>>>>>       ret = da8xx_register_mmcsd0(&da850_mmc_config);
>>>>> -     if (ret)
>>>>> +     if (ret) {
>>>>>               pr_warning("%s: MMC/SD0 registration failed: %d\n",
>>>>>                       __func__, ret);
>>>>> +             goto exp_setup_mmcsd_fail;
>>>>> +     }
>>>>> +             return;
>>>
>>>> This return has extra indentation.
>>>
>>>>> +
>>>>> +exp_setup_mmcsd_fail:
>>>>> +     gpio_free(DA850_HAWK_MMCSD_WP_PIN);
>>>>> +exp_setup_wp_fail:
>>>>> +     gpio_free(DA850_HAWK_MMCSD_CD_PIN);
>>>>> +exp_setup_cd_fail:
>>>>> +             return;
>>>
>>>> This one too.
>>>
>>>   Moreover, it's not needed at all.
>>
>> Ok
>>>> Other than that, it all looks good to me.
>>>
>>>   Except those 'exp_setup_'prefixes which I'm not sure where are coming
>>> from...
>>
>>
>> it comes from
>>
>> arch/arm/mach-davinci/board-da850-evm.c
>>
>> I took this as a template and I think that is better to keep this
>> exp_setup_  as a template
>>
>> but if you have any other suggestion please tell me
>
> I think those are based on the error handling in
> da850_evm_ui_expander_setup() which means that the 'exp' in
> 'exp_setup_' stands-for IO-expander.
>
> Based on the function names where the your exp_setup_* labels were
> introduced I think that the label could be renamed 'mmc_setup_*' and
> 'usb11_setup_*'.
>
I like it thanks I will submit the patches with that names. Sergei are
you ok with this ?

Regards

Victor Rodriguez


> Best Regards,
> Ben Gardiner
>
> ---
> Nanometrics Inc.
> http://www.nanometrics.ca
>