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

[Linus Walleij]

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.

Yours,
Linus Walleij