[PATCH 0/4] iio: adc: mcp3422 improvements

[PATCH 0/4] iio: adc: mcp3422 improvements

[Mitja Spes]

Fixes:
* iio: adc: mcp3422: fix scale read bug
  Scale was always returned for the last read input instead of the specified
  channel.

Improvements:
* iio: adc: mcp3422: allow setting gain and sampling per channel
  Sampling was the same for all channels. This patch adds the ability to select
  different gain and sampling per channel. They can be set together via scale
  attribute.
  Sampling can be set also via the standalone attribute which is now per
  channel. This might be a breaking change for some.

* iio: adc: mcp3422: add hardware gain attribute
  Setting via scale is cumbersome. This patch just adds a concise way to set
  the gain, since sampling can already be set separately.

* iio: adc: mcp3422: reduce sleep for fast sampling rates
  msleep can produce a delay which is fi. 30ms off the mark. This patch uses
  usleep_range for the higher sampling rates.

Mitja Spes (4):
  iio: adc: mcp3422: fix scale read bug
  iio: adc: mcp3422: allow setting gain and sampling per channel
  iio: adc: mcp3422: add hardware gain attribute
  iio: adc: mcp3422: reduce sleep for fast sampling rates

 drivers/iio/adc/mcp3422.c | 161 ++++++++++++++++++++++++--------------
 1 file changed, 103 insertions(+), 58 deletions(-)

-- 
2.34.1

[PATCH 1/4] iio: adc: mcp3422: fix scale read bug

[Mitja Spes]

Scale was returned for currently active channel instead of the specified
channel.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
Fixes: 07914c84ba30 ("iio: adc: Add driver for Microchip MCP3422/3/4 high resolution ADC")
---
 drivers/iio/adc/mcp3422.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index da353dcb1e9d..3d53de300c89 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 	struct mcp3422 *adc = iio_priv(iio);
 	int err;
 
+	u8 req_channel = channel->channel;
 	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
-	u8 pga		 = MCP3422_PGA(adc->config);
+	u8 pga		 = adc->pga[req_channel];
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -175,7 +176,6 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-
 		*val1 = 0;
 		*val2 = mcp3422_scales[sample_rate][pga];
 		return IIO_VAL_INT_PLUS_NANO;
-- 
2.34.1

Re: [PATCH 1/4] iio: adc: mcp3422: fix scale read bug

[Jonathan Cameron]

On Fri, 11 Nov 2022 12:26:53 +0100
Mitja Spes <mitja@lxnav.com> wrote:

> Scale was returned for currently active channel instead of the specified
> channel.
> 
> Signed-off-by: Mitja Spes <mitja@lxnav.com>
> Fixes: 07914c84ba30 ("iio: adc: Add driver for Microchip MCP3422/3/4 high resolution ADC")
Hi Mitja,

One trivial comment inline. I might tidy that up whilst applying if others are
otherwise happy with this patch.  If you do a v2 for some other reason please
get rid of that unrelated change.

Jonathan

> ---
>  drivers/iio/adc/mcp3422.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index da353dcb1e9d..3d53de300c89 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  	struct mcp3422 *adc = iio_priv(iio);
>  	int err;
>  
> +	u8 req_channel = channel->channel;
>  	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> -	u8 pga		 = MCP3422_PGA(adc->config);
> +	u8 pga		 = adc->pga[req_channel];
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -175,7 +176,6 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -
Unrelated change.  No problem with cleaning this up but definitely not
in a fix patch!

>  		*val1 = 0;
>  		*val2 = mcp3422_scales[sample_rate][pga];
>  		return IIO_VAL_INT_PLUS_NANO;

Re: [PATCH 1/4] iio: adc: mcp3422: fix scale read bug

[Mitja Špes]

Hi Jonathan,

On Sat, Nov 12, 2022 at 5:57 PM Jonathan Cameron <jic23@kernel.org> wrote:

> One trivial comment inline. I might tidy that up whilst applying if others are
> otherwise happy with this patch.  If you do a v2 for some other reason please
> get rid of that unrelated change.

Ok, I will move it to a styling patch and fix it in v2.

Kind regards,
Mitja

[PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Mitja Spes]

General improvements:
- allow setting gain and sampling per channel
- setting scale can also set sampling rate (combined setting)
- use per channel config setting
- do not update mcp register on setting write (we might be reading it...)
  instead it's updated on next value read
- output all scale values (sample rates x gain)

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/adc/mcp3422.c | 119 ++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 55 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 3d53de300c89..cfb629b964af 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -28,14 +28,19 @@
 #define MCP3422_CHANNEL_MASK	0x60
 #define MCP3422_PGA_MASK	0x03
 #define MCP3422_SRATE_MASK	0x0C
-#define MCP3422_SRATE_240	0x0
-#define MCP3422_SRATE_60	0x1
-#define MCP3422_SRATE_15	0x2
-#define MCP3422_SRATE_3	0x3
-#define MCP3422_PGA_1	0
-#define MCP3422_PGA_2	1
-#define MCP3422_PGA_4	2
-#define MCP3422_PGA_8	3
+
+#define MCP3422_SRATE_240	0
+#define MCP3422_SRATE_60	1
+#define MCP3422_SRATE_15	2
+#define MCP3422_SRATE_3		3
+#define MCP3422_SRATE_COUNT	4
+
+#define MCP3422_PGA_1		0
+#define MCP3422_PGA_2		1
+#define MCP3422_PGA_4		2
+#define MCP3422_PGA_8		3
+#define MCP3422_PGA_COUNT	4
+
 #define MCP3422_CONT_SAMPLING	0x10
 
 #define MCP3422_CHANNEL(config)	(((config) & MCP3422_CHANNEL_MASK) >> 5)
@@ -52,32 +57,32 @@
 		.indexed = 1, \
 		.channel = _index, \
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
-				| BIT(IIO_CHAN_INFO_SCALE), \
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+				| BIT(IIO_CHAN_INFO_SCALE) \
+				| BIT(IIO_CHAN_INFO_SAMP_FREQ), \
 	}
 
-static const int mcp3422_scales[4][4] = {
+static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
 	{ 1000000, 500000, 250000, 125000 },
 	{ 250000,  125000, 62500,  31250  },
 	{ 62500,   31250,  15625,  7812   },
 	{ 15625,   7812,   3906,   1953   } };
 
 /* Constant msleep times for data acquisitions */
-static const int mcp3422_read_times[4] = {
+static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = {
 	[MCP3422_SRATE_240] = 1000 / 240,
 	[MCP3422_SRATE_60] = 1000 / 60,
 	[MCP3422_SRATE_15] = 1000 / 15,
 	[MCP3422_SRATE_3] = 1000 / 3 };
 
 /* sample rates to integer conversion table */
-static const int mcp3422_sample_rates[4] = {
+static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = {
 	[MCP3422_SRATE_240] = 240,
 	[MCP3422_SRATE_60] = 60,
 	[MCP3422_SRATE_15] = 15,
 	[MCP3422_SRATE_3] = 3 };
 
 /* sample rates to sign extension table */
-static const int mcp3422_sign_extend[4] = {
+static const int mcp3422_sign_extend[MCP3422_SRATE_COUNT] = {
 	[MCP3422_SRATE_240] = 11,
 	[MCP3422_SRATE_60] = 13,
 	[MCP3422_SRATE_15] = 15,
@@ -87,8 +92,8 @@ static const int mcp3422_sign_extend[4] = {
 struct mcp3422 {
 	struct i2c_client *i2c;
 	u8 id;
-	u8 config;
-	u8 pga[4];
+	u8 active_config; // config currently set on mcp
+	u8 ch_config[4];  // per channel config
 	struct mutex lock;
 };
 
@@ -98,7 +103,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig)
 
 	ret = i2c_master_send(adc->i2c, &newconfig, 1);
 	if (ret > 0) {
-		adc->config = newconfig;
+		adc->active_config = newconfig;
 		ret = 0;
 	}
 
@@ -108,7 +113,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig)
 static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config)
 {
 	int ret = 0;
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
+	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->active_config);
 	u8 buf[4] = {0, 0, 0, 0};
 	u32 temp;
 
@@ -136,18 +141,13 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
 
 	mutex_lock(&adc->lock);
 
-	if (req_channel != MCP3422_CHANNEL(adc->config)) {
-		config = adc->config;
-		config &= ~MCP3422_CHANNEL_MASK;
-		config |= MCP3422_CHANNEL_VALUE(req_channel);
-		config &= ~MCP3422_PGA_MASK;
-		config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
-		ret = mcp3422_update_config(adc, config);
+	if (adc->ch_config[req_channel] != adc->active_config) {
+		ret = mcp3422_update_config(adc, adc->ch_config[req_channel]);
 		if (ret < 0) {
 			mutex_unlock(&adc->lock);
 			return ret;
 		}
-		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]);
+		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]);
 	}
 
 	ret = mcp3422_read(adc, value, &config);
@@ -164,9 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 	struct mcp3422 *adc = iio_priv(iio);
 	int err;
 
-	u8 req_channel = channel->channel;
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
-	u8 pga		 = adc->pga[req_channel];
+	u8 config = adc->ch_config[channel->channel];
+	u8 sample_rate = MCP3422_SAMPLE_RATE(config);
+	u8 pga = MCP3422_PGA(config);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -181,7 +181,7 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 		return IIO_VAL_INT_PLUS_NANO;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val1 = mcp3422_sample_rates[MCP3422_SAMPLE_RATE(adc->config)];
+		*val1 = mcp3422_sample_rates[sample_rate];
 		return IIO_VAL_INT;
 
 	default:
@@ -197,26 +197,25 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 {
 	struct mcp3422 *adc = iio_priv(iio);
 	u8 temp;
-	u8 config = adc->config;
 	u8 req_channel = channel->channel;
-	u8 sample_rate = MCP3422_SAMPLE_RATE(config);
-	u8 i;
+	u8 config = adc->ch_config[req_channel];
+	u8 i, j;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		if (val1 != 0)
 			return -EINVAL;
 
-		for (i = 0; i < ARRAY_SIZE(mcp3422_scales[0]); i++) {
-			if (val2 == mcp3422_scales[sample_rate][i]) {
-				adc->pga[req_channel] = i;
-
-				config &= ~MCP3422_CHANNEL_MASK;
-				config |= MCP3422_CHANNEL_VALUE(req_channel);
-				config &= ~MCP3422_PGA_MASK;
-				config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
-
-				return mcp3422_update_config(adc, config);
+		for (j = 0; j < MCP3422_SRATE_COUNT; j++) {
+			for (i = 0; i < MCP3422_PGA_COUNT; i++) {
+				if (val2 == mcp3422_scales[j][i]) {
+					config &= ~MCP3422_PGA_MASK;
+					config |= MCP3422_PGA_VALUE(i);
+					config &= ~MCP3422_SRATE_MASK;
+					config |= MCP3422_SAMPLE_RATE_VALUE(j);
+					adc->ch_config[req_channel] = config;
+					return 0;
+				}
 			}
 		}
 		return -EINVAL;
@@ -241,12 +240,10 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 			return -EINVAL;
 		}
 
-		config &= ~MCP3422_CHANNEL_MASK;
-		config |= MCP3422_CHANNEL_VALUE(req_channel);
 		config &= ~MCP3422_SRATE_MASK;
 		config |= MCP3422_SAMPLE_RATE_VALUE(temp);
-
-		return mcp3422_update_config(adc, config);
+		adc->ch_config[req_channel] = config;
+		return 0;
 
 	default:
 		break;
@@ -282,14 +279,18 @@ static ssize_t mcp3422_show_samp_freqs(struct device *dev,
 static ssize_t mcp3422_show_scales(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct mcp3422 *adc = iio_priv(dev_to_iio_dev(dev));
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
-
-	return sprintf(buf, "0.%09u 0.%09u 0.%09u 0.%09u\n",
-		mcp3422_scales[sample_rate][0],
-		mcp3422_scales[sample_rate][1],
-		mcp3422_scales[sample_rate][2],
-		mcp3422_scales[sample_rate][3]);
+	ssize_t count = 0;
+	int i;
+
+	for (i = 0; i < MCP3422_SRATE_COUNT; i++) {
+		count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s",
+			mcp3422_scales[i][0],
+			mcp3422_scales[i][1],
+			mcp3422_scales[i][2],
+			mcp3422_scales[i][3],
+			(i < MCP3422_SRATE_COUNT - 1 ? " " : "\n"));
+	}
+	return count;
 }
 
 static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
@@ -336,6 +337,7 @@ static int mcp3422_probe(struct i2c_client *client,
 	struct iio_dev *indio_dev;
 	struct mcp3422 *adc;
 	int err;
+	int i;
 	u8 config;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
@@ -376,6 +378,13 @@ static int mcp3422_probe(struct i2c_client *client,
 	}
 
 	/* meaningful default configuration */
+	for (i = 0; i < 4; i++) {
+		adc->ch_config[i] = (MCP3422_CONT_SAMPLING
+		| MCP3422_CHANNEL_VALUE(i)
+		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
+		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
+	}
+
 	config = (MCP3422_CONT_SAMPLING
 		| MCP3422_CHANNEL_VALUE(0)
 		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
-- 
2.34.1

Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Jonathan Cameron]

On Fri, 11 Nov 2022 12:26:54 +0100
Mitja Spes <mitja@lxnav.com> wrote:

> General improvements:
> - allow setting gain and sampling per channel
> - setting scale can also set sampling rate (combined setting)
> - use per channel config setting
> - do not update mcp register on setting write (we might be reading it...)
>   instead it's updated on next value read
> - output all scale values (sample rates x gain)
> 
> Signed-off-by: Mitja Spes <mitja@lxnav.com>
Hi Mitja,

Was it possible for these scales to differ before this change?
If not, then why was the previous patch a fix rather than simply a precursor
to this change (where it now matters).

There are a number of changes in here which are more stylistic cleanup
than anything to do with the functional change. Please pull those out
to a precursor patch where we can quickly check they make not functional changes
rather than having them mixed in here.

> ---
>  drivers/iio/adc/mcp3422.c | 119 ++++++++++++++++++++------------------
>  1 file changed, 64 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index 3d53de300c89..cfb629b964af 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -28,14 +28,19 @@
>  #define MCP3422_CHANNEL_MASK	0x60
>  #define MCP3422_PGA_MASK	0x03
>  #define MCP3422_SRATE_MASK	0x0C
> -#define MCP3422_SRATE_240	0x0
> -#define MCP3422_SRATE_60	0x1
> -#define MCP3422_SRATE_15	0x2
> -#define MCP3422_SRATE_3	0x3
> -#define MCP3422_PGA_1	0
> -#define MCP3422_PGA_2	1
> -#define MCP3422_PGA_4	2
> -#define MCP3422_PGA_8	3
> +
> +#define MCP3422_SRATE_240	0
> +#define MCP3422_SRATE_60	1
> +#define MCP3422_SRATE_15	2
> +#define MCP3422_SRATE_3		3
I have no particular problem with taking these from hex
to decimal, though I'm not really seeing the necessity.

However, it is really a style question and should not be in this
patch where it mostly adds noise making it slightly harder
to spot the functional changes.

> +#define MCP3422_SRATE_COUNT	4
> +
> +#define MCP3422_PGA_1		0
> +#define MCP3422_PGA_2		1
> +#define MCP3422_PGA_4		2
> +#define MCP3422_PGA_8		3
> +#define MCP3422_PGA_COUNT	4
> +
>  #define MCP3422_CONT_SAMPLING	0x10
>  
>  #define MCP3422_CHANNEL(config)	(((config) & MCP3422_CHANNEL_MASK) >> 5)
> @@ -52,32 +57,32 @@
>  		.indexed = 1, \
>  		.channel = _index, \
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> -				| BIT(IIO_CHAN_INFO_SCALE), \
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +				| BIT(IIO_CHAN_INFO_SCALE) \
> +				| BIT(IIO_CHAN_INFO_SAMP_FREQ), \

Hmm. This is an ABI change.  Hopefully no one will notice however.

>  	}
>  
> -static const int mcp3422_scales[4][4] = {
> +static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
>  	{ 1000000, 500000, 250000, 125000 },
>  	{ 250000,  125000, 62500,  31250  },
>  	{ 62500,   31250,  15625,  7812   },
>  	{ 15625,   7812,   3906,   1953   } };
>  
>  /* Constant msleep times for data acquisitions */
> -static const int mcp3422_read_times[4] = {
> +static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = {
Reasonable to make this change, but I think it belongs in a precursor patch.

>  	[MCP3422_SRATE_240] = 1000 / 240,
>  	[MCP3422_SRATE_60] = 1000 / 60,
>  	[MCP3422_SRATE_15] = 1000 / 15,
>  	[MCP3422_SRATE_3] = 1000 / 3 };
>  
>  /* sample rates to integer conversion table */
> -static const int mcp3422_sample_rates[4] = {
> +static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = {
>  	[MCP3422_SRATE_240] = 240,
>  	[MCP3422_SRATE_60] = 60,
>  	[MCP3422_SRATE_15] = 15,
>  	[MCP3422_SRATE_3] = 3 };
>  
>  /* sample rates to sign extension table */
> -static const int mcp3422_sign_extend[4] = {
> +static const int mcp3422_sign_extend[MCP3422_SRATE_COUNT] = {
>  	[MCP3422_SRATE_240] = 11,
>  	[MCP3422_SRATE_60] = 13,
>  	[MCP3422_SRATE_15] = 15,
> @@ -87,8 +92,8 @@ static const int mcp3422_sign_extend[4] = {
>  struct mcp3422 {
>  	struct i2c_client *i2c;
>  	u8 id;
> -	u8 config;
> -	u8 pga[4];
> +	u8 active_config; // config currently set on mcp
> +	u8 ch_config[4];  // per channel config
>  	struct mutex lock;
>  };
>  
> @@ -98,7 +103,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig)
>  
>  	ret = i2c_master_send(adc->i2c, &newconfig, 1);
>  	if (ret > 0) {
> -		adc->config = newconfig;
> +		adc->active_config = newconfig;
>  		ret = 0;
>  	}
>  
> @@ -108,7 +113,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig)
>  static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config)
>  {
>  	int ret = 0;
> -	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> +	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->active_config);
>  	u8 buf[4] = {0, 0, 0, 0};
>  	u32 temp;
>  
> @@ -136,18 +141,13 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
>  
>  	mutex_lock(&adc->lock);
>  
> -	if (req_channel != MCP3422_CHANNEL(adc->config)) {
> -		config = adc->config;
> -		config &= ~MCP3422_CHANNEL_MASK;
> -		config |= MCP3422_CHANNEL_VALUE(req_channel);
> -		config &= ~MCP3422_PGA_MASK;
> -		config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
> -		ret = mcp3422_update_config(adc, config);
> +	if (adc->ch_config[req_channel] != adc->active_config) {
> +		ret = mcp3422_update_config(adc, adc->ch_config[req_channel]);
>  		if (ret < 0) {
>  			mutex_unlock(&adc->lock);
>  			return ret;
>  		}
> -		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]);
> +		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]);
>  	}
>  
>  	ret = mcp3422_read(adc, value, &config);
> @@ -164,9 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  	struct mcp3422 *adc = iio_priv(iio);
>  	int err;
>  
> -	u8 req_channel = channel->channel;
> -	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> -	u8 pga		 = adc->pga[req_channel];
> +	u8 config = adc->ch_config[channel->channel];
> +	u8 sample_rate = MCP3422_SAMPLE_RATE(config);
> +	u8 pga = MCP3422_PGA(config);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -181,7 +181,7 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  		return IIO_VAL_INT_PLUS_NANO;
>  
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*val1 = mcp3422_sample_rates[MCP3422_SAMPLE_RATE(adc->config)];
> +		*val1 = mcp3422_sample_rates[sample_rate];
>  		return IIO_VAL_INT;
>  
>  	default:
> @@ -197,26 +197,25 @@ static int mcp3422_write_raw(struct iio_dev *iio,
>  {
>  	struct mcp3422 *adc = iio_priv(iio);
>  	u8 temp;
> -	u8 config = adc->config;
>  	u8 req_channel = channel->channel;
> -	u8 sample_rate = MCP3422_SAMPLE_RATE(config);
> -	u8 i;
> +	u8 config = adc->ch_config[req_channel];
> +	u8 i, j;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
>  		if (val1 != 0)
>  			return -EINVAL;
>  
> -		for (i = 0; i < ARRAY_SIZE(mcp3422_scales[0]); i++) {
> -			if (val2 == mcp3422_scales[sample_rate][i]) {
> -				adc->pga[req_channel] = i;
> -
> -				config &= ~MCP3422_CHANNEL_MASK;
> -				config |= MCP3422_CHANNEL_VALUE(req_channel);
> -				config &= ~MCP3422_PGA_MASK;
> -				config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
> -
> -				return mcp3422_update_config(adc, config);
> +		for (j = 0; j < MCP3422_SRATE_COUNT; j++) {
> +			for (i = 0; i < MCP3422_PGA_COUNT; i++) {
> +				if (val2 == mcp3422_scales[j][i]) {
> +					config &= ~MCP3422_PGA_MASK;
> +					config |= MCP3422_PGA_VALUE(i);
> +					config &= ~MCP3422_SRATE_MASK;
> +					config |= MCP3422_SAMPLE_RATE_VALUE(j);
> +					adc->ch_config[req_channel] = config;
> +					return 0;
> +				}
>  			}
>  		}
>  		return -EINVAL;
> @@ -241,12 +240,10 @@ static int mcp3422_write_raw(struct iio_dev *iio,
>  			return -EINVAL;
>  		}
>  
> -		config &= ~MCP3422_CHANNEL_MASK;
> -		config |= MCP3422_CHANNEL_VALUE(req_channel);
>  		config &= ~MCP3422_SRATE_MASK;
>  		config |= MCP3422_SAMPLE_RATE_VALUE(temp);
> -
> -		return mcp3422_update_config(adc, config);
> +		adc->ch_config[req_channel] = config;
> +		return 0;
>  
>  	default:
>  		break;
> @@ -282,14 +279,18 @@ static ssize_t mcp3422_show_samp_freqs(struct device *dev,
>  static ssize_t mcp3422_show_scales(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	struct mcp3422 *adc = iio_priv(dev_to_iio_dev(dev));
> -	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> -
> -	return sprintf(buf, "0.%09u 0.%09u 0.%09u 0.%09u\n",
> -		mcp3422_scales[sample_rate][0],
> -		mcp3422_scales[sample_rate][1],
> -		mcp3422_scales[sample_rate][2],
> -		mcp3422_scales[sample_rate][3]);
> +	ssize_t count = 0;
> +	int i;
> +
> +	for (i = 0; i < MCP3422_SRATE_COUNT; i++) {
> +		count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s",
> +			mcp3422_scales[i][0],
> +			mcp3422_scales[i][1],
> +			mcp3422_scales[i][2],
> +			mcp3422_scales[i][3],
> +			(i < MCP3422_SRATE_COUNT - 1 ? " " : "\n"));

What does the output of this now look like?
For available attributes we tend to only show the values available assuming
just the one thing is changing.  Hence hold sampling frequency static, then
show what scales are available
It can get complex if there are nasty interactions so we might have a
situation where one attribute allows to all the possible values.
So maybe we have all scales available and on a write try to find
the nearest frequency to the current at which we can deliver the
required scale.

Right now this looks like it prints repeated values...

My gut feeling for this device is make the sampling frequency the dominant
attribute.  So just list the available sampling frequencies not taking
scale into account.  For current sampling frequency just list the available
scales and only accept those to be written to the scale attr.

> +	}
> +	return count;
>  }
>  
>  static IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO,
> @@ -336,6 +337,7 @@ static int mcp3422_probe(struct i2c_client *client,
>  	struct iio_dev *indio_dev;
>  	struct mcp3422 *adc;
>  	int err;
> +	int i;
>  	u8 config;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> @@ -376,6 +378,13 @@ static int mcp3422_probe(struct i2c_client *client,
>  	}
>  
>  	/* meaningful default configuration */
> +	for (i = 0; i < 4; i++) {
> +		adc->ch_config[i] = (MCP3422_CONT_SAMPLING
> +		| MCP3422_CHANNEL_VALUE(i)
> +		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
> +		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> +	}
> +
>  	config = (MCP3422_CONT_SAMPLING
>  		| MCP3422_CHANNEL_VALUE(0)
>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)

Perhaps use the first channel configuration for this?

Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Mitja Špes]

Hi Jonathan,

On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> Was it possible for these scales to differ before this change?
Yes. The difference is that before this change you could only see and set
available scales that were available for specified sampling rate. Now you're
able to set gain and sampling rate via scale. So before the change you got
these (@240sps):

0.001000000 0.000500000 0.000250000 0.000125000

Now you get the complete set:
/*                 gain x1     gain x2     gain x4     gain x8  */
/* 240 sps    */ 0.001000000 0.000500000 0.000250000 0.000125000
/*  60 sps    */ 0.000250000 0.000125000 0.000062500 0.000031250
/*  15 sps    */ 0.000062500 0.000031250 0.000015625 0.000007812
/*   3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953

> If not, then why was the previous patch a fix rather than simply a precursor
> to this change (where it now matters).
I wanted to separate a bug fix from improvements, if these were rejected for
for some reason.

> There are a number of changes in here which are more stylistic cleanup
> than anything to do with the functional change. Please pull those out
> to a precursor patch where we can quickly check they make not functional changes
> rather than having them mixed in here.
Will do.

> I have no particular problem with taking these from hex
> to decimal, though I'm not really seeing the necessity.
>
> However, it is really a style question and should not be in this
> patch where it mostly adds noise making it slightly harder
> to spot the functional changes.
My styling preference. I think indexes should be decimal so they are not
confused with flags.

> >               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> > -                             | BIT(IIO_CHAN_INFO_SCALE), \
> > -             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > +                             | BIT(IIO_CHAN_INFO_SCALE) \
> > +                             | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>
> Hmm. This is an ABI change.  Hopefully no one will notice however.
Indeed. I've noted this in the cover letter.


> > -static const int mcp3422_read_times[4] = {
> > +static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = {
> Reasonable to make this change, but I think it belongs in a precursor patch.
Will fix.

> > +     for (i = 0; i < MCP3422_SRATE_COUNT; i++) {
> > +             count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s",
> > +                     mcp3422_scales[i][0],
> > +                     mcp3422_scales[i][1],
> > +                     mcp3422_scales[i][2],
> > +                     mcp3422_scales[i][3],
> > +                     (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n"));
>
> What does the output of this now look like?
0.001000000 0.000500000 0.000250000 0.000125000
0.000250000 0.000125000 0.000062500 0.000031250
0.000062500 0.000031250 0.000015625 0.000007812
0.000015625 0.000007812 0.000003906 0.000001953
All in one line.

> For available attributes we tend to only show the values available assuming
> just the one thing is changing.  Hence hold sampling frequency static, then
> show what scales are available
> It can get complex if there are nasty interactions so we might have a
> situation where one attribute allows to all the possible values.
> So maybe we have all scales available and on a write try to find
> the nearest frequency to the current at which we can deliver the
> required scale.
That's what's being done here:
+ for (j = 0; j < MCP3422_SRATE_COUNT; j++) {
+ for (i = 0; i < MCP3422_PGA_COUNT; i++) {
+ if (val2 == mcp3422_scales[j][i]) {
+ config &= ~MCP3422_PGA_MASK;
+ config |= MCP3422_PGA_VALUE(i);
+ config &= ~MCP3422_SRATE_MASK;
+ config |= MCP3422_SAMPLE_RATE_VALUE(j);
+ adc->ch_config[req_channel] = config;
+ return 0;
+ }
  }
  }

Before it looked at only one sampling frequency and all gains, now it looks at
all combinations.
Looking at this I agree that it would be better to find nearest instead of
looking for an exact match.

> My gut feeling for this device is make the sampling frequency the dominant
> attribute.  So just list the available sampling frequencies not taking
> scale into account.  For current sampling frequency just list the available
> scales and only accept those to be written to the scale attr.
That way the order in which you set attributes matters. Is that really better?
This device has a settable sampling rate and gain and they are independent.
But we could only set gain via scale which values were sampling rate dependent.

> >       /* meaningful default configuration */
> > +     for (i = 0; i < 4; i++) {
> > +             adc->ch_config[i] = (MCP3422_CONT_SAMPLING
> > +             | MCP3422_CHANNEL_VALUE(i)
> > +             | MCP3422_PGA_VALUE(MCP3422_PGA_1)
> > +             | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> > +     }
> > +
> >       config = (MCP3422_CONT_SAMPLING
> >               | MCP3422_CHANNEL_VALUE(0)
> >               | MCP3422_PGA_VALUE(MCP3422_PGA_1)
>
> Perhaps use the first channel configuration for this?
Will fix.

Kind regards,
Mitja

Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Jonathan Cameron]

On Sat, 12 Nov 2022 21:51:36 +0100
Mitja Špes <mitja@lxnav.com> wrote:

> Hi Jonathan,

Hi Mitja,

For future reference please keep the comments inline with the code and
reply there.  On this occasion I'm coming back this one day alter so it's
not too bad - but if it's a week later as often happens it can be hard
to follow if the code isn't there.

> 
> On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > Was it possible for these scales to differ before this change?  
> Yes. The difference is that before this change you could only see and set
> available scales that were available for specified sampling rate. Now you're
> able to set gain and sampling rate via scale. So before the change you got
> these (@240sps):
> 
> 0.001000000 0.000500000 0.000250000 0.000125000
> 
> Now you get the complete set:
> /*                 gain x1     gain x2     gain x4     gain x8  */
> /* 240 sps    */ 0.001000000 0.000500000 0.000250000 0.000125000
> /*  60 sps    */ 0.000250000 0.000125000 0.000062500 0.000031250
> /*  15 sps    */ 0.000062500 0.000031250 0.000015625 0.000007812
> /*   3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953

Ok. That doesn't work as a standard interface because userspace code wants to pick say
0.00062500 which appears twice.

I'm not understanding why you should see scales for other sampling rates.
We always allow any IIO attribute to have side effects on others, precisely
to allow for cases where they interact like here.  It's not ideal but there
isn't really a clean solution (as we are seeing here).

> 
> > If not, then why was the previous patch a fix rather than simply a precursor
> > to this change (where it now matters).  
> I wanted to separate a bug fix from improvements, if these were rejected for
> for some reason.

Is it a bug fix?  The way I read it is that, before this patch there is only
one scale that is applied to all channels.  As such, the current value == the
value set and the code works as expected.
So the previous patch is only necessary once this one is applied.  Hence no
bug, just a rework that is useful to enabling this feature.

> 
> > There are a number of changes in here which are more stylistic cleanup
> > than anything to do with the functional change. Please pull those out
> > to a precursor patch where we can quickly check they make not functional changes
> > rather than having them mixed in here.  
> Will do.
> 
> > I have no particular problem with taking these from hex
> > to decimal, though I'm not really seeing the necessity.
> >
> > However, it is really a style question and should not be in this
> > patch where it mostly adds noise making it slightly harder
> > to spot the functional changes.  
> My styling preference. I think indexes should be decimal so they are not
> confused with flags.

They are also field values as used in the existing code - rather than simply
indexes.  Still field values can be either hex or decimal so given the combined
use I'm fine with the change.  However, precursor patch so we don't have noise
in this patch where closer review is needed.

> 
> > >               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> > > -                             | BIT(IIO_CHAN_INFO_SCALE), \
> > > -             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> > > +                             | BIT(IIO_CHAN_INFO_SCALE) \
> > > +                             | BIT(IIO_CHAN_INFO_SAMP_FREQ), \  
> >
> > Hmm. This is an ABI change.  Hopefully no one will notice however.  
> Indeed. I've noted this in the cover letter.
No one reads cover letters :)  Also not in the git log so you want to call attention to
it in the description of this patch as well.

> 
> > > +     for (i = 0; i < MCP3422_SRATE_COUNT; i++) {
> > > +             count += sprintf(buf + count, "0.%09u 0.%09u 0.%09u 0.%09u%s",
> > > +                     mcp3422_scales[i][0],
> > > +                     mcp3422_scales[i][1],
> > > +                     mcp3422_scales[i][2],
> > > +                     mcp3422_scales[i][3],
> > > +                     (i < MCP3422_SRATE_COUNT - 1 ? " " : "\n"));  
> >
> > What does the output of this now look like?  
> 0.001000000 0.000500000 0.000250000 0.000125000
> 0.000250000 0.000125000 0.000062500 0.000031250
> 0.000062500 0.000031250 0.000015625 0.000007812
> 0.000015625 0.000007812 0.000003906 0.000001953
> All in one line.

Repetition is liable to confuse userspace so if we went this way you would
definitely need to stop doing that.

> 
> > For available attributes we tend to only show the values available assuming
> > just the one thing is changing.  Hence hold sampling frequency static, then
> > show what scales are available
> > It can get complex if there are nasty interactions so we might have a
> > situation where one attribute allows to all the possible values.
> > So maybe we have all scales available and on a write try to find
> > the nearest frequency to the current at which we can deliver the
> > required scale.  
> That's what's being done here:

I don't follow.  This seems to just take the scale and match the first
instance of that it sees. That may not match the option that was available
at the current sampling frequency because of the repetition of possible scales.

> + for (j = 0; j < MCP3422_SRATE_COUNT; j++) {
> + for (i = 0; i < MCP3422_PGA_COUNT; i++) {
> + if (val2 == mcp3422_scales[j][i]) {
> + config &= ~MCP3422_PGA_MASK;
> + config |= MCP3422_PGA_VALUE(i);
> + config &= ~MCP3422_SRATE_MASK;
> + config |= MCP3422_SAMPLE_RATE_VALUE(j);
> + adc->ch_config[req_channel] = config;
> + return 0;
> + }
>   }
>   }
> 
> Before it looked at only one sampling frequency and all gains, now it looks at
> all combinations.
> Looking at this I agree that it would be better to find nearest instead of
> looking for an exact match.
> 
> > My gut feeling for this device is make the sampling frequency the dominant
> > attribute.  So just list the available sampling frequencies not taking
> > scale into account.  For current sampling frequency just list the available
> > scales and only accept those to be written to the scale attr.  
> That way the order in which you set attributes matters. Is that really better?
> This device has a settable sampling rate and gain and they are independent.
> But we could only set gain via scale which values were sampling rate dependent.

This is a very common situation. If you expose all the scales, user who cares about
sampling frequency picks a scale that changes that as well as the pga value.
That is not what they expect to happen. So there is no good answer in cases
like this. Nature of ABI design is that we will always hit corner cases.  The
get of jail free card here was the fact we let changing one attribute affect
others.

Trade off needs to be made and that decision was made in original version of this
driver. I'm not seeing a strong enough reason to change it.

An improvement that does seem logical to me would be to have a change in either
sampling frequency or scale attempt to maintain the nearest value of the other.
In some cases it can be exactly the same.  Any userspace code that already taking
advantage of how the two attributes affect each other will set sampling frequency
first then set the scale.  This optimisation would not affect that so even though
the ABI would change a little it would not be in a way likely to break userspace
code.

Jonathan



> 
> > >       /* meaningful default configuration */
> > > +     for (i = 0; i < 4; i++) {
> > > +             adc->ch_config[i] = (MCP3422_CONT_SAMPLING
> > > +             | MCP3422_CHANNEL_VALUE(i)
> > > +             | MCP3422_PGA_VALUE(MCP3422_PGA_1)
> > > +             | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> > > +     }
> > > +
> > >       config = (MCP3422_CONT_SAMPLING
> > >               | MCP3422_CHANNEL_VALUE(0)
> > >               | MCP3422_PGA_VALUE(MCP3422_PGA_1)  
> >
> > Perhaps use the first channel configuration for this?  
> Will fix.
> 
> Kind regards,
> Mitja

Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Mitja Špes]

On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > Was it possible for these scales to differ before this change?
> > Yes. The difference is that before this change you could only see and set
> > available scales that were available for specified sampling rate. Now you're
> > able to set gain and sampling rate via scale. So before the change you got
> > these (@240sps):
> >
> > 0.001000000 0.000500000 0.000250000 0.000125000
> >
> > Now you get the complete set:
> > /*                 gain x1     gain x2     gain x4     gain x8  */
> > /* 240 sps    */ 0.001000000 0.000500000 0.000250000 0.000125000
> > /*  60 sps    */ 0.000250000 0.000125000 0.000062500 0.000031250
> > /*  15 sps    */ 0.000062500 0.000031250 0.000015625 0.000007812
> > /*   3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953
>
> Ok. That doesn't work as a standard interface because userspace code wants to pick say
> 0.00062500 which appears twice.
I don't know how I missed that. It's clear to me now that this patch is wrong.


> > > If not, then why was the previous patch a fix rather than simply a precursor
> > > to this change (where it now matters).
> > I wanted to separate a bug fix from improvements, if these were rejected for
> > for some reason.
>
> Is it a bug fix?  The way I read it is that, before this patch there is only
> one scale that is applied to all channels.  As such, the current value == the
> value set and the code works as expected.
> So the previous patch is only necessary once this one is applied.  Hence no
> bug, just a rework that is useful to enabling this feature.
I'll post the previous snippet here and write the comments inline:
----
@@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
  struct mcp3422 *adc = iio_priv(iio);
  int err;

+ u8 req_channel = channel->channel;
  u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
- u8 pga = MCP3422_PGA(adc->config);  /* <- this uses the "current" config
      which changes depending on the last read channel */
+ u8 pga = adc->pga[req_channel];          /* this now returns the PGA for the
      selected channel */

  switch (mask) {
  case IIO_CHAN_INFO_RAW:
----
I hope this clarifies the bugfix.


Thanks for in depth look at this and sorry for wasting your time with this
flawed patch.

Kind regards,
Mitja

Re: [PATCH 2/4] iio: adc: mcp3422: allow setting gain and sampling per channel

[Jonathan Cameron]

On Sun, 13 Nov 2022 14:39:03 +0100
Mitja Špes <mitja@lxnav.com> wrote:

> On Sun, Nov 13, 2022 at 12:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Sat, Nov 12, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > Was it possible for these scales to differ before this change?  
> > > Yes. The difference is that before this change you could only see and set
> > > available scales that were available for specified sampling rate. Now you're
> > > able to set gain and sampling rate via scale. So before the change you got
> > > these (@240sps):
> > >
> > > 0.001000000 0.000500000 0.000250000 0.000125000
> > >
> > > Now you get the complete set:
> > > /*                 gain x1     gain x2     gain x4     gain x8  */
> > > /* 240 sps    */ 0.001000000 0.000500000 0.000250000 0.000125000
> > > /*  60 sps    */ 0.000250000 0.000125000 0.000062500 0.000031250
> > > /*  15 sps    */ 0.000062500 0.000031250 0.000015625 0.000007812
> > > /*   3.75 sps */ 0.000015625 0.000007812 0.000003906 0.000001953  
> >
> > Ok. That doesn't work as a standard interface because userspace code wants to pick say
> > 0.00062500 which appears twice.  
> I don't know how I missed that. It's clear to me now that this patch is wrong.
> 
> 
> > > > If not, then why was the previous patch a fix rather than simply a precursor
> > > > to this change (where it now matters).  
> > > I wanted to separate a bug fix from improvements, if these were rejected for
> > > for some reason.  
> >
> > Is it a bug fix?  The way I read it is that, before this patch there is only
> > one scale that is applied to all channels.  As such, the current value == the
> > value set and the code works as expected.
> > So the previous patch is only necessary once this one is applied.  Hence no
> > bug, just a rework that is useful to enabling this feature.  
> I'll post the previous snippet here and write the comments inline:
> ----
> @@ -164,8 +164,9 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>   struct mcp3422 *adc = iio_priv(iio);
>   int err;
> 
> + u8 req_channel = channel->channel;
>   u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> - u8 pga = MCP3422_PGA(adc->config);  /* <- this uses the "current" config
>       which changes depending on the last read channel */
> + u8 pga = adc->pga[req_channel];          /* this now returns the PGA for the
>       selected channel */
> 
>   switch (mask) {
>   case IIO_CHAN_INFO_RAW:
> ----
> I hope this clarifies the bugfix.

Ah I see. The bit that threw me off was the title of this patch.
"allow setting gain ... per channel" which made me think that before this patch
there was one gain for all channels.  I was too lazy to actually check and discover
that it has always been per channel on the write side of things.

Jonathan


> 
> 
> Thanks for in depth look at this and sorry for wasting your time with this
> flawed patch.
> 
> Kind regards,
> Mitja

[PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

[Mitja Spes]

Allows setting gain separately from scale.

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/adc/mcp3422.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index cfb629b964af..eef35fb2fc22 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -58,7 +58,8 @@
 		.channel = _index, \
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
 				| BIT(IIO_CHAN_INFO_SCALE) \
-				| BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+				| BIT(IIO_CHAN_INFO_SAMP_FREQ) \
+				| BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
 	}
 
 static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
@@ -184,6 +185,10 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 		*val1 = mcp3422_sample_rates[sample_rate];
 		return IIO_VAL_INT;
 
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val1 = (1 << pga);
+		return IIO_VAL_INT;
+
 	default:
 		break;
 	}
@@ -245,6 +250,29 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 		adc->ch_config[req_channel] = config;
 		return 0;
 
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (val1) {
+		case 1:
+			temp = MCP3422_PGA_1;
+			break;
+		case 2:
+			temp = MCP3422_PGA_2;
+			break;
+		case 4:
+			temp = MCP3422_PGA_4;
+			break;
+		case 8:
+			temp = MCP3422_PGA_8;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		config &= ~MCP3422_PGA_MASK;
+		config |= MCP3422_PGA_VALUE(temp);
+		adc->ch_config[req_channel] = config;
+		return 0;
+
 	default:
 		break;
 	}
@@ -260,6 +288,8 @@ static int mcp3422_write_raw_get_fmt(struct iio_dev *indio_dev,
 		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
-- 
2.34.1

Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

[Jonathan Cameron]

On Fri, 11 Nov 2022 12:26:55 +0100
Mitja Spes <mitja@lxnav.com> wrote:

> Allows setting gain separately from scale.

How are the separate?  We normally only use hardwaregain if
changing it has no input on the scale that we need to apply in userspace
to raw channels.  This normally happens for two reasons
1) There is a micro controller on the sensor that is doing a bunch of
   maths so whilst changing the PGA value changes the range measurable it
   doesn't affect the representation when we read from the device.
2) The hardware gain is controlling say the sensitivity of a light sensor
   in a time of flight device - it affects if we can get a measurement, but
   not the measurement itself.

Any of that true here?

If not, we shouldn't be adding a hardwaregain attribute - which is why
almost no ADCs have them...

Jonathan

> 
> Signed-off-by: Mitja Spes <mitja@lxnav.com>
> ---
>  drivers/iio/adc/mcp3422.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index cfb629b964af..eef35fb2fc22 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -58,7 +58,8 @@
>  		.channel = _index, \
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
>  				| BIT(IIO_CHAN_INFO_SCALE) \
> -				| BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +				| BIT(IIO_CHAN_INFO_SAMP_FREQ) \
> +				| BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
>  	}
>  
>  static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
> @@ -184,6 +185,10 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  		*val1 = mcp3422_sample_rates[sample_rate];
>  		return IIO_VAL_INT;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		*val1 = (1 << pga);
> +		return IIO_VAL_INT;
> +
>  	default:
>  		break;
>  	}
> @@ -245,6 +250,29 @@ static int mcp3422_write_raw(struct iio_dev *iio,
>  		adc->ch_config[req_channel] = config;
>  		return 0;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		switch (val1) {
> +		case 1:
> +			temp = MCP3422_PGA_1;
> +			break;
> +		case 2:
> +			temp = MCP3422_PGA_2;
> +			break;
> +		case 4:
> +			temp = MCP3422_PGA_4;
> +			break;
> +		case 8:
> +			temp = MCP3422_PGA_8;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		config &= ~MCP3422_PGA_MASK;
> +		config |= MCP3422_PGA_VALUE(temp);
> +		adc->ch_config[req_channel] = config;
> +		return 0;
> +
>  	default:
>  		break;
>  	}
> @@ -260,6 +288,8 @@ static int mcp3422_write_raw_get_fmt(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}

Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

[Mitja Špes]

Hi Jonathan,

On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> How are the separate?  We normally only use hardwaregain if
> changing it has no input on the scale that we need to apply in userspace
> to raw channels.  This normally happens for two reasons
> 1) There is a micro controller on the sensor that is doing a bunch of
>    maths so whilst changing the PGA value changes the range measurable it
>    doesn't affect the representation when we read from the device.
> 2) The hardware gain is controlling say the sensitivity of a light sensor
>    in a time of flight device - it affects if we can get a measurement, but
>    not the measurement itself.
>
> Any of that true here?
No. I see I misunderstood the hardwaregain attribute. For me this is a user
friendly way of setting the gain and subsequently scale.

What I don't understand is why set scale at all? It's a result of sampling
rate and gain settings. Using it as a setting, for which input value range also
changes depending on another attribute is quite cumbersome.
To use a sensor the program has to do this:
1. set the sampling rate
2. read available scales for this sampling rate
3. set the scale according to desired gain
or know the scales for each sampling rate in advance...which makes available
scales attribute quite useless.

Kind regards,
Mitja

Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

[Jonathan Cameron]

On Sat, 12 Nov 2022 22:19:07 +0100
Mitja Špes <mitja@lxnav.com> wrote:

> Hi Jonathan,
> 
> On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > How are the separate?  We normally only use hardwaregain if
> > changing it has no input on the scale that we need to apply in userspace
> > to raw channels.  This normally happens for two reasons
> > 1) There is a micro controller on the sensor that is doing a bunch of
> >    maths so whilst changing the PGA value changes the range measurable it
> >    doesn't affect the representation when we read from the device.
> > 2) The hardware gain is controlling say the sensitivity of a light sensor
> >    in a time of flight device - it affects if we can get a measurement, but
> >    not the measurement itself.
> >
> > Any of that true here?  
> No. I see I misunderstood the hardwaregain attribute. For me this is a user
> friendly way of setting the gain and subsequently scale.
> 
> What I don't understand is why set scale at all? 

Key issue here is the ABI is not designed against one part. It is designed against
many. Also it is inherently biased in favour of the parts that were around when
we originally created it - I'll note that at that time the trade off of resolution
against sampling period (oversampling or cutting off the measurement) was not common.

That means userspace code has been written that assumes a certain set of attributes.
The cost of starting to use new attributes is high because no userspace code knows
about them.  Hence we put a lot of effort into avoiding doing so.  I agree that your
argument makes sense for your particular device - it doesn't for many other ADCs.

Many ADCs (I'd go as far as to say most though I could be wrong on that) do not
couple scale and sampling frequency at all. 

> It's a result of sampling
> rate and gain settings. Using it as a setting, for which input value range also
> changes depending on another attribute is quite cumbersome.
> To use a sensor the program has to do this:
> 1. set the sampling rate
> 2. read available scales for this sampling rate
> 3. set the scale according to desired gain
> or know the scales for each sampling rate in advance...which makes available
> scales attribute quite useless.

For this last point, I think trying to match against previous scale makes a lot of
sense as there is considerable overlap available here between the different rates.
I think that would be an improvement.  Another improvement would be to at least
expose the current resolution - that can be done by providing and _available
for the raw reading.  Not an idea way to undestand what is going on but it would
make more data available to userspace.  (_raw_available(max) * scale would give
the range of the device and allow an adjustment of the scale to achieve what the
user wants).

ABI design is hard.  We don't always get it right and often there is no right answer.
Reality is that sometimes userspace code needs to search the space if it is trying
to get as close as possible to a particular set of constraints.  However lets assume
in most cases the code has limits of:

1) A minimum data rate with which it is happy (controls
the sampling frequency - higher is normally fine, but wastes power etc).
To get best possible power usage (and in case of this device resolution) it will pick
the lowest sampling frequency to meet this constraint.

2) A range of values it is interested in (here related to the PGA settings but not
   the sampling frequency).  Adding _raw_avail would help it to have visibility of
   what the range is.

3) A resolution it cares about getting data at (scale)

We have to present 'scale' because that's necessary to allow userspace to calculate the
actual voltage.  That adds a constraint to the ABI design.  We also don't want to provide
more overlapping controls than absolutely necessary as that makes it hard for userspace
to know which one to use.

So upshot is that userspace has to search to find a value that works for it.

In this particular case the set of ranges at all sampling frequencies are the same.
So if we assume a constraint on how often data is wanted is more important than the
resolution (have to pick one or the other to be more important) then we set that
first to the minimum value to meet the requirement.  Then scale is tweaked to set
the PGA to match the range desired.  Sure, not super clean but consistent with the
ABI as it stands (and we can't change that other than very very carefully).

As a fun side note, if the device (or driver) had justified the lower resolutions the other way
(so the zeros ended up in least significant bits) a common solution would have been
to just present that to userspace as is, thus the scale would have been decoupled from
the sampling frequency.  Not this is what oversampling devices normally do...
We obviously could fake that now, but the issue would then be that it was a major
driver ABI change. So we can't.

Jonathan






> 
> Kind regards,
> Mitja

Re: [PATCH 3/4] iio: adc: mcp3422: add hardware gain attribute

[Mitja Špes]

Thank you for the explanations.

Kind regards,
Mitja

On Sun, Nov 13, 2022 at 1:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 12 Nov 2022 22:19:07 +0100
> Mitja Špes <mitja@lxnav.com> wrote:
>
> > Hi Jonathan,
> >
> > On Sat, Nov 12, 2022 at 6:20 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > How are the separate?  We normally only use hardwaregain if
> > > changing it has no input on the scale that we need to apply in userspace
> > > to raw channels.  This normally happens for two reasons
> > > 1) There is a micro controller on the sensor that is doing a bunch of
> > >    maths so whilst changing the PGA value changes the range measurable it
> > >    doesn't affect the representation when we read from the device.
> > > 2) The hardware gain is controlling say the sensitivity of a light sensor
> > >    in a time of flight device - it affects if we can get a measurement, but
> > >    not the measurement itself.
> > >
> > > Any of that true here?
> > No. I see I misunderstood the hardwaregain attribute. For me this is a user
> > friendly way of setting the gain and subsequently scale.
> >
> > What I don't understand is why set scale at all?
>
> Key issue here is the ABI is not designed against one part. It is designed against
> many. Also it is inherently biased in favour of the parts that were around when
> we originally created it - I'll note that at that time the trade off of resolution
> against sampling period (oversampling or cutting off the measurement) was not common.
>
> That means userspace code has been written that assumes a certain set of attributes.
> The cost of starting to use new attributes is high because no userspace code knows
> about them.  Hence we put a lot of effort into avoiding doing so.  I agree that your
> argument makes sense for your particular device - it doesn't for many other ADCs.
>
> Many ADCs (I'd go as far as to say most though I could be wrong on that) do not
> couple scale and sampling frequency at all.
>
> > It's a result of sampling
> > rate and gain settings. Using it as a setting, for which input value range also
> > changes depending on another attribute is quite cumbersome.
> > To use a sensor the program has to do this:
> > 1. set the sampling rate
> > 2. read available scales for this sampling rate
> > 3. set the scale according to desired gain
> > or know the scales for each sampling rate in advance...which makes available
> > scales attribute quite useless.
>
> For this last point, I think trying to match against previous scale makes a lot of
> sense as there is considerable overlap available here between the different rates.
> I think that would be an improvement.  Another improvement would be to at least
> expose the current resolution - that can be done by providing and _available
> for the raw reading.  Not an idea way to undestand what is going on but it would
> make more data available to userspace.  (_raw_available(max) * scale would give
> the range of the device and allow an adjustment of the scale to achieve what the
> user wants).
>
> ABI design is hard.  We don't always get it right and often there is no right answer.
> Reality is that sometimes userspace code needs to search the space if it is trying
> to get as close as possible to a particular set of constraints.  However lets assume
> in most cases the code has limits of:
>
> 1) A minimum data rate with which it is happy (controls
> the sampling frequency - higher is normally fine, but wastes power etc).
> To get best possible power usage (and in case of this device resolution) it will pick
> the lowest sampling frequency to meet this constraint.
>
> 2) A range of values it is interested in (here related to the PGA settings but not
>    the sampling frequency).  Adding _raw_avail would help it to have visibility of
>    what the range is.
>
> 3) A resolution it cares about getting data at (scale)
>
> We have to present 'scale' because that's necessary to allow userspace to calculate the
> actual voltage.  That adds a constraint to the ABI design.  We also don't want to provide
> more overlapping controls than absolutely necessary as that makes it hard for userspace
> to know which one to use.
>
> So upshot is that userspace has to search to find a value that works for it.
>
> In this particular case the set of ranges at all sampling frequencies are the same.
> So if we assume a constraint on how often data is wanted is more important than the
> resolution (have to pick one or the other to be more important) then we set that
> first to the minimum value to meet the requirement.  Then scale is tweaked to set
> the PGA to match the range desired.  Sure, not super clean but consistent with the
> ABI as it stands (and we can't change that other than very very carefully).
>
> As a fun side note, if the device (or driver) had justified the lower resolutions the other way
> (so the zeros ended up in least significant bits) a common solution would have been
> to just present that to userspace as is, thus the scale would have been decoupled from
> the sampling frequency.  Not this is what oversampling devices normally do...
> We obviously could fake that now, but the issue would then be that it was a major
> driver ABI change. So we can't.
>
> Jonathan
>
>
>
>
>
>
> >
> > Kind regards,
> > Mitja
>

[PATCH 4/4] iio: adc: mcp3422: reduce sleep for fast sampling rates

[Mitja Spes]

- reduced sleep time for 240 & 60 sps rates
- minor roundup fix for sleep times

Signed-off-by: Mitja Spes <mitja@lxnav.com>
---
 drivers/iio/adc/mcp3422.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index eef35fb2fc22..dbcc8fe91aaa 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -70,10 +70,11 @@ static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
 
 /* Constant msleep times for data acquisitions */
 static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = {
-	[MCP3422_SRATE_240] = 1000 / 240,
-	[MCP3422_SRATE_60] = 1000 / 60,
-	[MCP3422_SRATE_15] = 1000 / 15,
-	[MCP3422_SRATE_3] = 1000 / 3 };
+	[MCP3422_SRATE_240] = DIV_ROUND_UP(1000, 240),
+	[MCP3422_SRATE_60] = DIV_ROUND_UP(1000, 60),
+	[MCP3422_SRATE_15] = DIV_ROUND_UP(1000, 15),
+	[MCP3422_SRATE_3] = (100000 + 375 - 100) / 375 /* real rate is 3.75 sps */
+};
 
 /* sample rates to integer conversion table */
 static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = {
@@ -137,6 +138,7 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
 				struct iio_chan_spec const *channel, int *value)
 {
 	int ret;
+	int sleep_duration;
 	u8 config;
 	u8 req_channel = channel->channel;
 
@@ -148,7 +150,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
 			mutex_unlock(&adc->lock);
 			return ret;
 		}
-		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]);
+		sleep_duration = mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)];
+		if (sleep_duration < 20)
+			usleep_range(sleep_duration * 1000, sleep_duration * 1300);
+		else
+			msleep(sleep_duration);
 	}
 
 	ret = mcp3422_read(adc, value, &config);
-- 
2.34.1

Re: [PATCH 4/4] iio: adc: mcp3422: reduce sleep for fast sampling rates

[Jonathan Cameron]

On Fri, 11 Nov 2022 12:26:56 +0100
Mitja Spes <mitja@lxnav.com> wrote:

> - reduced sleep time for 240 & 60 sps rates
> - minor roundup fix for sleep times
> 
> Signed-off-by: Mitja Spes <mitja@lxnav.com>
This patch looks fine to me.

Jonathan

> ---
>  drivers/iio/adc/mcp3422.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index eef35fb2fc22..dbcc8fe91aaa 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -70,10 +70,11 @@ static const int mcp3422_scales[MCP3422_SRATE_COUNT][MCP3422_PGA_COUNT] = {
>  
>  /* Constant msleep times for data acquisitions */
>  static const int mcp3422_read_times[MCP3422_SRATE_COUNT] = {
> -	[MCP3422_SRATE_240] = 1000 / 240,
> -	[MCP3422_SRATE_60] = 1000 / 60,
> -	[MCP3422_SRATE_15] = 1000 / 15,
> -	[MCP3422_SRATE_3] = 1000 / 3 };
> +	[MCP3422_SRATE_240] = DIV_ROUND_UP(1000, 240),
> +	[MCP3422_SRATE_60] = DIV_ROUND_UP(1000, 60),
> +	[MCP3422_SRATE_15] = DIV_ROUND_UP(1000, 15),
> +	[MCP3422_SRATE_3] = (100000 + 375 - 100) / 375 /* real rate is 3.75 sps */
> +};
>  
>  /* sample rates to integer conversion table */
>  static const int mcp3422_sample_rates[MCP3422_SRATE_COUNT] = {
> @@ -137,6 +138,7 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
>  				struct iio_chan_spec const *channel, int *value)
>  {
>  	int ret;
> +	int sleep_duration;
>  	u8 config;
>  	u8 req_channel = channel->channel;
>  
> @@ -148,7 +150,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
>  			mutex_unlock(&adc->lock);
>  			return ret;
>  		}
> -		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)]);
> +		sleep_duration = mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->active_config)];
> +		if (sleep_duration < 20)
> +			usleep_range(sleep_duration * 1000, sleep_duration * 1300);
> +		else
> +			msleep(sleep_duration);
>  	}
>  
>  	ret = mcp3422_read(adc, value, &config);