Message-ID: <20250309164549.26d4cdfc@jic23-huawei>
Date: Sun, 9 Mar 2025 16:45:49 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Heidelberg via B4 Relay <devnull+david.ixit.cz@...nel.org>
Cc: david@...t.cz, Lars-Peter Clausen <lars@...afoo.de>, Svyatoslav Ryhel
 <clamor95@...il.com>, Robert Eckelmann <longnoserob@...il.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: light: al3010: Implement regmap support

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

> From: David Heidelberg <david@...t.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@...t.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);
>  }

Powered by blists - more mailing lists