> >   
> > > + /*
> > > +  * 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

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

Reply via email to