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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210322142722.000053a6@Huawei.com>
Date:   Mon, 22 Mar 2021 14:27:22 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
CC:     Jonathan Cameron <jic23@...nel.org>, <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


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