On Fri, Nov 17, 2023 at 06:42:06PM +0200, Petre Rodan wrote: > Adds driver for Honeywell TruStability HSC and SSC series pressure and > temperature sensors. > > Covers i2c and spi-based interfaces. For both series it supports all the > combinations of four transfer functions and all 118 pressure ranges. > In case of a special chip not covered by the nomenclature a custom range > can be specified. > > Devices tested: > HSCMLNN100PASA3 (SPI sensor) > HSCMRNN030PA2A3 (i2c sensor) > Datasheet: > [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf > [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf > [i2c comms] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/common/documents/sps-siot-i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-ciid-45841.pdf Make it a single tag per one URL as Datasheet: URL_#1 [xxx] Datasheet: URL_#2 [yyy] > No blank line in tags block. > Signed-off-by: Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx> ... > +config HSC030PA > + tristate "Honeywell HSC/SSC (TruStability pressure sensors series)" > + depends on (I2C || SPI_MASTER) > + select HSC030PA_I2C if (I2C) > + select HSC030PA_SPI if (SPI_MASTER) Unneeded parentheses > + help > + Say Y here to build support for the Honeywell HSC and SSC TruStability > + pressure and temperature sensor series. > + > + To compile this driver as a module, choose M here: the module will be > + called hsc030pa. Yeah besides indentation issues the LKP complain about this. ... > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/init.h> > +#include <linux/math64.h> > +#include <linux/units.h> > +#include <linux/mod_devicetable.h> > +#include <linux/printk.h> Keep them sorted alphabetically. Also there are missing at least these ones: array_size.h, types.h. + blank line > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> + blank line. > +#include "hsc030pa.h" ... > +// pressure range for current chip based on the nomenclature > +struct hsc_range_config { > + char name[HSC_RANGE_STR_LEN]; // 5-char string that defines the range - ie "030PA" > + s32 pmin; // minimal pressure in pascals > + s32 pmax; // maximum pressure in pascals > +}; Can you utilize linear ranges data types and APIs? (linear_range.h) ... > +/* > + * the first two bits from the first byte contain a status code > + * > + * 00 - normal operation, valid data > + * 01 - device in hidden factory command mode > + * 10 - stale data > + * 11 - diagnostic condition > + * > + * function returns 1 only if both bits are zero > + */ You really need to be consistent with style of multi-line comments. And also C++/C variants. Besides that, respect English grammar and punctuation. ... > +static bool hsc_measurement_is_valid(struct hsc_data *data) > +{ > + if (data->buffer[0] & 0xc0) > + return 0; > + > + return 1; You use bool and return integers. Besides, it can be just a oneliner. return !(buffer[0] & GENMASK(3, 2)); (Note, you will need bits.h for this.) > +} ... > +static int hsc_get_measurement(struct hsc_data *data) > +{ > + const struct hsc_chip_data *chip = data->chip; > + int ret; > + > + /* don't bother sensor more than once a second */ > + if (!time_after(jiffies, data->last_update + HZ)) > + return data->is_valid ? 0 : -EAGAIN; > + > + data->is_valid = false; > + data->last_update = jiffies; > + > + ret = data->xfer(data); > + Redundant blank line. > + if (ret < 0) > + return ret; > + ret = chip->valid(data); > + if (!ret) > + return -EAGAIN; > + > + data->is_valid = true; Can this be like bool is_valid; is_valid = chip->valid(...); if (!is_valid) return ... data->is_valid = is_valid; // Depending on the flow you can even use that field directly > + return 0; > +} > +static int hsc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > + struct hsc_data *data = iio_priv(indio_dev); > + int ret = -EINVAL; Just use value directly, no need to have this assignment. > + > + switch (mask) { > + Redundant blank line. > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&data->lock); > + ret = hsc_get_measurement(data); > + mutex_unlock(&data->lock); Use guard() operator from cleanup.h. > + if (ret) > + return ret; > + > + switch (channel->type) { > + case IIO_PRESSURE: > + *val = > + ((data->buffer[0] & 0x3f) << 8) + data->buffer[1]; > + return IIO_VAL_INT; > + case IIO_TEMP: > + *val = > + (data->buffer[2] << 3) + > + ((data->buffer[3] & 0xe0) >> 5); Is this some endianess / sign extension? Please convert using proper APIs. > + ret = 0; > + if (!ret) lol > + return IIO_VAL_INT; > + break; > + default: > + return -EINVAL; > + } > + break; > + > +/** > + * IIO ABI expects > + * value = (conv + offset) * scale > + * > + * datasheet provides the following formula for determining the temperature > + * temp[C] = conv * a + b > + * where a = 200/2047; b = -50 > + * > + * temp[C] = (conv + (b/a)) * a * (1000) > + * => > + * scale = a * 1000 = .097703957 * 1000 = 97.703957 > + * offset = b/a = -50 / .097703957 = -50000000 / 97704 > + * > + * based on the datasheet > + * pressure = (conv - HSC_OUTPUT_MIN) * Q + Pmin = > + * ((conv - HSC_OUTPUT_MIN) + Pmin/Q) * Q > + * => > + * scale = Q = (Pmax - Pmin) / (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) > + * offset = Pmin/Q = Pmin * (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) / (Pmax - Pmin) > + */ > + > + case IIO_CHAN_INFO_SCALE: > + switch (channel->type) { > + case IIO_TEMP: > + *val = 97; > + *val2 = 703957; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_PRESSURE: > + *val = data->p_scale; > + *val2 = data->p_scale_nano; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > + break; > + Dead code? > + case IIO_CHAN_INFO_OFFSET: > + switch (channel->type) { > + case IIO_TEMP: > + *val = -50000000; > + *val2 = 97704; > + return IIO_VAL_FRACTIONAL; > + case IIO_PRESSURE: > + *val = data->p_offset; > + *val2 = data->p_offset_nano; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > + } > + return ret; Use default with explicit error code. > +} ... > +static const struct iio_chan_spec hsc_channels[] = { > + { > + .type = IIO_PRESSURE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) > + }, > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) > + }, Strange indentation of }:s... > +}; ... > +int hsc_probe(struct iio_dev *indio_dev, struct device *dev, > + const char *name, int type) > +{ > + struct hsc_data *hsc; > + u64 tmp; > + int index; > + int found = 0; > + > + hsc = iio_priv(indio_dev); > + > + hsc->last_update = jiffies - HZ; > + hsc->chip = &hsc_chip; > + > + if (strcasecmp(hsc->range_str, "na") != 0) { > + // chip should be defined in the nomenclature > + for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) { > + if (strcasecmp > + (hsc_range_config[index].name, > + hsc->range_str) == 0) { > + hsc->pmin = hsc_range_config[index].pmin; > + hsc->pmax = hsc_range_config[index].pmax; > + found = 1; > + break; > + } > + } Reinventing match_string() / sysfs_match_string() ? > + if (hsc->pmin == hsc->pmax || !found) > + return dev_err_probe(dev, -EINVAL, > + "honeywell,range_str is invalid\n"); > + } > + > + hsc->outmin = hsc_func_spec[hsc->function].output_min; > + hsc->outmax = hsc_func_spec[hsc->function].output_max; > + > + // multiply with MICRO and then divide by NANO since the output needs > + // to be in KPa as per IIO ABI requirement > + tmp = div_s64(((s64) (hsc->pmax - hsc->pmin)) * MICRO, > + (hsc->outmax - hsc->outmin)); > + hsc->p_scale = div_s64_rem(tmp, NANO, &hsc->p_scale_nano); > + tmp = > + div_s64(((s64) hsc->pmin * (s64) (hsc->outmax - hsc->outmin)) * > + MICRO, hsc->pmax - hsc->pmin); No need to have space after castings > + hsc->p_offset = > + div_s64_rem(tmp, NANO, &hsc->p_offset_nano) - hsc->outmin; > + > + mutex_init(&hsc->lock); > + indio_dev->name = name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &hsc_info; > + indio_dev->channels = hsc->chip->channels; > + indio_dev->num_channels = hsc->chip->num_channels; > + > + return devm_iio_device_register(dev, indio_dev); > +} This one devm wrapped... > +void hsc_remove(struct iio_dev *indio_dev) > +{ > + iio_device_unregister(indio_dev); > +} ...but not this. Why do you even need remove if the above is using devm always? ... > + * Copyright (c) 2023 Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx> > + * Redundant blank line. ... > +#ifndef _HSC030PA_H > +#define _HSC030PA_H This header is using a lot and you are missing to include even a single provider. :-( At first glance: mutex.h types.h A few forward declarations: struct device; struct iio_chan_spec; struct iio_dev; struct hsc_chip_data; (Note blank lines in between.) ... > +struct hsc_data { > + void *client; // either i2c or spi kernel interface struct for current dev > + const struct hsc_chip_data *chip; > + struct mutex lock; // lock protecting chip reads > + int (*xfer)(struct hsc_data *data); // function that implements the chip reads > + bool is_valid; // false if last transfer has failed > + unsigned long last_update; // time of last successful conversion > + u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE]; // raw conversion data > + char range_str[HSC_RANGE_STR_LEN]; // range as defined by the chip nomenclature - ie "030PA" or "NA" > + s32 pmin; // min pressure limit > + s32 pmax; // max pressure limit > + u32 outmin; // minimum raw pressure in counts (based on transfer function) > + u32 outmax; // maximum raw pressure in counts (based on transfer function) > + u32 function; // transfer function > + s64 p_scale; // pressure scale > + s32 p_scale_nano; // pressure scale, decimal places > + s64 p_offset; // pressure offset > + s32 p_offset_nano; // pressure offset, decimal places > +}; > + > +struct hsc_chip_data { > + bool (*valid)(struct hsc_data *data); // function that checks the two status bits > + const struct iio_chan_spec *channels; // channel specifications > + u8 num_channels; // pressure and temperature channels > +}; Convert comments to kernel-doc format. ... > +enum hsc_func_id { > + HSC_FUNCTION_A, > + HSC_FUNCTION_B, > + HSC_FUNCTION_C, > + HSC_FUNCTION_F Leave trailing comma. It make code slightly better to maintain. > +}; > + > +enum hsc_variant { > + HSC, > + SSC Ditto. > +}; ... > +static int hsc_i2c_xfer(struct hsc_data *data) > +{ > + struct i2c_client *client = data->client; > + struct i2c_msg msg; > + int ret; > + > + msg.addr = client->addr; > + msg.flags = client->flags | I2C_M_RD; > + msg.len = HSC_REG_MEASUREMENT_RD_SIZE; > + msg.buf = (char *)&data->buffer; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + > + return (ret == 2) ? 0 : ret; > +} Can you use regmap I2C? ... > +static int hsc_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) No use of this function prototype, we have a new one. ... > + indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc)); > + if (!indio_dev) { > + dev_err(&client->dev, "Failed to allocate device\n"); > + return -ENOMEM; First of all, use return dev_err_probe(); Second, since it's ENOMEM, we do not issue an errors like this, error code is already enough. > + } > + > + hsc = iio_priv(indio_dev); > + if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > + hsc->xfer = hsc_i2c_xfer; > + else Redundant 'else', see below. > + return -EOPNOTSUPP; Use traditional pattern, i.e. checking for errors first: if (...) return ... ... > + ret = devm_regulator_get_enable_optional(dev, "vdd"); > + if (ret == -EPROBE_DEFER) > + return -EPROBE_DEFER; Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit interesting. ... > + if (!dev_fwnode(dev)) > + return -EOPNOTSUPP; Why do you need this? And why this error code? > + ret = device_property_read_u32(dev, > + "honeywell,transfer-function", > + &hsc->function); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,transfer-function could not be read\n"); ... > + ret = device_property_read_string(dev, > + "honeywell,range_str", &range_nom); One line. > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,range_str not defined\n"); ... > + memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1); > + hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0; Oh, why do you need this all and can use the property value directly? (Besides the fact the interesting operations are being used for strings.) > + if (strcasecmp(hsc->range_str, "na") == 0) { > + // "not available" > + // we got a custom-range chip not covered by the nomenclature > + ret = device_property_read_u32(dev, > + "honeywell,pmin-pascal", > + &hsc->pmin); > + 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", > + &hsc->pmax); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,pmax-pascal could not be read\n"); > + } ... > + i2c_set_clientdata(client, indio_dev); How is this being used? > + hsc->client = client; > + > + return hsc_probe(indio_dev, &client->dev, id->name, id->driver_data); > +} ... > +static const struct of_device_id hsc_i2c_match[] = { > + {.compatible = "honeywell,hsc",}, > + {.compatible = "honeywell,ssc",}, Inner commas are redundant. > + {}, Drop the comma in the terminator entry. > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(of, hsc_i2c_match); > + > +static const struct i2c_device_id hsc_i2c_id[] = { > + {"hsc", HSC}, > + {"ssc", SSC}, Both ID tables should use pointers in driver data and respective API to get that. > + {} > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(i2c, hsc_i2c_id); ... > +static struct i2c_driver hsc_i2c_driver = { > + .driver = { > + .name = "honeywell_hsc", > + .of_match_table = hsc_i2c_match, > + }, Indentation level can be dropped a bit. > + .probe = hsc_i2c_probe, > + .id_table = hsc_i2c_id, > +}; > + Redundant blank line. > +module_i2c_driver(hsc_i2c_driver); ... > +++ b/drivers/iio/pressure/hsc030pa_spi.c ... > + int ret; > + > + ret = spi_sync_transfer(data->client, &xfer, 1); > + > + return ret; So, why ret is needed? ... > + spi_set_drvdata(spi, indio_dev); How is this being used? > + spi->mode = SPI_MODE_0; > + spi->max_speed_hz = min(spi->max_speed_hz, 800000U); > + spi->bits_per_word = 8; > + ret = spi_setup(spi); > + if (ret < 0) > + return ret; Why the firmware can't provide the correct information to begin with? ... > + ret = devm_regulator_get_enable_optional(dev, "vdd"); > + if (ret == -EPROBE_DEFER) > + return -EPROBE_DEFER; As per I2C driver. But why is not in the main ->probe()? ... > + ret = device_property_read_u32(dev, > + "honeywell,transfer-function", > + &hsc->function); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,transfer-function could not be read\n"); > + if (hsc->function > HSC_FUNCTION_F) > + return dev_err_probe(dev, -EINVAL, > + "honeywell,transfer-function %d invalid\n", > + hsc->function); > + > + ret = > + device_property_read_string(dev, "honeywell,range_str", &range_nom); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,range_str not defined\n"); > + > + // minimal input sanitization > + memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1); > + hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0; > + > + if (strcasecmp(hsc->range_str, "na") == 0) { > + // range string "not available" > + // we got a custom chip not covered by the nomenclature with a custom range > + ret = device_property_read_u32(dev, "honeywell,pmin-pascal", > + &hsc->pmin); > + 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", > + &hsc->pmax); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,pmax-pascal could not be read\n"); > + } Ditto. Why is this duplication? ... > +static const struct of_device_id hsc_spi_match[] = { > + {.compatible = "honeywell,hsc",}, > + {.compatible = "honeywell,ssc",}, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(of, hsc_spi_match); > + > +static const struct spi_device_id hsc_spi_id[] = { > + {"hsc", HSC}, > + {"ssc", SSC}, > + {} > +}; > + > +MODULE_DEVICE_TABLE(spi, hsc_spi_id); > + > +static struct spi_driver hsc_spi_driver = { > + .driver = { > + .name = "honeywell_hsc", > + .of_match_table = hsc_spi_match, > + }, > + .probe = hsc_spi_probe, > + .id_table = hsc_spi_id, > +}; > + > +module_spi_driver(hsc_spi_driver); Same comments as per I2C driver above. -- With Best Regards, Andy Shevchenko
