Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
From: Andy Shevchenko
Date: Mon Nov 20 2023 - 07:35:52 EST
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