From: David Heidelberg <david@ixit.cz>

Modernize and make driver a bit cleaner.

Incorporate most of the feedback given on new AL3000A.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3010.c | 95 ++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
index 7cbb8b203300907a88f4a0ab87da89cabdd087f3..f6ed7246864a777fdb7d3861b74f5834e8af4105 100644
--- a/drivers/iio/light/al3010.c
+++ b/drivers/iio/light/al3010.c
@@ -4,7 +4,7 @@
  *
  * Copyright (c) 2014, Intel Corporation.
  * Copyright (c) 2016, Dyna-Image Corp.
- * Copyright (c) 2020, David Heidelberg, Michał Mirosław, Dmitry Osipenko
+ * Copyright (c) 2020 - 2025, David Heidelberg, Michał Mirosław, Dmitry Osipenko
  *
  * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
  *
@@ -17,6 +17,7 @@
 #include <linux/bitfield.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/mod_devicetable.h>
 
 #include <linux/iio/iio.h>
@@ -46,8 +47,14 @@ static const int al3010_scales[][2] = {
 	{0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
 };
 
+static const struct regmap_config al3010_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AL3010_REG_CONFIG,
+};
+
 struct al3010_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 };
 
 static const struct iio_chan_spec al3010_channels[] = {
@@ -69,40 +76,32 @@ static const struct attribute_group al3010_attribute_group = {
 	.attrs = al3010_attributes,
 };
 
-static int al3010_set_pwr(struct i2c_client *client, bool pwr)
+static int al3010_set_pwr_on(struct al3010_data *data)
 {
-	u8 val = pwr ? AL3010_CONFIG_ENABLE : AL3010_CONFIG_DISABLE;
-	return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, val);
+	return regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_ENABLE);
 }
 
 static void al3010_set_pwr_off(void *_data)
 {
 	struct al3010_data *data = _data;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
 
-	al3010_set_pwr(data->client, false);
+	ret = regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_DISABLE);
+	if (ret)
+		dev_err(dev, "failed to write system register\n");
 }
 
 static int al3010_init(struct al3010_data *data)
 {
 	int ret;
 
-	ret = al3010_set_pwr(data->client, true);
-	if (ret < 0)
-		return ret;
-
-	ret = devm_add_action_or_reset(&data->client->dev,
-				       al3010_set_pwr_off,
-				       data);
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
-					FIELD_PREP(AL3010_GAIN_MASK,
-						   AL3XXX_RANGE_3));
-	if (ret < 0)
+	ret = al3010_set_pwr_on(data);
+	if (ret)
 		return ret;
 
-	return 0;
+	return regmap_write(data->regmap, AL3010_REG_CONFIG,
+			    FIELD_PREP(AL3010_GAIN_MASK, AL3XXX_RANGE_3));
 }
 
 static int al3010_read_raw(struct iio_dev *indio_dev,
@@ -110,7 +109,7 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
 			   int *val2, long mask)
 {
 	struct al3010_data *data = iio_priv(indio_dev);
-	int ret;
+	int ret, value;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -119,21 +118,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
 		 * - low byte of output is stored at AL3010_REG_DATA_LOW
 		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
 		 */
-		ret = i2c_smbus_read_word_data(data->client,
-					       AL3010_REG_DATA_LOW);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &value);
+		if (ret)
 			return ret;
-		*val = ret;
+
+		*val = value;
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = i2c_smbus_read_byte_data(data->client,
-					       AL3010_REG_CONFIG);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &value);
+		if (ret)
 			return ret;
 
-		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
-		*val = al3010_scales[ret][0];
-		*val2 = al3010_scales[ret][1];
+		value = FIELD_GET(AL3010_GAIN_MASK, value);
+		*val = al3010_scales[value][0];
+		*val2 = al3010_scales[value][1];
 
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
@@ -145,7 +144,7 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
 			    int val2, long mask)
 {
 	struct al3010_data *data = iio_priv(indio_dev);
-	int i;
+	unsigned int i;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -154,9 +153,8 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
 			    val2 != al3010_scales[i][1])
 				continue;
 
-			return i2c_smbus_write_byte_data(data->client,
-					AL3010_REG_CONFIG,
-					FIELD_PREP(AL3010_GAIN_MASK, i));
+			return regmap_write(data->regmap, AL3010_REG_CONFIG,
+					    FIELD_PREP(AL3010_GAIN_MASK, i));
 		}
 		break;
 	}
@@ -172,16 +170,20 @@ static const struct iio_info al3010_info = {
 static int al3010_probe(struct i2c_client *client)
 {
 	struct al3010_data *data;
+	struct device *dev = &client->dev;
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, &al3010_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "cannot allocate regmap\n");
 
 	indio_dev->info = &al3010_info;
 	indio_dev->name = AL3010_DRV_NAME;
@@ -191,21 +193,30 @@ static int al3010_probe(struct i2c_client *client)
 
 	ret = al3010_init(data);
 	if (ret < 0) {
-		dev_err(&client->dev, "al3010 chip init failed\n");
+		dev_err(dev, "failed to init ALS\n");
 		return ret;
 	}
 
-	return devm_iio_device_register(&client->dev, indio_dev);
+	ret = devm_add_action_or_reset(dev, al3010_set_pwr_off, data);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int al3010_suspend(struct device *dev)
 {
-	return al3010_set_pwr(to_i2c_client(dev), false);
+	struct al3010_data *data = iio_priv(dev_get_drvdata(dev));
+
+	al3010_set_pwr_off(data);
+	return 0;
 }
 
 static int al3010_resume(struct device *dev)
 {
-	return al3010_set_pwr(to_i2c_client(dev), true);
+	struct al3010_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3010_set_pwr_on(data);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(al3010_pm_ops, al3010_suspend, al3010_resume);

-- 
2.47.2

On Sat, 08 Mar 2025 21:01:00 +0100
David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:

> From: David Heidelberg <david@ixit.cz>
> 
> Modernize and make driver a bit cleaner.
> 
> Incorporate most of the feedback given on new AL3000A.
Hi David,

Why does regmap bring benefits here?  This seems to be a like for like
change (no use of additional helpers / caching etc) so I'm not immediately
seeing the advantage.

Various comments inline. Main one is this is doing several not particularly
closely related changes that belong in separate patches.

Jonathan

> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3010.c | 95 ++++++++++++++++++++++++++--------------------
>  1 file changed, 53 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> index 7cbb8b203300907a88f4a0ab87da89cabdd087f3..f6ed7246864a777fdb7d3861b74f5834e8af4105 100644
> --- a/drivers/iio/light/al3010.c
> +++ b/drivers/iio/light/al3010.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright (c) 2014, Intel Corporation.
>   * Copyright (c) 2016, Dyna-Image Corp.
> - * Copyright (c) 2020, David Heidelberg, Michał Mirosław, Dmitry Osipenko
> + * Copyright (c) 2020 - 2025, David Heidelberg, Michał Mirosław, Dmitry Osipenko

This implies all 3 of you were involved in this update. If that's not
the case perhaps just add a new copyright line for this change.

>   *
>   * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
>   *

>  
>  static const struct iio_chan_spec al3010_channels[] = {
> @@ -69,40 +76,32 @@ static const struct attribute_group al3010_attribute_group = {
>  	.attrs = al3010_attributes,
>  };
>  
> -static int al3010_set_pwr(struct i2c_client *client, bool pwr)
> +static int al3010_set_pwr_on(struct al3010_data *data)
>  {
> -	u8 val = pwr ? AL3010_CONFIG_ENABLE : AL3010_CONFIG_DISABLE;
> -	return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, val);
> +	return regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_ENABLE);
Splitting this write into the on and off cases is a change that is
not closely related to regmap change, so probably wants to be in a separate
patch.

>  }
>  
>  static void al3010_set_pwr_off(void *_data)
>  {
>  	struct al3010_data *data = _data;
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
>  
> -	al3010_set_pwr(data->client, false);
> +	ret = regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_DISABLE);
> +	if (ret)
> +		dev_err(dev, "failed to write system register\n");
>  }
>  
>  static int al3010_init(struct al3010_data *data)
>  {
>  	int ret;
>  
> -	ret = al3010_set_pwr(data->client, true);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = devm_add_action_or_reset(&data->client->dev,
> -				       al3010_set_pwr_off,
> -				       data);

As below. Not obvious to me why we'd want to move this.

> -	if (ret < 0)
> -		return ret;
> -
> -	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
> -					FIELD_PREP(AL3010_GAIN_MASK,
> -						   AL3XXX_RANGE_3));
> -	if (ret < 0)
> +	ret = al3010_set_pwr_on(data);
> +	if (ret)
>  		return ret;
>  
> -	return 0;
> +	return regmap_write(data->regmap, AL3010_REG_CONFIG,
> +			    FIELD_PREP(AL3010_GAIN_MASK, AL3XXX_RANGE_3));
>  }
>  
>  static int al3010_read_raw(struct iio_dev *indio_dev,
> @@ -110,7 +109,7 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
>  			   int *val2, long mask)
>  {
>  	struct al3010_data *data = iio_priv(indio_dev);
> -	int ret;
> +	int ret, value;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -119,21 +118,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
>  		 * - low byte of output is stored at AL3010_REG_DATA_LOW
>  		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
>  		 */
> -		ret = i2c_smbus_read_word_data(data->client,
> -					       AL3010_REG_DATA_LOW);
> -		if (ret < 0)
> +		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &value);
> +		if (ret)
>  			return ret;
> -		*val = ret;
> +
> +		*val = value;
> +
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = i2c_smbus_read_byte_data(data->client,
> -					       AL3010_REG_CONFIG);
> -		if (ret < 0)
> +		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &value);
> +		if (ret)
>  			return ret;
>  
> -		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
> -		*val = al3010_scales[ret][0];
> -		*val2 = al3010_scales[ret][1];
> +		value = FIELD_GET(AL3010_GAIN_MASK, value);
I'm never a big fan of conflating use of one variable for the register value
(where value is a reasonable name) and the field extract from it where
it's not really. scale_idx or something like that would make more sense for
this second case.

> +		*val = al3010_scales[value][0];
> +		*val2 = al3010_scales[value][1];
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	}
> @@ -145,7 +144,7 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
>  			    int val2, long mask)
>  {
>  	struct al3010_data *data = iio_priv(indio_dev);
> -	int i;
> +	unsigned int i;

Looks like an unrelated change.  Possibly even one that isn't worth making.

>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -154,9 +153,8 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
>  			    val2 != al3010_scales[i][1])
>  				continue;
>  
> -			return i2c_smbus_write_byte_data(data->client,
> -					AL3010_REG_CONFIG,
> -					FIELD_PREP(AL3010_GAIN_MASK, i));
> +			return regmap_write(data->regmap, AL3010_REG_CONFIG,
> +					    FIELD_PREP(AL3010_GAIN_MASK, i));
>  		}
>  		break;
>  	}
> @@ -172,16 +170,20 @@ static const struct iio_info al3010_info = {
>  static int al3010_probe(struct i2c_client *client)
>  {
>  	struct al3010_data *data;
> +	struct device *dev = &client->dev;

This is confusing two things.  I'd prefer a precursor patch that
adds the local variable followed by one that adds the regmap stuff.

>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
> -	data->client = client;
> +	data->regmap = devm_regmap_init_i2c(client, &al3010_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "cannot allocate regmap\n");
>  
>  	indio_dev->info = &al3010_info;
>  	indio_dev->name = AL3010_DRV_NAME;
> @@ -191,21 +193,30 @@ static int al3010_probe(struct i2c_client *client)
>  
>  	ret = al3010_init(data);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "al3010 chip init failed\n");
> +		dev_err(dev, "failed to init ALS\n");
>  		return ret;
>  	}
>  
> -	return devm_iio_device_register(&client->dev, indio_dev);
> +	ret = devm_add_action_or_reset(dev, al3010_set_pwr_off, data);

Moving this out here doesn't look like a change related to regmap.
Generally I'd prefer that stayed next to where the power on is as this
is not obviously undoing the al3010_init() given naming etc.

> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
>  }

On 09/03/2025 17:45, Jonathan Cameron wrote:
> On Sat, 08 Mar 2025 21:01:00 +0100
> David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:
> 
>> From: David Heidelberg <david@ixit.cz>
>>
>> Modernize and make driver a bit cleaner.
>>
>> Incorporate most of the feedback given on new AL3000A.
> Hi David,
> 
> Why does regmap bring benefits here?  This seems to be a like for like
> change (no use of additional helpers / caching etc) so I'm not immediately
> seeing the advantage.

As I mentioned in the summary, the change is smaller binary size and 
being in sync with al3000a. Since al3xxx series drivers are pretty close 
to each other, I believe for future maintenance having them in pair, 
where it's possible is beneficial.
> 
> Various comments inline. Main one is this is doing several not particularly
> closely related changes that belong in separate patches.

I'm aware I should likely address the changes in smaller chunks, but as 
I get this patch tested, it's fairly small patch, so I would believe 
it's still bearable size of the change? If not, I'll split changes into 
separate patches.
> 
> Jonathan
> 
>>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
>>   drivers/iio/light/al3010.c | 95 ++++++++++++++++++++++++++--------------------
>>   1 file changed, 53 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
>> index 7cbb8b203300907a88f4a0ab87da89cabdd087f3..f6ed7246864a777fdb7d3861b74f5834e8af4105 100644
>> --- a/drivers/iio/light/al3010.c
>> +++ b/drivers/iio/light/al3010.c
>> @@ -4,7 +4,7 @@
>>    *
>>    * Copyright (c) 2014, Intel Corporation.
>>    * Copyright (c) 2016, Dyna-Image Corp.
>> - * Copyright (c) 2020, David Heidelberg, Michał Mirosław, Dmitry Osipenko
>> + * Copyright (c) 2020 - 2025, David Heidelberg, Michał Mirosław, Dmitry Osipenko
> 
> This implies all 3 of you were involved in this update. If that's not
> the case perhaps just add a new copyright line for this change.
> 
>>    *
>>    * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
>>    *
> 
>>   
>>   static const struct iio_chan_spec al3010_channels[] = {
>> @@ -69,40 +76,32 @@ static const struct attribute_group al3010_attribute_group = {
>>   	.attrs = al3010_attributes,
>>   };
>>   
>> -static int al3010_set_pwr(struct i2c_client *client, bool pwr)
>> +static int al3010_set_pwr_on(struct al3010_data *data)
>>   {
>> -	u8 val = pwr ? AL3010_CONFIG_ENABLE : AL3010_CONFIG_DISABLE;
>> -	return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, val);
>> +	return regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_ENABLE);
> Splitting this write into the on and off cases is a change that is
> not closely related to regmap change, so probably wants to be in a separate
> patch.

Sure, I can do.

> 
>>   }
>>   
>>   static void al3010_set_pwr_off(void *_data)
>>   {
>>   	struct al3010_data *data = _data;
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>>   
>> -	al3010_set_pwr(data->client, false);
>> +	ret = regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_DISABLE);
>> +	if (ret)
>> +		dev_err(dev, "failed to write system register\n");
>>   }
>>   
>>   static int al3010_init(struct al3010_data *data)
>>   {
>>   	int ret;
>>   
>> -	ret = al3010_set_pwr(data->client, true);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	ret = devm_add_action_or_reset(&data->client->dev,
>> -				       al3010_set_pwr_off,
>> -				       data);
> 
> As below. Not obvious to me why we'd want to move this.
> 
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
>> -					FIELD_PREP(AL3010_GAIN_MASK,
>> -						   AL3XXX_RANGE_3));
>> -	if (ret < 0)
>> +	ret = al3010_set_pwr_on(data);
>> +	if (ret)
>>   		return ret;
>>   
>> -	return 0;
>> +	return regmap_write(data->regmap, AL3010_REG_CONFIG,
>> +			    FIELD_PREP(AL3010_GAIN_MASK, AL3XXX_RANGE_3));
>>   }
>>   
>>   static int al3010_read_raw(struct iio_dev *indio_dev,
>> @@ -110,7 +109,7 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
>>   			   int *val2, long mask)
>>   {
>>   	struct al3010_data *data = iio_priv(indio_dev);
>> -	int ret;
>> +	int ret, value;
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_RAW:
>> @@ -119,21 +118,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
>>   		 * - low byte of output is stored at AL3010_REG_DATA_LOW
>>   		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
>>   		 */
>> -		ret = i2c_smbus_read_word_data(data->client,
>> -					       AL3010_REG_DATA_LOW);
>> -		if (ret < 0)
>> +		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &value);
>> +		if (ret)
>>   			return ret;
>> -		*val = ret;
>> +
>> +		*val = value;
>> +
>>   		return IIO_VAL_INT;
>>   	case IIO_CHAN_INFO_SCALE:
>> -		ret = i2c_smbus_read_byte_data(data->client,
>> -					       AL3010_REG_CONFIG);
>> -		if (ret < 0)
>> +		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &value);
>> +		if (ret)
>>   			return ret;
>>   
>> -		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
>> -		*val = al3010_scales[ret][0];
>> -		*val2 = al3010_scales[ret][1];
>> +		value = FIELD_GET(AL3010_GAIN_MASK, value);
> I'm never a big fan of conflating use of one variable for the register value
> (where value is a reasonable name) and the field extract from it where
> it's not really. scale_idx or something like that would make more sense for
> this second case.

I originally had name gain for this one, but decided to go with generic 
value to cover all cases. If you want, I can go back to custom name per 
case.
> 
>> +		*val = al3010_scales[value][0];
>> +		*val2 = al3010_scales[value][1];
>>   
>>   		return IIO_VAL_INT_PLUS_MICRO;
>>   	}
>> @@ -145,7 +144,7 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
>>   			    int val2, long mask)
>>   {
>>   	struct al3010_data *data = iio_priv(indio_dev);
>> -	int i;
>> +	unsigned int i;
> 
> Looks like an unrelated change.  Possibly even one that isn't worth making.

Well, I was at editing and as i is used within array id, it cannot be 
signed integer, second it's also compared against u8, so IMHO it make 
sense here.

> 
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_SCALE:
>> @@ -154,9 +153,8 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
>>   			    val2 != al3010_scales[i][1])
>>   				continue;
>>   
>> -			return i2c_smbus_write_byte_data(data->client,
>> -					AL3010_REG_CONFIG,
>> -					FIELD_PREP(AL3010_GAIN_MASK, i));
>> +			return regmap_write(data->regmap, AL3010_REG_CONFIG,
>> +					    FIELD_PREP(AL3010_GAIN_MASK, i));
>>   		}
>>   		break;
>>   	}
>> @@ -172,16 +170,20 @@ static const struct iio_info al3010_info = {
>>   static int al3010_probe(struct i2c_client *client)
>>   {
>>   	struct al3010_data *data;
>> +	struct device *dev = &client->dev;
> 
> This is confusing two things.  I'd prefer a precursor patch that
> adds the local variable followed by one that adds the regmap stuff.

Sure, I can split it into additional patch.

> 
>>   	struct iio_dev *indio_dev;
>>   	int ret;
>>   
>> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>>   	if (!indio_dev)
>>   		return -ENOMEM;
>>   
>>   	data = iio_priv(indio_dev);
>>   	i2c_set_clientdata(client, indio_dev);
>> -	data->client = client;
>> +	data->regmap = devm_regmap_init_i2c(client, &al3010_regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
>> +				     "cannot allocate regmap\n");
>>   
>>   	indio_dev->info = &al3010_info;
>>   	indio_dev->name = AL3010_DRV_NAME;
>> @@ -191,21 +193,30 @@ static int al3010_probe(struct i2c_client *client)
>>   
>>   	ret = al3010_init(data);
>>   	if (ret < 0) {
>> -		dev_err(&client->dev, "al3010 chip init failed\n");
>> +		dev_err(dev, "failed to init ALS\n");
>>   		return ret;
>>   	}
>>   
>> -	return devm_iio_device_register(&client->dev, indio_dev);
>> +	ret = devm_add_action_or_reset(dev, al3010_set_pwr_off, data);
> 
> Moving this out here doesn't look like a change related to regmap.
> Generally I'd prefer that stayed next to where the power on is as this
> is not obviously undoing the al3010_init() given naming etc.

Sure, I'll move it back.

Thank you for the feedback,
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>>   }

-- 
David Heidelberg

On Sun, 9 Mar 2025 22:11:57 +0100
David Heidelberg <david@ixit.cz> wrote:

> On 09/03/2025 17:45, Jonathan Cameron wrote:
> > On Sat, 08 Mar 2025 21:01:00 +0100
> > David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:
> >   
> >> From: David Heidelberg <david@ixit.cz>
> >>
> >> Modernize and make driver a bit cleaner.
> >>
> >> Incorporate most of the feedback given on new AL3000A.  
> > Hi David,
> > 
> > Why does regmap bring benefits here?  This seems to be a like for like
> > change (no use of additional helpers / caching etc) so I'm not immediately
> > seeing the advantage.  
> 
> As I mentioned in the summary, the change is smaller binary size and 
> being in sync with al3000a. Since al3xxx series drivers are pretty close 
> to each other, I believe for future maintenance having them in pair, 
> where it's possible is beneficial.

Description should be here (briefly) as this is what ends up in
the git change log.

> > 
> > Various comments inline. Main one is this is doing several not particularly
> > closely related changes that belong in separate patches.  
> 
> I'm aware I should likely address the changes in smaller chunks, but as 
> I get this patch tested, it's fairly small patch, so I would believe 
> it's still bearable size of the change? If not, I'll split changes into 
> separate patches.

Separate patches still preferred even when the overall changeset
is not large.  It allows for crisper description and easy review.

...

> >> @@ -119,21 +118,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
> >>   		 * - low byte of output is stored at AL3010_REG_DATA_LOW
> >>   		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
> >>   		 */
> >> -		ret = i2c_smbus_read_word_data(data->client,
> >> -					       AL3010_REG_DATA_LOW);
> >> -		if (ret < 0)
> >> +		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &value);
> >> +		if (ret)
> >>   			return ret;
> >> -		*val = ret;
> >> +
> >> +		*val = value;
> >> +
> >>   		return IIO_VAL_INT;
> >>   	case IIO_CHAN_INFO_SCALE:
> >> -		ret = i2c_smbus_read_byte_data(data->client,
> >> -					       AL3010_REG_CONFIG);
> >> -		if (ret < 0)
> >> +		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &value);
> >> +		if (ret)
> >>   			return ret;
> >>   
> >> -		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
> >> -		*val = al3010_scales[ret][0];
> >> -		*val2 = al3010_scales[ret][1];
> >> +		value = FIELD_GET(AL3010_GAIN_MASK, value);  
> > I'm never a big fan of conflating use of one variable for the register value
> > (where value is a reasonable name) and the field extract from it where
> > it's not really. scale_idx or something like that would make more sense for
> > this second case.  
> 
> I originally had name gain for this one, but decided to go with generic 
> value to cover all cases. If you want, I can go back to custom name per 
> case.

Please do.

> >   
> >> +		*val = al3010_scales[value][0];
> >> +		*val2 = al3010_scales[value][1];
> >>   
> >>   		return IIO_VAL_INT_PLUS_MICRO;
> >>   	}
> >> @@ -145,7 +144,7 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
> >>   			    int val2, long mask)
> >>   {
> >>   	struct al3010_data *data = iio_priv(indio_dev);
> >> -	int i;
> >> +	unsigned int i;  
> > 
> > Looks like an unrelated change.  Possibly even one that isn't worth making.  
> 
> Well, I was at editing and as i is used within array id, it cannot be 
> signed integer, second it's also compared against u8, so IMHO it make 
> sense here.

I wouldn't bother, but if you do want to, it's an unrelated change
so separate patch.

Jonathan