сб, 6 черв. 2026 р. о 09:53 Andy Shevchenko <[email protected]> пише:
>
> On Sat, Jun 06, 2026 at 07:57:26AM +0300, Svyatoslav Ryhel wrote:
> > Remove driver-specific regmap wrappers in favor of using regmap helpers
> > directly.
>
> I like the idea of this patch. Nevertheless I have some suggestions below.
>
> ...
>
> > {
> > struct lm3533_als *als = iio_priv(indio_dev);
> > u8 reg;
> > - u8 val;
> > + u32 val;
>
> Strictly speaking this should be unsigned int. The regmap API use unsigned
> int.
>
Yes, though regmap defines only 8 bit to be used so it should not matter
> ...
>
> > static int lm3533_als_set_int_mode(struct iio_dev *indio_dev, int enable)
> > {
> > struct lm3533_als *als = iio_priv(indio_dev);
> > - u8 mask = LM3533_ALS_INT_ENABLE_MASK;
> > - u8 val;
> > int ret;
> >
> > - if (enable)
> > - val = mask;
> > - else
> > - val = 0;
> > -
> > - ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> > + ret = regmap_assign_bits(als->lm3533->regmap,
> > LM3533_REG_ALS_ZONE_INFO,
> > + LM3533_ALS_INT_ENABLE_MASK, enable);
>
> In cases like this perhaps leaving mask would be fine and together with
I prefer to remove intermediate variables it the helper allows to
directly pass needed value.
>
> struct regmap *map = als->lm3533->regmap;
>
next patch drops lm3533 so there will be als->regmap which IMHO is
more logical instead of passing entire lm3533 to child devices.
> this be nice one-liner:
>
> ret = regmap_assign_bits(map, LM3533_REG_ALS_ZONE_INFO, mask, enable);
>
> > if (ret) {
> > dev_err(&indio_dev->dev, "failed to set int mode %d\n",
> > enable);
>
> In many cases it won't increase LoC count.
>
> ...
>
> > extern int lm3533_ctrlbank_set_brightness(struct lm3533_ctrlbank *cb, u8
> > val);
> > -extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u8
> > *val);
> > +extern int lm3533_ctrlbank_get_brightness(struct lm3533_ctrlbank *cb, u32
> > *val);
>
> > extern int lm3533_ctrlbank_set_pwm(struct lm3533_ctrlbank *cb, u8 val);
> > -extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u8 *val);
> > +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);
>
> Now they become asymmetrical. Perhaps to replace setters, but be careful about
> upper bits.
>
Yes, I have same thoughts. Upper beats should be irrelevant since
regmap should use only fist 8 bits but I will be careful, thanks.
> --
> With Best Regards,
> Andy Shevchenko
>
>
