| Message ID | 20231117164232.8474-2-petre.rodan@subdimension.ro (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: iio: pressure: add honeywell,hsc030 | expand |
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/202311181316.z2BmTZmP-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/202311181316.z2BmTZmP-lkp@intel.com/ 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
On Fri, Nov 17, 2023 at 06:42:06PM +0200, Petre Rodan wrote: > Adds driver for Honeywell TruStability HSC and SSC series pressure and > temperature sensors. > > Covers i2c and spi-based interfaces. For both series it supports all the > combinations of four transfer functions and all 118 pressure ranges. > In case of a special chip not covered by the nomenclature a custom range > can be specified. > > Devices tested: > HSCMLNN100PASA3 (SPI sensor) > HSCMRNN030PA2A3 (i2c sensor) > Datasheet: > [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf > [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf > [i2c comms] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/common/documents/sps-siot-i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-ciid-45841.pdf Make it a single tag per one URL as Datasheet: URL_#1 [xxx] Datasheet: URL_#2 [yyy] > No blank line in tags block. > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro> ... > +config HSC030PA > + tristate "Honeywell HSC/SSC (TruStability pressure sensors series)" > + depends on (I2C || SPI_MASTER) > + select HSC030PA_I2C if (I2C) > + select HSC030PA_SPI if (SPI_MASTER) Unneeded parentheses > + help > + Say Y here to build support for the Honeywell HSC and SSC TruStability > + pressure and temperature sensor series. > + > + To compile this driver as a module, choose M here: the module will be > + called hsc030pa. Yeah besides indentation issues the LKP complain about this. ... > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/init.h> > +#include <linux/math64.h> > +#include <linux/units.h> > +#include <linux/mod_devicetable.h> > +#include <linux/printk.h> Keep them sorted alphabetically. Also there are missing at least these ones: array_size.h, types.h. + blank line > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> + blank line. > +#include "hsc030pa.h" ... > +// pressure range for current chip based on the nomenclature > +struct hsc_range_config { > + char name[HSC_RANGE_STR_LEN]; // 5-char string that defines the range - ie "030PA" > + s32 pmin; // minimal pressure in pascals > + s32 pmax; // maximum pressure in pascals > +}; Can you utilize linear ranges data types and APIs? (linear_range.h) ... > +/* > + * the first two bits from the first byte contain a status code > + * > + * 00 - normal operation, valid data > + * 01 - device in hidden factory command mode > + * 10 - stale data > + * 11 - diagnostic condition > + * > + * function returns 1 only if both bits are zero > + */ You really need to be consistent with style of multi-line comments. And also C++/C variants. Besides that, respect English grammar and punctuation. ... > +static bool hsc_measurement_is_valid(struct hsc_data *data) > +{ > + if (data->buffer[0] & 0xc0) > + return 0; > + > + return 1; You use bool and return integers. Besides, it can be just a oneliner. return !(buffer[0] & GENMASK(3, 2)); (Note, you will need bits.h for this.) > +} ... > +static int hsc_get_measurement(struct hsc_data *data) > +{ > + const struct hsc_chip_data *chip = data->chip; > + int ret; > + > + /* don't bother sensor more than once a second */ > + if (!time_after(jiffies, data->last_update + HZ)) > + return data->is_valid ? 0 : -EAGAIN; > + > + data->is_valid = false; > + data->last_update = jiffies; > + > + ret = data->xfer(data); > + Redundant blank line. > + if (ret < 0) > + return ret; > + ret = chip->valid(data); > + if (!ret) > + return -EAGAIN; > + > + data->is_valid = true; Can this be like bool is_valid; is_valid = chip->valid(...); if (!is_valid) return ... data->is_valid = is_valid; // Depending on the flow you can even use that field directly > + return 0; > +} > +static int hsc_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > + struct hsc_data *data = iio_priv(indio_dev); > + int ret = -EINVAL; Just use value directly, no need to have this assignment. > + > + switch (mask) { > + Redundant blank line. > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&data->lock); > + ret = hsc_get_measurement(data); > + mutex_unlock(&data->lock); Use guard() operator from cleanup.h. > + if (ret) > + return ret; > + > + switch (channel->type) { > + case IIO_PRESSURE: > + *val = > + ((data->buffer[0] & 0x3f) << 8) + data->buffer[1]; > + return IIO_VAL_INT; > + case IIO_TEMP: > + *val = > + (data->buffer[2] << 3) + > + ((data->buffer[3] & 0xe0) >> 5); Is this some endianess / sign extension? Please convert using proper APIs. > + ret = 0; > + if (!ret) lol > + return IIO_VAL_INT; > + break; > + default: > + return -EINVAL; > + } > + break; > + > +/** > + * IIO ABI expects > + * value = (conv + offset) * scale > + * > + * datasheet provides the following formula for determining the temperature > + * temp[C] = conv * a + b > + * where a = 200/2047; b = -50 > + * > + * temp[C] = (conv + (b/a)) * a * (1000) > + * => > + * scale = a * 1000 = .097703957 * 1000 = 97.703957 > + * offset = b/a = -50 / .097703957 = -50000000 / 97704 > + * > + * based on the datasheet > + * pressure = (conv - HSC_OUTPUT_MIN) * Q + Pmin = > + * ((conv - HSC_OUTPUT_MIN) + Pmin/Q) * Q > + * => > + * scale = Q = (Pmax - Pmin) / (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) > + * offset = Pmin/Q = Pmin * (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) / (Pmax - Pmin) > + */ > + > + case IIO_CHAN_INFO_SCALE: > + switch (channel->type) { > + case IIO_TEMP: > + *val = 97; > + *val2 = 703957; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_PRESSURE: > + *val = data->p_scale; > + *val2 = data->p_scale_nano; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > + break; > + Dead code? > + case IIO_CHAN_INFO_OFFSET: > + switch (channel->type) { > + case IIO_TEMP: > + *val = -50000000; > + *val2 = 97704; > + return IIO_VAL_FRACTIONAL; > + case IIO_PRESSURE: > + *val = data->p_offset; > + *val2 = data->p_offset_nano; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > + } > + return ret; Use default with explicit error code. > +} ... > +static const struct iio_chan_spec hsc_channels[] = { > + { > + .type = IIO_PRESSURE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) > + }, > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) > + }, Strange indentation of }:s... > +}; ... > +int hsc_probe(struct iio_dev *indio_dev, struct device *dev, > + const char *name, int type) > +{ > + struct hsc_data *hsc; > + u64 tmp; > + int index; > + int found = 0; > + > + hsc = iio_priv(indio_dev); > + > + hsc->last_update = jiffies - HZ; > + hsc->chip = &hsc_chip; > + > + if (strcasecmp(hsc->range_str, "na") != 0) { > + // chip should be defined in the nomenclature > + for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) { > + if (strcasecmp > + (hsc_range_config[index].name, > + hsc->range_str) == 0) { > + hsc->pmin = hsc_range_config[index].pmin; > + hsc->pmax = hsc_range_config[index].pmax; > + found = 1; > + break; > + } > + } Reinventing match_string() / sysfs_match_string() ? > + if (hsc->pmin == hsc->pmax || !found) > + return dev_err_probe(dev, -EINVAL, > + "honeywell,range_str is invalid\n"); > + } > + > + hsc->outmin = hsc_func_spec[hsc->function].output_min; > + hsc->outmax = hsc_func_spec[hsc->function].output_max; > + > + // multiply with MICRO and then divide by NANO since the output needs > + // to be in KPa as per IIO ABI requirement > + tmp = div_s64(((s64) (hsc->pmax - hsc->pmin)) * MICRO, > + (hsc->outmax - hsc->outmin)); > + hsc->p_scale = div_s64_rem(tmp, NANO, &hsc->p_scale_nano); > + tmp = > + div_s64(((s64) hsc->pmin * (s64) (hsc->outmax - hsc->outmin)) * > + MICRO, hsc->pmax - hsc->pmin); No need to have space after castings > + hsc->p_offset = > + div_s64_rem(tmp, NANO, &hsc->p_offset_nano) - hsc->outmin; > + > + mutex_init(&hsc->lock); > + indio_dev->name = name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &hsc_info; > + indio_dev->channels = hsc->chip->channels; > + indio_dev->num_channels = hsc->chip->num_channels; > + > + return devm_iio_device_register(dev, indio_dev); > +} This one devm wrapped... > +void hsc_remove(struct iio_dev *indio_dev) > +{ > + iio_device_unregister(indio_dev); > +} ...but not this. Why do you even need remove if the above is using devm always? ... > + * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro> > + * 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.
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
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?
On Wed, 22 Nov 2023 08:08:27 +0200 Petre Rodan <petre.rodan@subdimension.ro> 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.
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 <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* + HOST AP DRIVER L: linux-wireless@vger.kernel.org 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 <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?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 <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..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 <petre.rodan@subdimension.ro> + * + */ + +#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 <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 + * 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 <petre.rodan@subdimension.ro>"); +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 <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 <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 <petre.rodan@subdimension.ro>"); +MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor spi driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_HONEYWELL_HSC);
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 <petre.rodan@subdimension.ro> --- 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