On Sun, 28 Jul 2024 20:49:54 +0530 Abhash Jha <abhashkumarjha123@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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 = <r390_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@xxxxxxxxx>");
