Thread (14 messages) 14 messages, 5 authors, 2021-03-29

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

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Date: 2021-03-22 14:29:33
Also in: linux-iio, lkml

quoted
  
quoted
+	/*
+	 * 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.
Sure
quoted
quoted
+
+#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.
Note that I'm only going to merge this driver with an explicit statement
from Dmitry as input maintainer that he is fine with this approach.

Jonathan
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help