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

[Schrempf Frieder]

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]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_mctrl_gpio.h#n121
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/sh-sci.c#n3290
>