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