[PATCH v3 0/3] Add support for Sensirion SDP500

[PATCH v3 0/3] Add support for Sensirion SDP500

[Petar Stoykov via B4 Relay]

This patch series introduces support for Sensirion SDP500 in the IIO
subsystem. The series is split into three patches:

1. The first patch adds the device tree bindings.
2. The second patch implements the device driver.
3. The third patch updates the MAINTAINERS file.

The driver is relatively simple. It provides a way to read the measured
differential pressure directly in Pa, as the device has a fixed scale
factor of 1/60. When an applications wants to read the pressure value,
3 bytes are read from the device, 2 are data and 1 is CRC8. If the crc
check passes, the raw value is made available.

The initialization of the device just starts the measurement process.

We have been using this device and driver in a product development for
almost a year now. There the pressure is read every 25ms and is used in a
control loop. We have not even seen crc errors. We are using the
"linux-imx" repository and not the mainline one but I see no risky kernel
functions in use so it should be fine here too.

All feedback is appreciated! Thank you for taking the time to review this.

Changelog
v1->v2:
	driver code:
* Removed the use of wrapper functions for logging
* Using built-in crc function instead of a custom one
* Removed the use of a wrapper function for i2c send and receive data
* Use get_unaligned_be16 instead of custom calculation
* Removed error log if devm_iio_device_alloc fails
* indio_dev->name set directly to "sdp500"
* Updated error logging to use "dev_err_probe" in probe function
* Added a sensor readout in the probe function (first one is always bad)
* Used devm_iio_device_register instead of iio_device_register
* Removed trailing comma in "sdp500_id" data
* Deleted sdp500_remove after using devm_iio_device_register in probe
	dt-bindings:
* Fixed dt-bindings example wording
* Added vdd-supply in dt-bindings example

v2->v3:
	driver code:
* Added link to datasheet at the start of the driver code
* Removed some unnecessary defines
* Removed an unused argument of sdp500_start_measurement function
* Renamed variable that holds the received CRC to "received_crc"
* Switched to returning RAW and SCALE values instead of PROCESSED
* Change logging to use data->dev instead of indio_dev->dev.parent
* Removed unnecessary debug log of the read value from the sensor
* Added vdd regulator handling
* Added "sensirion,sdp510" as compatible
* Removed the aligning of '=' in the sdp500_driver struct
	dt-bindings:
* Added "sensirion,sdp510" as possible compatible value in dt-bindings example

Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
---
Petar Stoykov (3):
      dt-bindings: iio: pressure: Add Sensirion SDP500
      iio: pressure: Add driver for Sensirion SDP500
      MAINTAINERS: Add Sensirion SDP500

 .../bindings/iio/pressure/sensirion,sdp500.yaml    |  41 ++++++
 MAINTAINERS                                        |   6 +
 drivers/iio/pressure/Kconfig                       |   9 ++
 drivers/iio/pressure/Makefile                      |   1 +
 drivers/iio/pressure/sdp500.c                      | 153 +++++++++++++++++++++
 5 files changed, 210 insertions(+)
---
base-commit: ab27740f76654ed58dd32ac0ba0031c18a6dea3b
change-id: 20240702-mainline_sdp500-0d9f8c499228

Best regards,
-- 
Petar Stoykov <pd.pstoykov@gmail.com>

[PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Petar Stoykov via B4 Relay]

From: Petar Stoykov <pd.pstoykov@gmail.com>

Sensirion SDP500 is a digital differential pressure sensor. It provides
a digital I2C output. Add devicetree bindings requiring the compatible
string and I2C slave address (reg).

Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
---
 .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
new file mode 100644
index 000000000000..6b3e54def367
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: sdp500/sdp510 pressure sensor with I2C bus interface
+
+maintainers:
+  - Petar Stoykov <petar.stoykov@prodrive-technologies.com>
+
+description: |
+  Pressure sensor from Sensirion with I2C bus interface.
+  There is no software difference between sdp500 and sdp510.
+
+properties:
+  compatible:
+    enum:
+      - sensirion,sdp500
+      - sensirion,sdp510
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      pressure@40 {
+        compatible = "sensirion,sdp500";
+        reg = <0x40>;
+        vdd-supply = <&foo>;
+      };
+    };

-- 
2.39.2

[PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Petar Stoykov via B4 Relay]

From: Petar Stoykov <pd.pstoykov@gmail.com>

Sensirion SDP500 is a digital differential pressure sensor. The sensor is
accessed over I2C.

Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
---
 drivers/iio/pressure/Kconfig  |   9 +++
 drivers/iio/pressure/Makefile |   1 +
 drivers/iio/pressure/sdp500.c | 153 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+)

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 95efa32e4289..5debdfbd5324 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -212,6 +212,15 @@ config MS5637
 	  This driver can also be built as a module. If so, the module will
 	  be called ms5637.
 
+config SDP500
+	tristate "Sensirion SDP500 differential pressure sensor I2C driver"
+	depends on I2C
+	help
+	  Say Y here to build support for Sensirion SDP500 differential pressure
+	  sensor I2C driver.
+	  To compile this driver as a module, choose M here: the core module
+	  will be called sdp500.
+
 config IIO_ST_PRESS
 	tristate "STMicroelectronics pressure sensor Driver"
 	depends on (I2C || SPI_MASTER) && SYSFS
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 436aec7e65f3..489ef7b7befa 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_MS5611) += ms5611_core.o
 obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o
 obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o
 obj-$(CONFIG_MS5637) += ms5637.o
+obj-$(CONFIG_SDP500) += sdp500.o
 obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
 st_pressure-y := st_pressure_core.o
 st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
new file mode 100644
index 000000000000..661c70bc1b5b
--- /dev/null
+++ b/drivers/iio/pressure/sdp500.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Sensirion sdp500 and sdp510 pressure sensors
+ *
+ * Datasheet: https://sensirion.com/resource/datasheet/sdp600
+ */
+
+#include <linux/i2c.h>
+#include <linux/crc8.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+#include <asm/unaligned.h>
+
+#define SDP500_CRC8_POLYNOMIAL  0x31   // x8 + x5 + x4 + 1 (normalized to 0x31)
+#define SDP500_READ_SIZE        3
+
+#define SDP500_I2C_START_MEAS 0xF1
+
+struct sdp500_data {
+	struct device *dev;
+};
+
+DECLARE_CRC8_TABLE(sdp500_crc8_table);
+
+static int sdp500_start_measurement(struct sdp500_data *data)
+{
+	struct i2c_client *client = to_i2c_client(data->dev);
+
+	return i2c_smbus_write_byte(client, SDP500_I2C_START_MEAS);
+}
+
+static const struct iio_chan_spec sdp500_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int sdp500_read_raw(struct iio_dev *indio_dev,
+			  struct iio_chan_spec const *chan,
+			  int *val, int *val2, long mask)
+{
+	int ret;
+	u8 rxbuf[SDP500_READ_SIZE];
+	u8 received_crc, calculated_crc;
+	struct sdp500_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = to_i2c_client(data->dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to receive data");
+			return ret;
+		}
+		if (ret != SDP500_READ_SIZE) {
+			dev_err(data->dev, "Data is received wrongly");
+			return -EIO;
+		}
+
+		received_crc = rxbuf[2];
+		calculated_crc = crc8(sdp500_crc8_table, rxbuf, sizeof(rxbuf) - 1, 0x00);
+		if (received_crc != calculated_crc) {
+			dev_err(data->dev, "calculated crc = 0x%.2X, received 0x%.2X",
+				calculated_crc, received_crc);
+			return -EIO;
+		}
+
+		*val = get_unaligned_be16(rxbuf);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1;
+		*val2 = 60;
+
+		return IIO_VAL_FRACTIONAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sdp500_info = {
+	.read_raw = &sdp500_read_raw,
+};
+
+static int sdp500_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct sdp500_data *data;
+	struct device *dev = &client->dev;
+	int ret;
+	u8 rxbuf[SDP500_READ_SIZE];
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get and enable regulator\n");
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	/* has to be done before the first i2c communication */
+	crc8_populate_msb(sdp500_crc8_table, SDP500_CRC8_POLYNOMIAL);
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+
+	indio_dev->name = "sdp500";
+	indio_dev->channels = sdp500_channels;
+	indio_dev->info = &sdp500_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = ARRAY_SIZE(sdp500_channels);
+
+	ret = sdp500_start_measurement(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to start measurement");
+
+	/* First measurement is not correct, read it out to get rid of it */
+	i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register indio_dev");
+
+	return 0;
+}
+
+static const struct i2c_device_id sdp500_id[] = {
+	{ "sdp500" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sdp500_id);
+
+static const struct of_device_id sdp500_of_match[] = {
+	{ .compatible = "sensirion,sdp500" },
+	{ .compatible = "sensirion,sdp510" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sdp500_of_match);
+
+static struct i2c_driver sdp500_driver = {
+	.driver = {
+		.name = "sensirion,sdp500",
+		.of_match_table = sdp500_of_match,
+	},
+	.probe = sdp500_probe,
+	.id_table = sdp500_id,
+};
+module_i2c_driver(sdp500_driver);
+
+MODULE_AUTHOR("Thomas Sioutas <thomas.sioutas@prodrive-technologies.com>");
+MODULE_DESCRIPTION("Driver for Sensirion SDP500 differential pressure sensor");
+MODULE_LICENSE("GPL");

-- 
2.39.2

[PATCH v3 3/3] MAINTAINERS: Add Sensirion SDP500

[Petar Stoykov via B4 Relay]

From: Petar Stoykov <pd.pstoykov@gmail.com>

Add myself as a maintainer for Sensirion SDP500 pressure sensor driver

Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 40c754b4c39c..65f9479ac343 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19533,6 +19533,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
 F:	drivers/iio/chemical/scd4x.c
 
+SENSIRION SDP500 DIFFERENTIAL PRESSURE SENSOR DRIVER
+M:	Petar Stoykov <petar.stoykov@prodrive-technologies.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/pressure/sdp500.yaml
+F:	drivers/iio/pressure/sdp500.c
+
 SENSIRION SGP40 GAS SENSOR DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 S:	Maintained

-- 
2.39.2

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Krzysztof Kozlowski]

On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Sensirion SDP500 is a digital differential pressure sensor. It provides
> a digital I2C output. Add devicetree bindings requiring the compatible
> string and I2C slave address (reg).
> 

You did not test your code before sending.

Please respond to existing feedback from v1 and v2, thus confirm that
you understood it and you are or are not going to implement it.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] MAINTAINERS: Add Sensirion SDP500

[Krzysztof Kozlowski]

On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Add myself as a maintainer for Sensirion SDP500 pressure sensor driver
> 
> Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 40c754b4c39c..65f9479ac343 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19533,6 +19533,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
>  F:	drivers/iio/chemical/scd4x.c
>  
> +SENSIRION SDP500 DIFFERENTIAL PRESSURE SENSOR DRIVER
> +M:	Petar Stoykov <petar.stoykov@prodrive-technologies.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/pressure/sdp500.yaml

There is no such file.

Best regards,
Krzysztof

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Krzysztof Kozlowski]

On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> accessed over I2C.
> 

...

> +
> +static const struct of_device_id sdp500_of_match[] = {
> +	{ .compatible = "sensirion,sdp500" },
> +	{ .compatible = "sensirion,sdp510" },

Devices look compatible, so express it in the bindings with fallback to
sdp500 (oneOf, see example-schema).

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Rob Herring (Arm)]

On Tue, 02 Jul 2024 16:59:08 +0200, Petar Stoykov wrote:
> Sensirion SDP500 is a digital differential pressure sensor. It provides
> a digital I2C output. Add devicetree bindings requiring the compatible
> string and I2C slave address (reg).
> 
> Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> ---
>  .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.example.dtb: pressure@40: 'vdd-supply' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240702-mainline_sdp500-v3-1-0902047b3eee@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 3/3] MAINTAINERS: Add Sensirion SDP500

[kernel test robot]

Hi Petar,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ab27740f76654ed58dd32ac0ba0031c18a6dea3b]

url:    https://github.com/intel-lab-lkp/linux/commits/Petar-Stoykov-via-B4-Relay/dt-bindings-iio-pressure-Add-Sensirion-SDP500/20240702-235054
base:   ab27740f76654ed58dd32ac0ba0031c18a6dea3b
patch link:    https://lore.kernel.org/r/20240702-mainline_sdp500-v3-3-0902047b3eee%40gmail.com
patch subject: [PATCH v3 3/3] MAINTAINERS: Add Sensirion SDP500
reproduce: (https://download.01.org/0day-ci/archive/20240703/202407030117.3F6Sm9vA-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/202407030117.3F6Sm9vA-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/watchdog/da90??-wdt.txt
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/iio/pressure/sdp500.yaml
   Warning: file ./Documentation/ABI/testing/sysfs-platform-silicom#20:
   What '/sys/devices/platform/silicom-platform/power_cycle' doesn't have a description
   Using alabaster theme

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

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Jonathan Cameron]

On Tue, 02 Jul 2024 16:59:09 +0200
Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:

> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> accessed over I2C.
> 
> Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
Hi Petar

Given you are going to be doing a v4, a few comments inline on things
to tidy up in here.

Note that I've already tagged what may be the last pull request from me
for the 6.11 merge window, so this is probably 6.12 material now anyway.
Hence plenty of time to tidy up.

Jonathan

> diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
> new file mode 100644
> index 000000000000..661c70bc1b5b
> --- /dev/null
> +++ b/drivers/iio/pressure/sdp500.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for Sensirion sdp500 and sdp510 pressure sensors
> + *
> + * Datasheet: https://sensirion.com/resource/datasheet/sdp600
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/crc8.h>
> +#include <linux/iio/iio.h>

#include <linux/mod_devicetable.h> appropriate for the id tables.

> +#include <linux/regulator/consumer.h>
> +#include <asm/unaligned.h>
> +
> +#define SDP500_CRC8_POLYNOMIAL  0x31   // x8 + x5 + x4 + 1 (normalized to 0x31)

For IIO we tend to just use c style comments /* xxx */
and not c++ ones.

> +#define SDP500_READ_SIZE        3

> +static int sdp500_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	int ret;
> +	u8 rxbuf[SDP500_READ_SIZE];
> +	u8 received_crc, calculated_crc;
> +	struct sdp500_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = to_i2c_client(data->dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to receive data");
> +			return ret;
> +		}
> +		if (ret != SDP500_READ_SIZE) {
> +			dev_err(data->dev, "Data is received wrongly");
> +			return -EIO;
> +		}
> +
> +		received_crc = rxbuf[2];
> +		calculated_crc = crc8(sdp500_crc8_table, rxbuf, sizeof(rxbuf) - 1, 0x00);
A line break in here to keep it under 80 chars would be good (see below).

> +		if (received_crc != calculated_crc) {
> +			dev_err(data->dev, "calculated crc = 0x%.2X, received 0x%.2X",
> +				calculated_crc, received_crc);
> +			return -EIO;
> +		}
> +
> +		*val = get_unaligned_be16(rxbuf);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1;
> +		*val2 = 60;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info sdp500_info = {
> +	.read_raw = &sdp500_read_raw,
> +};
> +
> +static int sdp500_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct sdp500_data *data;
> +	struct device *dev = &client->dev;
> +	int ret;
> +	u8 rxbuf[SDP500_READ_SIZE];
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable regulator\n");
Given you are going around again.  Add a line break before "
General rule is stay under 80 chars unless it hurts readability.
 
> +

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Jonathan Cameron]

On Tue, 02 Jul 2024 16:59:08 +0200
Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:

> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Sensirion SDP500 is a digital differential pressure sensor. It provides
> a digital I2C output. Add devicetree bindings requiring the compatible
> string and I2C slave address (reg).
> 
> Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> ---
>  .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> new file mode 100644
> index 000000000000..6b3e54def367
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#

Naming mismatch (as already reporteD).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: sdp500/sdp510 pressure sensor with I2C bus interface
> +
> +maintainers:
> +  - Petar Stoykov <petar.stoykov@prodrive-technologies.com>
> +
> +description: |
> +  Pressure sensor from Sensirion with I2C bus interface.
> +  There is no software difference between sdp500 and sdp510.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sensirion,sdp500
> +      - sensirion,sdp510

Include the vdd-supply both as a property and in requried.

  vdd-supply: true; 

is enough for the property.
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      pressure@40 {
> +        compatible = "sensirion,sdp500";
> +        reg = <0x40>;
> +        vdd-supply = <&foo>;
> +      };
> +    };
> 

Re: [PATCH v3 3/3] MAINTAINERS: Add Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 5:15 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> > From: Petar Stoykov <pd.pstoykov@gmail.com>
> >
> > Add myself as a maintainer for Sensirion SDP500 pressure sensor driver
> >
> > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> > ---
> >  MAINTAINERS | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 40c754b4c39c..65f9479ac343 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19533,6 +19533,12 @@ S:   Maintained
> >  F:   Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
> >  F:   drivers/iio/chemical/scd4x.c
> >
> > +SENSIRION SDP500 DIFFERENTIAL PRESSURE SENSOR DRIVER
> > +M:   Petar Stoykov <petar.stoykov@prodrive-technologies.com>
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/iio/pressure/sdp500.yaml
>
> There is no such file.

Ops, I forgot to update both references to the file after it got renamed.
Here and in the bindings file.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 5:17 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> > From: Petar Stoykov <pd.pstoykov@gmail.com>
> >
> > Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> > accessed over I2C.
> >
>
> ...
>
> > +
> > +static const struct of_device_id sdp500_of_match[] = {
> > +     { .compatible = "sensirion,sdp500" },
> > +     { .compatible = "sensirion,sdp510" },
>
> Devices look compatible, so express it in the bindings with fallback to
> sdp500 (oneOf, see example-schema).

Hi Krzysztof,
I tried to understand the explanation in the example-schema. I must say it is
a bit confusing, but I can't offer improvement because I'm not sure I
understand it fully yet.
Can you verify if this is what you expect the bindings file to read?
properties:
  compatible:
    oneOf:
      - items:
        - const: sensirion,sdp510
        - const: sensirion,sdp500
      - items:
        - const: sensirion,sdp500

So the device tree should have either this
  compatible = "sensirion,sdp510", "sensirion,sdp500"
Or
  compatible = "sensirion,sdp500"
The first would mean that 510 falls back to 500, right?

From what I've seen it is rare to have two strings in "compatible" so I didn't
know of this mechanism of "fallback" in the dts. I expected to just write the
name of my device (let's say "sensirion,sdp510") and the driver would handle it.
But I'm learning. Expectations change.

Regards,
Petar

>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 10:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 02 Jul 2024 16:59:09 +0200
> Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:
>
> > From: Petar Stoykov <pd.pstoykov@gmail.com>
> >
> > Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> > accessed over I2C.
> >
> > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> Hi Petar
>
> Given you are going to be doing a v4, a few comments inline on things
> to tidy up in here.
>
> Note that I've already tagged what may be the last pull request from me
> for the 6.11 merge window, so this is probably 6.12 material now anyway.
> Hence plenty of time to tidy up.
>
> Jonathan
>

Hi Jonathan,
I have no rush, thank you for the explanation!

> > diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
> > new file mode 100644
> > index 000000000000..661c70bc1b5b
> > --- /dev/null
> > +++ b/drivers/iio/pressure/sdp500.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Driver for Sensirion sdp500 and sdp510 pressure sensors
> > + *
> > + * Datasheet: https://sensirion.com/resource/datasheet/sdp600
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/crc8.h>
> > +#include <linux/iio/iio.h>
>
> #include <linux/mod_devicetable.h> appropriate for the id tables.

I don't understand. Why add an extra header to a working driver?
I will add it and test the driver in the meantime.

>
> > +#include <linux/regulator/consumer.h>
> > +#include <asm/unaligned.h>
> > +
> > +#define SDP500_CRC8_POLYNOMIAL  0x31   // x8 + x5 + x4 + 1 (normalized to 0x31)
>
> For IIO we tend to just use c style comments /* xxx */
> and not c++ ones.

I try to follow this rule but once in a while I slip to my old habits.
My teachers all used // for C for whatever reason. Will fix.

>
> > +#define SDP500_READ_SIZE        3
>
> > +static int sdp500_read_raw(struct iio_dev *indio_dev,
> > +                       struct iio_chan_spec const *chan,
> > +                       int *val, int *val2, long mask)
> > +{
> > +     int ret;
> > +     u8 rxbuf[SDP500_READ_SIZE];
> > +     u8 received_crc, calculated_crc;
> > +     struct sdp500_data *data = iio_priv(indio_dev);
> > +     struct i2c_client *client = to_i2c_client(data->dev);
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
> > +             if (ret < 0) {
> > +                     dev_err(data->dev, "Failed to receive data");
> > +                     return ret;
> > +             }
> > +             if (ret != SDP500_READ_SIZE) {
> > +                     dev_err(data->dev, "Data is received wrongly");
> > +                     return -EIO;
> > +             }
> > +
> > +             received_crc = rxbuf[2];
> > +             calculated_crc = crc8(sdp500_crc8_table, rxbuf, sizeof(rxbuf) - 1, 0x00);
> A line break in here to keep it under 80 chars would be good (see below).

I find the 80 chars too constraining and also saw that the limit was updated
to 100 somewhere in kernel.org which made me happy. But.. you seem to
feel strongly enough about this so I'll trim lines to 80.

>
> > +             if (received_crc != calculated_crc) {
> > +                     dev_err(data->dev, "calculated crc = 0x%.2X, received 0x%.2X",
> > +                             calculated_crc, received_crc);
> > +                     return -EIO;
> > +             }
> > +
> > +             *val = get_unaligned_be16(rxbuf);
> > +             return IIO_VAL_INT;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = 1;
> > +             *val2 = 60;
> > +
> > +             return IIO_VAL_FRACTIONAL;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static const struct iio_info sdp500_info = {
> > +     .read_raw = &sdp500_read_raw,
> > +};
> > +
> > +static int sdp500_probe(struct i2c_client *client)
> > +{
> > +     struct iio_dev *indio_dev;
> > +     struct sdp500_data *data;
> > +     struct device *dev = &client->dev;
> > +     int ret;
> > +     u8 rxbuf[SDP500_READ_SIZE];
> > +
> > +     ret = devm_regulator_get_enable(dev, "vdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to get and enable regulator\n");
> Given you are going around again.  Add a line break before "
> General rule is stay under 80 chars unless it hurts readability.
>
> > +

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 5:15 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
> > From: Petar Stoykov <pd.pstoykov@gmail.com>
> >
> > Sensirion SDP500 is a digital differential pressure sensor. It provides
> > a digital I2C output. Add devicetree bindings requiring the compatible
> > string and I2C slave address (reg).
> >
>
> You did not test your code before sending.

I tested the driver for sdp500 on our system and it worked well.
I must admit that I forgot to change the dts to sdp510 and retest.

>
> Please respond to existing feedback from v1 and v2, thus confirm that
> you understood it and you are or are not going to implement it.
>

I tried to reply to all previous comments. Sorry if I missed something.

> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

I didn't know about that dt_binding_check. Then I spent a few hours
yesterday fighting with dependencies to get it running.
Thanks!

>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 10:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 02 Jul 2024 16:59:08 +0200
> Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:
>
> > From: Petar Stoykov <pd.pstoykov@gmail.com>
> >
> > Sensirion SDP500 is a digital differential pressure sensor. It provides
> > a digital I2C output. Add devicetree bindings requiring the compatible
> > string and I2C slave address (reg).
> >
> > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> > ---
> >  .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> > new file mode 100644
> > index 000000000000..6b3e54def367
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#
>
> Naming mismatch (as already reporteD).

Same mistake as in the MAINTAINERS file. Sigh.

>
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: sdp500/sdp510 pressure sensor with I2C bus interface
> > +
> > +maintainers:
> > +  - Petar Stoykov <petar.stoykov@prodrive-technologies.com>
> > +
> > +description: |
> > +  Pressure sensor from Sensirion with I2C bus interface.
> > +  There is no software difference between sdp500 and sdp510.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sensirion,sdp500
> > +      - sensirion,sdp510
>
> Include the vdd-supply both as a property and in requried.
>
>   vdd-supply: true;
>
> is enough for the property.

Thank you for the example!
But I don't think it's a required property.
Our system is an example where it's not needed.
The device is always powered on and Linux has no control over it's supply.
One can argue that it's nice to write down what is that power supply but
to me it's just noise, because I can write anything there and it would still
work. So I prefer to not add anything instead of putting a value I can't trust.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      pressure@40 {
> > +        compatible = "sensirion,sdp500";
> > +        reg = <0x40>;
> > +        vdd-supply = <&foo>;
> > +      };
> > +    };
> >
>

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Petar Stoykov]

On Tue, Jul 2, 2024 at 6:30 PM Rob Herring (Arm) <robh@kernel.org> wrote:
>
>
> On Tue, 02 Jul 2024 16:59:08 +0200, Petar Stoykov wrote:
> > Sensirion SDP500 is a digital differential pressure sensor. It provides
> > a digital I2C output. Add devicetree bindings requiring the compatible
> > string and I2C slave address (reg).
> >
> > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> > ---
> >  .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
>          $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml
>         file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.example.dtb: pressure@40: 'vdd-supply' does not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240702-mainline_sdp500-v3-1-0902047b3eee@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade

I did not know about this tool before. And I did need to update a bunch of
dependencies to make it work. Thanks!

>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

The setting of DT_SCHEMA_FILES was a huge help in saving time!!

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Krzysztof Kozlowski]

On 04/07/2024 10:18, Petar Stoykov wrote:
> On Tue, Jul 2, 2024 at 5:15 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
>>> From: Petar Stoykov <pd.pstoykov@gmail.com>
>>>
>>> Sensirion SDP500 is a digital differential pressure sensor. It provides
>>> a digital I2C output. Add devicetree bindings requiring the compatible
>>> string and I2C slave address (reg).
>>>
>>
>> You did not test your code before sending.
> 
> I tested the driver for sdp500 on our system and it worked well.
> I must admit that I forgot to change the dts to sdp510 and retest.
> 
>>
>> Please respond to existing feedback from v1 and v2, thus confirm that
>> you understood it and you are or are not going to implement it.
>>
> 
> I tried to reply to all previous comments. Sorry if I missed something.
> 
>> It does not look like you tested the bindings, at least after quick
>> look. Please run `make dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>> Maybe you need to update your dtschema and yamllint.
> 
> I didn't know about that dt_binding_check. Then I spent a few hours
> yesterday fighting with dependencies to get it running.

It's just one command:
pipx install dtschema yamllint
(or pip, depending on your system)

Best regards,
Krzysztof

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Krzysztof Kozlowski]

On 04/07/2024 09:49, Petar Stoykov wrote:
> On Tue, Jul 2, 2024 at 5:17 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 02/07/2024 16:59, Petar Stoykov via B4 Relay wrote:
>>> From: Petar Stoykov <pd.pstoykov@gmail.com>
>>>
>>> Sensirion SDP500 is a digital differential pressure sensor. The sensor is
>>> accessed over I2C.
>>>
>>
>> ...
>>
>>> +
>>> +static const struct of_device_id sdp500_of_match[] = {
>>> +     { .compatible = "sensirion,sdp500" },
>>> +     { .compatible = "sensirion,sdp510" },
>>
>> Devices look compatible, so express it in the bindings with fallback to
>> sdp500 (oneOf, see example-schema).
> 
> Hi Krzysztof,
> I tried to understand the explanation in the example-schema. I must say it is
> a bit confusing, but I can't offer improvement because I'm not sure I
> understand it fully yet.
> Can you verify if this is what you expect the bindings file to read?
> properties:
>   compatible:
>     oneOf:
>       - items:
>         - const: sensirion,sdp510
>         - const: sensirion,sdp500
>       - items:
>         - const: sensirion,sdp500

Almost.

    oneOf:
      - items:
          - const: sensirion,sdp510
          - const: sensirion,sdp500
      - const: sensirion,sdp500

> 
> So the device tree should have either this
>   compatible = "sensirion,sdp510", "sensirion,sdp500"
> Or
>   compatible = "sensirion,sdp500"
> The first would mean that 510 falls back to 500, right?

Yes

> 
> From what I've seen it is rare to have two strings in "compatible" so I didn't

Rare? It's everywhere in SoC and in many places for standalone
devices/hardware.

> know of this mechanism of "fallback" in the dts. I expected to just write the
> name of my device (let's say "sensirion,sdp510") and the driver would handle it.
> But I'm learning. Expectations change.



Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Add Sensirion SDP500

[Jonathan Cameron]

On Thu, 4 Jul 2024 10:35:45 +0200
Petar Stoykov <pd.pstoykov@gmail.com> wrote:

> On Tue, Jul 2, 2024 at 10:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 02 Jul 2024 16:59:08 +0200
> > Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:
> >  
> > > From: Petar Stoykov <pd.pstoykov@gmail.com>
> > >
> > > Sensirion SDP500 is a digital differential pressure sensor. It provides
> > > a digital I2C output. Add devicetree bindings requiring the compatible
> > > string and I2C slave address (reg).
> > >
> > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>
> > > ---
> > >  .../bindings/iio/pressure/sensirion,sdp500.yaml    | 41 ++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> > > new file mode 100644
> > > index 000000000000..6b3e54def367
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/pressure/sensirion,sdp500.yaml
> > > @@ -0,0 +1,41 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/pressure/sdp500.yaml#  
> >
> > Naming mismatch (as already reporteD).  
> 
> Same mistake as in the MAINTAINERS file. Sigh.
> 
> >  
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: sdp500/sdp510 pressure sensor with I2C bus interface
> > > +
> > > +maintainers:
> > > +  - Petar Stoykov <petar.stoykov@prodrive-technologies.com>
> > > +
> > > +description: |
> > > +  Pressure sensor from Sensirion with I2C bus interface.
> > > +  There is no software difference between sdp500 and sdp510.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - sensirion,sdp500
> > > +      - sensirion,sdp510  
> >
> > Include the vdd-supply both as a property and in requried.
> >
> >   vdd-supply: true;
> >
> > is enough for the property.  
> 
> Thank you for the example!
> But I don't think it's a required property.
> Our system is an example where it's not needed.
> The device is always powered on and Linux has no control over it's supply.
> One can argue that it's nice to write down what is that power supply but
> to me it's just noise, because I can write anything there and it would still
> work. So I prefer to not add anything instead of putting a value I can't trust.
> 
I understand your nervousness, but put it in the binding doc anyway (not necessarily
in your dts though!)

There is a difference between what the binding requires (to work on
any operating system etc) and what the Linux driver stack requires.
The binding should specify any supply that is needed for the device to do anything
useful.

That doesn't mean that you need to provide it on a board if it's always
powered up.

The Linux driver should include regulator enabling for the supply. If it's not
provided in firmware because it's always one, then the dummy regulators that
will get used are fine.

devm_regulator_get_enable() is all that's needed




> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +      pressure@40 {
> > > +        compatible = "sensirion,sdp500";
> > > +        reg = <0x40>;
> > > +        vdd-supply = <&foo>;
> > > +      };
> > > +    };
> > >  
> >  

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Jonathan Cameron]

On Thu, 4 Jul 2024 10:09:26 +0200
Petar Stoykov <pd.pstoykov@gmail.com> wrote:

> On Tue, Jul 2, 2024 at 10:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 02 Jul 2024 16:59:09 +0200
> > Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@kernel.org> wrote:
> >  
> > > From: Petar Stoykov <pd.pstoykov@gmail.com>
> > >
> > > Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> > > accessed over I2C.
> > >
> > > Signed-off-by: Petar Stoykov <pd.pstoykov@gmail.com>  
> > Hi Petar
> >
> > Given you are going to be doing a v4, a few comments inline on things
> > to tidy up in here.
> >
> > Note that I've already tagged what may be the last pull request from me
> > for the 6.11 merge window, so this is probably 6.12 material now anyway.
> > Hence plenty of time to tidy up.
> >
> > Jonathan
> >  
> 
> Hi Jonathan,
> I have no rush, thank you for the explanation!
> 
> > > diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c
> > > new file mode 100644
> > > index 000000000000..661c70bc1b5b
> > > --- /dev/null
> > > +++ b/drivers/iio/pressure/sdp500.c
> > > @@ -0,0 +1,153 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Driver for Sensirion sdp500 and sdp510 pressure sensors
> > > + *
> > > + * Datasheet: https://sensirion.com/resource/datasheet/sdp600
> > > + */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/crc8.h>
> > > +#include <linux/iio/iio.h>  
> >
> > #include <linux/mod_devicetable.h> appropriate for the id tables.  
> 
> I don't understand. Why add an extra header to a working driver?
> I will add it and test the driver in the meantime.

It's included via another path today (at least on the architectures
you've tested with), but we more or less prefer to operate on an
include what you use principle.

There are exceptions where it is considered very obvious that
the headers will always be included but this isn't one of them.
(there is no hard list of which ones are included though :(
> 
> >  
> > > +#include <linux/regulator/consumer.h>
> > > +#include <asm/unaligned.h>
> > > +
> > > +#define SDP500_CRC8_POLYNOMIAL  0x31   // x8 + x5 + x4 + 1 (normalized to 0x31)  
> >
> > For IIO we tend to just use c style comments /* xxx */
> > and not c++ ones.  
> 
> I try to follow this rule but once in a while I slip to my old habits.
> My teachers all used // for C for whatever reason. Will fix.
> 
> >  
> > > +#define SDP500_READ_SIZE        3  
> >  
> > > +static int sdp500_read_raw(struct iio_dev *indio_dev,
> > > +                       struct iio_chan_spec const *chan,
> > > +                       int *val, int *val2, long mask)
> > > +{
> > > +     int ret;
> > > +     u8 rxbuf[SDP500_READ_SIZE];
> > > +     u8 received_crc, calculated_crc;
> > > +     struct sdp500_data *data = iio_priv(indio_dev);
> > > +     struct i2c_client *client = to_i2c_client(data->dev);
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE);
> > > +             if (ret < 0) {
> > > +                     dev_err(data->dev, "Failed to receive data");
> > > +                     return ret;
> > > +             }
> > > +             if (ret != SDP500_READ_SIZE) {
> > > +                     dev_err(data->dev, "Data is received wrongly");
> > > +                     return -EIO;
> > > +             }
> > > +
> > > +             received_crc = rxbuf[2];
> > > +             calculated_crc = crc8(sdp500_crc8_table, rxbuf, sizeof(rxbuf) - 1, 0x00);  
> > A line break in here to keep it under 80 chars would be good (see below).  
> 
> I find the 80 chars too constraining and also saw that the limit was updated
> to 100 somewhere in kernel.org which made me happy. But.. you seem to
> feel strongly enough about this so I'll trim lines to 80.

100 is fine if it helps readability.  I'd argue it doesn't significantly
help here.

Some parts of the kernel are fussier than others, but it's rare you'll get
told to increase your wrap length above 80 unless there is a readability
advantage.

Jonathan

Re: [PATCH v3 2/3] iio: pressure: Add driver for Sensirion SDP500

[Andy Shevchenko]

On Tue, Jul 02, 2024 at 04:59:09PM +0200, Petar Stoykov via B4 Relay wrote:
> From: Petar Stoykov <pd.pstoykov@gmail.com>
> 
> Sensirion SDP500 is a digital differential pressure sensor. The sensor is
> accessed over I2C.

...

+ array_size.h
+ bits.h
+ dev_printk.h
+ errno.h

> +#include <linux/i2c.h>
> +#include <linux/crc8.h>
> +#include <linux/iio/iio.h>

+ mod_devicetable.h
+ module.h

> +#include <linux/regulator/consumer.h>

+ types.h

Keep them ordered, also you may split iio/ group out

> +#include <asm/unaligned.h>

linux/*
...blank line...
asm/*
...blank line...
iio/*

...

> +struct sdp500_data {
> +	struct device *dev;
> +};

Why is this structure needed at all? You may put dev pointer directly, no?

-- 
With Best Regards,
Andy Shevchenko