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

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]

Message-ID: <ZWcUPkzfGqxYsysp@smile.fi.intel.com>
Date:   Wed, 29 Nov 2023 12:36:46 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Petre Rodan <petre.rodan@...dimension.ro>
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 v5 2/2] iio: pressure: driver for Honeywell HSC/SSC
 series pressure sensors

On Wed, Nov 29, 2023 at 09:04:49AM +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.

Thank you for an update!

Much better now, only one important thing is left unaddressed, see below.

...

> +	if (strncmp(triplet, "NA", 2) == 0) {

str_has_prefix()

...

> +		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> +			if (strncmp(hsc_range_config[index].triplet,
> +				    triplet,
> +				    HSC_PRESSURE_TRIPLET_LEN - 1) == 0) {
> +				hsc->pmin = hsc_range_config[index].pmin;
> +				hsc->pmax = hsc_range_config[index].pmax;
> +				found = 1;
> +				break;
> +			}
> +		}
> +		if (hsc->pmin == hsc->pmax || !found)
> +			return dev_err_probe(dev, -EINVAL,
> +				"honeywell,pressure-triplet is invalid\n");

This one is important. I think I told already twice that this is NIH
device_property_match_property_string(). Please, use this API directly.

...

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

I would leave MICRO on the previous line for better understanding the code.

...

> +#ifndef _HSC030PA_H
> +#define _HSC030PA_H

> +#include <linux/mutex.h>

> +#include <linux/property.h>

Is not used here.

> +#include <linux/types.h>

...

> +/*
> + * get all conversions (4 bytes) in one go
> + * since transfers are not address-based
> +*/

Missing space at the last line and missing capitalization and grammar period.

...

> +int hsc_common_probe(struct device *dev, void *client,
> +	hsc_recv_fn recv_fn, const char *name);

Indentation is unusual on the second line, also you can use just "recv" as
parameter name. But both are minor and I leave them to you and maintainer.

...

> +static const struct of_device_id hsc_spi_match[] = {
> +	{.compatible = "honeywell,hsc030pa"},

I believe Jonathan asks for inner spaces, like { ...foo... }.

> +	{}
> +};

...

> +static const struct spi_device_id hsc_spi_id[] = {
> +	{"hsc030pa"},

Ditto.

> +	{}
> +};

-- 
With Best Regards,
Andy Shevchenko