| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210322115635.GA14791@pengutronix.de>
Date: Mon, 22 Mar 2021 12:56:35 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Jonathan Cameron <jic23@...nel.org>
Cc: devicetree@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-iio@...r.kernel.org,
Robin van der Gracht <robin@...tonic.nl>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
linux-kernel@...r.kernel.org,
Andy Shevchenko <andy.shevchenko@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
David Jander <david@...tonic.nl>
Subject: Re: [PATCH v3 3/3] iio: adc: add ADC driver for the TI TSC2046
controller
Hi,
On Sat, Mar 20, 2021 at 03:46:01PM +0000, Jonathan Cameron wrote:
> On Fri, 19 Mar 2021 15:45:09 +0100
> Oleksij Rempel <o.rempel@...gutronix.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.
> >
> > 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@...gutronix.de>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> Not a lot to add to Andy's review. A few minor things inline.
> Biggest one is I think we should call out that we expect to add single
> channel polled reading + scales etc in future to enable the iio-hwmon
> usecase.
>
> Jonathan
>
> > ---
> > MAINTAINERS | 8 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-tsc2046.c | 728 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 749 insertions(+)
> > create mode 100644 drivers/iio/adc/ti-tsc2046.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0455cfd5c76c..2ea85a5bb4dd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18002,6 +18002,14 @@ S: Supported
> > F: Documentation/devicetree/bindings/net/nfc/trf7970a.txt
> > F: drivers/nfc/trf7970a.c
> >
> > +TI TSC2046 ADC DRIVER
> > +M: Oleksij Rempel <o.rempel@...gutronix.de>
> > +R: kernel@...gutronix.de
> > +L: linux-iio@...r.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/iio/adc/ti,tsc2046.yaml
> > +F: drivers/iio/adc/ti-tsc2046.c
> > +
> > TI TWL4030 SERIES SOC CODEC DRIVER
> > M: Peter Ujfalusi <peter.ujfalusi@...il.com>
> > L: alsa-devel@...a-project.org (moderated for non-subscribers)
> ...
> > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
> > new file mode 100644
> > index 000000000000..c8c0dd9087c5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-tsc2046.c
> > @@ -0,0 +1,728 @@
> > +/*
> > + * The PENIRQ of TSC2046 controller is implemented as level shifter attached to
> > + * the X+ line. If voltage of the X+ line reaches a specific level the IRQ will
> > + * be activated or deactivated.
> > + * To make this kind of IRQ reusable as trigger following additions were
> > + * implemented:
> > + * - rate limiting:
> > + * For typical touchscreen use case, we need to trigger about each 10ms.
> > + * - hrtimer:
> > + * Continue triggering at least once after the IRQ was deactivated. Then
> > + * deactivate this trigger to stop sampling in order to reduce power
> > + * consumption.
> > + */
>
> Good description :)
>
> ...
>
>
> > +
> > +struct tsc2046_adc_scan_buf {
> > + /* Scan data for each channel */
> > + u16 data[TI_TSC2046_MAX_CHAN];
> > + /* Timestamp */
> > + s64 ts __aligned(8);
> > +};
>
> ...
>
> > +struct tsc2046_adc_priv {
> > + struct spi_device *spi;
> > + const struct tsc2046_adc_dcfg *dcfg;
> > +
> > + struct iio_trigger *trig;
> > + struct hrtimer trig_timer;
> > + spinlock_t trig_lock;
> > + atomic_t trig_more_count;
> > +
> > + struct spi_transfer xfer;
> > + struct spi_message msg;
> > +
> > + struct tsc2046_adc_scan_buf scan_buf;
>
> Given the type tsc2046_adc_scan_buf is never used for anything else
> you could make things more compact by using
> struct {
> } scan_buf;
>
> > + /*
> > + * Lock to protect the layout and the spi transfer buffer.
> > + * tsc2046_adc_group_layout can be changed within update_scan_mode(),
> > + * in this case the l[] and tx/rx buffer will be out of sync to each
> > + * other.
> > + */
> > + struct mutex slock;
> > + struct tsc2046_adc_group_layout l[TI_TSC2046_MAX_CHAN];
> > + struct tsc2046_adc_atom *rx;
> > + struct tsc2046_adc_atom *tx;
> > +
> > + struct tsc2046_adc_atom *rx_one;
> > + struct tsc2046_adc_atom *tx_one;
> > +
> > + unsigned int count;
> > + unsigned int groups;
> > + u32 effective_speed_hz;
> > + u32 scan_interval_us;
> > + u32 time_per_scan_us;
> > + u32 time_per_bit_ns;
> > +
> > + struct tsc2046_adc_ch_cfg ch_cfg[TI_TSC2046_MAX_CHAN];
> > +};
> > +
> > +#define TI_TSC2046_V_CHAN(index, bits, name) \
> > +{ \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = index, \
> > + .datasheet_name = "#name", \
> > + .scan_index = index, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = bits, \
> > + .storagebits = 16, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > +}
>
> I'd not noticed this before but you are exposing nothing to the
> normal IIO interfaces.
>
> That means for usecases like iio-hwmon there is no access
> to polled readings, or information like channel scaling.
>
> I suppose that could be added later, but perhaps you want to call this
> out by mentioning it in the patch description.
If it is ok for you, then I'll add some words about it in to the patch
description.
> > +
> > +#define DECLARE_TI_TSC2046_8_CHANNELS(name, bits) \
> > +const struct iio_chan_spec name ## _channels[] = { \
> > + TI_TSC2046_V_CHAN(0, bits, TEMP0), \
> > + TI_TSC2046_V_CHAN(1, bits, Y), \
> > + TI_TSC2046_V_CHAN(2, bits, VBAT), \
> > + TI_TSC2046_V_CHAN(3, bits, Z1), \
> > + TI_TSC2046_V_CHAN(4, bits, Z2), \
> > + TI_TSC2046_V_CHAN(5, bits, X), \
> > + TI_TSC2046_V_CHAN(6, bits, AUX), \
> > + TI_TSC2046_V_CHAN(7, bits, TEMP1), \
> > + IIO_CHAN_SOFT_TIMESTAMP(8), \
> > +}
> > +
> > +static DECLARE_TI_TSC2046_8_CHANNELS(tsc2046_adc, 12);
> > +
> > +static const struct tsc2046_adc_dcfg tsc2046_adc_dcfg_tsc2046e = {
> > + .channels = tsc2046_adc_channels,
> > + .num_channels = ARRAY_SIZE(tsc2046_adc_channels),
> > +};
> > +
>
> Hmm. Flexibility that isn't yet used. Normally I'm pretty resistant
> to this going in, unless I'm reassured that there is support for additional
> devices already in the pipeline. Is that true here? Otherwise
> this is basically unneeded complexity.
In the long term this driver should replace
drivers/input/touchscreen/ads7846.c
This driver supports ti,ads7843, ti,ads7845, ti,ads7846.. at least with
following number of supported channels:
ti,ads7843 - 4 channels: x, y, aux0, aux1
ti,ads7845 - 3 channels: x, y, aux0
ti,ads7846 - 8 channels...
Currently I don't have this HW for testing and there a subtle
differences that have to be taken care of and tested.
--
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 |
Powered by blists - more mailing lists