On Thu, Feb 4, 2021 at 11:05 PM Marek Vasut marex@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