On Mon, Sep 25, 2023 at 02:26:09PM +0200, Flavio Suligoi wrote: > diff --git a/drivers/video/backlight/mp3309c.c b/drivers/video/backlight/mp3309c.c > new file mode 100644 > index 000000000000..923ac7f7b291 > --- /dev/null > +++ b/drivers/video/backlight/mp3309c.c > @@ -0,0 +1,398 @@ > ... > +static int mp3309c_bl_update_status(struct backlight_device *bl) > +{ > + struct mp3309c_chip *chip = bl_get_data(bl); > + int brightness = backlight_get_brightness(bl); > + struct pwm_state pwmstate; > + unsigned int analog_val, bits_val; > + int i, ret; > + > + if (chip->pdata->dimming_mode == DIMMING_PWM) { > + /* > + * PWM dimming mode > + */ > + pwm_get_state(chip->pwmd, &pwmstate); > + pwm_set_relative_duty_cycle(&pwmstate, brightness, > + chip->pdata->max_brightness); > + pwmstate.enabled = true; > + ret = pwm_apply_state(chip->pwmd, &pwmstate); > + if (ret) > + return ret; > + > + switch (chip->pdata->status) { > + case FIRST_POWER_ON: > + case BACKLIGHT_OFF: > + /* > + * After 20ms of low pwm signal level, the chip turns > + off automatically. In this case, before enabling the > + chip again, we must wait about 10ms for pwm signal to > + stabilize. > + */ > + if (brightness > 0) { > + msleep(10); > + mp3309c_enable_device(chip); > + chip->pdata->status = BACKLIGHT_ON; > + } else { > + chip->pdata->status = BACKLIGHT_OFF; > + } > + break; > + case BACKLIGHT_ON: > + if (brightness == 0) > + chip->pdata->status = BACKLIGHT_OFF; > + break; > + } > + } else { > + /* > + * Analog dimming (by I2C command) dimming mode > + * > + * The first time, before setting brightness, we must enable the > + * device > + */ > + if (chip->pdata->status == FIRST_POWER_ON) > + mp3309c_enable_device(chip); > + > + /* > + * Dimming mode I2C command > + * > + * The 5 bits of the dimming analog value D4..D0 is allocated > + * in the I2C register #0, in the following way: > + * > + * +--+--+--+--+--+--+--+--+ > + * |EN|D0|D1|D2|D3|D4|XX|XX| > + * +--+--+--+--+--+--+--+--+ > + */ > + analog_val = DIV_ROUND_UP(ANALOG_MAX_VAL * brightness, > + chip->pdata->max_brightness); Sorry to only notice after sharing a Reviewed-by:[1] but... Scaling brightness here isn't right. When running in I2C dimming mode then max_brightness *must* be 31 or lower, meaning the value in brightness can be applied directly to the hardware without scaling. Quoting the DT binding docs about how max-brightness should be interpretted: Normally the maximum brightness is determined by the hardware and this property is not required. This property is used to put a software limit on the brightness apart from what the driver says, as it could happen that a LED can be made so bright that it gets damaged or causes damage due to restrictions in a specific system, such as mounting conditions. Daniel. [1] I remember checking if this code could overflow the field but I was so distracted by that I ended up missing the obvious!