Re: [PATCH v3 05/11] mfd: lm3533: Convert to use OF bindings

[Jonathan Cameron <jic23@kernel.org>]

On Mon,  1 Jun 2026 18:18:25 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> Since there are no users of this driver via platform data, remove the
> platform data support and switch to using Device Tree bindings.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>


> index 52136ca1abc9..55b35467a722 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -16,16 +16,19 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/units.h>
>  
>  #include <linux/mfd/lm3533.h>
>  
>  
> -#define LM3533_ALS_RESISTOR_MIN			1
> -#define LM3533_ALS_RESISTOR_MAX			127
> +#define LM3533_ALS_RESISTOR_MIN			1575
> +#define LM3533_ALS_RESISTOR_MAX			200000
>  #define LM3533_ALS_CHANNEL_CURRENT_MAX		2
>  #define LM3533_ALS_THRESH_MAX			3
>  #define LM3533_ALS_ZONE_MAX			4
> @@ -57,6 +60,9 @@ struct lm3533_als {
>  
>  	atomic_t zone;
>  	struct mutex thresh_mutex;
> +
> +	bool pwm_mode;
> +	u32 r_select;
>  };
>  
>  
> @@ -411,7 +417,7 @@ static ssize_t show_thresh_either_en(struct device *dev,
>  	int enable;
>  	int ret;
>  
> -	if (als->irq) {
> +	if (als->irq > 0) {
>  		ret = lm3533_als_get_int_mode(indio_dev, &enable);
>  		if (ret)
>  			return ret;
> @@ -716,30 +722,34 @@ static const struct attribute_group lm3533_als_attribute_group = {
>  	.attrs = lm3533_als_attributes
>  };
>  
> -static int lm3533_als_setup(struct lm3533_als *als,
> -			    const struct lm3533_als_platform_data *pdata)
> +static int lm3533_als_setup(struct lm3533_als *als)
>  {
>  	struct device *dev = &als->pdev->dev;
>  	int ret;
>  
> +	als->pwm_mode = device_property_read_bool(dev, "ti,pwm-mode");
> +
>  	ret = regmap_update_bits(als->lm3533->regmap, LM3533_REG_ALS_CONF,
>  				 LM3533_ALS_INPUT_MODE_MASK,
> -				 pdata->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
> +				 als->pwm_mode ? LM3533_ALS_INPUT_MODE_MASK : 0);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to set input mode %d\n",
> -				     pdata->pwm_mode);
> -
> +				     als->pwm_mode);
>  
>  	/* ALS input is always high impedance in PWM-mode. */
> -	if (!pdata->pwm_mode) {
> -		if (pdata->r_select < LM3533_ALS_RESISTOR_MIN ||
> -		    pdata->r_select > LM3533_ALS_RESISTOR_MAX) {
> -			dev_err(&als->pdev->dev, "invalid resistor value\n");
> -			return -EINVAL;
> -		}
> +	if (!als->pwm_mode) {
> +		ret = device_property_read_u32(dev, "ti,resistor-value-ohms",
> +					       &als->r_select);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to ger resistor value\n");
> +
> +		als->r_select = clamp(als->r_select, LM3533_ALS_RESISTOR_MIN,
> +				      LM3533_ALS_RESISTOR_MAX);

If we are getting garbage from DT I think I'd rather error out that paper over
that problem.  So similar to before, check valid value and if not fail probe
so that hopefully someone goes and fixes it!

> +		als->r_select = DIV_ROUND_UP(2 * MICRO, 10 * als->r_select);
Why do we need this when we didn't before?  The range checks are the same
so it smells like it shouldn't need transforming. I'd also rather we didn't do
rewriting of the meaning of r_select like this.  Just use a local variable for
the intermediate result.

>  
>  		ret = regmap_write(als->lm3533->regmap, LM3533_REG_ALS_RESISTOR_SELECT,
> -				   pdata->r_select);
> +				   als->r_select);
>  		if (ret)
>  			return dev_err_probe(dev, ret, "failed to set resistor\n");