linux-kernel - Re: [PATCH v6 2/2] iio: pressure: driver for Honeywell HSC/SSC series

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Nov 2023 21:12:52 +0200
From:   Petre Rodan <petre.rodan@...dimension.ro>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Andreas Klinger <ak@...klinger.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v6 2/2] iio: pressure: driver for Honeywell HSC/SSC series


hello!

On Wed, Nov 29, 2023 at 07:24:31PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 29, 2023 at 07:04:12PM +0200, Petre Rodan wrote:
> > Adds driver for digital Honeywell TruStability HSC and SSC series
> > pressure and temperature sensors.
> > Communication is one way. The sensor only requires 4 bytes worth of
> > clock pulses on both i2c and spi in order to push the data out.
> > The i2c address is hardcoded and depends on the part number.
> > There is no additional GPIO control.
> 
> ...
> 
> > v6: modifications based on Andy's review
> >     - use str_has_prefix(), match_string() instead of strncmp()
> 
> And why not using the respective property API for that case where
> match_string() is used?

I'm lost again.

437:  ret = device_property_read_string(dev, "honeywell,pressure-triplet",
					&triplet);
[..]
455:	ret = match_string(hsc_triplet_variants, HSC_VARIANTS_MAX,
						triplet);
		if (ret < 0)
			return dev_err_probe(dev, -EINVAL,
				"honeywell,pressure-triplet is invalid\n");

		hsc->pmin = hsc_range_config[ret].pmin;
		hsc->pmax = hsc_range_config[ret].pmax;

triplet is got via device_property_read_string(), is there some other property
function I should be using?

> I'm also a bit tired to repeat about:
> - capitalization and punctuation in the multi-line comments;
> - broken indentation is some cases.

sorry about that. I removed all comments you complained about.
just to simplify the process.

> Otherwise it's a good stuff, I leave it now to Jonathan.
> 
> ...
> 
> > +	tmp = div_s64(((s64)(hsc->pmax - hsc->pmin)) * MICRO,
> > +		      hsc->outmax - hsc->outmin);
> > +	hsc->p_scale = div_s64_rem(tmp, NANO, &hsc->p_scale_dec);
> > +	tmp = div_s64(((s64)hsc->pmin * (s64)(hsc->outmax - hsc->outmin)) *
> > +		      MICRO, hsc->pmax - hsc->pmin);
> 
> Why not put MICRO on the previous line?

oh well, from the review I understood you were asking for the replacement of
NANO with MICRO on the previous instruction and it did not make much sense ( units
are in pascal and we need a kilopascal output to userland)

now I understood it's an indentation request. however moving MICRO will cross
the 80 column rule. but if there will be yet another modification request I'll move it.

cheers,
peter


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
petre rodan