Re: [PATCH v2 1/3] iio: light: ltr390: Add configurable gain and resolution

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 28 Jul 2024 20:49:54 +0530
Abhash Jha <abhashkumarjha123@gmail.com> wrote:

>     Add support for configuring and reading the gain and resolution
>     (integration time). Also provide the available values for gain and
>     resoltion respectively via `read_avail` callback.
Hi Abhash,

Don't indent patch description like this.

Also from a process point of view, for IIO (and I think most of the kernel)
don't reply to a previous version thread with a new version.
The upshot is that it ends up far from the most recent emails in everyone's
inboxes as pretty much everyone uses threading.

Also, if sending multiple patches, please add a cover letter.
--cover-letter in git.

That provides a general place for comments like this one and also
gives the series a nice pretty title in patch work and similar tooling ;)
https://patchwork.kernel.org/project/linux-iio/list/?
See the series title column - that's from cover letters.

Anyhow, on to the code.  Various minor comments inline.
In particularly for v3, please look for accidental or unnecessary changes
of code formatting etc.

They add considerable noise to a fairly simple real change.

Jonathan


> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@gmail.com>
> ---
>  drivers/iio/light/ltr390.c | 144 +++++++++++++++++++++++++++++++++----
>  1 file changed, 130 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index fff1e8990..9f00661c3 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -25,19 +25,26 @@
>  #include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
> -
> +#include <linux/iio/sysfs.h>
Keep a blank line after the iio includes.
However, why this include?  You aren't defining custom attributes in this
patch.


>  #include <asm/unaligned.h>
>  
> -#define LTR390_MAIN_CTRL      0x00
> -#define LTR390_PART_ID	      0x06
> -#define LTR390_UVS_DATA	      0x10
Hmm. Annoying to have to realign this.  However, it probably isn't
worth a precursor patch for this one.
If it were many more than 3 lines it would be.

> +#define LTR390_MAIN_CTRL		0x00
> +#define LTR390_ALS_UVS_MEAS_RATE	0x04
> +#define LTR390_ALS_UVS_GAIN		0x05
> +#define LTR390_PART_ID			0x06
> +#define LTR390_ALS_DATA			0x0D
> +#define LTR390_UVS_DATA			0x10
> +#define LTR390_INT_CFG			0x19

> +
> +#define LTR390_PART_NUMBER_ID		0xb
> +#define LTR390_ALS_UVS_GAIN_MASK	0x07
> +#define LTR390_ALS_UVS_INT_TIME_MASK	0x70
> +#define LTR390_ALS_UVS_INT_TIME(x)	FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, x)
Brackets around x
define LTR390_ALS_UVS_INT_TIME(x)	FIELD_PREP(LTR390_ALS_UVS_INT_TIME_MASK, (x))

>  
>  #define LTR390_SW_RESET	      BIT(4)
>  #define LTR390_UVS_MODE	      BIT(3)
>  #define LTR390_SENSOR_ENABLE  BIT(1)
>  
> -#define LTR390_PART_NUMBER_ID 0xb

..

> @@ -91,32 +98,135 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
>  {
> -	int ret;
>  	struct ltr390_data *data = iio_priv(iio_device);
> +	int ret;
>  
> +	guard(mutex)(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = ltr390_register_read(data, LTR390_UVS_DATA);
>  		if (ret < 0)
>  			return ret;
> +
Not related to the patch content, so shouldn't be here.
>  		*val = ret;
>  		return IIO_VAL_INT;
> +
Similarly.  It's a reasonable change, but not in this patch as it
adds noise.  Feel free to send another patch in the series that improves
the white space though if you like.

>  	case IIO_CHAN_INFO_SCALE:
>  		*val = LTR390_WINDOW_FACTOR;
>  		*val2 = LTR390_COUNTS_PER_UVI;
>  		return IIO_VAL_FRACTIONAL;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = data->int_time_us;
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
>  }

...

> +/* integration time in us */
> +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};

space after { and before } preferred.

> +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
>  
>  static const struct iio_chan_spec ltr390_channel = {
>  	.type = IIO_UVINDEX,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | BIT(IIO_CHAN_INFO_SCALE)
> +};
> +
> +static int ltr390_set_gain(struct ltr390_data *data, int val)
> +{
> +	int ret, idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(ltr390_gain_map); idx++) {
> +		if (ltr390_gain_map[idx] != val)
> +			continue;
> +
> +		guard(mutex)(&data->lock);
> +		ret = regmap_update_bits(data->regmap,
> +					LTR390_ALS_UVS_GAIN,
> +					LTR390_ALS_UVS_GAIN_MASK, idx);
> +		if (ret)
> +			return ret;
> +
> +		data->gain = ltr390_gain_map[idx];
> +		break;
As below. 
		return 0;
> +	}
> +
> +	return 0;
return -EINVAL; to indicate no match.


> +}
> +
> +static int ltr390_set_int_time(struct ltr390_data *data, int val)
> +{
> +	int ret, idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(ltr390_int_time_map_us); idx++) {
> +		if (ltr390_int_time_map_us[idx] != val)
> +			continue;
> +
> +		guard(mutex)(&data->lock);
> +		ret = regmap_update_bits(data->regmap,
> +					LTR390_ALS_UVS_MEAS_RATE,
> +					LTR390_ALS_UVS_INT_TIME_MASK,
> +					LTR390_ALS_UVS_INT_TIME(idx));
> +		if (ret)
> +			return ret;
> +
> +		data->int_time_us = ltr390_int_time_map_us[idx];
> +		break;
return 0;

No point in carrying on if we are done.

> +	}
> +
> +	return 0;
If you get here with suggested return 0 above, it will be an error as no
match occured.  In that case, return -EINVAL;

> +}

...

>  static int ltr390_probe(struct i2c_client *client)
> @@ -139,6 +249,11 @@ static int ltr390_probe(struct i2c_client *client)
>  				     "regmap initialization failed\n");
>  
>  	data->client = client;
> +	/* default value of integration time from pg: 15 of the datasheet */
> +	data->int_time_us = 100000;
> +	/* default value of gain from pg: 16 of the datasheet */
> +	data->gain = 3;
> +
>  	mutex_init(&data->lock);
>  
>  	indio_dev->info = &ltr390_info;
> @@ -162,7 +277,7 @@ static int ltr390_probe(struct i2c_client *client)
>  	usleep_range(1000, 2000);
>  
>  	ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> -			      LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> +				LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);

Avoid accidental white space changes. If you want to make them to cleanup
some inconsistencies or similar, that is fine, but they belong in a patch
that doesn't do anything else.  Here it is adding noise and slowing down
review.

>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
>  
> @@ -189,6 +304,7 @@ static struct i2c_driver ltr390_driver = {
>  	.probe = ltr390_probe,
>  	.id_table = ltr390_id,
>  };
> +
Don't add this line.  We often use whitespace to indicate connections and
it is common to do it in cases like this one where module_i2c_driver()
is tightly coupled with the i2c_driver structure.

>  module_i2c_driver(ltr390_driver);
>  
>  MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");