Re: [PATCH v3 02/11] mfd: lm3533: Remove driver specific regmap wrappers

[Andy Shevchenko]

On Mon, Jun 01, 2026 at 06:18:22PM +0300, Svyatoslav Ryhel wrote:
> Remove driver-specific regmap wrappers in favor of using regmap helpers
> directly. The wrappers are mostly equivalent to the standard helpers, with
> two exceptions: regmap_read requires an unsigned int pointer, and
> regmap_update_bits has the mask and value arguments swapped. These
> differences were accounted for and adjusted accordingly.

We refer to functions as func(), exempli gratia, regmap_read().

...

> static int lm3533_als_get_current(struct iio_dev *indio_dev, unsigned channel,
>  								int *val)
>  {
>  	u8 zone;
> -	u8 target;
> +	u32 target;
>  	int ret;

While at it, move towards reversed xmas tree order

	u32 target;
	u8 zone;
	int ret;


...

> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> +	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> +				 val, mask);

It's better to replace this to use _set_bits()/_clear_bits() or even move from
the above conditional (not in this context) to _assign_bits().

...

>  	else
>  		val = 0;	/* analog input */
>  
> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, val, mask);
> +	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> +				 mask, val);

Ditto.

>  	if (ret) {
>  		dev_err(&als->pdev->dev, "failed to set input mode %d\n",
>  								pwm_mode);

...

>  	/* Make sure interrupts are disabled. */
> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, 0, mask);
> +	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_ZONE_INFO,
> +				 mask, 0);

_clear_bits().

>  	if (ret) {
>  		dev_err(&als->pdev->dev, "failed to disable interrupts\n");
>  		return ret;

...

>  	u8 mask = LM3533_ALS_ENABLE_MASK;
>  	int ret;
>  
> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
> +	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> +				 mask, mask);

_set_bits()

>  	if (ret)
>  		dev_err(&als->pdev->dev, "failed to enable ALS\n");
>  

...

>  	u8 mask = LM3533_ALS_ENABLE_MASK;
>  	int ret;
>  
> -	ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
> +	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
> +				 mask, 0);

_clear_bits()

>  	if (ret)
>  		dev_err(&als->pdev->dev, "failed to disable ALS\n");

...

>  	else
>  		val = 0;
>  
> -	ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, mask);
> +	ret = regmap_update_bits(led->lm3533->regmap,
> +				 LM3533_REG_PATTERN_ENABLE, mask, val);

_assign_bits() and so on...

>  	if (ret) {
>  		dev_err(led->cdev.dev, "failed to enable pattern %d (%d)\n",
>  							pattern, enable);

...

>  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);

We don't need to keep 'extern' for ages.

>  extern int lm3533_ctrlbank_set_max_current(struct lm3533_ctrlbank *cb,
>  								u16 imax);
>  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_read(struct lm3533 *lm3533, u8 reg, u8 *val);
> -extern int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val);
> -extern int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask);
> +extern int lm3533_ctrlbank_get_pwm(struct lm3533_ctrlbank *cb, u32 *val);

-- 
With Best Regards,
Andy Shevchenko