[U-Boot] [PATCH V2 1/6] dm: gpio: add a default gpio xlate routine

[Eric Nelson]

Hi Stephen,

On 04/11/2016 10:16 AM, Stephen Warren wrote:
> On 04/11/2016 11:00 AM, Eric Nelson wrote:
>> Many drivers use a common form of offset + flags for device
>> tree nodes. e.g.:
>>     <&gpio1 2 GPIO_ACTIVE_LOW>
>>
>> This patch adds a common implementation of this type of parsing
>> and calls it when a gpio driver doesn't supply its' own xlate
>> routine.
>>
>> This will allow removal of the driver-specific versions in a
>> handful of drivers and simplify the addition of new drivers.
> 
> The series,
> Acked-by: Stephen Warren <swarren at nvidia.com>
> 
> Although one nit below.
> 
>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> 
>> +int gpio_xlate_offs_flags(struct udevice *dev,
>> +                     struct gpio_desc *desc,
>> +                     struct fdtdec_phandle_args *args)
>> +{
>> +    int ret = -EINVAL;
>> +    if (args->args_count > 1) {
>> +        desc->offset = args->args[0];
>> +        if (args->args[1] & GPIO_ACTIVE_LOW)
>> +            desc->flags = GPIOD_ACTIVE_LOW;
>> +        ret = 0;
>> +    }
>> +    return ret;
>> +}
> 
> Why not return the error first, and avoid the need for an extra
> indentation level for the rest of the function, and make it quite a bit
> more readable in my opinion. The default could also be made to cover
> more situations (e.g. bindings that define a single cell for the GPIO
> but not cell at all for any flags):
> 
> if (args->args_count < 1)
>     return -EINVAL;
> 
> desc->offset = args->args[0];
> 
> if (args->args_count < 2)
>     return 0;
> 
> if (args->args[1] & GPIO_ACTIVE_LOW)
>     desc->flags = GPIOD_ACTIVE_LOW;
> 
> return 0;

I like it. V3 of that one patch forthcoming.