Re: [PATCH v2 3/3] iio: light: opt3001: add support for TI's opt3002 light sensor

[Emil Gedenryd <Emil.Gedenryd@axis.com>]

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