2022-10-19 14:44:21

by Mitja Špes

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

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 <[email protected]>
---
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


2022-10-19 14:58:03

by Mitja Špes

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

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

Signed-off-by: Mitja Spes <[email protected]>
---
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

2022-10-19 16:07:12

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> 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

2022-10-20 06:37:13

by Mitja Špes

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

On Wed, Oct 19, 2022 at 5:49 PM Krzysztof Kozlowski
<[email protected]> 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

2022-10-20 12:27:45

by Krzysztof Kozlowski

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

On 20/10/2022 01:40, Mitja Špes wrote:
> On Wed, Oct 19, 2022 at 5:49 PM Krzysztof Kozlowski
> <[email protected]> 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