Re: [PATCH] serial: imx: Avoid probe failure in case of missing gpiolib

From: Schrempf Frieder
Date: Thu Aug 01 2019 - 07:00:00 EST

On 01.08.19 11:55, Uwe Kleine-KÃnig wrote:
> On Thu, Aug 01, 2019 at 09:28:33AM +0000, Schrempf Frieder wrote:
>> Hi Uwe,
>> On 01.08.19 10:48, Uwe Kleine-KÃnig wrote:
>>> On Thu, Aug 01, 2019 at 08:18:05AM +0000, Schrempf Frieder wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>>>> If CONFIG_GPIOLIB is not enabled, mctrl_gpio_init() will return
>>>> -ENOSYS and cause the probing of the imx UART to fail. As the
>>>> GPIOs are optional, we should continue probing in this case.
>>> Is this really still the case? On which version did you hit this
>>> problem?
>> Yes, I think it is. I used v5.2.5, that already has d99482673f95.
>>> I would expect that is gone with
>>> d99482673f950817b30caf3fcdfb31179b050ce1 if not earlier.
>> I think this is a different problem. If CONFIG_GPIOLIB is disabled,
>> mctrl_gpio_init() returns -ENOSYS unconditionally here: [1]. The
>> existing patch (d99482673f95) seems to handle the case when
>> CONFIG_GPIOLIB is enabled, but no or not all GPIOs are given in the dtb.
> Ah, I see.
> I don't think we should handle this on a per-driver basis. So my
> suggestion is to drop the dummy implementation for mctrl_gpio if GPIOLIB
> is disabled. Then the behaviour should be consistant with the gpio stuff
> returning NULL in this case. (Or alternatively adapt the dummy
> implementation to shortcut and behave identically.)

I get your point, but it seems a bit strange to go down all the way from
the driver to mctrl_gpio_init_noauto() and into the loop just to return
an empty struct mctrl_gpios to the driver that will be looped over again
on each call of mctrl_gpio_*().

So I would rather go with a variation of your second proposal and keep
the dummy implementation, but let it return NULL instead of an error
pointer, as all the mctrl_gpio_*() functions already seem to have a
check for gpios == NULL.

What do you think?

> (Having said that I don't like gpiolib's behaviour of returning NULL for
> the optional calls if it's disabled, but having mctrl_gpio behave
> differently is worse.)
>> The sh-sci.c driver has a similar check to skip this case: [2].
> This should than be dropped, too.
> Best regards
> Uwe
>> [1]
>> [2]