Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Tue, 28 Nov 2023 18:54:10 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Petre Rodan <petre.rodan@subdimension.ro>
Cc:     linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Angel Iglesias <ang.iglesiasg@gmail.com>,
        Matti Vaittinen <mazziesaccount@gmail.com>,
        Andreas Klinger <ak@it-klinger.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Subject: Re: [PATCH v4 2/2] iio: pressure: driver for Honeywell HSC/SSC
 series pressure sensors
Message-ID: <ZWYbMnYEDmwNteEy@smile.fi.intel.com>
References: <20231128124103.22765-1-petre.rodan@subdimension.ro>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231128124103.22765-1-petre.rodan@subdimension.ro>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Tue, Nov 28, 2023 at 02:40:58PM +0200, Petre Rodan wrote:
> Adds driver for digital Honeywell TruStability HSC and SSC series
> pressure and temperature sensors. 

Trailing white space.

> 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.

...

> +	default:
> +		return -EINVAL;
> +	}

> +	return -EINVAL;

Dead code.

...

> +int hsc_common_probe(struct device *dev, void *client,
> +	    int (*recv_fct)(struct hsc_data *data), const char *name)

Make it a typedef

typedef int (*hsc_recv_fn)(struct hsc_data *);

(note the fn suffix as standard in such cases).

...

> +	ret = device_property_read_u32(dev,
> +				       "honeywell,transfer-function",
> +				       &hsc->function);

Strange indentation.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			    "honeywell,transfer-function could not be read\n");

...

> +	ret = device_property_read_string(dev,
> +		"honeywell,pressure-triplet", &triplet);

Strange indentation.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"honeywell,pressure-triplet could not be read\n");

...

> +	if (strncmp(triplet, "NA", 2) == 0) {
> +		/* "not available" in the nomenclature
> +		   we got a custom-range chip so extract pmin, pmax from dt */

Wrong comment style. And I believe I already said, respect English grammar
and punctuation! Ditto for other multi-line comments in this patch.

> +		ret = device_property_read_u32(dev,
> +					       "honeywell,pmin-pascal",
> +					       &hsc->pmin);

Strange indentation.

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +				"honeywell,pmin-pascal could not be read\n");
> +		ret = device_property_read_u32(dev,
> +					       "honeywell,pmax-pascal",

Strange indentation.

> +					       &hsc->pmax);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +				"honeywell,pmax-pascal could not be read\n");
> +	} else {
> +		/* chip should be defined in the nomenclature */
> +		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");
> +	}

...

> +	tmp = div_s64(((s64)(hsc->pmax - hsc->pmin)) * MICRO,
> +		      (hsc->outmax - hsc->outmin));

Too many parentheses (denominator).

...

> +#include <linux/property.h>
> +#include <linux/types.h>

Missing mutex.h (in the common header file).

...

> +	int (*recv)(struct hsc_data *data);

See about typedef above.

...

> +int hsc_common_probe(struct device *dev, void *client,
> +	    int (*recv_fct)(struct hsc_data *data), const char *name);

Ditto.

...

> +#include <linux/i2c.h>
> +#include <linux/module.h>

Missing mod_devicetable.h, errno.h (I?C driver).

...

> +#include <linux/module.h>
> +#include <linux/spi/spi.h>

Missing mod_devicetable.h (SPI driver).
See below as well.

...

> +		.tx_buf = NULL,

NULL is defined via types.h IIRC.

...

> +static struct spi_driver hsc_spi_driver = {
> +	.driver = {
> +		.name = "hsc030pa",
> +		.of_match_table = hsc_spi_match,
> +		},

Wrong indentation.

> +	.probe = hsc_spi_probe,
> +	.id_table = hsc_spi_id,
> +};

-- 
With Best Regards,
Andy Shevchenko