| Message ID | 20240913-add_opt3002-v2-0-69e04f840360@axis.com |
|---|---|
| Headers | show |
| Series | iio: light: opt3001: add support for TI's opt3002 light sensor | expand |
On Fri, 13 Sep 2024 11:57:02 +0200 Emil Gedenryd <emil.gedenryd@axis.com> wrote: > The opt3001 driver uses predetermined full-scale range values to > determine what exponent to use for event trigger threshold values. > The problem is that one of the values specified in the datasheet is > missing from the implementation. This causes larger values to be > scaled down to an incorrect exponent, effectively reducing the > maximum settable threshold value by a factor of 2. > > Add missing full-scale range array value. > > Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor") > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> Applied to the fixes-togreg branch of iio.git and marked for stable. I'll probably send a pull request with this in shortly after rc1. Jonathan > --- > drivers/iio/light/opt3001.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > index 887c4b776a86..176e54bb48c3 100644 > --- a/drivers/iio/light/opt3001.c > +++ b/drivers/iio/light/opt3001.c > @@ -138,6 +138,10 @@ static const struct opt3001_scale opt3001_scales[] = { > .val = 20966, > .val2 = 400000, > }, > + { > + .val = 41932, > + .val2 = 800000, > + }, > { > .val = 83865, > .val2 = 600000, >
On Fri, 13 Sep 2024 11:57:04 +0200 Emil Gedenryd <emil.gedenryd@axis.com> wrote: > TI's opt3002 light sensor shares most properties with the opt3001 > model, with the exception of supporting a wider spectrum range. > > Add support for TI's opt3002 by extending the TI opt3001 driver. > > Datasheet: https://www.ti.com/product/OPT3002 > No blank line here. Datasheet: should be part of this tags block. > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> > --- > drivers/iio/light/Kconfig | 2 +- > drivers/iio/light/opt3001.c | 169 +++++++++++++++++++++++++++++++++++--------- > 2 files changed, 138 insertions(+), 33 deletions(-) > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index b68dcc1fbaca..c35bf962dae6 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -461,7 +461,7 @@ config OPT3001 > depends on I2C > help > If you say Y or M here, you get support for Texas Instruments > - OPT3001 Ambient Light Sensor. > + OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor. > > If built as a dynamically linked module, it will be called > opt3001. > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > index 176e54bb48c3..83c49f4517b7 100644 > --- a/drivers/iio/light/opt3001.c > +++ b/drivers/iio/light/opt3001.c > @@ -70,6 +70,21 @@ > #define OPT3001_RESULT_READY_SHORT 150 > #define OPT3001_RESULT_READY_LONG 1000 > > +struct opt3001_scale { > + int val; > + int val2; > +}; > + > +struct opt300x_chip_info { Don't use wild cards. Just call it opt3001_chip_info. Wild cards tend to bite us as manufacturers have an 'amusing' habit of filling in gaps with unrelated devices. > + const struct iio_chan_spec (*channels)[2]; > + enum iio_chan_type chan_type; > + const struct opt3001_scale (*scales)[12]; > + int factor_whole; > + int factor_integer; > + int factor_decimal; These three aren't obvious when just looking to fill in this structure. Add some docs to hint at what they are. > + bool has_id; > +}; > @@ -610,7 +690,7 @@ static int opt3001_read_id(struct opt3001 *opt) > ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID); > if (ret < 0) { > dev_err(opt->dev, "failed to read register %02x\n", > - OPT3001_DEVICE_ID); > + OPT3001_DEVICE_ID); In general whitespace only changes belong in their own patch, but I guess this one is pretty minor so we can skip that requirement this time. > return ret; > } > @@ -746,7 +827,7 @@ static int opt3001_probe(struct i2c_client *client) > struct iio_dev *iio; > struct opt3001 *opt; > int irq = client->irq; > - int ret; > + int ret = 0; > > iio = devm_iio_device_alloc(dev, sizeof(*opt)); > if (!iio) > @@ -755,12 +836,14 @@ static int opt3001_probe(struct i2c_client *client) > opt = iio_priv(iio); > opt->client = client; > opt->dev = dev; > + opt->chip_info = device_get_match_data(&client->dev); > > mutex_init(&opt->lock); > init_waitqueue_head(&opt->result_ready_queue); > i2c_set_clientdata(client, iio); > > - ret = opt3001_read_id(opt); > + if (opt->chip_info->has_id) > + ret = opt3001_read_id(opt); > if (ret) > return ret; > Only check the ret if the function ran. That way no need for the dance with ret = 0 above. > +static const struct iio_chan_spec opt3002_channels[] = { > + { > + .type = IIO_INTENSITY, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > + BIT(IIO_CHAN_INFO_INT_TIME), Generally intensity channels can't be processed because there are no defined units as what you measure depends entirely on the frequency sensitivity. There are some defined measurements such as illuminance that use a specific sensivitiy curve, but if it's just intensity we normally stick to _RAW.. > + .event_spec = opt3001_event_spec, > + .num_event_specs = ARRAY_SIZE(opt3001_event_spec), > + }, > + IIO_CHAN_SOFT_TIMESTAMP(1), > +}; > +
On Sat, 2024-09-14 at 17:06 +0100, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 11:57:02 +0200 > Emil Gedenryd <emil.gedenryd@axis.com> wrote: > > > The opt3001 driver uses predetermined full-scale range values to > > determine what exponent to use for event trigger threshold values. > > The problem is that one of the values specified in the datasheet is > > missing from the implementation. This causes larger values to be > > scaled down to an incorrect exponent, effectively reducing the > > maximum settable threshold value by a factor of 2. > > > > Add missing full-scale range array value. > > > > Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor") > > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> > Applied to the fixes-togreg branch of iio.git and marked for stable. > I'll probably send a pull request with this in shortly after rc1. > > Jonathan > > Great, thank you. Best Regards, Emil
On Sat, 2024-09-14 at 17:19 +0100, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 11:57:04 +0200 > Emil Gedenryd <emil.gedenryd@axis.com> wrote: > > > TI's opt3002 light sensor shares most properties with the opt3001 > > model, with the exception of supporting a wider spectrum range. > > > > Add support for TI's opt3002 by extending the TI opt3001 driver. > > > > Datasheet: https://www.ti.com/product/OPT3002 > > > No blank line here. Datasheet: should be part of this tags block. Okay, I'll remove it in the next version. > > > > + > > +struct opt300x_chip_info { > > Don't use wild cards. Just call it opt3001_chip_info. > Wild cards tend to bite us as manufacturers have an 'amusing' habit > of filling in gaps with unrelated devices. Good point! > > > > + const struct iio_chan_spec (*channels)[2]; > > + enum iio_chan_type chan_type; > > + const struct opt3001_scale (*scales)[12]; > > + int factor_whole; > > + int factor_integer; > > + int factor_decimal; > > These three aren't obvious when just looking to fill in this > structure. Add some docs to hint at what they are. Okay! > > + bool has_id; > > +}; > > > @@ -610,7 +690,7 @@ static int opt3001_read_id(struct opt3001 *opt) > > ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID); > > if (ret < 0) { > > dev_err(opt->dev, "failed to read register %02x\n", > > - OPT3001_DEVICE_ID); > > + OPT3001_DEVICE_ID); > > In general whitespace only changes belong in their own patch, but I guess > this one is pretty minor so we can skip that requirement this time. Thank you for the info, I'll keep that in mind in the future. > > > @@ -755,12 +836,14 @@ static int opt3001_probe(struct i2c_client *client) > > opt = iio_priv(iio); > > opt->client = client; > > opt->dev = dev; > > + opt->chip_info = device_get_match_data(&client->dev); > > > > mutex_init(&opt->lock); > > init_waitqueue_head(&opt->result_ready_queue); > > i2c_set_clientdata(client, iio); > > > > - ret = opt3001_read_id(opt); > > + if (opt->chip_info->has_id) > > + ret = opt3001_read_id(opt); > > if (ret) > > return ret; > > > Only check the ret if the function ran. That way no need for the > dance with ret = 0 above. Good point, I'll change that. > > > > +static const struct iio_chan_spec opt3002_channels[] = { > > + { > > + .type = IIO_INTENSITY, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > > + BIT(IIO_CHAN_INFO_INT_TIME), > > Generally intensity channels can't be processed because there are no > defined units as what you measure depends entirely on the frequency > sensitivity. There are some defined measurements such as illuminance > that use a specific sensivitiy curve, but if it's just intensity we > normally stick to _RAW.. Okay, I'll switch to the raw type instead. Thank you for the feedback, I'll start working on a new version as soon as possible. Best Regards, Emil > > + .event_spec = opt3001_event_spec, > > + .num_event_specs = ARRAY_SIZE(opt3001_event_spec), > > + }, > > + IIO_CHAN_SOFT_TIMESTAMP(1), > > +}; > > +
TI's opt3002 light-to-digital sensor provides the functionality of an optical power meter within a single device. It shares a lot of similarities with their opt3001 model but has a wide spectral bandwidth, ranging from 300 nm to 1000 nm. This patch set adds support for the TI opt3002 by extending the opt3001 driver. In addition, a missing full-scale range value for the opt3001 is added, resulting in higher precision when setting event trigger values. Datasheet: http://www.ti.com/product/OPT3002 Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> --- Changes in v2: - Move dt-binding patch to before implementation. - Fix dt-binding compatible definition. - Clarify bug description for missing full-scale range value. - Remove model enum from chip info and all in-function switch-case statements. - Move model-specific channels and mathematic constants to chip info. - Add valid match data to opt3001_id array - Skip call to function opt3001_read_id() if model doesn't have a device id register. - Link to v1: https://lore.kernel.org/r/20240905-add_opt3002-v1-0-a5ae21b924fb@axis.com --- Emil Gedenryd (3): iio: light: opt3001: add missing full-scale range value dt-bindings: iio: light: opt3001: add compatible for opt3002 iio: light: opt3001: add support for TI's opt3002 light sensor .../devicetree/bindings/iio/light/ti,opt3001.yaml | 4 +- drivers/iio/light/Kconfig | 2 +- drivers/iio/light/opt3001.c | 173 +++++++++++++++++---- 3 files changed, 145 insertions(+), 34 deletions(-) --- base-commit: 5be63fc19fcaa4c236b307420483578a56986a37 change-id: 20240828-add_opt3002-40552c1a2f77 Best regards,