On Thu, Feb 4, 2021 at 11:05 PM Marek Vasut <marex@xxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
