[PATCH 0/4] iio: light: Modernize al3010 and al3320a codebase

[PATCH 0/4] iio: light: Modernize al3010 and al3320a codebase

[David Heidelberg via B4 Relay]

This series targetting improved readability of code
and modernizing code to match today standards.

Except slightly improved error reporting,
there shouldn't be any function changes.

Size before:
72224 al3010.ko
72744 al3320a.ko

Size after:
58032 al3010.ko
58632 al3320a.ko

Signed-off-by: David Heidelberg <david@ixit.cz>
---
David Heidelberg (4):
      iio: light: al3320a: Drop deprecated email for Daniel
      iio: light: al3000a: Use DRV_NAME
      iio: light: al3010: Implement regmap support
      iio: light: al3320a: Implement regmap support

 drivers/iio/light/al3000a.c |   6 ++-
 drivers/iio/light/al3010.c  |  95 ++++++++++++++++++++++------------------
 drivers/iio/light/al3320a.c | 103 ++++++++++++++++++++++++--------------------
 3 files changed, 114 insertions(+), 90 deletions(-)
---
base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
change-id: 20250308-al3010-iio-regmap-038cea39f85d

Best regards,
-- 
David Heidelberg <david@ixit.cz>

[PATCH 1/4] iio: light: al3320a: Drop deprecated email for Daniel

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

He no longer works at Intel.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3320a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 497ea3fe337775b07efdfc56c80beb1aa55e394c..d34a91fdafa0affad4665d995e1f66d2aaa0373b 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -266,6 +266,6 @@ static struct i2c_driver al3320a_driver = {
 
 module_i2c_driver(al3320a_driver);
 
-MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_AUTHOR("Daniel Baluta");
 MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
 MODULE_LICENSE("GPL v2");

-- 
2.47.2

Re: [PATCH 1/4] iio: light: al3320a: Drop deprecated email for Daniel

[Jonathan Cameron]

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

> From: David Heidelberg <david@ixit.cz>
> 
> He no longer works at Intel.

True, but we have .mailmap to account for stale email addresses and I'm
not sure it's appropriate to drop this one.

Daniel, maybe you want to add a .mailmap entry?

Thanks,

Jonathan

> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3320a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 497ea3fe337775b07efdfc56c80beb1aa55e394c..d34a91fdafa0affad4665d995e1f66d2aaa0373b 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -266,6 +266,6 @@ static struct i2c_driver al3320a_driver = {
>  
>  module_i2c_driver(al3320a_driver);
>  
> -MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> +MODULE_AUTHOR("Daniel Baluta");
>  MODULE_DESCRIPTION("AL3320A Ambient Light Sensor driver");
>  MODULE_LICENSE("GPL v2");
> 

[PATCH 2/4] iio: light: al3000a: Use DRV_NAME

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

Sync syntax with other similar drivers.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3000a.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
index e2fbb1270040f43d9f0a97838861818a8eaef813..e1fa4543f336cec61140b5c44d3794df1fa485cd 100644
--- a/drivers/iio/light/al3000a.c
+++ b/drivers/iio/light/al3000a.c
@@ -13,6 +13,8 @@
 
 #include <linux/iio/iio.h>
 
+#define AL3000A_DRV_NAME "al3000a"
+
 #define AL3000A_REG_SYSTEM		0x00
 #define AL3000A_REG_DATA		0x05
 
@@ -148,7 +150,7 @@ static int al3000a_probe(struct i2c_client *client)
 				     "failed to get vdd regulator\n");
 
 	indio_dev->info = &al3000a_info;
-	indio_dev->name = "al3000a";
+	indio_dev->name = AL3000A_DRV_NAME;
 	indio_dev->channels = al3000a_channels;
 	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -195,7 +197,7 @@ MODULE_DEVICE_TABLE(of, al3000a_of_match);
 
 static struct i2c_driver al3000a_driver = {
 	.driver = {
-		.name = "al3000a",
+		.name = AL3000A_DRV_NAME,
 		.of_match_table = al3000a_of_match,
 		.pm = pm_sleep_ptr(&al3000a_pm_ops),
 	},

-- 
2.47.2

Re: [PATCH 2/4] iio: light: al3000a: Use DRV_NAME

[Jonathan Cameron]

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

> From: David Heidelberg <david@ixit.cz>
> 
> Sync syntax with other similar drivers.

Sorry no. This is a syntax I am against and frequently request people to not
use in drivers.

The  main reason is there is no explicit reason the two locations you use it
in here should use the same string.  Using a define makes that implication
and I'd prefer that we did not.  Note that if they were required to be the
same then a define would make sense.

I want to look at the code and see the string directly in these locations
rather than go look for a define that adds nothing.

Jonathan




> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3000a.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> index e2fbb1270040f43d9f0a97838861818a8eaef813..e1fa4543f336cec61140b5c44d3794df1fa485cd 100644
> --- a/drivers/iio/light/al3000a.c
> +++ b/drivers/iio/light/al3000a.c
> @@ -13,6 +13,8 @@
>  
>  #include <linux/iio/iio.h>
>  
> +#define AL3000A_DRV_NAME "al3000a"
> +
>  #define AL3000A_REG_SYSTEM		0x00
>  #define AL3000A_REG_DATA		0x05
>  
> @@ -148,7 +150,7 @@ static int al3000a_probe(struct i2c_client *client)
>  				     "failed to get vdd regulator\n");
>  
>  	indio_dev->info = &al3000a_info;
> -	indio_dev->name = "al3000a";
> +	indio_dev->name = AL3000A_DRV_NAME;
>  	indio_dev->channels = al3000a_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(al3000a_channels);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -195,7 +197,7 @@ MODULE_DEVICE_TABLE(of, al3000a_of_match);
>  
>  static struct i2c_driver al3000a_driver = {
>  	.driver = {
> -		.name = "al3000a",
> +		.name = AL3000A_DRV_NAME,
>  		.of_match_table = al3000a_of_match,
>  		.pm = pm_sleep_ptr(&al3000a_pm_ops),
>  	},
> 

[PATCH 3/4] iio: light: al3010: Implement regmap support

[David Heidelberg via B4 Relay]

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

Re: [PATCH 3/4] iio: light: al3010: Implement regmap support

[Jonathan Cameron]

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

Re: [PATCH 3/4] iio: light: al3010: Implement regmap support

[David Heidelberg]

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

Re: [PATCH 3/4] iio: light: al3010: Implement regmap support

[Jonathan Cameron]

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

[PATCH 4/4] iio: light: al3320a: Implement regmap support

[David Heidelberg via B4 Relay]

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/al3320a.c | 101 ++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 45 deletions(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index d34a91fdafa0affad4665d995e1f66d2aaa0373b..208b4f212f3543557e99342630c92f5e6bdaf05d 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -15,6 +15,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>
@@ -59,8 +60,14 @@ static const int al3320a_scales[][2] = {
 	{0, 512000}, {0, 128000}, {0, 32000}, {0, 10000}
 };
 
+static const struct regmap_config al3320a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AL3320A_REG_HIGH_THRESH_HIGH,
+};
+
 struct al3320a_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 };
 
 static const struct iio_chan_spec al3320a_channels[] = {
@@ -82,45 +89,42 @@ static const struct attribute_group al3320a_attribute_group = {
 	.attrs = al3320a_attributes,
 };
 
-static int al3320a_set_pwr(struct i2c_client *client, bool pwr)
+static int al3320a_set_pwr_on(struct al3320a_data *data)
 {
-	u8 val = pwr ? AL3320A_CONFIG_ENABLE : AL3320A_CONFIG_DISABLE;
-	return i2c_smbus_write_byte_data(client, AL3320A_REG_CONFIG, val);
+	return regmap_write(data->regmap, AL3320A_REG_CONFIG, AL3320A_CONFIG_ENABLE);
 }
 
 static void al3320a_set_pwr_off(void *_data)
 {
 	struct al3320a_data *data = _data;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
 
-	al3320a_set_pwr(data->client, false);
+	ret = regmap_write(data->regmap, AL3320A_REG_CONFIG, AL3320A_CONFIG_DISABLE);
+	if (ret)
+		dev_err(dev, "failed to write system register\n");
 }
 
 static int al3320a_init(struct al3320a_data *data)
 {
 	int ret;
 
-	ret = al3320a_set_pwr(data->client, true);
-
-	if (ret < 0)
+	ret = al3320a_set_pwr_on(data);
+	if (ret)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
-					FIELD_PREP(AL3320A_GAIN_MASK,
-						   AL3320A_RANGE_3));
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_MEAN_TIME,
-					AL3320A_DEFAULT_MEAN_TIME);
-	if (ret < 0)
+	ret = regmap_write(data->regmap, AL3320A_REG_CONFIG_RANGE,
+			   FIELD_PREP(AL3320A_GAIN_MASK, AL3320A_RANGE_3));
+	if (ret)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_WAIT,
-					AL3320A_DEFAULT_WAIT_TIME);
-	if (ret < 0)
+	ret = regmap_write(data->regmap, AL3320A_REG_MEAN_TIME,
+			   AL3320A_DEFAULT_MEAN_TIME);
+	if (ret)
 		return ret;
 
-	return 0;
+	return regmap_write(data->regmap, AL3320A_REG_WAIT,
+			    AL3320A_DEFAULT_WAIT_TIME);
 }
 
 static int al3320a_read_raw(struct iio_dev *indio_dev,
@@ -128,7 +132,7 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
 			    int *val2, long mask)
 {
 	struct al3320a_data *data = iio_priv(indio_dev);
-	int ret;
+	int ret, value;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -137,21 +141,21 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
 		 * - low byte of output is stored at AL3320A_REG_DATA_LOW
 		 * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
 		 */
-		ret = i2c_smbus_read_word_data(data->client,
-					       AL3320A_REG_DATA_LOW);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3320A_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,
-					       AL3320A_REG_CONFIG_RANGE);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3320A_REG_CONFIG_RANGE, &value);
+		if (ret)
 			return ret;
 
-		ret = FIELD_GET(AL3320A_GAIN_MASK, ret);
-		*val = al3320a_scales[ret][0];
-		*val2 = al3320a_scales[ret][1];
+		value = FIELD_GET(AL3320A_GAIN_MASK, value);
+		*val = al3320a_scales[value][0];
+		*val2 = al3320a_scales[value][1];
 
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
@@ -163,7 +167,7 @@ static int al3320a_write_raw(struct iio_dev *indio_dev,
 			     int val2, long mask)
 {
 	struct al3320a_data *data = iio_priv(indio_dev);
-	int i;
+	unsigned int i;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -172,9 +176,8 @@ static int al3320a_write_raw(struct iio_dev *indio_dev,
 			    val2 != al3320a_scales[i][1])
 				continue;
 
-			return i2c_smbus_write_byte_data(data->client,
-					AL3320A_REG_CONFIG_RANGE,
-					FIELD_PREP(AL3320A_GAIN_MASK, i));
+			return regmap_write(data->regmap, AL3320A_REG_CONFIG_RANGE,
+					    FIELD_PREP(AL3320A_GAIN_MASK, i));
 		}
 		break;
 	}
@@ -190,16 +193,21 @@ static const struct iio_info al3320a_info = {
 static int al3320a_probe(struct i2c_client *client)
 {
 	struct al3320a_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, &al3320a_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "cannot allocate regmap\n");
 
 	indio_dev->info = &al3320a_info;
 	indio_dev->name = AL3320A_DRV_NAME;
@@ -209,27 +217,30 @@ static int al3320a_probe(struct i2c_client *client)
 
 	ret = al3320a_init(data);
 	if (ret < 0) {
-		dev_err(&client->dev, "al3320a chip init failed\n");
+		dev_err(dev, "failed to init ALS\n");
 		return ret;
 	}
 
-	ret = devm_add_action_or_reset(&client->dev,
-					al3320a_set_pwr_off,
-					data);
+	ret = devm_add_action_or_reset(dev, al3320a_set_pwr_off, data);
 	if (ret < 0)
 		return ret;
 
-	return devm_iio_device_register(&client->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int al3320a_suspend(struct device *dev)
 {
-	return al3320a_set_pwr(to_i2c_client(dev), false);
+	struct al3320a_data *data = iio_priv(dev_get_drvdata(dev));
+
+	al3320a_set_pwr_off(data);
+	return 0;
 }
 
 static int al3320a_resume(struct device *dev)
 {
-	return al3320a_set_pwr(to_i2c_client(dev), true);
+	struct al3320a_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3320a_set_pwr_on(data);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(al3320a_pm_ops, al3320a_suspend,

-- 
2.47.2

Re: [PATCH 4/4] iio: light: al3320a: Implement regmap support

[Jonathan Cameron]

On Sat, 08 Mar 2025 21:01:01 +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.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
This one is more less just the regmap change, so main question is why does that
bring benefits?  Code looks fine to me though one or two comments from review
of previous patch may apply here as well,

Thanks,

Jonathan