Re: [RESEND 14/14] video: backlight: mt6370: Add Mediatek MT6370 support

[Daniel Thompson <daniel.thompson@linaro.org>]

On Fri, Jun 03, 2022 at 03:14:56AM +0800, ChiaEn Wu wrote:
> Daniel Thompson <daniel.thompson@linaro.org> 於 2022年6月1日 週三 下午5:46寫道：
> >
> > On Tue, May 31, 2022 at 07:19:00PM +0800, ChiaEn Wu wrote:
> > > +#define MT6370_DT_PROP_DECL(_name, _type, _reg, _mask, _max, _inv)   \
> > > +{                                                                    \
> > > +     .name = "mediatek,bled-" #_name,                                \
> >
> > I'd rather have the whole DT property in the macro (because it helps
> > with grepability).
> 
> Do you mean the _name parameter must be the full name of the DT
> property and do not use "#" to concat like following example?
> 
> // in declare
>             .name = _name,
> // in use
>             MT6370_DT_PROP_DECL(mediatek,bled-pwm-enable, ......)

Yes, I would prefer this form, although, as discussed below, I don't really
like MT6370_DT_PROP_DECL().


> > > +     .type = MT6370_PARSE_TYPE_##_type,                              \
> > > +     .reg = _reg,                                                    \
> > > +     .mask = _mask,                                                  \
> > > +     .max_val = _max,                                                \
> > > +     .invert = _inv,                                                 \
> > > +}
> > > +
> > > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv,
> > > +                                         struct backlight_properties *props)
> > > +{
> > > +     struct device *dev = priv->dev;
> > > +     u8 prop_val;
> > > +     u32 brightness;
> > > +     unsigned int mask, val;
> > > +     static const struct {
> > > +             char *name;
> > > +             enum mt6370_prop_type type;
> > > +             unsigned int reg;
> > > +             unsigned int mask;
> > > +             u8 max_val;
> > > +             bool invert;
> > > +     } vendor_opt_props[] = {
> > > +             MT6370_DT_PROP_DECL(pwm-enable, BOOL, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_EN_MASK, 1, false),
> > > +             MT6370_DT_PROP_DECL(pwm-hys-enable, BOOL, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_HYS_EN_MASK, 1, false),
> > > +             MT6370_DT_PROP_DECL(pwm-hys-sel, U8, MT6370_REG_BL_PWM,
> > > +                                 MT6370_BL_PWM_HYS_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ovp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OVP_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ovp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OVP_EN_MASK, 1, true),
> > > +             MT6370_DT_PROP_DECL(ocp-level-sel, U8, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OC_SEL_MASK, 3, false),
> > > +             MT6370_DT_PROP_DECL(ocp-shutdown, BOOL, MT6370_REG_BL_BSTCTRL,
> > > +                                 MT6370_BL_OC_EN_MASK, 1, true),
> > > +     }, *prop_now;
> > > +     int i, ret;
> > > +
> > > +     /* vendor optional properties */
> > > +     for (i = 0; i < ARRAY_SIZE(vendor_opt_props); i++) {
> > > +             prop_now = vendor_opt_props + i;
> > > +
> > > +             switch (prop_now->type) {
> > > +             case MT6370_PARSE_TYPE_BOOL:
> > > +                     if (device_property_read_bool(dev, prop_now->name))
> > > +                             val = 1;
> > > +                     else
> > > +                             val = 0;
> > > +                     break;
> > > +             case MT6370_PARSE_TYPE_U8:
> > > +                     ret = device_property_read_u8(dev, prop_now->name,
> > > +                                                   &prop_val);
> > > +                     /* Property not exist, keep value in default */
> > > +                     if (ret)
> > > +                             continue;
> > > +
> > > +                     val = min_t(u8, prop_val, prop_now->max_val);
> > > +                     break;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             if (prop_now->invert)
> > > +                     val = prop_now->max_val - val;
> > > +
> > > +             val <<= ffs(prop_now->mask) - 1;
> > > +
> > > +             ret = regmap_update_bits(priv->regmap, prop_now->reg,
> > > +                                      prop_now->mask, val);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > Is it really worth all this tricky code for 7 properties?
> >
> > The code would be much easier to read and maintain if it were coded
> > directly. For example, the inverted boolean code is hard to read and
> > can be written directly as:
> >
> >
> >         val = device_property_read_bool(dev, "mediatek,bled-ovp_shutdown");
> >         ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_BST_CTRL,
> >                                  MT6370_BL_OVP_EN_MASK,
> >                                  MT6370_BL_OVP_EN_MASK * !val);
> >         if (ret)
> >                 return ret;
> >
> > The direct coded approach will probably also pay off if you switch
> > the bindings over to microvolts/microamps since it becomes much more
> > natural to call out to a lookup function to convert it into a register
> > value.
> >
> 
> The purpose of my code is trying to avoid the repeat code in this
> function. And for loop can help to decrease the lines of code
> effectively, that's why I use these code to parse the DT properties.

I'm not really convinced that is uses fewer lines of code. It
certainly would if there were a very large number of properties
but here there is only seven.

However I guess what I'm really complaining about is how hard it is to
read the for loop. We have to study the macros, keep track six different
arguments per property and review the complex logic of the for loop
(which for example handles inverted u8's that don't actually exist).

To be clear, it's not that loops aren't useful for reducing boilerplate
code. They can be. However trying to handle booleans and integers in the
*same* loop ends up needlessly hard to read.

Also, I think that if/when you adopt microamps/microvolts then the
hard-to-read problem will get even worse unless you get loops to do only
one thing!


Daniel.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek