[PATCH 1/3] iio: pressure: ms5611: fixed value compensation bug

[PATCH 1/3] iio: pressure: ms5611: fixed value compensation bug

[Mitja Spes]

When using multiple instances of this driver the compensation PROM was
overwritten by the last initialized sensor. Now each sensor has own PROM
storage.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/pressure/ms5611.h      | 12 +++----
 drivers/iio/pressure/ms5611_core.c | 51 ++++++++++++++++--------------
 2 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/pressure/ms5611.h b/drivers/iio/pressure/ms5611.h
index cbc9349c342a..550b75b7186f 100644
--- a/drivers/iio/pressure/ms5611.h
+++ b/drivers/iio/pressure/ms5611.h
@@ -25,13 +25,6 @@ enum {
 	MS5607,
 };
 
-struct ms5611_chip_info {
-	u16 prom[MS5611_PROM_WORDS_NB];
-
-	int (*temp_and_pressure_compensate)(struct ms5611_chip_info *chip_info,
-					    s32 *temp, s32 *pressure);
-};
-
 /*
  * OverSampling Rate descriptor.
  * Warning: cmd MUST be kept aligned on a word boundary (see
@@ -50,12 +43,15 @@ struct ms5611_state {
 	const struct ms5611_osr *pressure_osr;
 	const struct ms5611_osr *temp_osr;
 
+	u16 prom[MS5611_PROM_WORDS_NB];
+
 	int (*reset)(struct ms5611_state *st);
 	int (*read_prom_word)(struct ms5611_state *st, int index, u16 *word);
 	int (*read_adc_temp_and_pressure)(struct ms5611_state *st,
 					  s32 *temp, s32 *pressure);
 
-	struct ms5611_chip_info *chip_info;
+	int (*compensate_temp_and_pressure)(struct ms5611_state *st, s32 *temp,
+					  s32 *pressure);
 	struct regulator *vdd;
 };
 
diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
index 717521de66c4..c564a1d6cafe 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -85,7 +85,7 @@ static int ms5611_read_prom(struct iio_dev *indio_dev)
 	struct ms5611_state *st = iio_priv(indio_dev);
 
 	for (i = 0; i < MS5611_PROM_WORDS_NB; i++) {
-		ret = st->read_prom_word(st, i, &st->chip_info->prom[i]);
+		ret = st->read_prom_word(st, i, &st->prom[i]);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"failed to read prom at %d\n", i);
@@ -93,7 +93,7 @@ static int ms5611_read_prom(struct iio_dev *indio_dev)
 		}
 	}
 
-	if (!ms5611_prom_is_valid(st->chip_info->prom, MS5611_PROM_WORDS_NB)) {
+	if (!ms5611_prom_is_valid(st->prom, MS5611_PROM_WORDS_NB)) {
 		dev_err(&indio_dev->dev, "PROM integrity check failed\n");
 		return -ENODEV;
 	}
@@ -114,21 +114,20 @@ static int ms5611_read_temp_and_pressure(struct iio_dev *indio_dev,
 		return ret;
 	}
 
-	return st->chip_info->temp_and_pressure_compensate(st->chip_info,
-							   temp, pressure);
+	return st->compensate_temp_and_pressure(st, temp, pressure);
 }
 
-static int ms5611_temp_and_pressure_compensate(struct ms5611_chip_info *chip_info,
+static int ms5611_temp_and_pressure_compensate(struct ms5611_state *st,
 					       s32 *temp, s32 *pressure)
 {
 	s32 t = *temp, p = *pressure;
 	s64 off, sens, dt;
 
-	dt = t - (chip_info->prom[5] << 8);
-	off = ((s64)chip_info->prom[2] << 16) + ((chip_info->prom[4] * dt) >> 7);
-	sens = ((s64)chip_info->prom[1] << 15) + ((chip_info->prom[3] * dt) >> 8);
+	dt = t - (st->prom[5] << 8);
+	off = ((s64)st->prom[2] << 16) + ((st->prom[4] * dt) >> 7);
+	sens = ((s64)st->prom[1] << 15) + ((st->prom[3] * dt) >> 8);
 
-	t = 2000 + ((chip_info->prom[6] * dt) >> 23);
+	t = 2000 + ((st->prom[6] * dt) >> 23);
 	if (t < 2000) {
 		s64 off2, sens2, t2;
 
@@ -154,17 +153,17 @@ static int ms5611_temp_and_pressure_compensate(struct ms5611_chip_info *chip_inf
 	return 0;
 }
 
-static int ms5607_temp_and_pressure_compensate(struct ms5611_chip_info *chip_info,
+static int ms5607_temp_and_pressure_compensate(struct ms5611_state *st,
 					       s32 *temp, s32 *pressure)
 {
 	s32 t = *temp, p = *pressure;
 	s64 off, sens, dt;
 
-	dt = t - (chip_info->prom[5] << 8);
-	off = ((s64)chip_info->prom[2] << 17) + ((chip_info->prom[4] * dt) >> 6);
-	sens = ((s64)chip_info->prom[1] << 16) + ((chip_info->prom[3] * dt) >> 7);
+	dt = t - (st->prom[5] << 8);
+	off = ((s64)st->prom[2] << 17) + ((st->prom[4] * dt) >> 6);
+	sens = ((s64)st->prom[1] << 16) + ((st->prom[3] * dt) >> 7);
 
-	t = 2000 + ((chip_info->prom[6] * dt) >> 23);
+	t = 2000 + ((st->prom[6] * dt) >> 23);
 	if (t < 2000) {
 		s64 off2, sens2, t2, tmp;
 
@@ -342,15 +341,6 @@ static int ms5611_write_raw(struct iio_dev *indio_dev,
 
 static const unsigned long ms5611_scan_masks[] = {0x3, 0};
 
-static struct ms5611_chip_info chip_info_tbl[] = {
-	[MS5611] = {
-		.temp_and_pressure_compensate = ms5611_temp_and_pressure_compensate,
-	},
-	[MS5607] = {
-		.temp_and_pressure_compensate = ms5607_temp_and_pressure_compensate,
-	}
-};
-
 static const struct iio_chan_spec ms5611_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -433,7 +423,20 @@ int ms5611_probe(struct iio_dev *indio_dev, struct device *dev,
 	struct ms5611_state *st = iio_priv(indio_dev);
 
 	mutex_init(&st->lock);
-	st->chip_info = &chip_info_tbl[type];
+
+	switch (type) {
+	case MS5611:
+		st->compensate_temp_and_pressure =
+			ms5611_temp_and_pressure_compensate;
+		break;
+	case MS5607:
+		st->compensate_temp_and_pressure =
+			ms5607_temp_and_pressure_compensate;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	st->temp_osr =
 		&ms5611_avail_temp_osr[ARRAY_SIZE(ms5611_avail_temp_osr) - 1];
 	st->pressure_osr =
-- 
2.34.1

[PATCH 2/3] iio: pressure: ms5611: changed hardcoded SPI speed to value limited

[Mitja Spes]

Don't hardcode the ms5611 SPI speed, limit it instead.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/pressure/ms5611_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/ms5611_spi.c b/drivers/iio/pressure/ms5611_spi.c
index 281b08398720..1b1e863783ec 100644
--- a/drivers/iio/pressure/ms5611_spi.c
+++ b/drivers/iio/pressure/ms5611_spi.c
@@ -91,7 +91,7 @@ static int ms5611_spi_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, indio_dev);
 
 	spi->mode = SPI_MODE_0;
-	spi->max_speed_hz = 20000000;
+	spi->max_speed_hz = min(spi->max_speed_hz, 20000000U);
 	spi->bits_per_word = 8;
 	ret = spi_setup(spi);
 	if (ret < 0)
-- 
2.34.1

[PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Mitja Spes]

Added max SPI frequency setting to the example. It is now honored by the
driver.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml b/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
index 4f06707450bf..08bd06e6dabe 100644
--- a/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
@@ -52,6 +52,7 @@ examples:
             compatible = "meas,ms5611";
             reg = <0>;
             vdd-supply = <&ldo_3v3_gnss>;
+            spi-max-frequency = <20000000>;
         };
     };
 ...
-- 
2.34.1

Re: [PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 3:55 PM Mitja Spes <mitja@lxnav.com> wrote:
>
> Added max SPI frequency setting to the example. It is now honored by the
> driver.

Is it possible to add a constraint here? So the schema validator will
issue the warning / error if the speed is too high.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] iio: pressure: ms5611: fixed value compensation bug

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 3:55 PM Mitja Spes <mitja@lxnav.com> wrote:
>
> When using multiple instances of this driver the compensation PROM was
> overwritten by the last initialized sensor. Now each sensor has own PROM

its own

> storage.

This change is just a good example why the chip_info should always be
static const, so the volatile data should come through the different
sources.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Krzysztof Kozlowski]

On 19/10/2022 08:52, Mitja Spes wrote:
> Added max SPI frequency setting to the example. It is now honored by the
> driver.

Use subject prefixes matching the subsystem (git log --oneline -- ...).

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> 
> Signed-off-by: Mitja Spes <mitja@lxnav.com>
> ---
>  Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml b/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
> index 4f06707450bf..08bd06e6dabe 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/meas,ms5611.yaml
> @@ -52,6 +52,7 @@ examples:
>              compatible = "meas,ms5611";
>              reg = <0>;
>              vdd-supply = <&ldo_3v3_gnss>;
> +            spi-max-frequency = <20000000>;

Whether it is honored by driver it matters less. More important is how
hardware can handle it. This should be included in the bindings/properties.

Best regards,
Krzysztof

Re: [PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Mitja Špes]

On Wed, Oct 19, 2022 at 3:23 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 3:55 PM Mitja Spes <mitja@lxnav.com> wrote:
> >
> > Added max SPI frequency setting to the example. It is now honored by the
> > driver.
>
> Is it possible to add a constraint here? So the schema validator will
> issue the warning / error if the speed is too high.

The constraint is already there:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/pressure/meas%2Cms5611.yaml
```
spi-max-frequency:
    maximum: 20000000
```

But in any case the patch 2/3 limits the max frequency.

Kind regards,
Mitja Spes

Re: [PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Mitja Špes]

On Wed, Oct 19, 2022 at 5:49 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

> Use subject prefixes matching the subsystem (git log --oneline -- ...).

Will do.

> Whether it is honored by driver it matters less. More important is how
> hardware can handle it. This should be included in the bindings/properties.

The hardware handles frequencies up to 20MHz. That constraint is already
written in meas,ms5611.yaml.
What my patch 2 does is allow the user to set a lower frequency and the patch
3 just emphasises that in the example.

Kind regards,
Mitja

Re: [PATCH 3/3] doc: iio: pressure: ms5611: added max SPI frequency setting to the example

[Krzysztof Kozlowski]

On 20/10/2022 01:40, Mitja Špes wrote:
> On Wed, Oct 19, 2022 at 5:49 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> 
>> Use subject prefixes matching the subsystem (git log --oneline -- ...).
> 
> Will do.
> 
>> Whether it is honored by driver it matters less. More important is how
>> hardware can handle it. This should be included in the bindings/properties.
> 
> The hardware handles frequencies up to 20MHz. That constraint is already
> written in meas,ms5611.yaml.
> What my patch 2 does is allow the user to set a lower frequency and the patch
> 3 just emphasises that in the example.

Then I am not sure whether it is worth changing. This is just an
example. Old value is correct.

Best regards,
Krzysztof