Hi Daniel, Thanks for your comments! Daniel Thompson <daniel.thompson@xxxxxxxxxx> 於 2022年6月23日 週四 晚上9:43寫道: > > On Thu, Jun 23, 2022 at 07:56:31PM +0800, ChiaEn Wu wrote: > > From: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx> > > > > Add Mediatek MT6370 Backlight support. > > > > Signed-off-by: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx> > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > index a003e02..7cd823d 100644 > > <snip> > > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv, > > + struct backlight_properties *props) > > +{ > > + struct device *dev = priv->dev; > > + u8 prop_val; > > + u32 brightness, ovp_uV, ocp_uA; > > + unsigned int mask, val; > > + int ret; > > + > > + /* Vendor optional properties */ > > + val = 0; > > + if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) > > + val |= MT6370_BL_PWM_EN_MASK; > > + > > + if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) > > + val |= MT6370_BL_PWM_HYS_EN_MASK; > > + > > + ret = device_property_read_u8(dev, > > + "mediatek,bled-pwm-hys-input-th-steps", > > + &prop_val); > > + if (!ret) { > > + prop_val = clamp_val(prop_val, > > + MT6370_BL_PWM_HYS_TH_MIN_STEP, > > + MT6370_BL_PWM_HYS_TH_MAX_STEP); > > + /* > > + * prop_val = 1 --> 1 steps --> 0x00 > > + * prop_val = 2 ~ 4 --> 4 steps --> 0x01 > > + * prop_val = 5 ~ 16 --> 16 steps --> 0x10 > > + * prop_val = 17 ~ 64 --> 64 steps --> 0x11 > > ^^^^^ > These numbers are binary, not hex, right? If so, the comments > should be 0b00 to 0b03 . Ohh! Yes! These numbers are binary! I so apologize for making this mistake... I will revise the comments in the next patch! Thank you so much! > > > > + */ > > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > > + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); > > + } > > + > > + ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM, > > + val, val); > > + if (ret) > > + return ret; > > Overall, I like this approach! Easy to read and understand. > > > > <snip> > > +static int mt6370_bl_probe(struct platform_device *pdev) > > +{ > > + struct mt6370_priv *priv; > > + struct backlight_properties props = { > > + .type = BACKLIGHT_RAW, > > + .scale = BACKLIGHT_SCALE_LINEAR, > > Sorry, I missed this before but the KConfig comment says that the > backlight can support both linear and exponential curves. > > Is there a good reason to default to linear? Well... The customers who used this PMIC have very few or even no use exponential curve, so I set the default to linear. If you think this is inappropriate, I will add a DT property to control this feature in the next patch! By the way, I found some mistakes in my probe() function... I didn't use "return" when I use dev_err_probe()... I will refine it in the next patch! > > > Daniel. > > Best regards, ChiaEn Wu