lkml.org 
[lkml]   [2022]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v1 1/1] iio: adc: tsc2046: add .read_raw support
    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 |

    \
     
     \ /
      Last update: 2022-01-11 14:19    [W:3.660 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site