| Message ID | 20230123160615.375969-2-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: mtk-disp: Fix backlight configuration at boot | expand |
On Mon, Jan 23, 2023 at 05:06:14PM +0100, AngeloGioacchino Del Regno wrote: > If shadow registers usage is not desired, disable that before performing > any write to CON0/1 registers in the .apply() callback, otherwise we may > lose clkdiv or period/width updates. > > Fixes: cd4b45ac449a ("pwm: Add MediaTek MT2701 display PWM driver support") > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/pwm/pwm-mtk-disp.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c > index 692a06121b28..82b430d881a2 100644 > --- a/drivers/pwm/pwm-mtk-disp.c > +++ b/drivers/pwm/pwm-mtk-disp.c > @@ -138,6 +138,19 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > high_width = mul_u64_u64_div_u64(state->duty_cycle, rate, div); > value = period | (high_width << PWM_HIGH_WIDTH_SHIFT); > > + if (mdp->data->bls_debug && !mdp->data->has_commit) { > + /* > + * For MT2701, disable double buffer before writing register Not necessarily part of this series, but I guess it would make sense to remove the "For MT2701". It's no longer exclusive to that SoC and the condition in the if above makes it clear when this happens. Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> On MT8192 Asurada Spherion. Thanks, Nícolas > + * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH. > + */ > + mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, > + mdp->data->bls_debug_mask, > + mdp->data->bls_debug_mask); > + mtk_disp_pwm_update_bits(mdp, mdp->data->con0, > + mdp->data->con0_sel, > + mdp->data->con0_sel); > + } > + > mtk_disp_pwm_update_bits(mdp, mdp->data->con0, > PWM_CLKDIV_MASK, > clk_div << PWM_CLKDIV_SHIFT); > @@ -152,17 +165,6 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > mtk_disp_pwm_update_bits(mdp, mdp->data->commit, > mdp->data->commit_mask, > 0x0); > - } else { > - /* > - * For MT2701, disable double buffer before writing register > - * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH. > - */ > - mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, > - mdp->data->bls_debug_mask, > - mdp->data->bls_debug_mask); > - mtk_disp_pwm_update_bits(mdp, mdp->data->con0, > - mdp->data->con0_sel, > - mdp->data->con0_sel); > } > > mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask, > -- > 2.39.0 > >
Il 26/01/23 16:06, Nícolas F. R. A. Prado ha scritto: > On Mon, Jan 23, 2023 at 05:06:14PM +0100, AngeloGioacchino Del Regno wrote: >> If shadow registers usage is not desired, disable that before performing >> any write to CON0/1 registers in the .apply() callback, otherwise we may >> lose clkdiv or period/width updates. >> >> Fixes: cd4b45ac449a ("pwm: Add MediaTek MT2701 display PWM driver support") >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> --- >> drivers/pwm/pwm-mtk-disp.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c >> index 692a06121b28..82b430d881a2 100644 >> --- a/drivers/pwm/pwm-mtk-disp.c >> +++ b/drivers/pwm/pwm-mtk-disp.c >> @@ -138,6 +138,19 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> high_width = mul_u64_u64_div_u64(state->duty_cycle, rate, div); >> value = period | (high_width << PWM_HIGH_WIDTH_SHIFT); >> >> + if (mdp->data->bls_debug && !mdp->data->has_commit) { >> + /* >> + * For MT2701, disable double buffer before writing register > > Not necessarily part of this series, but I guess it would make sense to remove > the "For MT2701". It's no longer exclusive to that SoC and the condition in the > if above makes it clear when this happens. Thanks for the review and test! Anyway, for that "For MT2701", well, it ... should actually be exclusive to that SoC (and some others) because, in reality, MT8183, MT8186, MT8192, MT8195 and also almost all MediaTek smartphone SoCs do have shadow registers which we should use for the backlight. The bls_debug writes that we do are to disable writing to shadow registers and performing commits because this driver does not (yet) support mtk_mutex handling. The right thing to do here would be to grab a mtk_mutex, lock it, write shadow registers, perform "backlighting adjustment magic" in mediatek_drm, commit, unlock. Now, the "backlighting adjustment magic" is something that we do not support (yet?) in mediatek_drm, and it's also not really easy to implement: part of that magic needs RGB ALS readings and implementation of some more IP, which serves the purpose of, for example, adjusting the backlight PWM while taking account of some GAMMA adjustments to enhance readability of the screen in direct sunlight, or to decrease (slightly) power consumption of a display lit at night. So... the current way that we're using right now (which is disabling the shadow registers and performing direct writes to working registers to atomically set the backlight) is something that *should* disappear in *a* future in which that "magic" gets somehow properly implemented. This 9k lines mail, just to say that "For MT2701" is technically exclusive to that SoC, between the number of supported SoCs of this driver :-P Cheers! Andlo > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > On MT8192 Asurada Spherion. > > Thanks, > Nícolas >
diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c index 692a06121b28..82b430d881a2 100644 --- a/drivers/pwm/pwm-mtk-disp.c +++ b/drivers/pwm/pwm-mtk-disp.c @@ -138,6 +138,19 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, high_width = mul_u64_u64_div_u64(state->duty_cycle, rate, div); value = period | (high_width << PWM_HIGH_WIDTH_SHIFT); + if (mdp->data->bls_debug && !mdp->data->has_commit) { + /* + * For MT2701, disable double buffer before writing register + * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH. + */ + mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, + mdp->data->bls_debug_mask, + mdp->data->bls_debug_mask); + mtk_disp_pwm_update_bits(mdp, mdp->data->con0, + mdp->data->con0_sel, + mdp->data->con0_sel); + } + mtk_disp_pwm_update_bits(mdp, mdp->data->con0, PWM_CLKDIV_MASK, clk_div << PWM_CLKDIV_SHIFT); @@ -152,17 +165,6 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, mtk_disp_pwm_update_bits(mdp, mdp->data->commit, mdp->data->commit_mask, 0x0); - } else { - /* - * For MT2701, disable double buffer before writing register - * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH. - */ - mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, - mdp->data->bls_debug_mask, - mdp->data->bls_debug_mask); - mtk_disp_pwm_update_bits(mdp, mdp->data->con0, - mdp->data->con0_sel, - mdp->data->con0_sel); } mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
If shadow registers usage is not desired, disable that before performing any write to CON0/1 registers in the .apply() callback, otherwise we may lose clkdiv or period/width updates. Fixes: cd4b45ac449a ("pwm: Add MediaTek MT2701 display PWM driver support") Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/pwm/pwm-mtk-disp.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)