Messages in this thread | | | Date | Tue, 28 Nov 2023 18:54:10 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors |
| |
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
| |