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
Signed-off-by: Petre Rodan <[email protected]>
---
MAINTAINERS | 7 +
drivers/iio/pressure/Kconfig | 22 ++
drivers/iio/pressure/Makefile | 3 +
drivers/iio/pressure/hsc030pa.c | 402 ++++++++++++++++++++++++++++
drivers/iio/pressure/hsc030pa.h | 59 ++++
drivers/iio/pressure/hsc030pa_i2c.c | 136 ++++++++++
drivers/iio/pressure/hsc030pa_spi.c | 129 +++++++++
7 files changed, 758 insertions(+)
create mode 100644 drivers/iio/pressure/hsc030pa.c
create mode 100644 drivers/iio/pressure/hsc030pa.h
create mode 100644 drivers/iio/pressure/hsc030pa_i2c.c
create mode 100644 drivers/iio/pressure/hsc030pa_spi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 482d428472e7..cba0d34c504a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9708,6 +9708,13 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
F: drivers/iio/pressure/mprls0025pa.c
+HONEYWELL HSC, SSC PRESSURE SENSOR SERIES IIO DRIVER
+M: Petre Rodan <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
+F: drivers/iio/pressure/hsc030pa*
+
HOST AP DRIVER
L: [email protected]
S: Obsolete
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 95efa32e4289..266688802fb3 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -109,6 +109,28 @@ config HP03
To compile this driver as a module, choose M here: the module
will be called hp03.
+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)
+ 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.
+
+config HSC030PA_I2C
+ tristate
+ depends on HSC030PA
+ depends on I2C
+
+config HSC030PA_SPI
+ tristate
+ depends on HSC030PA
+ depends on SPI_MASTER
+
config ICP10100
tristate "InvenSense ICP-101xx pressure and temperature sensor"
depends on I2C
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 436aec7e65f3..b0f8b94662f2 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -15,6 +15,9 @@ obj-$(CONFIG_DPS310) += dps310.o
obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
obj-$(CONFIG_HID_SENSOR_PRESS) += hid-sensor-press.o
obj-$(CONFIG_HP03) += hp03.o
+obj-$(CONFIG_HSC030PA) += hsc030pa.o
+obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
+obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
obj-$(CONFIG_ICP10100) += icp10100.o
obj-$(CONFIG_MPL115) += mpl115.o
obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o
diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
new file mode 100644
index 000000000000..b6ddfef7ee28
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <[email protected]>
+ *
+ * Datasheet: 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?download=false
+ */
+
+#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>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include "hsc030pa.h"
+
+struct hsc_func_spec {
+ u32 output_min;
+ u32 output_max;
+};
+
+static const struct hsc_func_spec hsc_func_spec[] = {
+ [HSC_FUNCTION_A] = {.output_min = 1638, .output_max = 14746}, // 10% - 90% of 2^14
+ [HSC_FUNCTION_B] = {.output_min = 819, .output_max = 15565}, // 5% - 95% of 2^14
+ [HSC_FUNCTION_C] = {.output_min = 819, .output_max = 13926}, // 5% - 85% of 2^14
+ [HSC_FUNCTION_F] = {.output_min = 655, .output_max = 15401}, // 4% - 94% of 2^14
+};
+
+// 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
+};
+
+// all min max limits have been converted to pascals
+// code generated by scripts/parse_variants_table.sh
+static const struct hsc_range_config hsc_range_config[] = {
+ {.name = "001BA", .pmin = 0, .pmax = 100000 },
+ {.name = "1.6BA", .pmin = 0, .pmax = 160000 },
+ {.name = "2.5BA", .pmin = 0, .pmax = 250000 },
+ {.name = "004BA", .pmin = 0, .pmax = 400000 },
+ {.name = "006BA", .pmin = 0, .pmax = 600000 },
+ {.name = "010BA", .pmin = 0, .pmax = 1000000 },
+ {.name = "1.6MD", .pmin = -160, .pmax = 160 },
+ {.name = "2.5MD", .pmin = -250, .pmax = 250 },
+ {.name = "004MD", .pmin = -400, .pmax = 400 },
+ {.name = "006MD", .pmin = -600, .pmax = 600 },
+ {.name = "010MD", .pmin = -1000, .pmax = 1000 },
+ {.name = "016MD", .pmin = -1600, .pmax = 1600 },
+ {.name = "025MD", .pmin = -2500, .pmax = 2500 },
+ {.name = "040MD", .pmin = -4000, .pmax = 4000 },
+ {.name = "060MD", .pmin = -6000, .pmax = 6000 },
+ {.name = "100MD", .pmin = -10000, .pmax = 10000 },
+ {.name = "160MD", .pmin = -16000, .pmax = 16000 },
+ {.name = "250MD", .pmin = -25000, .pmax = 25000 },
+ {.name = "400MD", .pmin = -40000, .pmax = 40000 },
+ {.name = "600MD", .pmin = -60000, .pmax = 60000 },
+ {.name = "001BD", .pmin = -100000, .pmax = 100000 },
+ {.name = "1.6BD", .pmin = -160000, .pmax = 160000 },
+ {.name = "2.5BD", .pmin = -250000, .pmax = 250000 },
+ {.name = "004BD", .pmin = -400000, .pmax = 400000 },
+ {.name = "2.5MG", .pmin = 0, .pmax = 250 },
+ {.name = "004MG", .pmin = 0, .pmax = 400 },
+ {.name = "006MG", .pmin = 0, .pmax = 600 },
+ {.name = "010MG", .pmin = 0, .pmax = 1000 },
+ {.name = "016MG", .pmin = 0, .pmax = 1600 },
+ {.name = "025MG", .pmin = 0, .pmax = 2500 },
+ {.name = "040MG", .pmin = 0, .pmax = 4000 },
+ {.name = "060MG", .pmin = 0, .pmax = 6000 },
+ {.name = "100MG", .pmin = 0, .pmax = 10000 },
+ {.name = "160MG", .pmin = 0, .pmax = 16000 },
+ {.name = "250MG", .pmin = 0, .pmax = 25000 },
+ {.name = "400MG", .pmin = 0, .pmax = 40000 },
+ {.name = "600MG", .pmin = 0, .pmax = 60000 },
+ {.name = "001BG", .pmin = 0, .pmax = 100000 },
+ {.name = "1.6BG", .pmin = 0, .pmax = 160000 },
+ {.name = "2.5BG", .pmin = 0, .pmax = 250000 },
+ {.name = "004BG", .pmin = 0, .pmax = 400000 },
+ {.name = "006BG", .pmin = 0, .pmax = 600000 },
+ {.name = "010BG", .pmin = 0, .pmax = 1000000 },
+ {.name = "100KA", .pmin = 0, .pmax = 100000 },
+ {.name = "160KA", .pmin = 0, .pmax = 160000 },
+ {.name = "250KA", .pmin = 0, .pmax = 250000 },
+ {.name = "400KA", .pmin = 0, .pmax = 400000 },
+ {.name = "600KA", .pmin = 0, .pmax = 600000 },
+ {.name = "001GA", .pmin = 0, .pmax = 1000000 },
+ {.name = "160LD", .pmin = -160, .pmax = 160 },
+ {.name = "250LD", .pmin = -250, .pmax = 250 },
+ {.name = "400LD", .pmin = -400, .pmax = 400 },
+ {.name = "600LD", .pmin = -600, .pmax = 600 },
+ {.name = "001KD", .pmin = -1000, .pmax = 1000 },
+ {.name = "1.6KD", .pmin = -1600, .pmax = 1600 },
+ {.name = "2.5KD", .pmin = -2500, .pmax = 2500 },
+ {.name = "004KD", .pmin = -4000, .pmax = 4000 },
+ {.name = "006KD", .pmin = -6000, .pmax = 6000 },
+ {.name = "010KD", .pmin = -10000, .pmax = 10000 },
+ {.name = "016KD", .pmin = -16000, .pmax = 16000 },
+ {.name = "025KD", .pmin = -25000, .pmax = 25000 },
+ {.name = "040KD", .pmin = -40000, .pmax = 40000 },
+ {.name = "060KD", .pmin = -60000, .pmax = 60000 },
+ {.name = "100KD", .pmin = -100000, .pmax = 100000 },
+ {.name = "160KD", .pmin = -160000, .pmax = 160000 },
+ {.name = "250KD", .pmin = -250000, .pmax = 250000 },
+ {.name = "400KD", .pmin = -400000, .pmax = 400000 },
+ {.name = "250LG", .pmin = 0, .pmax = 250 },
+ {.name = "400LG", .pmin = 0, .pmax = 400 },
+ {.name = "600LG", .pmin = 0, .pmax = 600 },
+ {.name = "001KG", .pmin = 0, .pmax = 1000 },
+ {.name = "1.6KG", .pmin = 0, .pmax = 1600 },
+ {.name = "2.5KG", .pmin = 0, .pmax = 2500 },
+ {.name = "004KG", .pmin = 0, .pmax = 4000 },
+ {.name = "006KG", .pmin = 0, .pmax = 6000 },
+ {.name = "010KG", .pmin = 0, .pmax = 10000 },
+ {.name = "016KG", .pmin = 0, .pmax = 16000 },
+ {.name = "025KG", .pmin = 0, .pmax = 25000 },
+ {.name = "040KG", .pmin = 0, .pmax = 40000 },
+ {.name = "060KG", .pmin = 0, .pmax = 60000 },
+ {.name = "100KG", .pmin = 0, .pmax = 100000 },
+ {.name = "160KG", .pmin = 0, .pmax = 160000 },
+ {.name = "250KG", .pmin = 0, .pmax = 250000 },
+ {.name = "400KG", .pmin = 0, .pmax = 400000 },
+ {.name = "600KG", .pmin = 0, .pmax = 600000 },
+ {.name = "001GG", .pmin = 0, .pmax = 1000000 },
+ {.name = "015PA", .pmin = 0, .pmax = 103425 },
+ {.name = "030PA", .pmin = 0, .pmax = 206850 },
+ {.name = "060PA", .pmin = 0, .pmax = 413700 },
+ {.name = "100PA", .pmin = 0, .pmax = 689500 },
+ {.name = "150PA", .pmin = 0, .pmax = 1034250 },
+ {.name = "0.5ND", .pmin = -125, .pmax = 125 },
+ {.name = "001ND", .pmin = -249, .pmax = 249 },
+ {.name = "002ND", .pmin = -498, .pmax = 498 },
+ {.name = "004ND", .pmin = -996, .pmax = 996 },
+ {.name = "005ND", .pmin = -1245, .pmax = 1245 },
+ {.name = "010ND", .pmin = -2491, .pmax = 2491 },
+ {.name = "020ND", .pmin = -4982, .pmax = 4982 },
+ {.name = "030ND", .pmin = -7473, .pmax = 7473 },
+ {.name = "001PD", .pmin = -6895, .pmax = 6895 },
+ {.name = "005PD", .pmin = -34475, .pmax = 34475 },
+ {.name = "015PD", .pmin = -103425, .pmax = 103425 },
+ {.name = "030PD", .pmin = -206850, .pmax = 206850 },
+ {.name = "060PD", .pmin = -413700, .pmax = 413700 },
+ {.name = "001NG", .pmin = 0, .pmax = 249 },
+ {.name = "002NG", .pmin = 0, .pmax = 498 },
+ {.name = "004NG", .pmin = 0, .pmax = 996 },
+ {.name = "005NG", .pmin = 0, .pmax = 1245 },
+ {.name = "010NG", .pmin = 0, .pmax = 2491 },
+ {.name = "020NG", .pmin = 0, .pmax = 4982 },
+ {.name = "030NG", .pmin = 0, .pmax = 7473 },
+ {.name = "001PG", .pmin = 0, .pmax = 6895 },
+ {.name = "005PG", .pmin = 0, .pmax = 34475 },
+ {.name = "015PG", .pmin = 0, .pmax = 103425 },
+ {.name = "030PG", .pmin = 0, .pmax = 206850 },
+ {.name = "060PG", .pmin = 0, .pmax = 413700 },
+ {.name = "100PG", .pmin = 0, .pmax = 689500 },
+ {.name = "150PG", .pmin = 0, .pmax = 1034250 }
+};
+
+/*
+ * 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
+ */
+static bool hsc_measurement_is_valid(struct hsc_data *data)
+{
+ if (data->buffer[0] & 0xc0)
+ return 0;
+
+ return 1;
+}
+
+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);
+
+ if (ret < 0)
+ return ret;
+
+ ret = chip->valid(data);
+ if (!ret)
+ return -EAGAIN;
+
+ data->is_valid = true;
+
+ return 0;
+}
+
+/*
+ * 4 bytes are read, the dissection looks like
+ *
+ * . 0 . 1 . 2 . 3 . 4 . 5 . 6 . 7 .
+ * byte 0:
+ * | s1 | s0 | b13 | b12 | b11 | b10 | b9 | b8 |
+ * | status | bridge data (pressure) MSB |
+ * byte 1:
+ * | b7 | b6 | b5 | b4 | b3 | b2 | b1 | b0 |
+ * | bridge data (pressure) LSB |
+ * byte 2:
+ * | t10 | t9 | t8 | t7 | t6 | t5 | t4 | t3 |
+ * | temperature data MSB |
+ * byte 3:
+ * | t2 | t1 | t0 | X | X | X | X | X |
+ * | temperature LSB | ignore |
+ *
+ * . 0 . 1 . 2 . 3 . 4 . 5 . 6 . 7 .
+ */
+
+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;
+
+ switch (mask) {
+
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&data->lock);
+ ret = hsc_get_measurement(data);
+ mutex_unlock(&data->lock);
+
+ 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);
+ ret = 0;
+ if (!ret)
+ 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;
+
+ 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;
+}
+
+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)
+ },
+};
+
+static const struct iio_info hsc_info = {
+ .read_raw = hsc_read_raw,
+};
+
+static const struct hsc_chip_data hsc_chip = {
+ .valid = hsc_measurement_is_valid,
+ .channels = hsc_channels,
+ .num_channels = ARRAY_SIZE(hsc_channels),
+};
+
+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;
+ }
+ }
+ 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);
+ 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);
+}
+EXPORT_SYMBOL_NS(hsc_probe, IIO_HONEYWELL_HSC);
+
+void hsc_remove(struct iio_dev *indio_dev)
+{
+ iio_device_unregister(indio_dev);
+}
+EXPORT_SYMBOL_NS(hsc_remove, IIO_HONEYWELL_HSC);
+
+MODULE_AUTHOR("Petre Rodan <[email protected]>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
new file mode 100644
index 000000000000..bc2a4877465b
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <[email protected]>
+ *
+ */
+
+#ifndef _HSC030PA_H
+#define _HSC030PA_H
+
+#define HSC_REG_MEASUREMENT_RD_SIZE 4 // get all conversions in one go since transfers are not address-based
+#define HSC_RANGE_STR_LEN 6
+
+struct hsc_chip_data;
+
+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
+};
+
+enum hsc_func_id {
+ HSC_FUNCTION_A,
+ HSC_FUNCTION_B,
+ HSC_FUNCTION_C,
+ HSC_FUNCTION_F
+};
+
+enum hsc_variant {
+ HSC,
+ SSC
+};
+
+int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
+ const char *name, int type);
+void hsc_remove(struct iio_dev *indio_dev);
+
+#endif
diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
new file mode 100644
index 000000000000..8d3fb174285a
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <[email protected]>
+ *
+ * Datasheet: 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
+ * i2c-related datasheet: 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
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include "hsc030pa.h"
+
+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;
+}
+
+static int hsc_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct iio_dev *indio_dev;
+ struct hsc_data *hsc;
+ const char *range_nom;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+ if (!indio_dev) {
+ dev_err(&client->dev, "Failed to allocate device\n");
+ return -ENOMEM;
+ }
+
+ hsc = iio_priv(indio_dev);
+
+ if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ hsc->xfer = hsc_i2c_xfer;
+ else
+ return -EOPNOTSUPP;
+
+ ret = devm_regulator_get_enable_optional(dev, "vdd");
+ if (ret == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ if (!dev_fwnode(dev))
+ return -EOPNOTSUPP;
+
+ 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");
+
+ 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) {
+ // "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);
+ 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",},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, hsc_i2c_match);
+
+static const struct i2c_device_id hsc_i2c_id[] = {
+ {"hsc", HSC},
+ {"ssc", SSC},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, hsc_i2c_id);
+
+static struct i2c_driver hsc_i2c_driver = {
+ .driver = {
+ .name = "honeywell_hsc",
+ .of_match_table = hsc_i2c_match,
+ },
+ .probe = hsc_i2c_probe,
+ .id_table = hsc_i2c_id,
+};
+
+module_i2c_driver(hsc_i2c_driver);
+
+MODULE_AUTHOR("Petre Rodan <[email protected]>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor i2c driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC);
diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
new file mode 100644
index 000000000000..e7a9b64ac84b
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <[email protected]>
+ *
+ * Datasheet: 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
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+#include "hsc030pa.h"
+
+static int hsc_spi_xfer(struct hsc_data *data)
+{
+ struct spi_transfer xfer = {
+ .tx_buf = NULL,
+ .rx_buf = (char *)&data->buffer,
+ .len = HSC_REG_MEASUREMENT_RD_SIZE,
+ };
+ int ret;
+
+ ret = spi_sync_transfer(data->client, &xfer, 1);
+
+ return ret;
+}
+
+static int hsc_spi_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct hsc_data *hsc;
+ const char *range_nom;
+ int ret;
+ struct device *dev = &spi->dev;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ spi_set_drvdata(spi, indio_dev);
+
+ 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;
+
+ hsc = iio_priv(indio_dev);
+ hsc->xfer = hsc_spi_xfer;
+ hsc->client = spi;
+
+ ret = devm_regulator_get_enable_optional(dev, "vdd");
+ if (ret == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ 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");
+ }
+
+ return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
+ spi_get_device_id(spi)->driver_data);
+}
+
+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);
+
+MODULE_AUTHOR("Petre Rodan <[email protected]>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor spi driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC);
--
2.41.0
Hi Petre,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.7-rc1]
[cannot apply to next-20231117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Petre-Rodan/iio-pressure-driver-for-Honeywell-HSC-SSC-series-pressure-sensors/20231118-072654
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20231117164232.8474-2-petre.rodan%40subdimension.ro
patch subject: [PATCH v2 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
config: riscv-rv32_defconfig (attached as .config)
compiler: riscv32-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/iio/pressure/Kconfig:120: syntax error
drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
drivers/iio/pressure/Kconfig:121: unknown statement "To"
drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:122: unknown statement "called"
make[3]: *** [scripts/kconfig/Makefile:77: oldconfig] Error 1
make[2]: *** [Makefile:685: oldconfig] Error 2
make[1]: *** [Makefile:234: __sub-make] Error 2
make[1]: Target 'oldconfig' not remade because of errors.
make: *** [Makefile:234: __sub-make] Error 2
make: Target 'oldconfig' not remade because of errors.
--
>> drivers/iio/pressure/Kconfig:120: syntax error
drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
drivers/iio/pressure/Kconfig:121: unknown statement "To"
drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:122: unknown statement "called"
make[3]: *** [scripts/kconfig/Makefile:77: olddefconfig] Error 1
make[2]: *** [Makefile:685: olddefconfig] Error 2
make[1]: *** [Makefile:234: __sub-make] Error 2
make[1]: Target 'olddefconfig' not remade because of errors.
make: *** [Makefile:234: __sub-make] Error 2
make: Target 'olddefconfig' not remade because of errors.
--
>> drivers/iio/pressure/Kconfig:120: syntax error
drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
drivers/iio/pressure/Kconfig:121: unknown statement "To"
drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
drivers/iio/pressure/Kconfig:122: unknown statement "called"
make[5]: *** [scripts/kconfig/Makefile:87: defconfig] Error 1
make[4]: *** [Makefile:685: defconfig] Error 2
make[3]: *** [Makefile:350: __build_one_by_one] Error 2
make[3]: Target 'defconfig' not remade because of errors.
make[3]: Target '32-bit.config' not remade because of errors.
make[2]: *** [arch/riscv/Makefile:190: rv32_defconfig] Error 2
make[1]: *** [Makefile:234: __sub-make] Error 2
make[1]: Target 'rv32_defconfig' not remade because of errors.
make: *** [Makefile:234: __sub-make] Error 2
make: Target 'rv32_defconfig' not remade because of errors.
vim +120 drivers/iio/pressure/Kconfig
8
9 config ABP060MG
10 tristate "Honeywell ABP pressure sensor driver"
11 depends on I2C
12 help
13 Say yes here to build support for the Honeywell ABP pressure
14 sensors.
15
16 To compile this driver as a module, choose M here: the module
17 will be called abp060mg.
18
19 config ROHM_BM1390
20 tristate "ROHM BM1390GLV-Z pressure sensor driver"
21 depends on I2C
22 help
23 Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z
24 can measure pressures ranging from 300 hPa to 1300 hPa with
25 configurable measurement averaging and internal FIFO. The
26 sensor does also provide temperature measurements.
27
28 config BMP280
29 tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor driver"
30 depends on (I2C || SPI_MASTER)
31 select REGMAP
32 select BMP280_I2C if (I2C)
33 select BMP280_SPI if (SPI_MASTER)
34 help
35 Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
36 and BMP580 pressure and temperature sensors. Also supports the BME280 with
37 an additional humidity sensor channel.
38
39 To compile this driver as a module, choose M here: the core module
40 will be called bmp280 and you will also get bmp280-i2c for I2C
41 and/or bmp280-spi for SPI support.
42
43 config BMP280_I2C
44 tristate
45 depends on BMP280
46 depends on I2C
47 select REGMAP_I2C
48
49 config BMP280_SPI
50 tristate
51 depends on BMP280
52 depends on SPI_MASTER
53 select REGMAP
54
55 config IIO_CROS_EC_BARO
56 tristate "ChromeOS EC Barometer Sensor"
57 depends on IIO_CROS_EC_SENSORS_CORE
58 help
59 Say yes here to build support for the Barometer sensor when
60 presented by the ChromeOS EC Sensor hub.
61
62 To compile this driver as a module, choose M here: the module
63 will be called cros_ec_baro.
64
65 config DLHL60D
66 tristate "All Sensors DLHL60D and DLHL60G low voltage digital pressure sensors"
67 depends on I2C
68 select IIO_BUFFER
69 select IIO_TRIGGERED_BUFFER
70 help
71 Say yes here to build support for the All Sensors DLH series
72 pressure sensors driver.
73
74 To compile this driver as a module, choose M here: the module
75 will be called dlhl60d.
76
77 config DPS310
78 tristate "Infineon DPS310 pressure and temperature sensor"
79 depends on I2C
80 select REGMAP_I2C
81 help
82 Support for the Infineon DPS310 digital barometric pressure sensor.
83 It can be accessed over I2C bus.
84
85 This driver can also be built as a module. If so, the module will be
86 called dps310.
87
88 config HID_SENSOR_PRESS
89 depends on HID_SENSOR_HUB
90 select IIO_BUFFER
91 select HID_SENSOR_IIO_COMMON
92 select HID_SENSOR_IIO_TRIGGER
93 tristate "HID PRESS"
94 help
95 Say yes here to build support for the HID SENSOR
96 Pressure driver
97
98 To compile this driver as a module, choose M here: the module
99 will be called hid-sensor-press.
100
101 config HP03
102 tristate "Hope RF HP03 temperature and pressure sensor driver"
103 depends on I2C
104 select REGMAP_I2C
105 help
106 Say yes here to build support for Hope RF HP03 pressure and
107 temperature sensor.
108
109 To compile this driver as a module, choose M here: the module
110 will be called hp03.
111
112 config HSC030PA
113 tristate "Honeywell HSC/SSC (TruStability pressure sensors series)"
114 depends on (I2C || SPI_MASTER)
115 select HSC030PA_I2C if (I2C)
116 select HSC030PA_SPI if (SPI_MASTER)
117 help
118 Say Y here to build support for the Honeywell HSC and SSC TruStability
119 pressure and temperature sensor series.
> 120
121 To compile this driver as a module, choose M here: the module will be
122 called hsc030pa.
123
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 <[email protected]>
...
> +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 <[email protected]>
> + *
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
hello,
first of all, thank you for the code review.
in the interest of brevity I will skip all comments where I simply remove the block, blankline, or fix indentation.
On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
> > + select HSC030PA_I2C if (I2C)
> > + select HSC030PA_SPI if (SPI_MASTER)
>
> Unneeded parentheses
ack
> > + 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.
fixed indentation and now it compiles fine with git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
sorry, what is 'LKP' in this context and how do I reproduce?
> > +#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.
'#include <linux/array_size.h>' is a weird one.
$ cd /usr/src/linux/drivers
$ grep -r ARRAY_SIZE * | grep '\.c:' | wc -l
32396
$ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | wc -l
11
$ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | grep -v '^pinctrl' | wc -l
0
plus on a 6.1 version kernel, `make modules` actually reports that the header can't be found if I include it. can't comprehend that.
so I'll be skipping that particular include.
> > +// 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)
not fit for this purpose, sorry.
> > +/*
> > + * 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.
ok, I changed all comments to /* */.
this particular block was rewritten (the legend is taken from honeywell's i2c-related datasheet).
> > +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.
rewritten as a one-liner, without GENMASK.
> return !(buffer[0] & GENMASK(3, 2));
>
> (Note, you will need bits.h for this.)
>
> > +}
>
...
> > + 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
ack
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&data->lock);
> > + ret = hsc_get_measurement(data);
> > + mutex_unlock(&data->lock);
>
> Use guard() operator from cleanup.h.
I'm not familiar with that, for the time being I'll stick to mutex_lock/unlock if you don't mind.
> > + 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.
the raw conversion data is spread over 4 bytes and interlaced with other info (see comment above the function).
I'm just cherry-picking the bits I'm interested in, in a way my brain can understand what is going on.
> > + ret = 0;
> > + if (!ret)
>
> lol
I should leave that in for comic relief. missed it after a lot of code changes.
> > + 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.
ack.
> > +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...
I blame `indent -linux --line-length 80` for these and weirdly-spaced pointer declarations.
are you using something else?
> > +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() ?
match_string() is case-sensitive and operates on string arrays, so unfit for this purpose.
> > +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.
ack. switched to kernel-doc format in multiple places.
> > +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.
ack
> > +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?
the communication is one-way as in the sensors do not expect anything except 4 bytes-worth of clock signals per 'packet' for both the i2c and spi versions.
regmap is suited to sensors with an actual memory map.
> > +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.
oops, I was hoping my 6.1.38 kernel is using the same API as 6.7.0
fixed.
> > + 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.
ack
>
> > + }
> > +
> > + 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 ...
ack
> > + 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.
since I'm unable to test this I'd rather remove the block altogether.
if I go the ENODEV route my module will never load since I can't see any vdd-supply support on my devboard.
> > + if (!dev_fwnode(dev))
> > + return -EOPNOTSUPP;
>
> Why do you need this?
> And why this error code?
it's intentional.
this module has a hard requirement on the correct parameters (transfer function and pressure range) being provided in the devicetree.
without those I don't want to provide any measurements since there can't be a default transfer function and pressure range for a generic driver that supports an infinite combination of those.
echo hsc030pa 0x28 > /sys/bus/i2c/devices/i2c-0/new_device
I want iio_info to detect 0 devices.
> > + 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.)
using directly and moved to main probe() file.
> > +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.
re-written based on bindings thread.
> > + spi_set_drvdata(spi, indio_dev);
>
> How is this being used?
removed.
> > + 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?
moved 800kHz max requirement to the bindings file.
> > + ret = device_property_read_u32(dev,
> > + "honeywell,transfer-function",
> > + &hsc->function);
..
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "honeywell,pmax-pascal could not be read\n");
> > + }
>
> Ditto. Why is this duplication?
you're right, moved to main probe()
> --
> With Best Regards,
> Andy Shevchenko
patches are ready, but awaiting any aditional feedback to this message.
thanks again,
peter
--
petre rodan
On Wed, Nov 22, 2023 at 08:08:27AM +0200, Petre Rodan wrote:
> On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
...
> sorry, what is 'LKP' in this context and how do I reproduce?
It's an acronym for CI system run by Intel. You should have had an email in
your mailbox with complains. It also duplicates them to a mailing list which
address I don't know by heart.
...
> > Also there are missing at least these ones: array_size.h, types.h.
>
> '#include <linux/array_size.h>' is a weird one.
Why?
> $ cd /usr/src/linux/drivers
> $ grep -r ARRAY_SIZE * | grep '\.c:' | wc -l
> 32396
> $ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | wc -l
> 11
> $ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | grep -v '^pinctrl' | wc -l
> 0
Hint, use `git grep ...` which much, much faster against the Git indexed data.
> plus on a 6.1 version kernel, `make modules` actually reports that the header
> can't be found if I include it. can't comprehend that. so I'll be skipping
> that particular include.
No, the new code is always should be submitted against latest release cycle,
v6.7-rcX as of today. There is the header. Please, use it.
...
> > Can you utilize linear ranges data types and APIs? (linear_range.h)
>
> not fit for this purpose, sorry.
NP.
...
> > > + if (data->buffer[0] & 0xc0)
> > > + return 0;
> > > +
> > > + return 1;
> >
> > You use bool and return integers.
> >
> > Besides, it can be just a oneliner.
>
> rewritten as a one-liner, without GENMASK.
>
> > return !(buffer[0] & GENMASK(3, 2));
> >
> > (Note, you will need bits.h for this.)
> >
> > > +}
Why no GENMASK() ? What the meaning of the 0xc0?
Ideally it should be
#define ...meaningful name... GENMASK()
...
> > > + mutex_lock(&data->lock);
> > > + ret = hsc_get_measurement(data);
> > > + mutex_unlock(&data->lock);
> >
> > Use guard() operator from cleanup.h.
>
> I'm not familiar with that, for the time being I'll stick to
> mutex_lock/unlock if you don't mind.
I do mind. RAII is a method to make code more robust against forgotten
unlock/free calls.
...
> > > + 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.
>
> the raw conversion data is spread over 4 bytes and interlaced with other info
> (see comment above the function). I'm just cherry-picking the bits I'm
> interested in, in a way my brain can understand what is going on.
So, perhaps you need to use get_unaligned_.e32() and then FIELD_*() from
bitfield.h. This will be much better in terms of understanding the semantics
of these magic bit shifts and masks.
...
> > > + ret = 0;
> > > + if (!ret)
> >
> > lol
>
> I should leave that in for comic relief. missed it after a lot of code
> changes.
I understand, that's why no shame on you, just fun code to see :-)
...
> > Strange indentation of }:s...
>
> I blame `indent -linux --line-length 80` for these and weirdly-spaced pointer
> declarations. are you using something else?
Some maintainers suggest to use clang-format. I find it weird in some corner
cases. So, I would suggest to use it and reread the code and fix some
strangenesses.
...
> > > + 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() ?
>
> match_string() is case-sensitive and operates on string arrays, so unfit for
> this purpose.
Let's put it this way: Why do you care of the relaxed case?
I.o.w. why can we be slightly stricter?
...
> > Can you use regmap I2C?
>
> the communication is one-way as in the sensors do not expect anything except
> 4 bytes-worth of clock signals per 'packet' for both the i2c and spi
> versions. regmap is suited to sensors with an actual memory map.
If not yet, worse to add in the comment area of the patch
(after the cutter '---' line).
...
> > No use of this function prototype, we have a new one.
>
> oops, I was hoping my 6.1.38 kernel is using the same API as 6.7.0
> fixed.
Same way with a (new) header :-)
...
> > > + 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.
>
> since I'm unable to test this I'd rather remove the block altogether.
> if I go the ENODEV route my module will never load since I can't see any
> vdd-supply support on my devboard.
No, what I meant is to have something like
if (ret) {
if (ret != -ENODEV)
return ret;
...regulator is not present...
}
This is how it's being used in dozens of places in the kernel. Just utilize
`git grep ...` which should be a top-10 tool for the Linux kernel developer.
Q: ...
A: Try `git grep ...` to find your answer in the existing code.
...
> > > + if (!dev_fwnode(dev))
> > > + return -EOPNOTSUPP;
> >
> > Why do you need this?
> > And why this error code?
>
> it's intentional.
> this module has a hard requirement on the correct parameters (transfer
> function and pressure range) being provided in the devicetree. without those
> I don't want to provide any measurements since there can't be a default
> transfer function and pressure range for a generic driver that supports an
> infinite combination of those.
>
> echo hsc030pa 0x28 > /sys/bus/i2c/devices/i2c-0/new_device
> I want iio_info to detect 0 devices.
So, fine, but the very first mandatory property check will fail as it has
the very same check inside. So, why do you need a double check?
--
With Best Regards,
Andy Shevchenko
On Wed, 22 Nov 2023 08:08:27 +0200
Petre Rodan <[email protected]> wrote:
> hello,
>
> first of all, thank you for the code review.
> in the interest of brevity I will skip all comments where I simply remove the block, blankline, or fix indentation.
>
> On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
> > > + select HSC030PA_I2C if (I2C)
> > > + select HSC030PA_SPI if (SPI_MASTER)
> >
> > Unneeded parentheses
>
> ack
Where you agree, just crop it out. Saves on scrolling!
> > > + case IIO_CHAN_INFO_RAW:
> > > + mutex_lock(&data->lock);
> > > + ret = hsc_get_measurement(data);
> > > + mutex_unlock(&data->lock);
> >
> > Use guard() operator from cleanup.h.
>
> I'm not familiar with that, for the time being I'll stick to mutex_lock/unlock if you don't mind.
>
It's simple and worth taking a look for new drivers as it makes some error paths much much simpler.
I'm sitting on a big set that applies it to quite few IIO drivers.
> > > + 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.
>
> since I'm unable to test this I'd rather remove the block altogether.
> if I go the ENODEV route my module will never load since I can't see any vdd-supply support on my devboard.
Problem here is why do you think that regulator is optional? Does your device
work with out power? What is optional is whether the regulator is fixed and
on and hence doesn't need to be in DT or whether it is specified there.
That's unconnected to the enabling in driver.
The call you have here is for when the power supply really is optional.
That is the driver does something different if nothing is supplied on the pin.
Typically this is used when we have option of either an internal reference voltage
or supplying an external one. The absence on an external one means we fallback
to only enabling the internal one.
Jonathan
>
> > > > + 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.
> >
> > since I'm unable to test this I'd rather remove the block altogether.
> > if I go the ENODEV route my module will never load since I can't see any
> > vdd-supply support on my devboard.
>
> No, what I meant is to have something like
>
> if (ret) {
> if (ret != -ENODEV)
> return ret;
> ...regulator is not present...
> }
>
> This is how it's being used in dozens of places in the kernel. Just utilize
> `git grep ...` which should be a top-10 tool for the Linux kernel developer.
As per my very late reply to previous email. Nope. This regulator is never
not present. It's just a question of whether the firmware tells us what
it is, or it is supplied with a stub regulator.