| Message ID | 20210428073208.19570-1-o.rempel@pengutronix.de |
|---|---|
| Headers | show |
| Series | mainline ti tsc2046 adc driver | expand |
On Wed, 28 Apr 2021 09:32:08 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for > the touchscreen use case. By implementing it as an IIO ADC device, we can > make use of resistive-adc-touch and iio-hwmon drivers. > > Polled readings are currently not implemented to keep this patch small, so > iio-hwmon will not work out of the box for now. > > So far, this driver was tested with a custom version of resistive-adc-touch driver, > since it needs to be extended to make use of Z1 and Z2 channels. The X/Y > are working without additional changes. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> A few really small things inline that I'm happy to clean up whilst applying, or can get rolled into a v7 if you have to do one due to review from others. ... > + > +static int tsc2046_adc_read_one(struct tsc2046_adc_priv *priv, int ch_idx, > + u32 *effective_speed_hz) > +{ > + struct spi_transfer xfer; > + struct spi_message msg; > + int ret; > + > + 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); > + spi_message_init(&msg); > + spi_message_add_tail(&xfer, &msg); Slight preference for spi_message_init_with_transfers(&msg, &xfer, 1); Same elsewhere where we get this pattern. > + > + /* > + * We aren't using spi_write_then_read() because we need to be able > + * to get hold of the effective_speed_hz from the xfer > + */ > + ret = spi_sync(priv->spi, &msg); > + if (ret) { > + dev_err_ratelimited(&priv->spi->dev, "SPI transfer failed %pe\n", > + ERR_PTR(ret)); > + return ret; > + } > + > + if (effective_speed_hz) > + *effective_speed_hz = xfer.effective_speed_hz; > + > + return tsc2046_adc_get_value(priv->rx_one); > +} > + ... > +static int tsc2046_adc_setup_spi_msg(struct tsc2046_adc_priv *priv) > +{ > + unsigned int ch_idx; > + size_t size; > + int ret; > + > + priv->tx_one = devm_kzalloc(&priv->spi->dev, sizeof(*priv->tx_one), > + GFP_KERNEL); > + if (!priv->tx_one) > + return -ENOMEM; > + > + priv->rx_one = devm_kzalloc(&priv->spi->dev, sizeof(*priv->rx_one), > + GFP_KERNEL); > + if (!priv->rx_one) > + return -ENOMEM; > + > + /* > + * Make dummy read to set initial power state and get real SPI clock > + * freq. It seems to be not important which channel is used for this > + * case. > + */ > + ret = tsc2046_adc_read_one(priv, TI_TSC2046_ADDR_TEMP0, > + &priv->effective_speed_hz); > + if (ret < 0) > + return ret; > + > + /* > + * In case SPI controller do not report effective_speed_hz, use > + * configure value and hope it will match. > + */ > + if (!priv->effective_speed_hz) > + priv->effective_speed_hz = priv->spi->max_speed_hz; > + > + > + priv->scan_interval_us = TI_TSC2046_SAMPLE_INTERVAL_US; > + priv->time_per_bit_ns = DIV_ROUND_UP(NSEC_PER_SEC, > + priv->effective_speed_hz); > + > + /* > + * Calculate and allocate maximal size buffer if all channels are > + * enabled. > + */ > + size = 0; > + for (ch_idx = 0; ch_idx < priv->dcfg->num_channels; ch_idx++) > + size += tsc2046_adc_group_set_layout(priv, ch_idx, ch_idx); > + > + priv->tx = devm_kzalloc(&priv->spi->dev, size, GFP_KERNEL); > + if (!priv->tx) > + return -ENOMEM; > + > + priv->rx = devm_kzalloc(&priv->spi->dev, size, GFP_KERNEL); > + if (!priv->rx) > + return -ENOMEM; > + > + spi_message_init(&priv->msg); > + priv->msg.context = priv; Why is this set as we never set priv->msg.complete? I can drop this whilst merging if we aren't going around again. > + > + priv->xfer.tx_buf = priv->tx; > + priv->xfer.rx_buf = priv->rx; > + priv->xfer.len = size; > + spi_message_add_tail(&priv->xfer, &priv->msg); Having dropped the above, can also change to spi_message_init_with_transfers() > + > + return 0; > +} > + ... > +static int tsc2046_adc_probe(struct spi_device *spi) > +{ > + const struct tsc2046_adc_dcfg *dcfg; > + struct device *dev = &spi->dev; > + struct tsc2046_adc_priv *priv; > + struct iio_dev *indio_dev; > + struct iio_trigger *trig; > + int ret; > + > + if (spi->max_speed_hz > TI_TSC2046_MAX_CLK_FREQ) { > + dev_err(dev, "SPI max_speed_hz is too high: %d Hz. Max supported freq is %zu Hz\n", > + spi->max_speed_hz, TI_TSC2046_MAX_CLK_FREQ); > + return -EINVAL; > + } > + > + dcfg = device_get_match_data(dev); > + if (!dcfg) > + return -EINVAL; > + > + spi->bits_per_word = 8; > + spi->mode &= ~SPI_MODE_X_MASK; > + spi->mode |= SPI_MODE_0; > + ret = spi_setup(spi); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Error in SPI setup\n"); > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); > + if (!indio_dev) > + return -ENOMEM; > + > + priv = iio_priv(indio_dev); > + priv->dcfg = dcfg; > + > + spi_set_drvdata(spi, indio_dev); > + > + priv->spi = spi; > + > + indio_dev->name = TI_TSC2046_NAME; > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; > + indio_dev->channels = dcfg->channels; > + indio_dev->num_channels = dcfg->num_channels; > + indio_dev->info = &tsc2046_adc_info; > + > + tsc2046_adc_parse_fwnode(priv); > + > + ret = tsc2046_adc_setup_spi_msg(priv); > + if (ret) > + return ret; > + > + mutex_init(&priv->slock); > + > + /* TODO: remove IRQ_NOAUTOEN after needed patches are mainline */ > + irq_set_status_flags(spi->irq, IRQ_NOAUTOEN); They are now but I can do this whilst merging if that's all we have to change. > + ret = devm_request_irq(dev, spi->irq, &tsc2046_adc_irq, > + 0, indio_dev->name, indio_dev); > + if (ret) > + return ret; > + > + trig = devm_iio_trigger_alloc(dev, "touchscreen-%s", indio_dev->name); > + if (!trig) > + return -ENOMEM; > + > + priv->trig = trig; > + iio_trigger_set_drvdata(trig, indio_dev); > + trig->ops = &tsc2046_adc_trigger_ops; > + > + spin_lock_init(&priv->trig_lock); > + hrtimer_init(&priv->trig_timer, CLOCK_MONOTONIC, > + HRTIMER_MODE_REL_SOFT); > + priv->trig_timer.function = tsc2046_adc_trig_more; > + > + ret = devm_iio_trigger_register(dev, trig); > + if (ret) { > + dev_err(dev, "failed to register trigger\n"); > + return ret; > + } > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > + &tsc2046_adc_trigger_handler, NULL); > + if (ret) { > + dev_err(dev, "Failed to setup triggered buffer\n"); > + return ret; > + } > + > + /* set default trigger */ > + indio_dev->trig = iio_trigger_get(priv->trig); > + > + return devm_iio_device_register(dev, indio_dev); > +} > + thanks, J
On Wed, Apr 28, 2021 at 05:59:31PM +0100, Jonathan Cameron wrote: > On Wed, 28 Apr 2021 09:32:06 +0200 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > Settling time and over sampling is a typical challenge for different IIO ADC > > devices. So, introduce channel specific settling-time-us and oversampling-ratio > > properties to cover this use case. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > This and patch 2 both look good to me. Given Rob gave a minor comment on the > previous version I don't feel I need him to look at this again. > > Will pick up in a few days if no other reviews come in to require a v7. Ok, thank you! > Thanks, > > Jonathan > > > --- > > Documentation/devicetree/bindings/iio/adc/adc.yaml | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml > > index 912a7635edc4..db348fcbb52c 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adc.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml > > @@ -39,4 +39,16 @@ properties: > > The first value specifies the positive input pin, the second > > specifies the negative input pin. > > > > + settling-time-us: > > + description: > > + Time between enabling the channel and first stable readings. > > + > > + oversampling-ratio: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Oversampling is used as replacement of or addition to the low-pass filter. > > + In some cases, the desired filtering characteristics are a function the > > + device design and can interact with other characteristics such as > > + settling time. > > + > > additionalProperties: true > > > -- 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 |
On Wed, 28 Apr 2021 09:32:06 +0200, Oleksij Rempel wrote: > Settling time and over sampling is a typical challenge for different IIO ADC > devices. So, introduce channel specific settling-time-us and oversampling-ratio > properties to cover this use case. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > Documentation/devicetree/bindings/iio/adc/adc.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, 28 Apr 2021 09:32:05 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: Hi Oleksij, Series applied with the tweaks as per review to patch 3. Please check I didn't mess those up though. Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to poke at it. Thanks, Jonathan > changes v6: > - get blessing from Dmitry Torokhov > - rebase against latest iio/testing > - use simple name for iio_dev->name > - use Jonathan's version for oversampling-ratio description > > changes v5: > - remove type for the settling-time-us property > > changes v4: > - spell fixes > - add more comments > - make code more readable > - move scan_buf to the priv > - use FIELD_GET to extract ADC data > - make some multi line code as one line > - do not use atomic API for trig_more_count > - fix build warning on 64bit system > - add NULL check for the devm_kasprintf() > - use return devm_iio_device_register(), without additional error > printing. > > changes v3: > - different spell fixes > - add some notes about driver structure > - rename the trigger to point on the touchscreen nature of it > - rename DT binding to oversampling-ratio > - make sure we have some defaults in case no DT property is set > > changes v2: > - rework and extend DT binding properties > - remove touchscreen related code from the IIO ADC driver > - make trigger be active longer then IRQ is requesting. This is needed > to get "inactive" samples > - make oversampling and settle time configurable > > TI TSC2046 is a touchscreen controller based on 8 channel ADC. Since most of > this ADC based touchscreen controller share same set of challenges, it > is better keep then as simple IIO ADC devices attached to a generic > resistive-adc-touch driver. > > This driver can replace drivers/input/touchscreen/ads7846.c and has > following advantages over it: > - less code to maintain > - shared code paths (resistive-adc-touch, iio-hwmon, etc) > - can be used as plain IIO ADC to investigate signaling issues or test > real capacity of the plates and attached low-pass filters > (or use the touchscreen as a microphone if you like ;) ) > > Oleksij Rempel (3): > dt-bindings:iio:adc: add generic settling-time-us and > oversampling-ratio channel properties > dt-bindings:iio:adc: add documentation for TI TSC2046 controller > iio: adc: add ADC driver for the TI TSC2046 controller > > .../devicetree/bindings/iio/adc/adc.yaml | 12 + > .../bindings/iio/adc/ti,tsc2046.yaml | 115 +++ > MAINTAINERS | 8 + > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ti-tsc2046.c | 720 ++++++++++++++++++ > 6 files changed, 868 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml > create mode 100644 drivers/iio/adc/ti-tsc2046.c >
Hi Jonathan, On Mon, May 03, 2021 at 12:28:18PM +0100, Jonathan Cameron wrote: > On Wed, 28 Apr 2021 09:32:05 +0200 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > Hi Oleksij, > > Series applied with the tweaks as per review to patch 3. Please > check I didn't mess those up though. > > Applied to the togreg branch of iio.git and pushed out as testing for > the autobuilders to poke at it. It works. Thx! Now i need to make configurable iio buffer layout for the drivers/input/touchscreen/resistive-adc-touch.c Do you have ideas what is the proper way to make it? Regards, Oleksij > > Jonathan > > > changes v6: > > - get blessing from Dmitry Torokhov > > - rebase against latest iio/testing > > - use simple name for iio_dev->name > > - use Jonathan's version for oversampling-ratio description > > > > changes v5: > > - remove type for the settling-time-us property > > > > changes v4: > > - spell fixes > > - add more comments > > - make code more readable > > - move scan_buf to the priv > > - use FIELD_GET to extract ADC data > > - make some multi line code as one line > > - do not use atomic API for trig_more_count > > - fix build warning on 64bit system > > - add NULL check for the devm_kasprintf() > > - use return devm_iio_device_register(), without additional error > > printing. > > > > changes v3: > > - different spell fixes > > - add some notes about driver structure > > - rename the trigger to point on the touchscreen nature of it > > - rename DT binding to oversampling-ratio > > - make sure we have some defaults in case no DT property is set > > > > changes v2: > > - rework and extend DT binding properties > > - remove touchscreen related code from the IIO ADC driver > > - make trigger be active longer then IRQ is requesting. This is needed > > to get "inactive" samples > > - make oversampling and settle time configurable > > > > TI TSC2046 is a touchscreen controller based on 8 channel ADC. Since most of > > this ADC based touchscreen controller share same set of challenges, it > > is better keep then as simple IIO ADC devices attached to a generic > > resistive-adc-touch driver. > > > > This driver can replace drivers/input/touchscreen/ads7846.c and has > > following advantages over it: > > - less code to maintain > > - shared code paths (resistive-adc-touch, iio-hwmon, etc) > > - can be used as plain IIO ADC to investigate signaling issues or test > > real capacity of the plates and attached low-pass filters > > (or use the touchscreen as a microphone if you like ;) ) > > > > Oleksij Rempel (3): > > dt-bindings:iio:adc: add generic settling-time-us and > > oversampling-ratio channel properties > > dt-bindings:iio:adc: add documentation for TI TSC2046 controller > > iio: adc: add ADC driver for the TI TSC2046 controller > > > > .../devicetree/bindings/iio/adc/adc.yaml | 12 + > > .../bindings/iio/adc/ti,tsc2046.yaml | 115 +++ > > MAINTAINERS | 8 + > > drivers/iio/adc/Kconfig | 12 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/ti-tsc2046.c | 720 ++++++++++++++++++ > > 6 files changed, 868 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml > > create mode 100644 drivers/iio/adc/ti-tsc2046.c > > > > -- 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 |
On Fri, 14 May 2021 09:57:31 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Hi Jonathan, > > On Mon, May 03, 2021 at 12:28:18PM +0100, Jonathan Cameron wrote: > > On Wed, 28 Apr 2021 09:32:05 +0200 > > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > > Hi Oleksij, > > > > Series applied with the tweaks as per review to patch 3. Please > > check I didn't mess those up though. > > > > Applied to the togreg branch of iio.git and pushed out as testing for > > the autobuilders to poke at it. > > It works. Thx! > > Now i need to make configurable iio buffer layout > > for the drivers/input/touchscreen/resistive-adc-touch.c > > Do you have ideas what is the proper way to make it? So IIRC the issue here was making the ordering of the channels more flexible? We should be able to do that using the names in DT. Right now the touch screen driver just grabs them all an assumes a particular order, but if you look at the binding it requires naming https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt That naming should associate 'which channel' is which when we then do the get_all() in the driver. If it matches the existing layout, nothing to do, but if we have something more complex then we can do data shuffling etc in the touchscreen driver to compensate for that. A useful starting point is probably to do a yaml conversion of that binding doc. Then follow that up by making the doc more flexible so that it copes with what you want to do. That should give us a good basis on which to then implement the handling in driver (which is often easier than defining the binding!) Jonathan > > Regards, > Oleksij > > > > > Jonathan > > > > > changes v6: > > > - get blessing from Dmitry Torokhov > > > - rebase against latest iio/testing > > > - use simple name for iio_dev->name > > > - use Jonathan's version for oversampling-ratio description > > > > > > changes v5: > > > - remove type for the settling-time-us property > > > > > > changes v4: > > > - spell fixes > > > - add more comments > > > - make code more readable > > > - move scan_buf to the priv > > > - use FIELD_GET to extract ADC data > > > - make some multi line code as one line > > > - do not use atomic API for trig_more_count > > > - fix build warning on 64bit system > > > - add NULL check for the devm_kasprintf() > > > - use return devm_iio_device_register(), without additional error > > > printing. > > > > > > changes v3: > > > - different spell fixes > > > - add some notes about driver structure > > > - rename the trigger to point on the touchscreen nature of it > > > - rename DT binding to oversampling-ratio > > > - make sure we have some defaults in case no DT property is set > > > > > > changes v2: > > > - rework and extend DT binding properties > > > - remove touchscreen related code from the IIO ADC driver > > > - make trigger be active longer then IRQ is requesting. This is needed > > > to get "inactive" samples > > > - make oversampling and settle time configurable > > > > > > TI TSC2046 is a touchscreen controller based on 8 channel ADC. Since most of > > > this ADC based touchscreen controller share same set of challenges, it > > > is better keep then as simple IIO ADC devices attached to a generic > > > resistive-adc-touch driver. > > > > > > This driver can replace drivers/input/touchscreen/ads7846.c and has > > > following advantages over it: > > > - less code to maintain > > > - shared code paths (resistive-adc-touch, iio-hwmon, etc) > > > - can be used as plain IIO ADC to investigate signaling issues or test > > > real capacity of the plates and attached low-pass filters > > > (or use the touchscreen as a microphone if you like ;) ) > > > > > > Oleksij Rempel (3): > > > dt-bindings:iio:adc: add generic settling-time-us and > > > oversampling-ratio channel properties > > > dt-bindings:iio:adc: add documentation for TI TSC2046 controller > > > iio: adc: add ADC driver for the TI TSC2046 controller > > > > > > .../devicetree/bindings/iio/adc/adc.yaml | 12 + > > > .../bindings/iio/adc/ti,tsc2046.yaml | 115 +++ > > > MAINTAINERS | 8 + > > > drivers/iio/adc/Kconfig | 12 + > > > drivers/iio/adc/Makefile | 1 + > > > drivers/iio/adc/ti-tsc2046.c | 720 ++++++++++++++++++ > > > 6 files changed, 868 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml > > > create mode 100644 drivers/iio/adc/ti-tsc2046.c > > > > > > > >
changes v6: - get blessing from Dmitry Torokhov - rebase against latest iio/testing - use simple name for iio_dev->name - use Jonathan's version for oversampling-ratio description changes v5: - remove type for the settling-time-us property changes v4: - spell fixes - add more comments - make code more readable - move scan_buf to the priv - use FIELD_GET to extract ADC data - make some multi line code as one line - do not use atomic API for trig_more_count - fix build warning on 64bit system - add NULL check for the devm_kasprintf() - use return devm_iio_device_register(), without additional error printing. changes v3: - different spell fixes - add some notes about driver structure - rename the trigger to point on the touchscreen nature of it - rename DT binding to oversampling-ratio - make sure we have some defaults in case no DT property is set changes v2: - rework and extend DT binding properties - remove touchscreen related code from the IIO ADC driver - make trigger be active longer then IRQ is requesting. This is needed to get "inactive" samples - make oversampling and settle time configurable TI TSC2046 is a touchscreen controller based on 8 channel ADC. Since most of this ADC based touchscreen controller share same set of challenges, it is better keep then as simple IIO ADC devices attached to a generic resistive-adc-touch driver. This driver can replace drivers/input/touchscreen/ads7846.c and has following advantages over it: - less code to maintain - shared code paths (resistive-adc-touch, iio-hwmon, etc) - can be used as plain IIO ADC to investigate signaling issues or test real capacity of the plates and attached low-pass filters (or use the touchscreen as a microphone if you like ;) ) Oleksij Rempel (3): dt-bindings:iio:adc: add generic settling-time-us and oversampling-ratio channel properties dt-bindings:iio:adc: add documentation for TI TSC2046 controller iio: adc: add ADC driver for the TI TSC2046 controller .../devicetree/bindings/iio/adc/adc.yaml | 12 + .../bindings/iio/adc/ti,tsc2046.yaml | 115 +++ MAINTAINERS | 8 + drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ti-tsc2046.c | 720 ++++++++++++++++++ 6 files changed, 868 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml create mode 100644 drivers/iio/adc/ti-tsc2046.c