[PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Petre Rodan]

Adds driver for digital Honeywell TruStability HSC and SSC series
pressure and temperature sensors. 
Communication is one way. The sensor only requires 4 bytes worth of
clock signals on both i2c and spi in order to push the data out.
The i2c address is hardcoded and depends on the part number.
There is no additional GPIO control.

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 [HSC]
Datasheet:
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 [SSC]
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
V2: - No change
V3: - huge cleanup based on Andy's review. Thanks!
    - fixed pressure offset calculation for differential sensors
    - rename driver from honeywell,hsc to honeywell,hsc030pa
    - main probe() now uses strncmp for case-insensitive mathing
      can't use match_string() because I ain't planning on splitting the
      hsc_range_config struct.
---
 MAINTAINERS                         |   7 +
 drivers/iio/pressure/Kconfig        |  22 ++
 drivers/iio/pressure/Makefile       |   3 +
 drivers/iio/pressure/hsc030pa.c     | 414 ++++++++++++++++++++++++++++
 drivers/iio/pressure/hsc030pa.h     |  78 ++++++
 drivers/iio/pressure/hsc030pa_i2c.c |  81 ++++++
 drivers/iio/pressure/hsc030pa_spi.c |  71 +++++
 7 files changed, 676 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 509281e9e169..3ee65cda2d72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9702,6 +9702,13 @@ F:	lib/test_hmm*
 F:	mm/hmm*
 F:	tools/testing/selftests/mm/*hmm*
 
+HONEYWELL HSC030PA PRESSURE SENSOR SERIES IIO DRIVER
+M:	Petre Rodan <petre.rodan@subdimension.ro>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
+F:	drivers/iio/pressure/hsc030pa*
+
 HONEYWELL MPRLS0025PA PRESSURE SENSOR SERIES IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 95efa32e4289..79adfd059c3a 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 sensor 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 TruStability
+	  HSC and SSC 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..a118d27e4342
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -0,0 +1,414 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * 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/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/init.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/printk.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#include "hsc030pa.h"
+
+#define     HSC_PRESSURE_TRIPLET_LEN  6
+#define              HSC_STATUS_MASK  GENMASK(7, 6)
+#define         HSC_TEMPERATURE_MASK  GENMASK(15, 5)
+#define            HSC_PRESSURE_MASK  GENMASK(29, 16)
+
+struct hsc_func_spec {
+	u32 output_min;
+	u32 output_max;
+};
+
+/**
+ * function A: 10% - 90% of 2^14
+ * function B:  5% - 95% of 2^14
+ * function C:  5% - 85% of 2^14
+ * function F:  4% - 94% of 2^14
+ */
+static const struct hsc_func_spec hsc_func_spec[] = {
+	[HSC_FUNCTION_A] = {.output_min = 1638, .output_max = 14746},
+	[HSC_FUNCTION_B] = {.output_min =  819, .output_max = 15565},
+	[HSC_FUNCTION_C] = {.output_min =  819, .output_max = 13926},
+	[HSC_FUNCTION_F] = {.output_min =  655, .output_max = 15401},
+};
+
+/**
+ * struct hsc_range_config - pressure ranges based on the nomenclature
+ * @name: 5-char string triplet that defines the range, measurement unit
+ *        and type
+ * @pmin: lowest pressure that can be measured
+ * @pmax: highest pressure that can be measured
+ */
+struct hsc_range_config {
+	char name[HSC_PRESSURE_TRIPLET_LEN];
+	s32 pmin;
+	u32 pmax;
+};
+
+/* all min max limits have been converted to pascals */
+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 =  103421 },
+	{.name = "030PA", .pmin =       0, .pmax =  206843 },
+	{.name = "060PA", .pmin =       0, .pmax =  413685 },
+	{.name = "100PA", .pmin =       0, .pmax =  689476 },
+	{.name = "150PA", .pmin =       0, .pmax = 1034214 },
+	{.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 =  -34474, .pmax =   34474 },
+	{.name = "015PD", .pmin = -103421, .pmax =  103421 },
+	{.name = "030PD", .pmin = -206843, .pmax =  206843 },
+	{.name = "060PD", .pmin = -413685, .pmax =  413685 },
+	{.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 =   34474 },
+	{.name = "015PG", .pmin =       0, .pmax =  103421 },
+	{.name = "030PG", .pmin =       0, .pmax =  206843 },
+	{.name = "060PG", .pmin =       0, .pmax =  413685 },
+	{.name = "100PG", .pmin =       0, .pmax =  689476 },
+	{.name = "150PG", .pmin =       0, .pmax = 1034214 }
+};
+
+/*
+ * hsc_measurement_is_valid() - validate last conversion via status bits
+ * @data: structure containing instantiated sensor data
+ * Return: true only if both status bits are zero
+ *
+ * the two MSB from the first transfered byte contain a status code
+ *   00 - normal operation, valid data
+ *   01 - device in factory programming mode
+ *   10 - stale data
+ *   11 - diagnostic condition
+ */
+static bool hsc_measurement_is_valid(struct hsc_data *data)
+{
+	return !(data->buffer[0] & HSC_STATUS_MASK);
+}
+
+static int hsc_get_measurement(struct hsc_data *data)
+{
+	const struct hsc_chip_data *chip = data->chip;
+	int ret;
+
+	guard(mutex)(&data->lock);
+	ret = data->xfer(data);
+	if (ret < 0)
+		return ret;
+
+	data->is_valid = chip->valid(data);
+	if (!data->is_valid)
+		return -EAGAIN;
+
+	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;
+	u32 xfer;
+	int raw;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = hsc_get_measurement(data);
+		if (ret)
+			return ret;
+
+		xfer = get_unaligned_be32(data->buffer);
+		switch (channel->type) {
+		case IIO_PRESSURE:
+			raw = FIELD_GET(HSC_PRESSURE_MASK, xfer);
+			*val = raw;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			raw = FIELD_GET(HSC_TEMPERATURE_MASK, xfer);
+			*val = raw;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+
+/**
+ * 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 - Omin) * Q + Pmin =
+ *          ((conv - Omin) + Pmin/Q) * Q
+ *  =>
+ *  scale = Q = (Pmax - Pmin) / (Omax - Omin)
+ *  offset = Pmin/Q - Omin = Pmin * (Omax - Omin) / (Pmax - Pmin) - Omin
+ */
+
+	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_dec;
+			return IIO_VAL_INT_PLUS_NANO;
+		default:
+			return -EINVAL;
+		}
+
+	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_dec;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;
+}
+
+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;
+	const char *triplet;
+	u64 tmp;
+	int index;
+	int found = 0;
+	int ret;
+
+	hsc = iio_priv(indio_dev);
+	hsc->chip = &hsc_chip;
+
+	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,pressure-triplet", &triplet);
+	if (ret)
+		return dev_err_probe(dev, ret,
+			"honeywell,pressure-triplet could not be read\n");
+
+	if (strncmp(triplet, "NA", 2) == 0) {
+		// "not available" in the nomenclature
+		// we got a custom-range chip so extract pmin, pmax from dt
+		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");
+	} else {
+		// chip should be defined in the nomenclature
+		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
+			if (strncmp(hsc_range_config[index].name,
+					triplet,
+					HSC_PRESSURE_TRIPLET_LEN - 1) == 0) {
+				hsc->pmin = hsc_range_config[index].pmin;
+				hsc->pmax = hsc_range_config[index].pmax;
+				found = 1;
+				break;
+			}
+		}
+		if (hsc->pmin == hsc->pmax || !found)
+			return dev_err_probe(dev, -EINVAL,
+				"honeywell,pressure-triplet is invalid\n");
+	}
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "can't get vdd supply\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 Pa * 1000 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_dec);
+	tmp = div_s64(((s64)hsc->pmin * (s64)(hsc->outmax - hsc->outmin)) *
+		      MICRO, hsc->pmax - hsc->pmin);
+	tmp -= (s64)hsc->outmin * MICRO;
+	hsc->p_offset = div_s64_rem(tmp, MICRO, &hsc->p_offset_dec);
+
+	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_HSC030PA);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+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..cf1674d36485
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ */
+
+#ifndef _HSC030PA_H
+#define _HSC030PA_H
+
+#include <linux/property.h>
+#include <linux/types.h>
+
+/**
+ * get all conversions (4 bytes) in one go
+ * since transfers are not address-based
+*/
+#define  HSC_REG_MEASUREMENT_RD_SIZE  4
+
+struct device;
+
+struct iio_chan_spec;
+struct iio_dev;
+
+struct hsc_chip_data;
+
+/**
+ * struct hsc_data
+ * @client: either i2c or spi kernel interface struct for current dev
+ * @lock: lock protecting chip reads
+ * @xfer: function that implements the chip reads
+ * @is_valid: false if last transfer has failed
+ * @buffer: raw conversion data
+ * @pmin: minimum measurable pressure limit
+ * @pmax: maximum measurable pressure limit
+ * @outmin: minimum raw pressure in counts (based on transfer function)
+ * @outmax: maximum raw pressure in counts (based on transfer function)
+ * @function: transfer function
+ * @p_scale: pressure scale
+ * @p_scale_dec: pressure scale, decimal places
+ * @p_offset: pressure offset
+ * @p_offset_dec: pressure offset, decimal places
+ */
+struct hsc_data {
+	void *client;
+	const struct hsc_chip_data *chip;
+	struct mutex lock;
+	int (*xfer)(struct hsc_data *data);
+	bool is_valid;
+	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];
+	s32 pmin;
+	s32 pmax;
+	u32 outmin;
+	u32 outmax;
+	u32 function;
+	s64 p_scale;
+	s32 p_scale_dec;
+	s64 p_offset;
+	s32 p_offset_dec;
+};
+
+struct hsc_chip_data {
+	bool (*valid)(struct hsc_data *data);
+	const struct iio_chan_spec *channels;
+	u8 num_channels;
+};
+
+enum hsc_func_id {
+	HSC_FUNCTION_A,
+	HSC_FUNCTION_B,
+	HSC_FUNCTION_C,
+	HSC_FUNCTION_F,
+};
+
+int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
+	      const char *name, int type);
+
+#endif
diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
new file mode 100644
index 000000000000..4132db4e356a
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * 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 [hsc]
+ * 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 [i2c related]
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.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)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct hsc_data *hsc;
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	hsc = iio_priv(indio_dev);
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	hsc->xfer = hsc_i2c_xfer;
+	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,hsc030pa"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, hsc_i2c_match);
+
+static const struct i2c_device_id hsc_i2c_id[] = {
+	{"hsc030pa"},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, hsc_i2c_id);
+
+static struct i2c_driver hsc_i2c_driver = {
+	.driver = {
+		.name = "hsc030pa",
+		.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 <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor i2c driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC030PA);
diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
new file mode 100644
index 000000000000..d99688a65f04
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * 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 "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,
+	};
+
+	return spi_sync_transfer(data->client, &xfer, 1);
+}
+
+static int hsc_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct hsc_data *hsc;
+	struct device *dev = &spi->dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	hsc = iio_priv(indio_dev);
+	hsc->xfer = hsc_spi_xfer;
+	hsc->client = spi;
+
+	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,hsc030pa",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, hsc_spi_match);
+
+static const struct spi_device_id hsc_spi_id[] = {
+	{"hsc030pa"},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, hsc_spi_id);
+
+static struct spi_driver hsc_spi_driver = {
+	.driver = {
+		.name = "hsc030pa",
+		.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 <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor spi driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC030PA);
-- 
2.41.0

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[kernel test robot]

Hi Petre,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.7-rc2 next-20231124]
[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/20231126-182901
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231126102721.15322-1-petre.rodan%40subdimension.ro
patch subject: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20231126/202311262123.aVuN3lZS-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231126/202311262123.aVuN3lZS-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311262123.aVuN3lZS-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/pressure/hsc030pa.c:48: warning: cannot understand function prototype: 'const struct hsc_func_spec hsc_func_spec[] = '
>> drivers/iio/pressure/hsc030pa.c:253: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * IIO ABI expects


vim +48 drivers/iio/pressure/hsc030pa.c

    41	
    42	/**
    43	 * function A: 10% - 90% of 2^14
    44	 * function B:  5% - 95% of 2^14
    45	 * function C:  5% - 85% of 2^14
    46	 * function F:  4% - 94% of 2^14
    47	 */
  > 48	static const struct hsc_func_spec hsc_func_spec[] = {
    49		[HSC_FUNCTION_A] = {.output_min = 1638, .output_max = 14746},
    50		[HSC_FUNCTION_B] = {.output_min =  819, .output_max = 15565},
    51		[HSC_FUNCTION_C] = {.output_min =  819, .output_max = 13926},
    52		[HSC_FUNCTION_F] = {.output_min =  655, .output_max = 15401},
    53	};
    54	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Jonathan Cameron]

On Sun, 26 Nov 2023 12:27:17 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Adds driver for digital Honeywell TruStability HSC and SSC series
> pressure and temperature sensors. 
> Communication is one way. The sensor only requires 4 bytes worth of
> clock signals on both i2c and spi in order to push the data out.
> The i2c address is hardcoded and depends on the part number.
> There is no additional GPIO control.
> 
> 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 [HSC]
> Datasheet:
> 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 [SSC]
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Hi Petre

A quick end of day review.

Jonathan

> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
> new file mode 100644
> index 000000000000..a118d27e4342
> --- /dev/null
> +++ b/drivers/iio/pressure/hsc030pa.c
> @@ -0,0 +1,414 @@
...

> +
> +#include "hsc030pa.h"
> +
> +#define     HSC_PRESSURE_TRIPLET_LEN  6

Can you make this length based on something like a structure length, or number
of registers?  That would make it self documenting which is always nice to have.

> +#define              HSC_STATUS_MASK  GENMASK(7, 6)
> +#define         HSC_TEMPERATURE_MASK  GENMASK(15, 5)
> +#define            HSC_PRESSURE_MASK  GENMASK(29, 16)

Unusual indenting.  Don't do this.  Just use a single space
as it's much less noise as a driver gets modified over time.

> +
> +struct hsc_func_spec {
> +	u32 output_min;
> +	u32 output_max;
> +};
> +
> +/**
> + * function A: 10% - 90% of 2^14
> + * function B:  5% - 95% of 2^14
> + * function C:  5% - 85% of 2^14
> + * function F:  4% - 94% of 2^14
> + */
> +static const struct hsc_func_spec hsc_func_spec[] = {
> +	[HSC_FUNCTION_A] = {.output_min = 1638, .output_max = 14746},
> +	[HSC_FUNCTION_B] = {.output_min =  819, .output_max = 15565},
> +	[HSC_FUNCTION_C] = {.output_min =  819, .output_max = 13926},
> +	[HSC_FUNCTION_F] = {.output_min =  655, .output_max = 15401},
Space after { and before }
We need a consistent style, and that's my preferred one for IIO.

> +};

> +/* all min max limits have been converted to pascals */
> +static const struct hsc_range_config hsc_range_config[] = {
> +	{.name = "001BA", .pmin =       0, .pmax =  100000 },

space after { in all these.


> +};
>
> +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;
> +	u32 xfer;
> +	int raw;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = hsc_get_measurement(data);
> +		if (ret)
> +			return ret;
> +
> +		xfer = get_unaligned_be32(data->buffer);
> +		switch (channel->type) {
> +		case IIO_PRESSURE:
> +			raw = FIELD_GET(HSC_PRESSURE_MASK, xfer);
> +			*val = raw;
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			raw = FIELD_GET(HSC_TEMPERATURE_MASK, xfer);
> +			*val = raw;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +/**
Not kernel-doc so /* only.

> + * 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 - Omin) * Q + Pmin =
> + *          ((conv - Omin) + Pmin/Q) * Q
> + *  =>
> + *  scale = Q = (Pmax - Pmin) / (Omax - Omin)
> + *  offset = Pmin/Q - Omin = Pmin * (Omax - Omin) / (Pmax - Pmin) - Omin
> + */
> +
> +	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_dec;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	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_dec;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return -EINVAL;
If you can get to a final return after a case statement you are doing
it wrong. Add a default in the switch - makes it explicit that other
cases are errors rather than having to look down here.


> +}

> +
> +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) },
Format this like every other driver does it.

	{
		.type = IIO_TEMP,
		.info_mask_....
	},
	{
		...
	},

etc
> +};

> +
> +int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
> +	      const char *name, int type)
> +{
> +	struct hsc_data *hsc;
> +	const char *triplet;
> +	u64 tmp;
> +	int index;
> +	int found = 0;
> +	int ret;
> +
> +	hsc = iio_priv(indio_dev);
> +	hsc->chip = &hsc_chip;
> +
> +	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,pressure-triplet", &triplet);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"honeywell,pressure-triplet could not be read\n");
> +
> +	if (strncmp(triplet, "NA", 2) == 0) {
> +		// "not available" in the nomenclature
> +		// we got a custom-range chip so extract pmin, pmax from dt
> +		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");
> +	} else {
> +		// chip should be defined in the nomenclature

All comments in IIO use /* */ syntax.

> +		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> +			if (strncmp(hsc_range_config[index].name,
> +					triplet,
> +					HSC_PRESSURE_TRIPLET_LEN - 1) == 0) {
> +				hsc->pmin = hsc_range_config[index].pmin;
> +				hsc->pmax = hsc_range_config[index].pmax;
> +				found = 1;
> +				break;
> +			}
> +		}
> +		if (hsc->pmin == hsc->pmax || !found)
> +			return dev_err_probe(dev, -EINVAL,
> +				"honeywell,pressure-triplet is invalid\n");
> +	}

> diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
> new file mode 100644
> index 000000000000..cf1674d36485
> --- /dev/null
> +++ b/drivers/iio/pressure/hsc030pa.h
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Honeywell TruStability HSC Series pressure/temperature sensor
> + *
> + * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
> + */
> +
> +#ifndef _HSC030PA_H
> +#define _HSC030PA_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +/**
> + * get all conversions (4 bytes) in one go
> + * since transfers are not address-based
> +*/
> +#define  HSC_REG_MEASUREMENT_RD_SIZE  4
> +
> +struct device;
> +
> +struct iio_chan_spec;
> +struct iio_dev;
> +
> +struct hsc_chip_data;
> +
> +/**
> + * struct hsc_data
> + * @client: either i2c or spi kernel interface struct for current dev
Docs don't match structure

> + * @lock: lock protecting chip reads
> + * @xfer: function that implements the chip reads
> + * @is_valid: false if last transfer has failed
> + * @buffer: raw conversion data
> + * @pmin: minimum measurable pressure limit
> + * @pmax: maximum measurable pressure limit
> + * @outmin: minimum raw pressure in counts (based on transfer function)
> + * @outmax: maximum raw pressure in counts (based on transfer function)
> + * @function: transfer function
> + * @p_scale: pressure scale
> + * @p_scale_dec: pressure scale, decimal places
> + * @p_offset: pressure offset
> + * @p_offset_dec: pressure offset, decimal places
> + */
> +struct hsc_data {
> +	void *client;
> +	const struct hsc_chip_data *chip;
> +	struct mutex lock;
> +	int (*xfer)(struct hsc_data *data);
> +	bool is_valid;
> +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];

This is used for SPI transfers so should be DMA safe. It's not currently.
Look at how IIO_DMA_MINALIGN is used in other drivers to ensure there is
no unsafe sharing of cachelines.

On some architectures this is fixed by the stuff that bounces all small transfers
but I don't think that is universal yet.  If you want more info find the talk
by Wolfram Sang from a few years ago an ELCE on I2C DMA safe buffers.

> +	s32 pmin;
> +	s32 pmax;
> +	u32 outmin;
> +	u32 outmax;
> +	u32 function;
> +	s64 p_scale;
> +	s32 p_scale_dec;
> +	s64 p_offset;
> +	s32 p_offset_dec;
> +};

> diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
> new file mode 100644
> index 000000000000..4132db4e356a
> --- /dev/null
> +++ b/drivers/iio/pressure/hsc030pa_i2c.c
> @@ -0,0 +1,81 @@

Very similar comments to spi file, so I haven't repeated them.

> diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
> new file mode 100644
> index 000000000000..d99688a65f04
> --- /dev/null
> +++ b/drivers/iio/pressure/hsc030pa_spi.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Honeywell TruStability HSC Series pressure/temperature sensor
> + *
> + * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
> + *
> + * 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 "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,
> +	};
> +
> +	return spi_sync_transfer(data->client, &xfer, 1);
> +}
> +
> +static int hsc_spi_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct hsc_data *hsc;
> +	struct device *dev = &spi->dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	hsc = iio_priv(indio_dev);
> +	hsc->xfer = hsc_spi_xfer;

If it's read only I'd name it to reflect that rather than xfer which
implies both ways.

Also, pass the callback and spi->dev into hsc probe. Easy to use
a container_of() to get back to the struct spi_device *spi

That should let you do the iio_dev allocation etc all inside the generic code
and keep this bus driver to just the bits that are bus specific.

> +	hsc->client = spi;
> +
> +	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
> +			 spi_get_device_id(spi)->driver_data);
Don't use anything form spi_get_device_id()

Name is a fixed string currently so pass that directly.
For driver data, there isn't any yet but if there were use
spi_get_device_match_data() and make sure to provide the data in all the
id tables.  That function will search the firmware ones first then call
back to the spi specific varient.

> +}
> +

> +static struct spi_driver hsc_spi_driver = {
> +	.driver = {
> +		.name = "hsc030pa",
> +		.of_match_table = hsc_spi_match,
> +		},

alignment unusual here.
	.driver = {
		.name = "..",
		.of_match_table = ...
	},
is most common form.

> +	.probe = hsc_spi_probe,
> +	.id_table = hsc_spi_id,
> +};
> +module_spi_driver(hsc_spi_driver);
> +
> +MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
> +MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor spi driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_HONEYWELL_HSC030PA);

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Petre Rodan]

Hello!

On Sun, Nov 26, 2023 at 06:33:34PM +0000, Jonathan Cameron wrote:
> On Sun, 26 Nov 2023 12:27:17 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Adds driver for digital Honeywell TruStability HSC and SSC series
> > pressure and temperature sensors. 
>
> Hi Petre
> 
> A quick end of day review.
> 
> Jonathan

welcome back.
amazing how you were able to review so many code sets in one day.
thank you for your input.

> > +#define     HSC_PRESSURE_TRIPLET_LEN  6
> 
> Can you make this length based on something like a structure length, or number
> of registers?  That would make it self documenting which is always nice to have.

I added a comment in V4, this length is simply based on the string used by
honeywell to differentiate these chips based on their pressura range, 
measurement unit and sensor type. see the first column in Table 8, 9, 10 in [1]

[1] 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

> > +struct hsc_data {
> > +	void *client;
> > +	const struct hsc_chip_data *chip;
> > +	struct mutex lock;
> > +	int (*xfer)(struct hsc_data *data);
> > +	bool is_valid;
> > +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];
> 
> This is used for SPI transfers so should be DMA safe. It's not currently.
> Look at how IIO_DMA_MINALIGN is used in other drivers to ensure there is
> no unsafe sharing of cachelines.
> 
> On some architectures this is fixed by the stuff that bounces all small transfers
> but I don't think that is universal yet.  If you want more info find the talk
> by Wolfram Sang from a few years ago an ELCE on I2C DMA safe buffers.

that was a nice rabbit hole, thanks for the pointer.

now, based on [2] I will skip explicit i2c dma-related code since my requests
are 4 bytes long. according to the document, any i2c xfer below 8bytes is not
worth the overhead.

[2] https://www.kernel.org/doc/html/latest/i2c/dma-considerations.html

> > +static int hsc_spi_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct hsc_data *hsc;
> > +	struct device *dev = &spi->dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	hsc = iio_priv(indio_dev);
> > +	hsc->xfer = hsc_spi_xfer;
> 
> Also, pass the callback and spi->dev into hsc probe. Easy to use
> a container_of() to get back to the struct spi_device *spi

I'd rather simply pass along the client struct.

> > +	hsc->client = spi;
> > +
> > +	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
> > +			 spi_get_device_id(spi)->driver_data);
> Don't use anything form spi_get_device_id()
> 
> Name is a fixed string currently so pass that directly.
> For driver data, there isn't any yet but if there were use
> spi_get_device_match_data() and make sure to provide the data in all the
> id tables.  That function will search the firmware ones first then call
> back to the spi specific varient.

along the way driver_data became redundant, so it was removed from the function
prototype.

best regards,
peter

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Jonathan Cameron]

> > > +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE];  
> > 
> > This is used for SPI transfers so should be DMA safe. It's not currently.
> > Look at how IIO_DMA_MINALIGN is used in other drivers to ensure there is
> > no unsafe sharing of cachelines.
> > 
> > On some architectures this is fixed by the stuff that bounces all small transfers
> > but I don't think that is universal yet.  If you want more info find the talk
> > by Wolfram Sang from a few years ago an ELCE on I2C DMA safe buffers.  
> 
> that was a nice rabbit hole, thanks for the pointer.

:) 

> 
> now, based on [2] I will skip explicit i2c dma-related code since my requests
> are 4 bytes long. according to the document, any i2c xfer below 8bytes is not
> worth the overhead.
> 
> [2] https://www.kernel.org/doc/html/latest/i2c/dma-considerations.html
> 
> > > +static int hsc_spi_probe(struct spi_device *spi)
> > > +{
> > > +	struct iio_dev *indio_dev;
> > > +	struct hsc_data *hsc;
> > > +	struct device *dev = &spi->dev;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	hsc = iio_priv(indio_dev);
> > > +	hsc->xfer = hsc_spi_xfer;  
> > 
> > Also, pass the callback and spi->dev into hsc probe. Easy to use
> > a container_of() to get back to the struct spi_device *spi  
> 
> I'd rather simply pass along the client struct.
> 

I don't like the fact it has to be a void *

The core code has no idea what is in there.  At least we constraint it
somewhat with a struct device.

If you want to use a union of the possible types, that would also be fine.

> > > +	hsc->client = spi;
> > > +
> > > +	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
> > > +			 spi_get_device_id(spi)->driver_data);  
> > Don't use anything form spi_get_device_id()
> > 
> > Name is a fixed string currently so pass that directly.
> > For driver data, there isn't any yet but if there were use
> > spi_get_device_match_data() and make sure to provide the data in all the
> > id tables.  That function will search the firmware ones first then call
> > back to the spi specific varient.  
> 
> along the way driver_data became redundant, so it was removed from the function
> prototype.
> 
> best regards,
> peter
> 
> 

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Petre Rodan]

Hello!

On Fri, Dec 01, 2023 at 06:24:53PM +0000, Jonathan Cameron wrote:
> > > > +static int hsc_spi_probe(struct spi_device *spi)
> > > > +{
> > > > +	struct iio_dev *indio_dev;
> > > > +	struct hsc_data *hsc;
> > > > +	struct device *dev = &spi->dev;
> > > > +
> > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > > > +	if (!indio_dev)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	hsc = iio_priv(indio_dev);
> > > > +	hsc->xfer = hsc_spi_xfer;
> > > 
> > > Also, pass the callback and spi->dev into hsc probe. Easy to use
> > > a container_of() to get back to the struct spi_device *spi  
> > 
> > I'd rather simply pass along the client struct.
> > 
> 
> I don't like the fact it has to be a void *
> 
> The core code has no idea what is in there.  At least we constraint it
> somewhat with a struct device.

but ...
that is the nice part. the core code never needs to know what exactly is behind
that pointer, since it only gets used by the i2c/spi module that provided that
pointer in the first place. I've never seen a better use of void * :)

I could define a

struct client_handle;

in the .h, use a pointer to that that as function argument, do a lot of
castings, but I feel like it's still a void * with extra steps.

cheers,
peter

Re: [PATCH v3 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Jonathan Cameron]

On Sat, 2 Dec 2023 18:01:55 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello!
> 
> On Fri, Dec 01, 2023 at 06:24:53PM +0000, Jonathan Cameron wrote:
> > > > > +static int hsc_spi_probe(struct spi_device *spi)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev;
> > > > > +	struct hsc_data *hsc;
> > > > > +	struct device *dev = &spi->dev;
> > > > > +
> > > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > > > > +	if (!indio_dev)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	hsc = iio_priv(indio_dev);
> > > > > +	hsc->xfer = hsc_spi_xfer;  
> > > > 
> > > > Also, pass the callback and spi->dev into hsc probe. Easy to use
> > > > a container_of() to get back to the struct spi_device *spi    
> > > 
> > > I'd rather simply pass along the client struct.
> > >   
> > 
> > I don't like the fact it has to be a void *
> > 
> > The core code has no idea what is in there.  At least we constraint it
> > somewhat with a struct device.  
> 
> but ...
> that is the nice part. the core code never needs to know what exactly is behind
> that pointer, since it only gets used by the i2c/spi module that provided that
> pointer in the first place. I've never seen a better use of void * :)
> 
> I could define a
> 
> struct client_handle;
> 
> in the .h, use a pointer to that that as function argument, do a lot of
> castings, but I feel like it's still a void * with extra steps.

Usual trick for this is either use struct device and container_of or
an anonymous union wrapped up in a struct.

struct hsc_client_handle {
	union {
		struct i2c_client *i2c_client;
		struct spithingy *spi_client;
	};
};

Then assign appropriate element and pass the containing struct around.
No casting needed.
Aim is to define it as a constrained type that can only take one or the other of
the types in the union. 

Jonathan


> 
> cheers,
> peter
>