| Message ID | 20240916-add_opt3002-v3-3-984b190cd68c@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: opt3001: add support for TI's opt3002 light sensor | expand |
On Mon, 16 Sep 2024 16:56:39 +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 > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> Hi Emil, A few things inline, Thanks, Jonathan > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c > index 176e54bb48c3..5e3fe42c5b59 100644 > --- a/drivers/iio/light/opt3001.c > +++ b/drivers/iio/light/opt3001.c > @@ -479,6 +565,9 @@ static int opt3001_write_event_value(struct iio_dev *iio, > { > struct opt3001 *opt = iio_priv(iio); > int ret; > + int whole; > + int integer; > + int decimal; > > u16 mantissa; > u16 value; > @@ -497,7 +586,12 @@ static int opt3001_write_event_value(struct iio_dev *iio, > goto err; > } > > - mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; > + whole = opt->chip_info->factor_whole; > + integer = opt->chip_info->factor_integer; > + decimal = opt->chip_info->factor_decimal; > + > + mantissa = (((val * integer) + (val2 / decimal)) / whole) >> exponent; > + > value = (exponent << 12) | mantissa; > > switch (dir) { > @@ -610,7 +704,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); Unrelated change so in theory should be in a separate patch but meh, it's trivial so leave it here if you like. > return ret; > } > @@ -755,22 +850,25 @@ 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); Use the i2c specific way to to do this. https://elixir.bootlin.com/linux/v6.11/source/drivers/i2c/i2c-core-base.c#L120 i2c_get_match_data() because it will also handle falling back to matching via the i2c_match_id() path against the old style match tables in a few cases. > > mutex_init(&opt->lock); > init_waitqueue_head(&opt->result_ready_queue); > i2c_set_clientdata(client, iio); > > - ret = opt3001_read_id(opt); > - if (ret) > - return ret; > + if (opt->chip_info->has_id) { > + ret = opt3001_read_id(opt); > + if (ret) > + return ret; > + } > > ret = opt3001_configure(opt); > if (ret) > return ret; > > iio->name = client->name; > - iio->channels = opt3001_channels; > - iio->num_channels = ARRAY_SIZE(opt3001_channels); > + iio->channels = *opt->chip_info->channels; > + iio->num_channels = ARRAY_SIZE(*opt->chip_info->channels); This won't work as it has no way to perform a sizeof through a pointer. Add a num_channels filed to your opt3001_chip_info structure as then it can be ARRAY_SIZE(&opt3001_channels) which can work. > iio->modes = INDIO_DIRECT_MODE; > iio->info = &opt3001_info; > > @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client) > } > } > > +static const struct opt3001_chip_info opt3001_chip_information = { > + .channels = &opt3001_channels, > + .chan_type = IIO_LIGHT, > + .scales = &opt3001_scales, > + .factor_whole = 10, > + .factor_integer = 1000, > + .factor_decimal = 1000, > + .has_id = true, > +}; > + > +static const struct opt3001_chip_info opt3002_chip_information = { > + .channels = &opt3002_channels, > + .chan_type = IIO_INTENSITY, > + .scales = &opt3002_scales, > + .factor_whole = 12, > + .factor_integer = 10, > + .factor_decimal = 100000, > + .has_id = false, > +}; > + > static const struct i2c_device_id opt3001_id[] = { > - { "opt3001" }, > + { "opt3001", (kernel_ulong_t)&opt3001_chip_information }, > + { "opt3002", (kernel_ulong_t)&opt3002_chip_information }, > { } /* Terminating Entry */ > }; > MODULE_DEVICE_TABLE(i2c, opt3001_id); > > static const struct of_device_id opt3001_of_match[] = { > - { .compatible = "ti,opt3001" }, > + { .compatible = "ti,opt3001", .data = &opt3001_chip_information }, > + { .compatible = "ti,opt3002", .data = &opt3002_chip_information }, > { } > }; > MODULE_DEVICE_TABLE(of, opt3001_of_match); >
On Sat, 2024-09-28 at 17:05 +0100, Jonathan Cameron wrote: > On Mon, 16 Sep 2024 16:56:39 +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 > > Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> > Hi Emil, > > A few things inline, > > Thanks, > > Jonathan Hi Jonathan, Thank you for the comments, I'll start working on fixing them during the week. Best Regards, Emil > > > > > @@ -610,7 +704,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); > > Unrelated change so in theory should be in a separate patch but > meh, it's trivial so leave it here if you like. Good to know! I'll keep it since it's minor but I'll remember this for the future. > > > return ret; > > } > > > @@ -755,22 +850,25 @@ 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); > > Use the i2c specific way to to do this. > https://elixir.bootlin.com/linux/v6.11/source/drivers/i2c/i2c-core-base.c#L120 > i2c_get_match_data() because it will also handle falling back to matching > via the i2c_match_id() path against the old style match tables in a few > cases. Okay! > > > > mutex_init(&opt->lock); > > init_waitqueue_head(&opt->result_ready_queue); > > i2c_set_clientdata(client, iio); > > > > - ret = opt3001_read_id(opt); > > - if (ret) > > - return ret; > > + if (opt->chip_info->has_id) { > > + ret = opt3001_read_id(opt); > > + if (ret) > > + return ret; > > + } > > > > ret = opt3001_configure(opt); > > if (ret) > > return ret; > > > > iio->name = client->name; > > - iio->channels = opt3001_channels; > > - iio->num_channels = ARRAY_SIZE(opt3001_channels); > > + iio->channels = *opt->chip_info->channels; > > + iio->num_channels = ARRAY_SIZE(*opt->chip_info->channels); > This won't work as it has no way to perform a sizeof > through a pointer. > > Add a num_channels filed to your opt3001_chip_info structure > as then it can be ARRAY_SIZE(&opt3001_channels) which can work. Good catch and thanks for the suggestion, I'll do it that way! > > > iio->modes = INDIO_DIRECT_MODE; > > iio->info = &opt3001_info; > > > > @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client) > > } > > } > > > > +static const struct opt3001_chip_info opt3001_chip_information = { > > + .channels = &opt3001_channels, > > + .chan_type = IIO_LIGHT, > > + .scales = &opt3001_scales, > > + .factor_whole = 10, > > + .factor_integer = 1000, > > + .factor_decimal = 1000, > > + .has_id = true, > > +}; > > + > > +static const struct opt3001_chip_info opt3002_chip_information = { > > + .channels = &opt3002_channels, > > + .chan_type = IIO_INTENSITY, > > + .scales = &opt3002_scales, > > + .factor_whole = 12, > > + .factor_integer = 10, > > + .factor_decimal = 100000, > > + .has_id = false, > > +}; > > + > > static const struct i2c_device_id opt3001_id[] = { > > - { "opt3001" }, > > + { "opt3001", (kernel_ulong_t)&opt3001_chip_information }, > > + { "opt3002", (kernel_ulong_t)&opt3002_chip_information }, > > { } /* Terminating Entry */ > > }; > > MODULE_DEVICE_TABLE(i2c, opt3001_id); > > > > static const struct of_device_id opt3001_of_match[] = { > > - { .compatible = "ti,opt3001" }, > > + { .compatible = "ti,opt3001", .data = &opt3001_chip_information }, > > + { .compatible = "ti,opt3002", .data = &opt3002_chip_information }, > > { } > > }; > > MODULE_DEVICE_TABLE(of, opt3001_of_match); > > >
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..5e3fe42c5b59 100644 --- a/drivers/iio/light/opt3001.c +++ b/drivers/iio/light/opt3001.c @@ -70,6 +70,34 @@ #define OPT3001_RESULT_READY_SHORT 150 #define OPT3001_RESULT_READY_LONG 1000 +struct opt3001_scale { + int val; + int val2; +}; + +struct opt3001_chip_info { + const struct iio_chan_spec (*channels)[2]; + enum iio_chan_type chan_type; + const struct opt3001_scale (*scales)[12]; + + /* + * Factor as specified by conversion equation in datasheet. + * eg. 0.01 (scaled to integer 10) for opt3001. + */ + int factor_whole; + /* + * Factor to compensate for potentially scaled factor_whole. + */ + int factor_integer; + /* + * Factor used to align decimal part of proccessed value to six decimal + * places. + */ + int factor_decimal; + + bool has_id; +}; + struct opt3001 { struct i2c_client *client; struct device *dev; @@ -79,6 +107,7 @@ struct opt3001 { bool result_ready; wait_queue_head_t result_ready_queue; u16 result; + const struct opt3001_chip_info *chip_info; u32 int_time; u32 mode; @@ -92,11 +121,6 @@ struct opt3001 { bool use_irq; }; -struct opt3001_scale { - int val; - int val2; -}; - static const struct opt3001_scale opt3001_scales[] = { { .val = 40, @@ -148,21 +172,68 @@ static const struct opt3001_scale opt3001_scales[] = { }, }; +static const struct opt3001_scale opt3002_scales[] = { + { + .val = 4914, + .val2 = 0, + }, + { + .val = 9828, + .val2 = 0, + }, + { + .val = 19656, + .val2 = 0, + }, + { + .val = 39312, + .val2 = 0, + }, + { + .val = 78624, + .val2 = 0, + }, + { + .val = 157248, + .val2 = 0, + }, + { + .val = 314496, + .val2 = 0, + }, + { + .val = 628992, + .val2 = 0, + }, + { + .val = 1257984, + .val2 = 0, + }, + { + .val = 2515968, + .val2 = 0, + }, + { + .val = 5031936, + .val2 = 0, + }, + { + .val = 10063872, + .val2 = 0, + }, +}; + static int opt3001_find_scale(const struct opt3001 *opt, int val, int val2, u8 *exponent) { int i; - - for (i = 0; i < ARRAY_SIZE(opt3001_scales); i++) { - const struct opt3001_scale *scale = &opt3001_scales[i]; - + for (i = 0; i < ARRAY_SIZE(*opt->chip_info->scales); i++) { + const struct opt3001_scale *scale = &(*opt->chip_info->scales)[i]; /* - * Combine the integer and micro parts for comparison - * purposes. Use milli lux precision to avoid 32-bit integer - * overflows. + * Compare the integer and micro parts to determine value scale. */ - if ((val * 1000 + val2 / 1000) <= - (scale->val * 1000 + scale->val2 / 1000)) { + if (val < scale->val || + (val == scale->val && val2 <= scale->val2)) { *exponent = i; return 0; } @@ -174,11 +245,14 @@ static int opt3001_find_scale(const struct opt3001 *opt, int val, static void opt3001_to_iio_ret(struct opt3001 *opt, u8 exponent, u16 mantissa, int *val, int *val2) { - int lux; + int ret; + int whole = opt->chip_info->factor_whole; + int integer = opt->chip_info->factor_integer; + int decimal = opt->chip_info->factor_decimal; - lux = 10 * (mantissa << exponent); - *val = lux / 1000; - *val2 = (lux - (*val * 1000)) * 1000; + ret = whole * (mantissa << exponent); + *val = ret / integer; + *val2 = (ret - (*val * integer)) * decimal; } static void opt3001_set_mode(struct opt3001 *opt, u16 *reg, u16 mode) @@ -225,7 +299,18 @@ static const struct iio_chan_spec opt3001_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(1), }; -static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2) +static const struct iio_chan_spec opt3002_channels[] = { + { + .type = IIO_INTENSITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_INT_TIME), + .event_spec = opt3001_event_spec, + .num_event_specs = ARRAY_SIZE(opt3001_event_spec), + }, + IIO_CHAN_SOFT_TIMESTAMP(1), +}; + +static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2) { int ret; u16 mantissa; @@ -397,14 +482,15 @@ static int opt3001_read_raw(struct iio_dev *iio, if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) return -EBUSY; - if (chan->type != IIO_LIGHT) + if (chan->type != opt->chip_info->chan_type) return -EINVAL; mutex_lock(&opt->lock); switch (mask) { + case IIO_CHAN_INFO_RAW: case IIO_CHAN_INFO_PROCESSED: - ret = opt3001_get_lux(opt, val, val2); + ret = opt3001_get_processed(opt, val, val2); break; case IIO_CHAN_INFO_INT_TIME: ret = opt3001_get_int_time(opt, val, val2); @@ -428,7 +514,7 @@ static int opt3001_write_raw(struct iio_dev *iio, if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) return -EBUSY; - if (chan->type != IIO_LIGHT) + if (chan->type != opt->chip_info->chan_type) return -EINVAL; if (mask != IIO_CHAN_INFO_INT_TIME) @@ -479,6 +565,9 @@ static int opt3001_write_event_value(struct iio_dev *iio, { struct opt3001 *opt = iio_priv(iio); int ret; + int whole; + int integer; + int decimal; u16 mantissa; u16 value; @@ -497,7 +586,12 @@ static int opt3001_write_event_value(struct iio_dev *iio, goto err; } - mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; + whole = opt->chip_info->factor_whole; + integer = opt->chip_info->factor_integer; + decimal = opt->chip_info->factor_decimal; + + mantissa = (((val * integer) + (val2 / decimal)) / whole) >> exponent; + value = (exponent << 12) | mantissa; switch (dir) { @@ -610,7 +704,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); return ret; } @@ -692,6 +786,7 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) struct opt3001 *opt = iio_priv(iio); int ret; bool wake_result_ready_queue = false; + enum iio_chan_type chan_type = opt->chip_info->chan_type; if (!opt->ok_to_ignore_lock) mutex_lock(&opt->lock); @@ -707,13 +802,13 @@ static irqreturn_t opt3001_irq(int irq, void *_iio) OPT3001_CONFIGURATION_M_CONTINUOUS) { if (ret & OPT3001_CONFIGURATION_FH) iio_push_event(iio, - IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0, + IIO_UNMOD_EVENT_CODE(chan_type, 0, IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING), iio_get_time_ns(iio)); if (ret & OPT3001_CONFIGURATION_FL) iio_push_event(iio, - IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0, + IIO_UNMOD_EVENT_CODE(chan_type, 0, IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING), iio_get_time_ns(iio)); @@ -755,22 +850,25 @@ 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 (ret) - return ret; + if (opt->chip_info->has_id) { + ret = opt3001_read_id(opt); + if (ret) + return ret; + } ret = opt3001_configure(opt); if (ret) return ret; iio->name = client->name; - iio->channels = opt3001_channels; - iio->num_channels = ARRAY_SIZE(opt3001_channels); + iio->channels = *opt->chip_info->channels; + iio->num_channels = ARRAY_SIZE(*opt->chip_info->channels); iio->modes = INDIO_DIRECT_MODE; iio->info = &opt3001_info; @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client) } } +static const struct opt3001_chip_info opt3001_chip_information = { + .channels = &opt3001_channels, + .chan_type = IIO_LIGHT, + .scales = &opt3001_scales, + .factor_whole = 10, + .factor_integer = 1000, + .factor_decimal = 1000, + .has_id = true, +}; + +static const struct opt3001_chip_info opt3002_chip_information = { + .channels = &opt3002_channels, + .chan_type = IIO_INTENSITY, + .scales = &opt3002_scales, + .factor_whole = 12, + .factor_integer = 10, + .factor_decimal = 100000, + .has_id = false, +}; + static const struct i2c_device_id opt3001_id[] = { - { "opt3001" }, + { "opt3001", (kernel_ulong_t)&opt3001_chip_information }, + { "opt3002", (kernel_ulong_t)&opt3002_chip_information }, { } /* Terminating Entry */ }; MODULE_DEVICE_TABLE(i2c, opt3001_id); static const struct of_device_id opt3001_of_match[] = { - { .compatible = "ti,opt3001" }, + { .compatible = "ti,opt3001", .data = &opt3001_chip_information }, + { .compatible = "ti,opt3002", .data = &opt3002_chip_information }, { } }; MODULE_DEVICE_TABLE(of, opt3001_of_match);
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 Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com> --- drivers/iio/light/Kconfig | 2 +- drivers/iio/light/opt3001.c | 186 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 154 insertions(+), 34 deletions(-)