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@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 > > > 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), > > +}; > > +
