On Mon, 16 Sep 2024 16:56:39 +0200 Emil Gedenryd <emil.gedenryd@xxxxxxxx> 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@xxxxxxxx> 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); >
