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
