On Fri, Jun 04, 2021 at 08:32:01AM +0200, Linus Walleij wrote: > Remove interrupt disablement during backlight setting. It is > way to dangerous and makes platforms instable by having it > miss vblank IRQs leading to the graphics derailing. > > The code is using ndelay() which is not available on > platforms such as ARM and will result in 32 * udelay(1) > which is substantial. > > Add some code to detect if an interrupt occurs during the > tight loop and in that case just redo it from the top. > > Fixes: 5317f37e48b9 ("backlight: Add Kinetic KTD253 backlight driver") > Cc: Stephan Gerhold <stephan@xxxxxxxxxxx> > Reported-by: newbyte@xxxxxxxxxxx > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> Reviewed-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx> Daniel. > --- > ChangeLog v2->v3: > - Read my own patch and realized a bug: when we get a timeout > we bounce back to max period, but still count down the pwm > with one leading to an off-by-one error. Fix it by extending > some else clauses. > ChangeLog v1->v2: > - Alter the dimming code to check for how many ns the pulse > is low, and if it gets to ~100 us then redo from start. > This is to account for the advent that an IRQ arrives while > setting backlight and hits the low pulse making it way > too long. > --- > drivers/video/backlight/ktd253-backlight.c | 75 ++++++++++++++++------ > 1 file changed, 55 insertions(+), 20 deletions(-) > > diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c > index a7df5bcca9da..37aa5a669530 100644 > --- a/drivers/video/backlight/ktd253-backlight.c > +++ b/drivers/video/backlight/ktd253-backlight.c > @@ -25,6 +25,7 @@ > > #define KTD253_T_LOW_NS (200 + 10) /* Additional 10ns as safety factor */ > #define KTD253_T_HIGH_NS (200 + 10) /* Additional 10ns as safety factor */ > +#define KTD253_T_OFF_CRIT_NS 100000 /* 100 us, now it doesn't look good */ > #define KTD253_T_OFF_MS 3 > > struct ktd253_backlight { > @@ -34,13 +35,50 @@ struct ktd253_backlight { > u16 ratio; > }; > > +static void ktd253_backlight_set_max_ratio(struct ktd253_backlight *ktd253) > +{ > + gpiod_set_value_cansleep(ktd253->gpiod, 1); > + ndelay(KTD253_T_HIGH_NS); > + /* We always fall back to this when we power on */ > +} > + > +static int ktd253_backlight_stepdown(struct ktd253_backlight *ktd253) > +{ > + /* > + * These GPIO operations absolutely can NOT sleep so no _cansleep > + * suffixes, and no using GPIO expanders on slow buses for this! > + * > + * The maximum number of cycles of the loop is 32 so the time taken > + * should nominally be: > + * (T_LOW_NS + T_HIGH_NS + loop_time) * 32 > + * > + * Architectures do not always support ndelay() and we will get a few us > + * instead. If we get to a critical time limit an interrupt has likely > + * occured in the low part of the loop and we need to restart from the > + * top so we have the backlight in a known state. > + */ > + u64 ns; > + > + ns = ktime_get_ns(); > + gpiod_set_value(ktd253->gpiod, 0); > + ndelay(KTD253_T_LOW_NS); > + gpiod_set_value(ktd253->gpiod, 1); > + ns = ktime_get_ns() - ns; > + if (ns >= KTD253_T_OFF_CRIT_NS) { > + dev_err(ktd253->dev, "PCM on backlight took too long (%llu ns)\n", ns); > + return -EAGAIN; > + } > + ndelay(KTD253_T_HIGH_NS); > + return 0; > +} > + > static int ktd253_backlight_update_status(struct backlight_device *bl) > { > struct ktd253_backlight *ktd253 = bl_get_data(bl); > int brightness = backlight_get_brightness(bl); > u16 target_ratio; > u16 current_ratio = ktd253->ratio; > - unsigned long flags; > + int ret; > > dev_dbg(ktd253->dev, "new brightness/ratio: %d/32\n", brightness); > > @@ -62,37 +100,34 @@ static int ktd253_backlight_update_status(struct backlight_device *bl) > } > > if (current_ratio == 0) { > - gpiod_set_value_cansleep(ktd253->gpiod, 1); > - ndelay(KTD253_T_HIGH_NS); > - /* We always fall back to this when we power on */ > + ktd253_backlight_set_max_ratio(ktd253); > current_ratio = KTD253_MAX_RATIO; > } > > - /* > - * WARNING: > - * The loop to set the correct current level is performed > - * with interrupts disabled as it is timing critical. > - * The maximum number of cycles of the loop is 32 > - * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32, > - */ > - local_irq_save(flags); > while (current_ratio != target_ratio) { > /* > * These GPIO operations absolutely can NOT sleep so no > * _cansleep suffixes, and no using GPIO expanders on > * slow buses for this! > */ > - gpiod_set_value(ktd253->gpiod, 0); > - ndelay(KTD253_T_LOW_NS); > - gpiod_set_value(ktd253->gpiod, 1); > - ndelay(KTD253_T_HIGH_NS); > - /* After 1/32 we loop back to 32/32 */ > - if (current_ratio == KTD253_MIN_RATIO) > + ret = ktd253_backlight_stepdown(ktd253); > + if (ret == -EAGAIN) { > + /* > + * Something disturbed the backlight setting code when > + * running so we need to bring the PWM back to a known > + * state. This shouldn't happen too much. > + */ > + gpiod_set_value_cansleep(ktd253->gpiod, 0); > + msleep(KTD253_T_OFF_MS); > + ktd253_backlight_set_max_ratio(ktd253); > + current_ratio = KTD253_MAX_RATIO; > + } else if (current_ratio == KTD253_MIN_RATIO) { > + /* After 1/32 we loop back to 32/32 */ > current_ratio = KTD253_MAX_RATIO; > - else > + } else { > current_ratio--; > + } > } > - local_irq_restore(flags); > ktd253->ratio = current_ratio; > > dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio); > -- > 2.31.1 >