RE: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C
Hi Daniel,
...
> > +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, );
> > + pwm_set_relative_duty_cycle(, brightness,
> > + chip->pdata->max_brightness);
> > + pwmstate.enabled = true;
> > + ret = pwm_apply_state(chip->pwmd, );
> > + 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.
ok, right; max brightness is 31, fixed
>
> 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.
>
ok
>
> 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!
Thanks and best regards,
Flavio
Re: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C
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 ..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, );
> + pwm_set_relative_duty_cycle(, brightness,
> + chip->pdata->max_brightness);
> + pwmstate.enabled = true;
> + ret = pwm_apply_state(chip->pwmd, );
> + 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!
