[PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support

[PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support

[Oleksij Rempel]

Add read_raw() support to make use of iio_hwmon and other iio clients.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/iio/adc/ti-tsc2046.c | 114 ++++++++++++++++++++++++++++++++---
 1 file changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
index 8126084616e6..55787d18e2cd 100644
--- a/drivers/iio/adc/ti-tsc2046.c
+++ b/drivers/iio/adc/ti-tsc2046.c
@@ -82,6 +82,7 @@
 #define TI_TSC2046_DATA_12BIT			GENMASK(14, 3)
 
 #define TI_TSC2046_MAX_CHAN			8
+#define TI_TSC2046_INT_VREF			2500
 
 /* Represents a HW sample */
 struct tsc2046_adc_atom {
@@ -178,6 +179,11 @@ struct tsc2046_adc_priv {
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
 	.channel = index,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |	\
+			BIT(IIO_CHAN_INFO_DEBOUNCE_COUNT) |	\
+			BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 	.datasheet_name = "#name",				\
 	.scan_index = index,					\
 	.scan_type = {						\
@@ -241,6 +247,14 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx,
 	else
 		pd = 0;
 
+	switch (ch_idx) {
+	case TI_TSC2046_ADDR_TEMP1:
+	case TI_TSC2046_ADDR_AUX:
+	case TI_TSC2046_ADDR_VBAT:
+	case TI_TSC2046_ADDR_TEMP0:
+		pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON;
+	}
+
 	return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd;
 }
 
@@ -252,16 +266,47 @@ static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
 static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
 				u32 *effective_speed_hz)
 {
+	struct tsc2046_adc_ch_cfg *ch = &priv->ch_cfg[ch_idx];
+	struct tsc2046_adc_atom *rx_buf, *tx_buf;
+	unsigned int val, val_normalized = 0;
+	int ret, i, count_skip = 0, max_count;
 	struct spi_transfer xfer;
 	struct spi_message msg;
-	int ret;
+	u8 cmd;
+
+	if (!effective_speed_hz) {
+		count_skip = tsc2046_adc_time_to_count(priv, ch->settling_time_us);
+		max_count = count_skip + ch->oversampling_ratio;
+	} else {
+		max_count = 1;
+	}
+
+	tx_buf = kcalloc(max_count, sizeof(*tx_buf), GFP_KERNEL);
+	if (!tx_buf)
+		return -ENOMEM;
+
+	rx_buf = kcalloc(max_count, sizeof(*rx_buf), GFP_KERNEL);
+	if (!rx_buf) {
+		ret = -ENOMEM;
+		goto free_tx;
+	}
+
+	/*
+	 * Do not enable automatic power down on working samples. Otherwise the
+	 * plates will never be completely charged.
+	 */
+	cmd = tsc2046_adc_get_cmd(priv, ch_idx, true);
+
+	for (i = 0; i < max_count - 1; i++)
+		tx_buf[i].cmd = cmd;
+
+	/* automatically power down on last sample */
+	tx_buf[i].cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
 
 	memset(&xfer, 0, sizeof(xfer));
-	priv->tx_one->cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
-	priv->tx_one->data = 0;
-	xfer.tx_buf = priv->tx_one;
-	xfer.rx_buf = priv->rx_one;
-	xfer.len = sizeof(*priv->tx_one);
+	xfer.tx_buf = tx_buf;
+	xfer.rx_buf = rx_buf;
+	xfer.len = sizeof(*tx_buf) * max_count;
 	spi_message_init_with_transfers(&msg, &xfer, 1);
 
 	/*
@@ -272,13 +317,25 @@ static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
 	if (ret) {
 		dev_err_ratelimited(&priv->spi->dev, "SPI transfer failed %pe\n",
 				    ERR_PTR(ret));
-		return ret;
+		goto free_bufs;
 	}
 
 	if (effective_speed_hz)
 		*effective_speed_hz = xfer.effective_speed_hz;
 
-	return tsc2046_adc_get_value(priv->rx_one);
+	for (i = 0; i < max_count - count_skip; i++) {
+		val = tsc2046_adc_get_value(&rx_buf[count_skip + i]);
+		val_normalized += val;
+	}
+
+	ret = DIV_ROUND_UP(val_normalized, max_count - count_skip);
+
+free_bufs:
+	kfree(rx_buf);
+free_tx:
+	kfree(tx_buf);
+
+	return ret;
 }
 
 static size_t tsc2046_adc_group_set_layout(struct tsc2046_adc_priv *priv,
@@ -385,6 +442,46 @@ static irqreturn_t tsc2046_adc_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int tsc2046_adc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long m)
+{
+	struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = tsc2046_adc_read_one(priv, chan->channel, NULL);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Note: the TSC2046 has internal voltage divider on the VBAT
+		 * line. This divider can be influenced by external divider.
+		 * So, it is better to use external voltage-divider.
+		 */
+		*val = TI_TSC2046_INT_VREF;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = priv->ch_cfg[chan->channel].oversampling_ratio;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_DEBOUNCE_COUNT:
+		*val = tsc2046_adc_time_to_count(priv,
+				priv->ch_cfg[chan->channel].settling_time_us);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_DEBOUNCE_TIME:
+		*val = priv->ch_cfg[chan->channel].settling_time_us;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
 static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
 					const unsigned long *active_scan_mask)
 {
@@ -415,6 +512,7 @@ static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
 }
 
 static const struct iio_info tsc2046_adc_info = {
+	.read_raw	  = tsc2046_adc_read_raw,
 	.update_scan_mode = tsc2046_adc_update_scan_mode,
 };
 
-- 
2.30.2

Re: [PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support

[Jonathan Cameron]

On Fri,  7 Jan 2022 10:35:27 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Add read_raw() support to make use of iio_hwmon and other iio clients.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Hi Oleksij

Main questions in here are around settling time and the interface used for that.

> ---
>  drivers/iio/adc/ti-tsc2046.c | 114 ++++++++++++++++++++++++++++++++---
>  1 file changed, 106 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
> index 8126084616e6..55787d18e2cd 100644
> --- a/drivers/iio/adc/ti-tsc2046.c
> +++ b/drivers/iio/adc/ti-tsc2046.c
> @@ -82,6 +82,7 @@
>  #define TI_TSC2046_DATA_12BIT			GENMASK(14, 3)
>  
>  #define TI_TSC2046_MAX_CHAN			8
> +#define TI_TSC2046_INT_VREF			2500
>  
>  /* Represents a HW sample */
>  struct tsc2046_adc_atom {
> @@ -178,6 +179,11 @@ struct tsc2046_adc_priv {
>  	.type = IIO_VOLTAGE,					\
>  	.indexed = 1,						\
>  	.channel = index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |	\
> +			BIT(IIO_CHAN_INFO_DEBOUNCE_COUNT) |	\
> +			BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>  	.datasheet_name = "#name",				\
>  	.scan_index = index,					\
>  	.scan_type = {						\
> @@ -241,6 +247,14 @@ static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx,
>  	else
>  		pd = 0;
>  
> +	switch (ch_idx) {
> +	case TI_TSC2046_ADDR_TEMP1:
> +	case TI_TSC2046_ADDR_AUX:
> +	case TI_TSC2046_ADDR_VBAT:
> +	case TI_TSC2046_ADDR_TEMP0:
> +		pd |= TI_TSC2046_SER | TI_TSC2046_PD1_VREF_ON;
> +	}
> +
>  	return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd;
>  }
>  
> @@ -252,16 +266,47 @@ static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
>  static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
>  				u32 *effective_speed_hz)
>  {
> +	struct tsc2046_adc_ch_cfg *ch = &priv->ch_cfg[ch_idx];
> +	struct tsc2046_adc_atom *rx_buf, *tx_buf;
> +	unsigned int val, val_normalized = 0;
> +	int ret, i, count_skip = 0, max_count;
>  	struct spi_transfer xfer;
>  	struct spi_message msg;
> -	int ret;
> +	u8 cmd;
> +
> +	if (!effective_speed_hz) {
> +		count_skip = tsc2046_adc_time_to_count(priv, ch->settling_time_us);
> +		max_count = count_skip + ch->oversampling_ratio;
> +	} else {
> +		max_count = 1;
> +	}
> +
> +	tx_buf = kcalloc(max_count, sizeof(*tx_buf), GFP_KERNEL);
> +	if (!tx_buf)
> +		return -ENOMEM;
> +
> +	rx_buf = kcalloc(max_count, sizeof(*rx_buf), GFP_KERNEL);
> +	if (!rx_buf) {
> +		ret = -ENOMEM;
> +		goto free_tx;
> +	}

I guess these are fine to do everytime because you expect this to be used in
paths which aren't called at a particularly high frequency?

These buffers could get rather large so maybe you need a cap on settling time?


> +
> +	/*
> +	 * Do not enable automatic power down on working samples. Otherwise the
> +	 * plates will never be completely charged.
> +	 */
> +	cmd = tsc2046_adc_get_cmd(priv, ch_idx, true);
> +
> +	for (i = 0; i < max_count - 1; i++)
> +		tx_buf[i].cmd = cmd;
> +
> +	/* automatically power down on last sample */
> +	tx_buf[i].cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
>  
>  	memset(&xfer, 0, sizeof(xfer));
> -	priv->tx_one->cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
> -	priv->tx_one->data = 0;
> -	xfer.tx_buf = priv->tx_one;
> -	xfer.rx_buf = priv->rx_one;

Are these used for anything else?  If not probably need to drop them and
their allocation.

> -	xfer.len = sizeof(*priv->tx_one);
> +	xfer.tx_buf = tx_buf;
> +	xfer.rx_buf = rx_buf;
> +	xfer.len = sizeof(*tx_buf) * max_count;

This could be very big and more than possible some spi controllers will fail
it (or does the SPI core handle splitting very large transfers?)  Maybe a loop
is needed with smaller fixed size transfers?

>  	spi_message_init_with_transfers(&msg, &xfer, 1);
>  
>  	/*
> @@ -272,13 +317,25 @@ static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
>  	if (ret) {
>  		dev_err_ratelimited(&priv->spi->dev, "SPI transfer failed %pe\n",
>  				    ERR_PTR(ret));
> -		return ret;
> +		goto free_bufs;
>  	}
>  
>  	if (effective_speed_hz)
>  		*effective_speed_hz = xfer.effective_speed_hz;
>  
> -	return tsc2046_adc_get_value(priv->rx_one);
> +	for (i = 0; i < max_count - count_skip; i++) {
> +		val = tsc2046_adc_get_value(&rx_buf[count_skip + i]);
> +		val_normalized += val;
> +	}
> +
> +	ret = DIV_ROUND_UP(val_normalized, max_count - count_skip);
> +
> +free_bufs:
> +	kfree(rx_buf);
> +free_tx:
> +	kfree(tx_buf);
> +
> +	return ret;
>  }
>  
>  static size_t tsc2046_adc_group_set_layout(struct tsc2046_adc_priv *priv,
> @@ -385,6 +442,46 @@ static irqreturn_t tsc2046_adc_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int tsc2046_adc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long m)
> +{
> +	struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = tsc2046_adc_read_one(priv, chan->channel, NULL);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Note: the TSC2046 has internal voltage divider on the VBAT
> +		 * line. This divider can be influenced by external divider.
> +		 * So, it is better to use external voltage-divider.
> +		 */
> +		*val = TI_TSC2046_INT_VREF;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = priv->ch_cfg[chan->channel].oversampling_ratio;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_DEBOUNCE_COUNT:

These are unusual. I think they've only been used for the more literal bounce suppression
of a human step counting algorithm.

I'd probably not expect to see the both even if we decide this is applicable.

> +		*val = tsc2046_adc_time_to_count(priv,
> +				priv->ch_cfg[chan->channel].settling_time_us);

Setting time is often about external circuitry so it's a bit unusual to expose
it to userspace rather than making it a device tree property and just making
sure the driver doesn't provide a reading until appropriate debounce has passed.
Here is coming from DT anyway, so what benefit do these two read only channel
properties provide?

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_DEBOUNCE_TIME:
> +		*val = priv->ch_cfg[chan->channel].settling_time_us;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
>  					const unsigned long *active_scan_mask)
>  {
> @@ -415,6 +512,7 @@ static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
>  }
>  
>  static const struct iio_info tsc2046_adc_info = {
> +	.read_raw	  = tsc2046_adc_read_raw,
>  	.update_scan_mode = tsc2046_adc_update_scan_mode,
>  };
>  

Re: [PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support

[Oleksij Rempel]

Hi Jonathan,

On Sun, Jan 09, 2022 at 04:00:09PM +0000, Jonathan Cameron wrote:
> On Fri,  7 Jan 2022 10:35:27 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > Add read_raw() support to make use of iio_hwmon and other iio clients.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Hi Oleksij
> 
> Main questions in here are around settling time and the interface used for that.
> 
> > ---
> >  drivers/iio/adc/ti-tsc2046.c | 114 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 106 insertions(+), 8 deletions(-)
> > 
> > @@ -252,16 +266,47 @@ static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
> >  static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
> >  				u32 *effective_speed_hz)
> >  {
> > +	struct tsc2046_adc_ch_cfg *ch = &priv->ch_cfg[ch_idx];
> > +	struct tsc2046_adc_atom *rx_buf, *tx_buf;
> > +	unsigned int val, val_normalized = 0;
> > +	int ret, i, count_skip = 0, max_count;
> >  	struct spi_transfer xfer;
> >  	struct spi_message msg;
> > -	int ret;
> > +	u8 cmd;
> > +
> > +	if (!effective_speed_hz) {
> > +		count_skip = tsc2046_adc_time_to_count(priv, ch->settling_time_us);
> > +		max_count = count_skip + ch->oversampling_ratio;
> > +	} else {
> > +		max_count = 1;
> > +	}
> > +
> > +	tx_buf = kcalloc(max_count, sizeof(*tx_buf), GFP_KERNEL);
> > +	if (!tx_buf)
> > +		return -ENOMEM;
> > +
> > +	rx_buf = kcalloc(max_count, sizeof(*rx_buf), GFP_KERNEL);
> > +	if (!rx_buf) {
> > +		ret = -ENOMEM;
> > +		goto free_tx;
> > +	}
> 
> I guess these are fine to do everytime because you expect this to be used in
> paths which aren't called at a particularly high frequency?

Yes, this was my assumption as well. Instead of preallocating buffer of
max size, I hope it is less ugly.

> These buffers could get rather large so maybe you need a cap on settling time?

What do you mean by "cap on settling"?

> 
> > +
> > +	/*
> > +	 * Do not enable automatic power down on working samples. Otherwise the
> > +	 * plates will never be completely charged.
> > +	 */
> > +	cmd = tsc2046_adc_get_cmd(priv, ch_idx, true);
> > +
> > +	for (i = 0; i < max_count - 1; i++)
> > +		tx_buf[i].cmd = cmd;
> > +
> > +	/* automatically power down on last sample */
> > +	tx_buf[i].cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
> >  
> >  	memset(&xfer, 0, sizeof(xfer));
> > -	priv->tx_one->cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
> > -	priv->tx_one->data = 0;
> > -	xfer.tx_buf = priv->tx_one;
> > -	xfer.rx_buf = priv->rx_one;
> 
> Are these used for anything else?  If not probably need to drop them and
> their allocation.

done

> > -	xfer.len = sizeof(*priv->tx_one);
> > +	xfer.tx_buf = tx_buf;
> > +	xfer.rx_buf = rx_buf;
> > +	xfer.len = sizeof(*tx_buf) * max_count;
> 
> This could be very big and more than possible some spi controllers will fail
> it (or does the SPI core handle splitting very large transfers?)  Maybe a loop
> is needed with smaller fixed size transfers?

I can't exclude possible issue with some of SPI drivers. But SPI level
optimizations should be done on SPI driver or framework level.

> >  	spi_message_init_with_transfers(&msg, &xfer, 1);
> >  
> >  	/*
> > @@ -272,13 +317,25 @@ static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
> >  	if (ret) {
> >  		dev_err_ratelimited(&priv->spi->dev, "SPI transfer failed %pe\n",
> >  				    ERR_PTR(ret));
> > +		*val2 = chan->scan_type.realbits;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		*val = priv->ch_cfg[chan->channel].oversampling_ratio;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_DEBOUNCE_COUNT:
> 
> These are unusual. I think they've only been used for the more literal bounce suppression
> of a human step counting algorithm.
> 
> I'd probably not expect to see the both even if we decide this is applicable.

Ok, i do not need this information so far. I'll remove it

> > +		*val = tsc2046_adc_time_to_count(priv,
> > +				priv->ch_cfg[chan->channel].settling_time_us);
> 
> Setting time is often about external circuitry so it's a bit unusual to expose
> it to userspace rather than making it a device tree property and just making
> sure the driver doesn't provide a reading until appropriate debounce has passed.
> Here is coming from DT anyway, so what benefit do these two read only channel
> properties provide?

No benefit. Will remove it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support

[Jonathan Cameron]

On Tue, 11 Jan 2022 14:18:48 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Jonathan,
> 
> On Sun, Jan 09, 2022 at 04:00:09PM +0000, Jonathan Cameron wrote:
> > On Fri,  7 Jan 2022 10:35:27 +0100
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >   
> > > Add read_raw() support to make use of iio_hwmon and other iio clients.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>  
> > Hi Oleksij
> > 
> > Main questions in here are around settling time and the interface used for that.
> >   
> > > ---
> > >  drivers/iio/adc/ti-tsc2046.c | 114 ++++++++++++++++++++++++++++++++---
> > >  1 file changed, 106 insertions(+), 8 deletions(-)
> > > 
> > > @@ -252,16 +266,47 @@ static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
> > >  static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
> > >  				u32 *effective_speed_hz)
> > >  {
> > > +	struct tsc2046_adc_ch_cfg *ch = &priv->ch_cfg[ch_idx];
> > > +	struct tsc2046_adc_atom *rx_buf, *tx_buf;
> > > +	unsigned int val, val_normalized = 0;
> > > +	int ret, i, count_skip = 0, max_count;
> > >  	struct spi_transfer xfer;
> > >  	struct spi_message msg;
> > > -	int ret;
> > > +	u8 cmd;
> > > +
> > > +	if (!effective_speed_hz) {
> > > +		count_skip = tsc2046_adc_time_to_count(priv, ch->settling_time_us);
> > > +		max_count = count_skip + ch->oversampling_ratio;
> > > +	} else {
> > > +		max_count = 1;
> > > +	}
> > > +
> > > +	tx_buf = kcalloc(max_count, sizeof(*tx_buf), GFP_KERNEL);
> > > +	if (!tx_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	rx_buf = kcalloc(max_count, sizeof(*rx_buf), GFP_KERNEL);
> > > +	if (!rx_buf) {
> > > +		ret = -ENOMEM;
> > > +		goto free_tx;
> > > +	}  
> > 
> > I guess these are fine to do everytime because you expect this to be used in
> > paths which aren't called at a particularly high frequency?  
> 
> Yes, this was my assumption as well. Instead of preallocating buffer of
> max size, I hope it is less ugly.
> 
> > These buffers could get rather large so maybe you need a cap on settling time?  
> 
> What do you mean by "cap on settling"?

In theory the buffer needed could get very large, so perhap set a maximum reasonable
size (1 page perhaps) and report an error if the settling time is too large to fit
in that space.

> 
> >   
> > > +
> > > +	/*
> > > +	 * Do not enable automatic power down on working samples. Otherwise the
> > > +	 * plates will never be completely charged.
> > > +	 */
> > > +	cmd = tsc2046_adc_get_cmd(priv, ch_idx, true);
> > > +
> > > +	for (i = 0; i < max_count - 1; i++)
> > > +		tx_buf[i].cmd = cmd;
> > > +
> > > +	/* automatically power down on last sample */
> > > +	tx_buf[i].cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
> > >  
> > >  	memset(&xfer, 0, sizeof(xfer));
> > > -	priv->tx_one->cmd = tsc2046_adc_get_cmd(priv, ch_idx, false);
> > > -	priv->tx_one->data = 0;
> > > -	xfer.tx_buf = priv->tx_one;
> > > -	xfer.rx_buf = priv->rx_one;  
> > 
> > Are these used for anything else?  If not probably need to drop them and
> > their allocation.  
> 
> done
> 
> > > -	xfer.len = sizeof(*priv->tx_one);
> > > +	xfer.tx_buf = tx_buf;
> > > +	xfer.rx_buf = rx_buf;
> > > +	xfer.len = sizeof(*tx_buf) * max_count;  
> > 
> > This could be very big and more than possible some spi controllers will fail
> > it (or does the SPI core handle splitting very large transfers?)  Maybe a loop
> > is needed with smaller fixed size transfers?  
> 
> I can't exclude possible issue with some of SPI drivers. But SPI level
> optimizations should be done on SPI driver or framework level.

As above, I think you want to set a reasonable limit otherwise it will fail
on an awful lot of hardware if someone sets a silly value...

> 
> > >  	spi_message_init_with_transfers(&msg, &xfer, 1);
> > >  
> > >  	/*
> > > @@ -272,13 +317,25 @@ static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx,
> > >  	if (ret) {
> > >  		dev_err_ratelimited(&priv->spi->dev, "SPI transfer failed %pe\n",
> > >  				    ERR_PTR(ret));
> > > +		*val2 = chan->scan_type.realbits;
> > > +		return IIO_VAL_FRACTIONAL_LOG2;
> > > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > > +		*val = priv->ch_cfg[chan->channel].oversampling_ratio;
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_DEBOUNCE_COUNT:  
> > 
> > These are unusual. I think they've only been used for the more literal bounce suppression
> > of a human step counting algorithm.
> > 
> > I'd probably not expect to see the both even if we decide this is applicable.  
> 
> Ok, i do not need this information so far. I'll remove it
> 
> > > +		*val = tsc2046_adc_time_to_count(priv,
> > > +				priv->ch_cfg[chan->channel].settling_time_us);  
> > 
> > Setting time is often about external circuitry so it's a bit unusual to expose
> > it to userspace rather than making it a device tree property and just making
> > sure the driver doesn't provide a reading until appropriate debounce has passed.
> > Here is coming from DT anyway, so what benefit do these two read only channel
> > properties provide?  
> 
> No benefit. Will remove it.
> 
> Regards,
> Oleksij