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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ffb90ad1b5426a987f4e644a9fa663d96a0d31e7.camel@axis.com>
Date: Mon, 16 Sep 2024 07:17:39 +0000
From: Emil Gedenryd <Emil.Gedenryd@...s.com>
To: "jic23@...nel.org" <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "robh@...nel.org"
	<robh@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "dannenberg@...com" <dannenberg@...com>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "lars@...afoo.de"
	<lars@...afoo.de>, "conor+dt@...nel.org" <conor+dt@...nel.org>, Kernel
	<Kernel@...s.com>
Subject: Re: [PATCH v2 3/3] iio: light: opt3001: add support for TI's opt3002
 light sensor

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