[PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver

[Marek Vasut]

On 2/12/21 9:43 AM, Linus Walleij wrote:
> On Thu, Feb 4, 2021 at 11:05 PM Marek Vasut <marex at denx.de> wrote:
>> On 2/4/21 10:10 PM, Doug Anderson wrote:
> 
>>>>>> +       /*
>>>>>> +        * Reset the chip, pull EN line low for t_reset=10ms,
>>>>>> +        * then high for t_en=1ms.
>>>>>> +        */
>>>>>> +       gpiod_set_value(ctx->enable_gpio, 0);
>>>>>
>>>>> Why not use the "cansleep" version to give some flexibility?
>>>>
>>>> Does that make a difference in non-interrupt context ?
>>>
>>> As I understand it:
>>>
>>> * If a client calls gpiod_set_value() then the underlying GPIO can
>>> only be one that doesn't sleep.
>>>
>>> * If a client calls gpiod_set_value_cansleep() then the underlying
>>> GPIO can be either one that does or doesn't sleep.
>>>
>>> * A client is only allowed to call gpiod_set_value_cansleep() if it's
>>> not in interrupt context.
>>>
>>> You are definitely not in an interrupt context (right?), so calling
>>> the "cansleep" version has no downsides but allows board designers to
>>> hook up an enable that can sleep.
>>
>> Linus, can you please confirm this ? I find this hard to believe, since
>> there are plenty of places in the kernel which use gpiod_set_value()
>> without the _cansleep, are those problematic then ?
> 
> The function looks like so:
> 
> void gpiod_set_value(struct gpio_desc *desc, int value)
> {
>          VALIDATE_DESC_VOID(desc);
>          /* Should be using gpiod_set_value_cansleep() */
>          WARN_ON(desc->gdev->chip->can_sleep);
>          gpiod_set_value_nocheck(desc, value);
> }
> 
> So if the chip has set ->can_sleep (i,e, this GPIO is on something
> like an I2C bus) then a warning will appear.
> 
> The warning only really appear if you have a driver that was
> previously only used on a gpiochip with "fast" (write a register)
> GPIOs and somebody start to use the same driver with a
> GPIO on e.g. an I2C bus, which will define ->can_sleep.
> 
> The simple solution to the warning is to switch to the
> _cansleep() variant but really it is a sign to check that
> this is not being called in atomic context and just check
> that the driver overall will survive sleeping context
> here.
> 
> In a way _cansleep() is just syntactic sugar.
> 
> In this driver, you can use _cansleep() if you like but not
> using it mostly works too, if used with SoC-type GPIOs.
> Whoever uses the driver with a GPIO on an I2C chip
> or similar gets to fix it.

Nice, I'm gonna archive this, this explanation is very clear, thanks.