| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAG=0RqL1GxCKdzDzUjqECEsfQmunCwnv+g_5cqM1fcfBsg+P0w@mail.gmail.com>
Date: Fri, 26 Jul 2024 18:25:14 +0530
From: Abhash jha <abhashkumarjha123@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: anshulusr@...il.com, linux-iio@...r.kernel.org, lars@...afoo.de,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: ltr390: Add configurable gain, resolution and
ALS reading
On Sat, Jul 20, 2024 at 9:26 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 18 Jul 2024 16:19:45 +0530
> Abhash Jha <abhashkumarjha123@...il.com> wrote:
>
> > 1) Add support for configuring the gain and resolution(integration time)
> > for the sensor.
> > 2) Add a channel for ALS and provide support for reading the raw and
> > scale values.
> > 3) Add automatic mode switching between UVS and ALS based on the
> > channel type.
> > 4) Calculate 'counts_per_uvi' based on the current gain and integration
> > time.
>
> Hi Abhash,
>
> When a patch lists more than one thing, key thing to think is
> "maybe this should be multiple patches?"
>
> Here at very least separate resolution / gain into one or two patches
> and the new channel support into another.
> Probably yet another patch for point 4,
>
> Various other comments inline.
>
> Jonathan
>
> >
> > Signed-off-by: Abhash Jha <abhashkumarjha123@...il.com>
> > ---
> > drivers/iio/light/ltr390.c | 256 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 238 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> > index fff1e8990..56f3c74ae 100644
> > --- a/drivers/iio/light/ltr390.c
> > +++ b/drivers/iio/light/ltr390.c
> > @@ -25,19 +25,33 @@
> > #include <linux/regmap.h>
> >
> > #include <linux/iio/iio.h>
> > -
> > +#include <linux/iio/sysfs.h>
>
> > #include <asm/unaligned.h>
> >
> > #define LTR390_MAIN_CTRL 0x00
> > #define LTR390_PART_ID 0x06
> > #define LTR390_UVS_DATA 0x10
> >
> > +#define LTR390_ALS_DATA 0x0D
> > +#define LTR390_ALS_UVS_GAIN 0x05
> > +#define LTR390_ALS_UVS_MEAS_RATE 0x04
> > +#define LTR390_INT_CFG 0x19
> If these are register addresses put them in numeric order so
> it is easy to compare with a datasheet table
>
> > +
> > #define LTR390_SW_RESET BIT(4)
> > #define LTR390_UVS_MODE BIT(3)
> > #define LTR390_SENSOR_ENABLE BIT(1)
> >
> > #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_MASK_SHIFT 4
>
> Used FIELD_GET() and FIELD_PREP() then you never
> need a separate SHIFT defintion.
>
> > +
> > +#define LTR390_SET_ALS_MODE 1
> > +#define LTR390_SET_UVS_MODE 2
>
> If these are being use to pick options and not writen to hw
> use an enum. I don't think we care what value they take.
>
>
> > +
> > +#define LTR390_FRACTIONAL_PRECISION 100
> > +
> > /*
> > * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
> > * the sensor are equal to 1 UV Index [Datasheet Page#8].
> > @@ -60,6 +74,9 @@ struct ltr390_data {
> > struct i2c_client *client;
> > /* Protects device from simulataneous reads */
> > struct mutex lock;
> > + int mode;
> > + int gain;
> > + int int_time_us;
> > };
> >
> > static const struct regmap_config ltr390_regmap_config = {
> > @@ -87,36 +104,232 @@ static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> > return get_unaligned_le24(recieve_buffer);
> > }
> >
> > +
> one blank line is neough.
>
> > +static int ltr390_set_mode(struct ltr390_data *data, int mode)
> As suggested above, use an enum for mode. Give than a type name and you
> can use that here.
>
> > +{
> > + if (data->mode == mode)
> > + return 0;
> > +
> > + if (mode == LTR390_SET_ALS_MODE) {
> > + regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> > + data->mode = LTR390_SET_ALS_MODE;
> > + } else if (mode == LTR390_SET_UVS_MODE) {
> > + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_UVS_MODE);
> > + data->mode = LTR390_SET_UVS_MODE;
> Drop this out of the if / else stack and use
> data->mode = mode;
> A switch statement may be more appropriate here even if it's a few more lines of
> code.
>
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ltr390_counts_per_uvi(struct ltr390_data *data)
> > +{
> > + int orig_gain = 18;
> > + int orig_int_time = 400;
> > + int divisor = orig_gain * orig_int_time;
> > + int gain = data->gain;
> > +
> > + int int_time_ms = DIV_ROUND_CLOSEST(data->int_time_us, 1000);
> > + int uvi = DIV_ROUND_CLOSEST(2300*gain*int_time_ms, divisor);
>
> Spaces around *
>
> > +
> > + return uvi;
> > +}
> > +
> > 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;
> Don't move code unless there is a strong reason. Fine to
> tidy this sort of thing up, but not in a patch doing anything else
> as it becomes noise.
>
> >
> Almost certainly need locking here as concurrent accesses to sysfs
> files will result in mode changing whilst the read has not yet happened.
>
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > - ret = ltr390_register_read(data, LTR390_UVS_DATA);
> > - if (ret < 0)
> > - return ret;
> > + switch (chan->type) {
> > + case IIO_UVINDEX:
> > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ltr390_register_read(data, LTR390_UVS_DATA);
> Fix the alignment - looks like mix of spaces and tabs.
> scripts/checkpatch.pl would have pointed that out.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + break;
> > +
> > + case IIO_INTENSITY:
> > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ltr390_register_read(data, LTR390_ALS_DATA);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> return here. Otherwise you overwrite the value below.
>
> > + }
> > +
> > *val = ret;
> > - return IIO_VAL_INT;
> > + ret = IIO_VAL_INT;
> return here and drop the break.
> It is much simpler to follow code if it doesn't unnecessarily not
> return in cases like this as we have to scroll down to see if anything else
> happens.
>
> > + break;
> > +
> > case IIO_CHAN_INFO_SCALE:
> > - *val = LTR390_WINDOW_FACTOR;
> > - *val2 = LTR390_COUNTS_PER_UVI;
> > - return IIO_VAL_FRACTIONAL;
> > + mutex_lock(&data->lock);
> Add appropriate scope using {} and use
> guard(mutex)(&data->lock) as then in error paths you can
> return without unlocking...
> > +
> > + switch (chan->type) {
> > + case IIO_UVINDEX:
> > + ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> > + if (ret < 0)
> mutex held. Result is deadlock. Above scoped unlocking avoids that without
> needing to make sure you unlock in all paths.
>
>
> > + return ret;
> > +
> > + *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> > + *val2 = ltr390_counts_per_uvi(data);
> > + ret = IIO_VAL_FRACTIONAL;
> return here.
>
> > + break;
> > +
> > + case IIO_INTENSITY:
> > + ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = LTR390_WINDOW_FACTOR;
> > + *val2 = data->gain;
> > +
> > + ret = IIO_VAL_FRACTIONAL;
> > + break;
> return here.
> > +
> > + default:
> > + ret = -EINVAL;
> return here.
> > + }
> > +
> > + mutex_unlock(&data->lock);
> With guard() change above, not needed.
> But close scope here with }
> > + break;
> > +
> > + case IIO_CHAN_INFO_INT_TIME:
> > + mutex_lock(&data->lock);
> Given all paths other than invalid ones need the lock, maybe just take
> it outside of the switch statement - still use guard() though to avoid
> need to manually unlock.
>
> > + *val = data->int_time_us;
> > + mutex_unlock(&data->lock);
> > + ret = IIO_VAL_INT;
> > + break;
> > +
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > }
> > +
> > + return ret;
> This is a bad change as now I need to read to end of function in all
> code paths. Some code styles insist on single exit points, but
> the kernel style does not. (not worth a long discussion of why the
> two common styles came about). Keep those early returns.
>
>
> > }
> >
> > -static const struct iio_info ltr390_info = {
> > - .read_raw = ltr390_read_raw,
> > +/* integration time in us */
> > +static const int ltr390_int_time_map_us[] = {400000, 200000, 100000, 50000, 25000, 12500};
> > +static const int ltr390_gain_map[] = {1, 3, 6, 9, 18};
> > +
> > +static IIO_CONST_ATTR_INT_TIME_AVAIL("400000 200000 100000 50000 25000 12500");
> Please use read_avail() callback and the appropriate mask to provide this.
> That enables it to be used from in kernel consumers and enforces the
> ABI without a reviewer having to check what you have aligns.
>
> > +static IIO_CONST_ATTR(gain_available, "1 3 6 9 18");
> Given we don't have a 'gain' control, what is the available applying to?
>
The gain gets controlled by writing to the iio_info_scale attribute,
we write one of the above available values.
So that we can scale the raw ALS and UVI values. I could use
read_avail() for this too for the IIO_INFO_SCALE channel. Should I do
that?
Can you elaborate more on your comment?
> > +
> > +static struct attribute *ltr390_attributes[] = {
> > + &iio_const_attr_integration_time_available.dev_attr.attr,
> > + &iio_const_attr_gain_available.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group ltr390_attribute_group = {
> > + .attrs = ltr390_attributes,
> > };
> >
> > -static const struct iio_chan_spec ltr390_channel = {
> > +static const struct iio_chan_spec ltr390_channels[] = {
> > + /* UV sensor */
> > + {
> > .type = IIO_UVINDEX,
> > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> > + .scan_index = 0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> Fix style.
> {
> .type = ...
>
> > + },
> > + /* ALS sensor */
> > + {
> > + .type = IIO_INTENSITY,
> > + .scan_index = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME)
> > + },
> > +};
> > +
> > +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) {
> > + mutex_lock(&data->lock);
> guard here.
> > + ret = regmap_update_bits(data->regmap,
> > + LTR390_ALS_UVS_GAIN,
> > + LTR390_ALS_UVS_GAIN_MASK, idx);
> > + if (!ret)
> if (ret)
> return ret;
> prefer to keep error paths as the out of line ones as if you review
> a lot of code, predictability helps review quickly.
>
> > + data->gain = ltr390_gain_map[idx];
> > +
> > + mutex_unlock(&data->lock);
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +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) {
> flip logic to reduce indent.
> if (ltr390_int_time_map_us[idx] != val)
> continue;
>
> guard(mutex)...
>
> > + mutex_lock(&data->lock);
> > + ret = regmap_update_bits(data->regmap,
> > + LTR390_ALS_UVS_MEAS_RATE,
> > + LTR390_ALS_UVS_INT_TIME_MASK,
> > + idx<<LTR390_ALS_UVS_INT_TIME_MASK_SHIFT);
> spaces around <<
> Though FIELD_PREP() probably better solution.
>
> > + if (!ret)
> As in previous funciton.
> > + data->int_time_us = ltr390_int_time_map_us[idx];
> > +
> > + mutex_unlock(&data->lock);
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ltr390_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + if (val2 != 0)
> > + ret = -EINVAL;
> > +
> > + ret = ltr390_set_gain(data, val);
> > + break;
> > +
> > + case IIO_CHAN_INFO_INT_TIME:
> > + if (val2 != 0)
> > + ret = -EINVAL;
> > +
> > + ret = ltr390_set_int_time(data, val);
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret;
> Use early returns.
>
> > +}
> > +
> > +static const struct iio_info ltr390_info = {
> > + .attrs = <r390_attribute_group,
> > + .read_raw = ltr390_read_raw,
> > + .write_raw = ltr390_write_raw,
> > };
> >
> > static int ltr390_probe(struct i2c_client *client)
> > @@ -139,11 +352,18 @@ static int ltr390_probe(struct i2c_client *client)
> > "regmap initialization failed\n");
> >
> > data->client = client;
> > + /* default value of int time from pg: 15 of the datasheet */
> I'd spell out integration in the comment.
>
> > + data->int_time_us = 100000;
> > + /* default value of gain from pg: 16 of the datasheet */
> > + data->gain = 3;
> > + /* default mode for ltr390 is ALS mode */
> > + data->mode = LTR390_SET_ALS_MODE;
> > +
> > mutex_init(&data->lock);
> >
> > indio_dev->info = <r390_info;
> > - indio_dev->channels = <r390_channel;
> > - indio_dev->num_channels = 1;
> > + indio_dev->channels = ltr390_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
> > indio_dev->name = "ltr390";
> >
> > ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
> > @@ -161,8 +381,7 @@ static int ltr390_probe(struct i2c_client *client)
> > /* Wait for the registers to reset before proceeding */
> > usleep_range(1000, 2000);
> >
> > - ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> > - LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> > + ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to enable the sensor\n");
> >
> > @@ -189,6 +408,7 @@ static struct i2c_driver ltr390_driver = {
> > .probe = ltr390_probe,
> > .id_table = ltr390_id,
> > };
> > +
> Lack of space is intentional to keep the macro closely coupled to what
> it applies to.
>
> > module_i2c_driver(ltr390_driver);
> >
> > MODULE_AUTHOR("Anshul Dalal <anshulusr@...il.com>");
>
ACK. Will do the necessary changes and send V2 after splitting it into
4 patches (replying again because I missed replying with CC last time)
Powered by blists - more mailing lists