Re: [PATCH v3 3/3] iio: adc: add ADC driver for the TI TSC2046 controller

[Jonathan Cameron]

On Fri, 19 Mar 2021 15:45:09 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
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@xxxxxxxxxxxxxx>
> +R:	kernel@xxxxxxxxxxxxxx
> +L:	linux-iio@xxxxxxxxxxxxxxx
> +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@xxxxxxxxx>
>  L:	alsa-devel@xxxxxxxxxxxxxxxx (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.


> +
> +#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.

...

Jonathan