Re: [PATCH 3/4] iio: light: al3010: Implement regmap support
From: David Heidelberg
Date: Sun Mar 09 2025 - 17:12:18 EST
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@xxxxxxxxxx> wrote:
From: David Heidelberg <david@xxxxxxx>
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@xxxxxxx>
---
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