[U-Boot] [PATCH v1 1/3] drivers: gpio: Handle gracefully NULL pointers

[Jean-Jacques Hiblot]

On 22/10/2019 00:53, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
>>
>> On 18/10/2019 22:38, Simon Glass wrote:
>>> Hi Jean-Jacques,
>>>
>>> On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
>>>> Prepare the way for a managed GPIO API by handling NULL pointers without
>>>> crashing nor failing.
>>>> VALIDATE_DESC() and validate_desc() come straight from Linux.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>>>> ---
>>>>
>>>>    drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
>>>>    include/asm-generic/gpio.h |  2 +-
>>>>    2 files changed, 57 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>>>> index 01cfa2f788..63c10f438b 100644
>>>> --- a/drivers/gpio/gpio-uclass.c
>>>> +++ b/drivers/gpio/gpio-uclass.c
>>>> @@ -18,6 +18,33 @@
>>>>
>>>>    DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> +/*
>>>> + * This descriptor validation needs to be inserted verbatim into each
>>>> + * function taking a descriptor, so we need to use a preprocessor
>>>> + * macro to avoid endless duplication. If the desc is NULL it is an
>>>> + * optional GPIO and calls should just bail out.
>>>> + */
>>>> +static int validate_desc(const struct gpio_desc *desc, const char *func)
>>>> +{
>>>> +       if (!desc)
>>>> +               return 0;
>>>> +       if (IS_ERR(desc)) {
>>>> +               pr_warn("%s: invalid GPIO (errorpointer)\n", func);
>>>> +               return PTR_ERR(desc);
>>>> +       }
>>>> +       if (!desc->dev) {
>>>> +               pr_warn("%s: invalid GPIO (no device)\n", func);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +       return 1;
>>>> +}
>>>> +
>>>> +#define VALIDATE_DESC(desc) do { \
>>>> +       int __valid = validate_desc(desc, __func__); \
>>>> +       if (__valid <= 0) \
>>>> +               return __valid; \
>>>> +       } while (0)
>>> This adds to code size so should be behind a CONFIG I think.
>> I'm not sure we really want to keep this out. Most of the added code
>> size, will be about the error messages. I would rather remove them (or
>> use a debug() or warn_non_spl()
> You should probably do that anyway.
>
> But these checks do add to code size, and we should be careful not to
> have unnecessary checks in the final firmware. We can enable them
> during development, but don't want the code bloated with lots of
> pointless checks.

I see, but I don't agree that this is pointless. It allows for optional 
GPIO support.

Practically speaking, in u-boot, that will probably be used only by the 
users of the managed API: devm_gpiod_get_optional() and 
devm_gpiod_get_index_optional()


>
>>>> +
>>>>    /**
>>>> I't the implicit return that I am not keen on. Is it needed? I don't
>>>> see that sort of thing in U-Boot at present.

Well it bothered me too at first when I looked at how it was done in 
linux. But I got used to it pretty quickly :)

I'll remove this in the v2


Thanks for the review.


JJ

>>>>
>>>> Regards,
>>>> Simon
>>>>