[PATCH 0/6] iio:pressure:ms5637: add ms5803 support

[PATCH 0/6] iio:pressure:ms5637: add ms5803 support

[Alexandre Belloni]

Hello,

This series adds support for the Measurement Specialities ms5803. It is
very similar to the ms5805 but has a different PROM layout (which I
suspect predates the ms5805 PROM layout). Also it supports less
frequency sampling options.

After a bit of preparatory work in the ms5637 driver and its common
library, mainly to handle the PROM layou and sample frequencies, adding
support is trivial.

Alexandre Belloni (6):
  iio:pressure:ms5637: switch to probe_new
  iio:pressure:ms5637: introduce hardware differentiation
  iio:pressure:ms5637: limit available sample frequencies
  iio:common:ms_sensors:ms_sensors_i2c: rework CRC calculation helper
  iio:common:ms_sensors:ms_sensors_i2c: add support for alternative PROM
    layout
  iio:pressure:ms5637: add ms5803 support

 .../devicetree/bindings/trivial-devices.yaml  |  2 +
 .../iio/common/ms_sensors/ms_sensors_i2c.c    | 76 +++++++++++++++----
 .../iio/common/ms_sensors/ms_sensors_i2c.h    | 15 +++-
 drivers/iio/pressure/ms5637.c                 | 73 +++++++++++++-----
 4 files changed, 128 insertions(+), 38 deletions(-)

-- 
2.28.0

[PATCH 1/6] iio:pressure:ms5637: switch to probe_new

[Alexandre Belloni]

Switch to the modern i2c probe_new callback and drop the i2c_device_id
array.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/pressure/ms5637.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
index 5b59a4137d32..7c3f0ccd917c 100644
--- a/drivers/iio/pressure/ms5637.c
+++ b/drivers/iio/pressure/ms5637.c
@@ -126,8 +126,7 @@ static const struct iio_info ms5637_info = {
 	.attrs = &ms5637_attribute_group,
 };
 
-static int ms5637_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int ms5637_probe(struct i2c_client *client)
 {
 	struct ms_tp_dev *dev_data;
 	struct iio_dev *indio_dev;
@@ -152,7 +151,7 @@ static int ms5637_probe(struct i2c_client *client,
 	mutex_init(&dev_data->lock);
 
 	indio_dev->info = &ms5637_info;
-	indio_dev->name = id->name;
+	indio_dev->name = device_get_match_data(&client->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ms5637_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ms5637_channels);
@@ -170,27 +169,17 @@ static int ms5637_probe(struct i2c_client *client,
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
-static const struct i2c_device_id ms5637_id[] = {
-	{"ms5637", 0},
-	{"ms5805", 0},
-	{"ms5837", 0},
-	{"ms8607-temppressure", 0},
-	{}
-};
-MODULE_DEVICE_TABLE(i2c, ms5637_id);
-
 static const struct of_device_id ms5637_of_match[] = {
-	{ .compatible = "meas,ms5637", },
-	{ .compatible = "meas,ms5805", },
-	{ .compatible = "meas,ms5837", },
-	{ .compatible = "meas,ms8607-temppressure", },
+	{ .compatible = "meas,ms5637", .data = "ms5637" },
+	{ .compatible = "meas,ms5805", .data = "ms5805" },
+	{ .compatible = "meas,ms5837", .data = "ms5837" },
+	{ .compatible = "meas,ms8607-temppressure", .data = "ms8607-temppressure" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ms5637_of_match);
 
 static struct i2c_driver ms5637_driver = {
-	.probe = ms5637_probe,
-	.id_table = ms5637_id,
+	.probe_new = ms5637_probe,
 	.driver = {
 		   .name = "ms5637",
 		   .of_match_table = ms5637_of_match,
-- 
2.28.0

Re: [PATCH 1/6] iio:pressure:ms5637: switch to probe_new

[Andy Shevchenko]

On Thu, Dec 10, 2020 at 2:01 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Switch to the modern i2c probe_new callback and drop the i2c_device_id
> array.

First part is okay.
The second is interesting. It depends if we would like to keep a
possibility to instantiate devices from user space (strictly speaking
it's an ABI breakage).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] iio:pressure:ms5637: switch to probe_new

[Jonathan Cameron]

On Sat, 12 Dec 2020 15:26:17 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 10, 2020 at 2:01 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Switch to the modern i2c probe_new callback and drop the i2c_device_id
> > array.  
> 
> First part is okay.
> The second is interesting. It depends if we would like to keep a
> possibility to instantiate devices from user space (strictly speaking
> it's an ABI breakage).
> 
We've also been bitten by this recently via greybus which does
it's instantiations using the i2c_device_id table as I understand it.
That's resulted in us reverting a few specific cases where we'd
done pretty much what you have done here.

So drop that part of the change.

Thanks,

Jonathan

[PATCH 2/6] iio:pressure:ms5637: introduce hardware differentiation

[Alexandre Belloni]

Some sensors in the ms58xx family have a different PROM length and a
different number of available resolution. introduce struct ms_tp_hw_data to
handle those differences.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 .../iio/common/ms_sensors/ms_sensors_i2c.h    | 11 ++++++
 drivers/iio/pressure/ms5637.c                 | 35 +++++++++++++++----
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
index bad09c80e47a..f4a88148c113 100644
--- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
+++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
@@ -25,6 +25,16 @@ struct ms_ht_dev {
 	u8 res_index;
 };
 
+/**
+ * struct ms_hw_data - Temperature/Pressure sensor hardware data
+ * @prom_len:		number of words in the PROM
+ * @max_res_index:	maximum sensor resolution index
+ */
+struct ms_tp_hw_data {
+	u8 prom_len;
+	u8 max_res_index;
+};
+
 /**
  * struct ms_tp_dev - Temperature/Pressure sensor device structure
  * @client:	i2c client
@@ -36,6 +46,7 @@ struct ms_ht_dev {
 struct ms_tp_dev {
 	struct i2c_client *client;
 	struct mutex lock;
+	const struct ms_tp_hw_data *hw;
 	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB + 1];
 	u8 res_index;
 };
diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
index 7c3f0ccd917c..351bfdb24fb4 100644
--- a/drivers/iio/pressure/ms5637.c
+++ b/drivers/iio/pressure/ms5637.c
@@ -30,6 +30,11 @@
 
 #include "../common/ms_sensors/ms_sensors_i2c.h"
 
+struct ms_tp_data {
+	const char *name;
+	const struct ms_tp_hw_data *hw;
+};
+
 static const int ms5637_samp_freq[6] = { 960, 480, 240, 120, 60, 30 };
 /* String copy of the above const for readability purpose */
 static const char ms5637_show_samp_freq[] = "960 480 240 120 60 30";
@@ -128,6 +133,7 @@ static const struct iio_info ms5637_info = {
 
 static int ms5637_probe(struct i2c_client *client)
 {
+	const struct ms_tp_data *data = device_get_match_data(&client->dev);
 	struct ms_tp_dev *dev_data;
 	struct iio_dev *indio_dev;
 	int ret;
@@ -147,11 +153,12 @@ static int ms5637_probe(struct i2c_client *client)
 
 	dev_data = iio_priv(indio_dev);
 	dev_data->client = client;
-	dev_data->res_index = 5;
+	dev_data->res_index = data->hw->max_res_index;
+	dev_data->hw = data->hw;
 	mutex_init(&dev_data->lock);
 
 	indio_dev->info = &ms5637_info;
-	indio_dev->name = device_get_match_data(&client->dev);
+	indio_dev->name = data->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ms5637_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ms5637_channels);
@@ -169,11 +176,27 @@ static int ms5637_probe(struct i2c_client *client)
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
+static const struct ms_tp_hw_data ms5637_hw_data  = {
+	.prom_len = 7,
+	.max_res_index = 5
+};
+
+static const struct ms_tp_data ms5637_data = { .name = "ms5637", .hw = &ms5637_hw_data };
+
+static const struct ms_tp_data ms5805_data = { .name = "ms5805", .hw = &ms5637_hw_data };
+
+static const struct ms_tp_data ms5837_data = { .name = "ms5837", .hw = &ms5637_hw_data };
+
+static const struct ms_tp_data ms8607_data = {
+	.name = "ms8607-temppressure",
+	.hw = &ms5637_hw_data,
+};
+
 static const struct of_device_id ms5637_of_match[] = {
-	{ .compatible = "meas,ms5637", .data = "ms5637" },
-	{ .compatible = "meas,ms5805", .data = "ms5805" },
-	{ .compatible = "meas,ms5837", .data = "ms5837" },
-	{ .compatible = "meas,ms8607-temppressure", .data = "ms8607-temppressure" },
+	{ .compatible = "meas,ms5637", .data = &ms5637_data },
+	{ .compatible = "meas,ms5805", .data = &ms5805_data },
+	{ .compatible = "meas,ms5837", .data = &ms5837_data },
+	{ .compatible = "meas,ms8607-temppressure", .data = &ms8607_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ms5637_of_match);
-- 
2.28.0

Re: [PATCH 2/6] iio:pressure:ms5637: introduce hardware differentiation

[Jonathan Cameron]

On Thu, 10 Dec 2020 00:48:53 +0100
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Some sensors in the ms58xx family have a different PROM length and a
> different number of available resolution. introduce struct ms_tp_hw_data to

Introduce

> handle those differences.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  .../iio/common/ms_sensors/ms_sensors_i2c.h    | 11 ++++++
>  drivers/iio/pressure/ms5637.c                 | 35 +++++++++++++++----
>  2 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> index bad09c80e47a..f4a88148c113 100644
> --- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> +++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
> @@ -25,6 +25,16 @@ struct ms_ht_dev {
>  	u8 res_index;
>  };
>  
> +/**
> + * struct ms_hw_data - Temperature/Pressure sensor hardware data
> + * @prom_len:		number of words in the PROM
> + * @max_res_index:	maximum sensor resolution index
> + */
> +struct ms_tp_hw_data {
> +	u8 prom_len;
> +	u8 max_res_index;
> +};
> +
>  /**
>   * struct ms_tp_dev - Temperature/Pressure sensor device structure
>   * @client:	i2c client
> @@ -36,6 +46,7 @@ struct ms_ht_dev {
>  struct ms_tp_dev {
>  	struct i2c_client *client;
>  	struct mutex lock;
> +	const struct ms_tp_hw_data *hw;
>  	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB + 1];
>  	u8 res_index;
>  };
> diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
> index 7c3f0ccd917c..351bfdb24fb4 100644
> --- a/drivers/iio/pressure/ms5637.c
> +++ b/drivers/iio/pressure/ms5637.c
> @@ -30,6 +30,11 @@
>  
>  #include "../common/ms_sensors/ms_sensors_i2c.h"
>  
> +struct ms_tp_data {
> +	const char *name;
> +	const struct ms_tp_hw_data *hw;
> +};
> +
>  static const int ms5637_samp_freq[6] = { 960, 480, 240, 120, 60, 30 };
>  /* String copy of the above const for readability purpose */
>  static const char ms5637_show_samp_freq[] = "960 480 240 120 60 30";
> @@ -128,6 +133,7 @@ static const struct iio_info ms5637_info = {
>  
>  static int ms5637_probe(struct i2c_client *client)
>  {
> +	const struct ms_tp_data *data = device_get_match_data(&client->dev);

As a follow up to the earlier fun with greybus etc, have to jump through
some hoops to have a fallback here if we have a firmware type that can't
do get_match_data driver/base/sw_node.c is the one greybus is using.

We have drivers that don't do this because frankly I didn't know about it
until a month or two ago.  However, I'm not keen to introduce any
more.

Thanks,

Jonathan

>  	struct ms_tp_dev *dev_data;
>  	struct iio_dev *indio_dev;
>  	int ret;
> @@ -147,11 +153,12 @@ static int ms5637_probe(struct i2c_client *client)
>  
>  	dev_data = iio_priv(indio_dev);
>  	dev_data->client = client;
> -	dev_data->res_index = 5;
> +	dev_data->res_index = data->hw->max_res_index;
> +	dev_data->hw = data->hw;
>  	mutex_init(&dev_data->lock);
>  
>  	indio_dev->info = &ms5637_info;
> -	indio_dev->name = device_get_match_data(&client->dev);
> +	indio_dev->name = data->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = ms5637_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ms5637_channels);
> @@ -169,11 +176,27 @@ static int ms5637_probe(struct i2c_client *client)
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
> +static const struct ms_tp_hw_data ms5637_hw_data  = {
> +	.prom_len = 7,
> +	.max_res_index = 5
> +};
> +
> +static const struct ms_tp_data ms5637_data = { .name = "ms5637", .hw = &ms5637_hw_data };
> +
> +static const struct ms_tp_data ms5805_data = { .name = "ms5805", .hw = &ms5637_hw_data };
> +
> +static const struct ms_tp_data ms5837_data = { .name = "ms5837", .hw = &ms5637_hw_data };
> +
> +static const struct ms_tp_data ms8607_data = {
> +	.name = "ms8607-temppressure",
> +	.hw = &ms5637_hw_data,
> +};
> +
>  static const struct of_device_id ms5637_of_match[] = {
> -	{ .compatible = "meas,ms5637", .data = "ms5637" },
> -	{ .compatible = "meas,ms5805", .data = "ms5805" },
> -	{ .compatible = "meas,ms5837", .data = "ms5837" },
> -	{ .compatible = "meas,ms8607-temppressure", .data = "ms8607-temppressure" },
> +	{ .compatible = "meas,ms5637", .data = &ms5637_data },
> +	{ .compatible = "meas,ms5805", .data = &ms5805_data },
> +	{ .compatible = "meas,ms5837", .data = &ms5837_data },
> +	{ .compatible = "meas,ms8607-temppressure", .data = &ms8607_data },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, ms5637_of_match);

Re: [PATCH 2/6] iio:pressure:ms5637: introduce hardware differentiation

[Alexandre Belloni]

On 13/12/2020 17:12:37+0000, Jonathan Cameron wrote:
> >  static const int ms5637_samp_freq[6] = { 960, 480, 240, 120, 60, 30 };
> >  /* String copy of the above const for readability purpose */
> >  static const char ms5637_show_samp_freq[] = "960 480 240 120 60 30";
> > @@ -128,6 +133,7 @@ static const struct iio_info ms5637_info = {
> >  
> >  static int ms5637_probe(struct i2c_client *client)
> >  {
> > +	const struct ms_tp_data *data = device_get_match_data(&client->dev);
> 
> As a follow up to the earlier fun with greybus etc, have to jump through
> some hoops to have a fallback here if we have a firmware type that can't
> do get_match_data driver/base/sw_node.c is the one greybus is using.
> 
> We have drivers that don't do this because frankly I didn't know about it
> until a month or two ago.  However, I'm not keen to introduce any
> more.
> 

Couldn't greybus be fixed in that regard? Using the i2c_device_id has
been deprecated for a while now.

what we could do is only provide ms5803 support when there is an
of_node. So this doesn't break the ABI and doesn't break greybus and at
the same time doesn't unnecessarily add complexity to the probe for
something that will probably never be used.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 3/6] iio:pressure:ms5637: limit available sample frequencies

[Alexandre Belloni]

Avoid exposing all the sampling frequencies for chip that only support a
subset.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/pressure/ms5637.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
index 351bfdb24fb4..2943b88734b3 100644
--- a/drivers/iio/pressure/ms5637.c
+++ b/drivers/iio/pressure/ms5637.c
@@ -36,8 +36,19 @@ struct ms_tp_data {
 };
 
 static const int ms5637_samp_freq[6] = { 960, 480, 240, 120, 60, 30 };
-/* String copy of the above const for readability purpose */
-static const char ms5637_show_samp_freq[] = "960 480 240 120 60 30";
+
+static ssize_t ms5637_show_samp_freq(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ms_tp_dev *dev_data = iio_priv(indio_dev);
+	int i, len = 0;
+
+	for (i = 0; i <= dev_data->hw->max_res_index; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", ms5637_samp_freq[i]);
+	buf[len - 1] = '\n';
+
+	return len;
+}
 
 static int ms5637_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *channel, int *val,
@@ -114,10 +125,10 @@ static const struct iio_chan_spec ms5637_channels[] = {
 	}
 };
 
-static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(ms5637_show_samp_freq);
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(ms5637_show_samp_freq);
 
 static struct attribute *ms5637_attributes[] = {
-	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
 
-- 
2.28.0

Re: [PATCH 3/6] iio:pressure:ms5637: limit available sample frequencies

[Andy Shevchenko]

On Thu, Dec 10, 2020 at 2:03 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Avoid exposing all the sampling frequencies for chip that only support a
> subset.

> +static ssize_t ms5637_show_samp_freq(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct ms_tp_dev *dev_data = iio_priv(indio_dev);
> +       int i, len = 0;
> +
> +       for (i = 0; i <= dev_data->hw->max_res_index; i++)
> +               len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", ms5637_samp_freq[i]);

Doesn't IIO core have a helper?
Also, it's better to use sysfs_emit().

> +       buf[len - 1] = '\n';
> +
> +       return len;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] iio:pressure:ms5637: limit available sample frequencies

[Jonathan Cameron]

On Sat, 12 Dec 2020 20:26:16 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Dec 10, 2020 at 2:03 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Avoid exposing all the sampling frequencies for chip that only support a
> > subset.  
> 
> > +static ssize_t ms5637_show_samp_freq(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +       struct ms_tp_dev *dev_data = iio_priv(indio_dev);
> > +       int i, len = 0;
> > +
> > +       for (i = 0; i <= dev_data->hw->max_res_index; i++)
> > +               len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", ms5637_samp_freq[i]);  
> 
> Doesn't IIO core have a helper?
read_avail() callback and matching masks provide the infrastructure to do this.
It's not a huge saving in code by the time you've wired it up, but has the
advantage that consumer drivers can get hold of the values.  Mind you
I'm not sure what consumers we are likely to get for pressure drivers any
time soon.

> Also, it's better to use sysfs_emit().

New one to me. Thanks.  sysfs_emit_at() here I guess.

Nice.

Jonathan

> 
> > +       buf[len - 1] = '\n';
> > +
> > +       return len;
> > +}  
> 

[PATCH 4/6] iio:common:ms_sensors:ms_sensors_i2c: rework CRC calculation helper

[Alexandre Belloni]

The CRC calculation always happens on 8 words which is why there is an
extra element in the prom array of struct ms_tp_dev. However, on ms5637 and
similar, only 7 words are readable.

Then, set MS_SENSORS_TP_PROM_WORDS_NB to 8 and stop passing a len parameter
to ms_sensors_tp_crc_valid as this simply hide the fact that it is
hardcoded.

Finally, use the newly introduced hw->prom_len to know how many words can be
read.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/common/ms_sensors/ms_sensors_i2c.c | 12 +++++-------
 drivers/iio/common/ms_sensors/ms_sensors_i2c.h |  4 ++--
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
index b9e2038d05ef..872f90459e2e 100644
--- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
+++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
@@ -493,19 +493,18 @@ EXPORT_SYMBOL(ms_sensors_ht_read_humidity);
  *     This function is only used when reading PROM coefficients
  *
  * @prom:	pointer to PROM coefficients array
- * @len:	length of PROM coefficients array
  *
  * Return: True if CRC is ok.
  */
-static bool ms_sensors_tp_crc_valid(u16 *prom, u8 len)
+static bool ms_sensors_tp_crc_valid(u16 *prom)
 {
 	unsigned int cnt, n_bit;
 	u16 n_rem = 0x0000, crc_read = prom[0], crc = (*prom & 0xF000) >> 12;
 
-	prom[len - 1] = 0;
+	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
 	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
 
-	for (cnt = 0; cnt < len * 2; cnt++) {
+	for (cnt = 0; cnt < MS_SENSORS_TP_PROM_WORDS_NB * 2; cnt++) {
 		if (cnt % 2 == 1)
 			n_rem ^= prom[cnt >> 1] & 0x00FF;
 		else
@@ -537,7 +536,7 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
 {
 	int i, ret;
 
-	for (i = 0; i < MS_SENSORS_TP_PROM_WORDS_NB; i++) {
+	for (i = 0; i < dev_data->hw->prom_len; i++) {
 		ret = ms_sensors_read_prom_word(
 			dev_data->client,
 			MS_SENSORS_TP_PROM_READ + (i << 1),
@@ -547,8 +546,7 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
 			return ret;
 	}
 
-	if (!ms_sensors_tp_crc_valid(dev_data->prom,
-				     MS_SENSORS_TP_PROM_WORDS_NB + 1)) {
+	if (!ms_sensors_tp_crc_valid(dev_data->prom)) {
 		dev_err(&dev_data->client->dev,
 			"Calibration coefficients crc check error\n");
 		return -ENODEV;
diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
index f4a88148c113..f15b973f27c6 100644
--- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
+++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.h
@@ -11,7 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/mutex.h>
 
-#define MS_SENSORS_TP_PROM_WORDS_NB		7
+#define MS_SENSORS_TP_PROM_WORDS_NB		8
 
 /**
  * struct ms_ht_dev - Humidity/Temperature sensor device structure
@@ -47,7 +47,7 @@ struct ms_tp_dev {
 	struct i2c_client *client;
 	struct mutex lock;
 	const struct ms_tp_hw_data *hw;
-	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB + 1];
+	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB];
 	u8 res_index;
 };
 
-- 
2.28.0

[PATCH 5/6] iio:common:ms_sensors:ms_sensors_i2c: add support for alternative PROM layout

[Alexandre Belloni]

Currently, only the 112bit PROM on 7 words is supported. However the ms58xx
family also have devices with a 128bit PROM on 8 words. See AN520:
C-CODE EXAMPLE FOR MS56XX, MS57XX (EXCEPT ANALOG SENSOR), AND MS58XX SERIES
PRESSURE SENSORS and the various device datasheets.

The difference is that the CRC is the 4 LSBs of word7 instead of being the
4 MSBs of word0.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 .../iio/common/ms_sensors/ms_sensors_i2c.c    | 70 ++++++++++++++++---
 1 file changed, 59 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
index 872f90459e2e..d97ca3e1b1d7 100644
--- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
+++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
@@ -488,21 +488,18 @@ int ms_sensors_ht_read_humidity(struct ms_ht_dev *dev_data,
 EXPORT_SYMBOL(ms_sensors_ht_read_humidity);
 
 /**
- * ms_sensors_tp_crc_valid() - CRC check function for
+ * ms_sensors_tp_crc4() - Calculate PROM CRC for
  *     Temperature and pressure devices.
  *     This function is only used when reading PROM coefficients
  *
  * @prom:	pointer to PROM coefficients array
  *
- * Return: True if CRC is ok.
+ * Return: CRC.
  */
-static bool ms_sensors_tp_crc_valid(u16 *prom)
+static u8 ms_sensors_tp_crc4(u16 *prom)
 {
 	unsigned int cnt, n_bit;
-	u16 n_rem = 0x0000, crc_read = prom[0], crc = (*prom & 0xF000) >> 12;
-
-	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
-	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
+	u16 n_rem = 0x0000;
 
 	for (cnt = 0; cnt < MS_SENSORS_TP_PROM_WORDS_NB * 2; cnt++) {
 		if (cnt % 2 == 1)
@@ -517,10 +514,55 @@ static bool ms_sensors_tp_crc_valid(u16 *prom)
 				n_rem <<= 1;
 		}
 	}
-	n_rem >>= 12;
-	prom[0] = crc_read;
 
-	return n_rem == crc;
+	return n_rem >> 12;
+}
+
+/**
+ * ms_sensors_tp_crc_valid_112() - CRC check function for
+ *     Temperature and pressure devices for 112bit PROM.
+ *     This function is only used when reading PROM coefficients
+ *
+ * @prom:	pointer to PROM coefficients array
+ *
+ * Return: CRC.
+ */
+static bool ms_sensors_tp_crc_valid_112(u16 *prom)
+{
+	u16 w0 = prom[0], crc_read = (w0 & 0xF000) >> 12;
+	u8 crc;
+
+	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
+	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
+
+	crc = ms_sensors_tp_crc4(prom);
+
+	prom[0] = w0;
+
+	return crc == crc_read;
+}
+
+/**
+ * ms_sensors_tp_crc_valid_128() - CRC check function for
+ *     Temperature and pressure devices for 128bit PROM.
+ *     This function is only used when reading PROM coefficients
+ *
+ * @prom:	pointer to PROM coefficients array
+ *
+ * Return: CRC.
+ */
+static bool ms_sensors_tp_crc_valid_128(u16 *prom)
+{
+	u16 w7 = prom[7], crc_read = w7 & 0x000F;
+	u8 crc;
+
+	prom[7] &= 0xFF00;      /* Clear the CRC and LSB part */
+
+	crc = ms_sensors_tp_crc4(prom);
+
+	prom[7] = w7;
+
+	return crc == crc_read;
 }
 
 /**
@@ -535,6 +577,7 @@ static bool ms_sensors_tp_crc_valid(u16 *prom)
 int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
 {
 	int i, ret;
+	bool valid;
 
 	for (i = 0; i < dev_data->hw->prom_len; i++) {
 		ret = ms_sensors_read_prom_word(
@@ -546,7 +589,12 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
 			return ret;
 	}
 
-	if (!ms_sensors_tp_crc_valid(dev_data->prom)) {
+	if (dev_data->hw->prom_len == 8)
+		valid = ms_sensors_tp_crc_valid_128(dev_data->prom);
+	else
+		valid = ms_sensors_tp_crc_valid_112(dev_data->prom);
+
+	if (!valid) {
 		dev_err(&dev_data->client->dev,
 			"Calibration coefficients crc check error\n");
 		return -ENODEV;
-- 
2.28.0

Re: [PATCH 5/6] iio:common:ms_sensors:ms_sensors_i2c: add support for alternative PROM layout

[Jonathan Cameron]

On Thu, 10 Dec 2020 00:48:56 +0100
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Currently, only the 112bit PROM on 7 words is supported. However the ms58xx

PROM _with_ 7 words (perhaps?)

> family also have devices with a 128bit PROM on 8 words. See AN520:
> C-CODE EXAMPLE FOR MS56XX, MS57XX (EXCEPT ANALOG SENSOR), AND MS58XX SERIES
> PRESSURE SENSORS and the various device datasheets.
> 
> The difference is that the CRC is the 4 LSBs of word7 instead of being the
> 4 MSBs of word0.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  .../iio/common/ms_sensors/ms_sensors_i2c.c    | 70 ++++++++++++++++---
>  1 file changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> index 872f90459e2e..d97ca3e1b1d7 100644
> --- a/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> +++ b/drivers/iio/common/ms_sensors/ms_sensors_i2c.c
> @@ -488,21 +488,18 @@ int ms_sensors_ht_read_humidity(struct ms_ht_dev *dev_data,
>  EXPORT_SYMBOL(ms_sensors_ht_read_humidity);
>  
>  /**
> - * ms_sensors_tp_crc_valid() - CRC check function for
> + * ms_sensors_tp_crc4() - Calculate PROM CRC for
>   *     Temperature and pressure devices.
>   *     This function is only used when reading PROM coefficients
>   *
>   * @prom:	pointer to PROM coefficients array
>   *
> - * Return: True if CRC is ok.
> + * Return: CRC.
>   */
> -static bool ms_sensors_tp_crc_valid(u16 *prom)
> +static u8 ms_sensors_tp_crc4(u16 *prom)
>  {
>  	unsigned int cnt, n_bit;
> -	u16 n_rem = 0x0000, crc_read = prom[0], crc = (*prom & 0xF000) >> 12;
> -
> -	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
> -	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
> +	u16 n_rem = 0x0000;
>  
>  	for (cnt = 0; cnt < MS_SENSORS_TP_PROM_WORDS_NB * 2; cnt++) {
>  		if (cnt % 2 == 1)
> @@ -517,10 +514,55 @@ static bool ms_sensors_tp_crc_valid(u16 *prom)
>  				n_rem <<= 1;
>  		}
>  	}
> -	n_rem >>= 12;
> -	prom[0] = crc_read;
>  
> -	return n_rem == crc;
> +	return n_rem >> 12;
> +}
> +
> +/**
> + * ms_sensors_tp_crc_valid_112() - CRC check function for
> + *     Temperature and pressure devices for 112bit PROM.
> + *     This function is only used when reading PROM coefficients
> + *
> + * @prom:	pointer to PROM coefficients array
> + *
> + * Return: CRC.

That's a bit confusing.  Perhaps return if CRC correct
Sometimes CRC is used to refer to particular bits and sometimes
to the check (i.e. whether it is right).

> + */
> +static bool ms_sensors_tp_crc_valid_112(u16 *prom)
> +{
> +	u16 w0 = prom[0], crc_read = (w0 & 0xF000) >> 12;
> +	u8 crc;
> +
> +	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
> +	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
> +
> +	crc = ms_sensors_tp_crc4(prom);
> +
> +	prom[0] = w0;
> +
> +	return crc == crc_read;
> +}
> +
> +/**
> + * ms_sensors_tp_crc_valid_128() - CRC check function for
> + *     Temperature and pressure devices for 128bit PROM.
> + *     This function is only used when reading PROM coefficients
> + *
> + * @prom:	pointer to PROM coefficients array
> + *
> + * Return: CRC.
> + */
> +static bool ms_sensors_tp_crc_valid_128(u16 *prom)
> +{
> +	u16 w7 = prom[7], crc_read = w7 & 0x000F;
> +	u8 crc;
> +
> +	prom[7] &= 0xFF00;      /* Clear the CRC and LSB part */
> +
> +	crc = ms_sensors_tp_crc4(prom);
> +
> +	prom[7] = w7;
> +
> +	return crc == crc_read;
>  }
>  
>  /**
> @@ -535,6 +577,7 @@ static bool ms_sensors_tp_crc_valid(u16 *prom)
>  int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
>  {
>  	int i, ret;
> +	bool valid;
>  
>  	for (i = 0; i < dev_data->hw->prom_len; i++) {
>  		ret = ms_sensors_read_prom_word(
> @@ -546,7 +589,12 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
>  			return ret;
>  	}
>  
> -	if (!ms_sensors_tp_crc_valid(dev_data->prom)) {
> +	if (dev_data->hw->prom_len == 8)
> +		valid = ms_sensors_tp_crc_valid_128(dev_data->prom);
> +	else
> +		valid = ms_sensors_tp_crc_valid_112(dev_data->prom);
> +
> +	if (!valid) {
>  		dev_err(&dev_data->client->dev,
>  			"Calibration coefficients crc check error\n");
>  		return -ENODEV;

Re: [PATCH 5/6] iio:common:ms_sensors:ms_sensors_i2c: add support for alternative PROM layout

[Alexandre Belloni]

On 13/12/2020 17:20:42+0000, Jonathan Cameron wrote:
> > +/**
> > + * ms_sensors_tp_crc_valid_112() - CRC check function for
> > + *     Temperature and pressure devices for 112bit PROM.
> > + *     This function is only used when reading PROM coefficients
> > + *
> > + * @prom:	pointer to PROM coefficients array
> > + *
> > + * Return: CRC.
> 
> That's a bit confusing.  Perhaps return if CRC correct
> Sometimes CRC is used to refer to particular bits and sometimes
> to the check (i.e. whether it is right).
> 

Roght, this should have been "Return: True if CRC is ok."

> > + */
> > +static bool ms_sensors_tp_crc_valid_112(u16 *prom)
> > +{
> > +	u16 w0 = prom[0], crc_read = (w0 & 0xF000) >> 12;
> > +	u8 crc;
> > +
> > +	prom[0] &= 0x0FFF;      /* Clear the CRC computation part */
> > +	prom[MS_SENSORS_TP_PROM_WORDS_NB - 1] = 0;
> > +
> > +	crc = ms_sensors_tp_crc4(prom);
> > +
> > +	prom[0] = w0;
> > +
> > +	return crc == crc_read;
> > +}
> > +
> > +/**
> > + * ms_sensors_tp_crc_valid_128() - CRC check function for
> > + *     Temperature and pressure devices for 128bit PROM.
> > + *     This function is only used when reading PROM coefficients
> > + *
> > + * @prom:	pointer to PROM coefficients array
> > + *
> > + * Return: CRC.
> > + */
> > +static bool ms_sensors_tp_crc_valid_128(u16 *prom)
> > +{
> > +	u16 w7 = prom[7], crc_read = w7 & 0x000F;
> > +	u8 crc;
> > +
> > +	prom[7] &= 0xFF00;      /* Clear the CRC and LSB part */
> > +
> > +	crc = ms_sensors_tp_crc4(prom);
> > +
> > +	prom[7] = w7;
> > +
> > +	return crc == crc_read;
> >  }
> >  
> >  /**
> > @@ -535,6 +577,7 @@ static bool ms_sensors_tp_crc_valid(u16 *prom)
> >  int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
> >  {
> >  	int i, ret;
> > +	bool valid;
> >  
> >  	for (i = 0; i < dev_data->hw->prom_len; i++) {
> >  		ret = ms_sensors_read_prom_word(
> > @@ -546,7 +589,12 @@ int ms_sensors_tp_read_prom(struct ms_tp_dev *dev_data)
> >  			return ret;
> >  	}
> >  
> > -	if (!ms_sensors_tp_crc_valid(dev_data->prom)) {
> > +	if (dev_data->hw->prom_len == 8)
> > +		valid = ms_sensors_tp_crc_valid_128(dev_data->prom);
> > +	else
> > +		valid = ms_sensors_tp_crc_valid_112(dev_data->prom);
> > +
> > +	if (!valid) {
> >  		dev_err(&dev_data->client->dev,
> >  			"Calibration coefficients crc check error\n");
> >  		return -ENODEV;
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 6/6] iio:pressure:ms5637: add ms5803 support

[Alexandre Belloni]

The ms5803 is very similar to the ms5805 but has less resolution options
and has the 128bit PROM layout.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 drivers/iio/pressure/ms5637.c                          | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index ab623ba930d5..84b0e44235c1 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -132,6 +132,8 @@ properties:
           - mcube,mc3230
             # MEMSIC 2-axis 8-bit digital accelerometer
           - memsic,mxc6225
+            # Measurement Specialities I2C pressure and temperature sensor
+          - meas,ms5803
             # Microchip differential I2C ADC, 1 Channel, 18 bit
           - microchip,mcp3421
             # Microchip differential I2C ADC, 2 Channel, 18 bit
diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
index 2943b88734b3..39830a51ca78 100644
--- a/drivers/iio/pressure/ms5637.c
+++ b/drivers/iio/pressure/ms5637.c
@@ -192,8 +192,15 @@ static const struct ms_tp_hw_data ms5637_hw_data  = {
 	.max_res_index = 5
 };
 
+static const struct ms_tp_hw_data ms5803_hw_data  = {
+	.prom_len = 8,
+	.max_res_index = 4
+};
+
 static const struct ms_tp_data ms5637_data = { .name = "ms5637", .hw = &ms5637_hw_data };
 
+static const struct ms_tp_data ms5803_data = { .name = "ms5803", .hw = &ms5803_hw_data };
+
 static const struct ms_tp_data ms5805_data = { .name = "ms5805", .hw = &ms5637_hw_data };
 
 static const struct ms_tp_data ms5837_data = { .name = "ms5837", .hw = &ms5637_hw_data };
@@ -205,6 +212,7 @@ static const struct ms_tp_data ms8607_data = {
 
 static const struct of_device_id ms5637_of_match[] = {
 	{ .compatible = "meas,ms5637", .data = &ms5637_data },
+	{ .compatible = "meas,ms5803", .data = &ms5803_data },
 	{ .compatible = "meas,ms5805", .data = &ms5805_data },
 	{ .compatible = "meas,ms5837", .data = &ms5837_data },
 	{ .compatible = "meas,ms8607-temppressure", .data = &ms8607_data },
-- 
2.28.0

Re: [PATCH 6/6] iio:pressure:ms5637: add ms5803 support

[Rob Herring]

On Thu, Dec 10, 2020 at 12:48:57AM +0100, Alexandre Belloni wrote:
> The ms5803 is very similar to the ms5805 but has less resolution options
> and has the 128bit PROM layout.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>  drivers/iio/pressure/ms5637.c                          | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index ab623ba930d5..84b0e44235c1 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -132,6 +132,8 @@ properties:
>            - mcube,mc3230
>              # MEMSIC 2-axis 8-bit digital accelerometer
>            - memsic,mxc6225
> +            # Measurement Specialities I2C pressure and temperature sensor
> +          - meas,ms5803

Alphabetical order please.

>              # Microchip differential I2C ADC, 1 Channel, 18 bit
>            - microchip,mcp3421
>              # Microchip differential I2C ADC, 2 Channel, 18 bit
> diff --git a/drivers/iio/pressure/ms5637.c b/drivers/iio/pressure/ms5637.c
> index 2943b88734b3..39830a51ca78 100644
> --- a/drivers/iio/pressure/ms5637.c
> +++ b/drivers/iio/pressure/ms5637.c
> @@ -192,8 +192,15 @@ static const struct ms_tp_hw_data ms5637_hw_data  = {
>  	.max_res_index = 5
>  };
>  
> +static const struct ms_tp_hw_data ms5803_hw_data  = {
> +	.prom_len = 8,
> +	.max_res_index = 4
> +};
> +
>  static const struct ms_tp_data ms5637_data = { .name = "ms5637", .hw = &ms5637_hw_data };
>  
> +static const struct ms_tp_data ms5803_data = { .name = "ms5803", .hw = &ms5803_hw_data };
> +
>  static const struct ms_tp_data ms5805_data = { .name = "ms5805", .hw = &ms5637_hw_data };
>  
>  static const struct ms_tp_data ms5837_data = { .name = "ms5837", .hw = &ms5637_hw_data };
> @@ -205,6 +212,7 @@ static const struct ms_tp_data ms8607_data = {
>  
>  static const struct of_device_id ms5637_of_match[] = {
>  	{ .compatible = "meas,ms5637", .data = &ms5637_data },
> +	{ .compatible = "meas,ms5803", .data = &ms5803_data },
>  	{ .compatible = "meas,ms5805", .data = &ms5805_data },
>  	{ .compatible = "meas,ms5837", .data = &ms5837_data },
>  	{ .compatible = "meas,ms8607-temppressure", .data = &ms8607_data },
> -- 
> 2.28.0
> 

Re: [PATCH 6/6] iio:pressure:ms5637: add ms5803 support

[Alexandre Belloni]

On 10/12/2020 21:34:13-0600, Rob Herring wrote:
> On Thu, Dec 10, 2020 at 12:48:57AM +0100, Alexandre Belloni wrote:
> > The ms5803 is very similar to the ms5805 but has less resolution options
> > and has the 128bit PROM layout.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> >  drivers/iio/pressure/ms5637.c                          | 8 ++++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index ab623ba930d5..84b0e44235c1 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -132,6 +132,8 @@ properties:
> >            - mcube,mc3230
> >              # MEMSIC 2-axis 8-bit digital accelerometer
> >            - memsic,mxc6225
> > +            # Measurement Specialities I2C pressure and temperature sensor
> > +          - meas,ms5803
> 
> Alphabetical order please.

Ah, this was my intention, it will conflict with the togreg branch of
iio.git anyway.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com