linux-kernel - Re: [PATCH v2 2/2] iio: chemical: Add driver for SEN0322

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: <4bvmhlzbk3wxnjskfio3i7wyimaclnjt7mlk5bydmn4ycur7fi@pdkvjofbrurw>
Date: Tue, 6 May 2025 09:52:05 +0200
From: Tóth János <gomba007@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 
	Tóth János via B4 Relay <devnull+gomba007.gmail.com@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: chemical: Add driver for SEN0322

Hi!

Thank you for the review!

> Checkpatch is lagging behind the times, but it is fine to use this
> as a formal tag in the tag block..
> > 
> Datasheet: https://wiki.dfrobot.com/Gravity_I2C_Oxygen_Sensor_SKU_SEN0322
> 
> > Signed-off-by: Tóth János <gomba007@...il.com>

Sure.

> > +	if (val) {
> > +		*num = val;
> > +		*den = 100000;
> > +	} else {
> > +		*num = 209;
> > +		*den = 120000;
> 
> This is odd enough, that perhaps we could add a comment on why, or at least
> a cross reference to where these numbers come from?
> What is the special meaning of 0?

Okay, I'll add some explanation.

> > +	u8 data[4] = { 0 };
> 
> If you are only read 3 bytes, why is this 4 long?

It is the closest power of 2, to pacify my OCD, but you are right.

> > +	ret = regmap_bulk_read(sen0322->regmap, SEN0322_REG_DATA, data, 3);
> 
> Having shortened above, use sizeof(data) for that 3 to avoid
> any potential future mismatch in sizes.

Agreed.

> > +	dev_dbg(regmap_get_device(sen0322->regmap), "data: %d\n", ret);
> 
> Given you more or less directly provide this to userspace now I'd drop
> the dev_dbg() as not adding any value for debugging.
>

I just like to see if the function actually ran and not reading some buffered 
value stuck somewhere, but okay.

> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION:
> As the sensor only does concentration, you could either drop this
> check on basis we can't get here without it or if you want 
> a strong sanity check do it outside the switch statement as
> 	if (chan->type != IIO_CONCENTRATION)
> 		return -EINVAL;

I did not want to deviate from the pattern, but yes, it will make the code
more readable.

Best regards,
János