Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Tue, 9 Mar 2021 12:40:46 +0100 Oleksij Rempel wrote: > Hi Jonathan, > > On Sat, Mar 06, 2021 at 02:59:59PM +, Jonathan Cameron wrote: > > On Sat, 6 Mar 2021 14:28:52 +0100 > > Oleksij Rempel wrote: > > > > > On Fri, Mar 05, 2021 at 07:02:39PM +, Jonathan Cameron wrote: > > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > > > Oleksij Rempel wrote: > > > > > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > > > optimized for > > > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > > > > > So far, this driver was tested with custom version of > > > > > resistive-adc-touch driver, > > > > > since it need to be extended to make use of Z1 and Z2 channels. The > > > > > X/Y > > > > > are working without additional changes. > > > > > > > > > > Signed-off-by: Oleksij Rempel > > > > > > > > Hi Oleksij, > > > > > > > > To consider this as a possible long term route instead of just making > > > > this > > > > a touchscreen driver, we'll want to see those mods to the > > > > resistive-adc-touch. > > > > Of course that doesn't stop review of this in the meantime. > > > > > > ok. > > > > > > I had following issues with the existing resistive-adc-touch driver: > > > - the buffer layout is not configurable over DT or i didn't understood > > > how to properly configure it > > > - the "pressure" channel provide pre processed data driver or > > > controller, this information cannot be extracted directly from the > > > touchscreen plates. > > > > > > I did following changes to make it work for my use case: > > > > > > --- a/drivers/input/touchscreen/resistive-adc-touch.c > > > +++ b/drivers/input/touchscreen/resistive-adc-touch.c > > > @@ -44,15 +44,34 @@ static int grts_cb(const void *data, void *private) > > > { > > > const u16 *touch_info = data; > > > struct grts_state *st = private; > > > - unsigned int x, y, press = 0x0; > > > + unsigned int x, y, press = 0x0, z1, z2; > > > + unsigned int Rt; > > > > > > /* channel data coming in buffer in the order below */ > > > - x = touch_info[0]; > > > - y = touch_info[1]; > > > + // TODO: make sure we get buffers in proper order > > > > Ah. So to figure this out we'll need to read some more info about the > > channels. The phandle order for the touchscreen binding > > should probably be specified (if it's not already) and that should let > > us establish the ordering of channels. > > Ack. So this should be done in the touchscreen driver and can be done > later? Sure. I don't recall if we have handy look up functions to make this easy but we can add some as needed for the touchscreen side of things. > > > > + x = touch_info[3]; > > > + z2 = touch_info[2]; > > > + z1 = touch_info[1]; > > > + y = touch_info[0]; > > > + > > > + if (z1) { > > > + Rt = z2; > > > > So for this we are going to need to define it in a generic fashion - > > probably > > via a mode + coefficients in DT? > > I assume, mode will be needed any way, coefficients can stay as is and > if we get some different use case add an overwrite binding to the > devicetree. Ok, we can use these as default values. > > > > + Rt -= z1; > > > + Rt *= 800; > > > + //Rt *= ts->x_plate_ohms; > > > + Rt = DIV_ROUND_CLOSEST(Rt, 16); > > > + Rt *= x; > > > + Rt /= z1; > > > + Rt = DIV_ROUND_CLOSEST(Rt, 256); > > > + } else > > > + Rt = 0x400; > > > + > > > if (st->pressure) > > > - press = touch_info[2]; > > > + press = Rt; > > > > > > - if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > > > + //printk("%s:%i: x: %x, y %x, z1: %x, z2: %x, press: %x\n", __func__, > > > __LINE__, x, y, z1, z2, press); > > > + //if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > > > + if ((!x && !y) || (st->pressure && (press > 0x350))) { > > > /* report end of touch */ > > > input_report_key(st->input, BTN_TOUCH, 0); > > > input_sync(st->input); > > > @@ -116,7 +135,7 @@ static int grts_probe(struct platform_device *pdev) > > > } > > > > > > chan = >iio_chans[0]; > > > - st->pressure = false; > > > + st->pressure = true; > > > while (chan && chan->indio_dev) { > > > > > > > > > > > > > There are quite a few things in here that feel pretty specific to the > > > > touchscreen > > > > usecase. That makes me wonder if this is a sensible approach or not. > > > > > > I'm sure it is the right way to go. Here is why: > > > > > > A typical resistive touchscreen can be described as 2 resistors (plates) > > > shorted to each other on pressure: > > > > > > o Y+ > > > | > > > # > > > # > > > # / shorted on pressure > > > |/ > > > o---###---+---###--o > > > X-|X+ > > > # > > > # > > > #
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Fri, Mar 5, 2021 at 3:40 PM Oleksij Rempel wrote: I have heard it will be a new version, so below a lot of nit-picks. > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for > the touchscreen use case. By implementing it as IIO ADC device, we can an IIO > make use of resistive-adc-touch and iio-hwmon drivers. > > So far, this driver was tested with custom version of resistive-adc-touch > driver, > since it need to be extended to make use of Z1 and Z2 channels. The X/Y needs > are working without additional changes. ... > +#include Usually asm/* goes after linux/* > +#include > +#include > +#include > +#include > +#include > +#include Can we move this group separately after all other includes? > +#include > +#include ... > +/* this driver doesn't aim at the peak continuous sample rate */ This > +/* > + * Default settling time. This time depends on the: > + * - PCB design > + * - touch plates size, temperature, etc > + * - initial power state of the ADC > + * > + * Since most values higher than 100us seems to be good, it make sense to seem makes > + * have some default value. These values were measuring get by testing on a were measured on a > + * PLYM2M board at 2MHz SPI CLK rate. > + * > + * Sometimes there are extra signal filter capacitors on the touchscreen > + * signals, which make it 10 or 100 times worse. > + */ ... > +#define TI_TSC2046_TIMESTAMP_SIZE (sizeof(int64_t) / > sizeof(int16_t)) Hmm... shouldn't we use a struct approach below? ... > +/* represents a HW sample */ Represents Kernel doc with explanation on the fields? ... > +/* layout of atomic buffers within big buffer */ Ditto. ... > + u16 scan_buf[TI_TSC2046_MAX_CHAN + 2 + TI_TSC2046_TIMESTAMP_SIZE]; Shouldn't we use a struct approach here? ... > + /* > +* Lock to protect the layout and the spi transfer buffer. SPI > +* 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. > +*/ ... > + dev_dbg(>spi->dev, "%s effective speed %u, time per bit: %u, > count bits: %u, count samples: %u\n", > + __func__, priv->effective_speed_hz, priv->time_per_bit_ns, > + bit_count, sample_count); Drop all these __func__ from everywhere. For debug they may be enabled via Dynamic Debug interface. ... > + switch (ch_idx) { > + case TI_TSC2046_ADDR_X: > + case TI_TSC2046_ADDR_Y: > + case TI_TSC2046_ADDR_Z1: > + case TI_TSC2046_ADDR_Z2: > + settle_time = TI_TSC2046_SETTLING_TIME_XYZ_DEF_US; > + break; > + default: > + settle_time = 0; break; > + } ... > +static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx, > + bool keep_power) > +{ > + u32 pd = 0; /* power down (pd) bits */ > + > + /* > +* if PD bits are 0, controller will automatically disable ADC, VREF > and > +* enable IRQ. > +*/ > + if (keep_power) > + pd = TI_TSC2046_PD0_ADC_ON; else pd = 0; ? Otherwise comments are kinda not fully clear. > + return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd; > +} ... > + /* last 3 bits on the wire are empty */ Last > + return get_unaligned_be16(>data) >> 3; Doesn't mean we will lose precision when the driver will be used as AD/C? ... > +static size_t tsc2046_adc_group_set_layout(struct tsc2046_adc_priv *priv, > + unsigned int group, > + unsigned int ch_idx) > +{ > + struct tsc2046_adc_group_layout *l = >l[group]; Hmm... > + unsigned int max_count, count_skip; > + unsigned int offset = 0; > + > + count_skip = tsc2046_adc_get_settle_count(priv, ch_idx); > + if (group != 0) { > + l = >l[group - 1]; > + offset = l->offset + l->count; > + } > + > + l = >l[group]; Why do you need to reassign this? Wouldn't be simpler to rewrite it as if (group) offset = ...; ? > + max_count = count_skip + TI_TSC2046_SAMPLES_XYZ_DEF; > + > + l->offset = offset; > + l->count = max_count; > + l->skip = count_skip; > + > + return sizeof(*priv->tx) * max_count; > +} ... > + switch (ch_idx) { > + case TI_TSC2046_ADDR_Y: > + if (val == 0xfff) > + return -EAGAIN; > + break; > + case TI_TSC2046_ADDR_X: > + if (!val) > + return -EAGAIN; > + break; default? > + } ... > + if (valid) { > + /* > +* Validate data to stop sampling and reduce power
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Tue, Mar 9, 2021 at 2:18 PM Oleksij Rempel wrote: > > On Tue, Mar 09, 2021 at 01:46:55PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 9, 2021 at 1:42 PM Oleksij Rempel > > wrote: > > > On Tue, Mar 09, 2021 at 01:05:27PM +0200, Andy Shevchenko wrote: > > > > On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron > > > > wrote: > > > > > > > > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > > > > Oleksij Rempel wrote: > > > > > > > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > > > > optimized for > > > > > > the touchscreen use case. By implementing it as IIO ADC device, we > > > > > > can > > > > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > > > > > > > So far, this driver was tested with custom version of > > > > > > resistive-adc-touch driver, > > > > > > since it need to be extended to make use of Z1 and Z2 channels. The > > > > > > X/Y > > > > > > are working without additional changes. > > > > > > > > > > > > Signed-off-by: Oleksij Rempel > > > > > > > > > > Hi Oleksij, > > > > > > > > > > To consider this as a possible long term route instead of just making > > > > > this > > > > > a touchscreen driver, we'll want to see those mods to the > > > > > resistive-adc-touch. > > > > > Of course that doesn't stop review of this in the meantime. > > > > > > > > > > There are quite a few things in here that feel pretty specific to the > > > > > touchscreen > > > > > usecase. That makes me wonder if this is a sensible approach or not. > > > > > > > > I'm wondering if this has any similarities with existing drivers under > > > > drivers/input/touchscreen. > > > > > > Yes, for example: drivers/input/touchscreen/ads7846.c > > > > Then I have a few questions here: > > 1/ why the above mentioned driver can't be extended to cover this? > > It is not possible to keep old device tree binding compatible with the > new driver at least not for currently existing abstraction: ADC + > touchscreen node. > > It is too expensive to overwrite the old driver, we do not have enough time > and > resource to do it. I lardy spend some weeks to do it and I would need a > many more weeks to make it by tiny slices without solving actual > problem. Many resistive touchscreen driver should share a lot of code. > > Since there is already existing IIO based components, it seems to me > better to spend available resource and making it properly in a way, > which reflect modern best practices. > > > 2/ or why is the proposed driver named after the touchscreen instead > > of the real AD/C chip behind it? > > I do not understand this question. The proposed driver is named after > the chip which provides ADC functionality, In this case, it is TSC2046. > The touchscreen is a separate physical module. > > The idea of this proposition is to keep physically separate components > separately on the kernel side. > > > 3/ maybe we can introduce a simple AD/C driver under IIO for that? > > There are already simple ADC drivers for that: > iio-hwmon: drivers/hwmon/iio_hwmon.c > resistive-adc-touch: drivers/input/touchscreen/resistive-adc-touch.c > > This two driver + the proposed one, will replace functionality of ads7846.c Okay, then maybe you can elaborate all this in the cover letter to make sure that maintainers will know why the new driver appeared instead of modifications to the old one. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Tue, Mar 09, 2021 at 01:46:55PM +0200, Andy Shevchenko wrote: > On Tue, Mar 9, 2021 at 1:42 PM Oleksij Rempel wrote: > > On Tue, Mar 09, 2021 at 01:05:27PM +0200, Andy Shevchenko wrote: > > > On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron > > > wrote: > > > > > > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > > > Oleksij Rempel wrote: > > > > > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > > > optimized for > > > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > > > > > So far, this driver was tested with custom version of > > > > > resistive-adc-touch driver, > > > > > since it need to be extended to make use of Z1 and Z2 channels. The > > > > > X/Y > > > > > are working without additional changes. > > > > > > > > > > Signed-off-by: Oleksij Rempel > > > > > > > > Hi Oleksij, > > > > > > > > To consider this as a possible long term route instead of just making > > > > this > > > > a touchscreen driver, we'll want to see those mods to the > > > > resistive-adc-touch. > > > > Of course that doesn't stop review of this in the meantime. > > > > > > > > There are quite a few things in here that feel pretty specific to the > > > > touchscreen > > > > usecase. That makes me wonder if this is a sensible approach or not. > > > > > > I'm wondering if this has any similarities with existing drivers under > > > drivers/input/touchscreen. > > > > Yes, for example: drivers/input/touchscreen/ads7846.c > > Then I have a few questions here: > 1/ why the above mentioned driver can't be extended to cover this? It is not possible to keep old device tree binding compatible with the new driver at least not for currently existing abstraction: ADC + touchscreen node. It is too expensive to overwrite the old driver, we do not have enough time and resource to do it. I lardy spend some weeks to do it and I would need a many more weeks to make it by tiny slices without solving actual problem. Many resistive touchscreen driver should share a lot of code. Since there is already existing IIO based components, it seems to me better to spend available resource and making it properly in a way, which reflect modern best practices. > 2/ or why is the proposed driver named after the touchscreen instead > of the real AD/C chip behind it? I do not understand this question. The proposed driver is named after the chip which provides ADC functionality, In this case, it is TSC2046. The touchscreen is a separate physical module. The idea of this proposition is to keep physically separate components separately on the kernel side. > 3/ maybe we can introduce a simple AD/C driver under IIO for that? There are already simple ADC drivers for that: iio-hwmon: drivers/hwmon/iio_hwmon.c resistive-adc-touch: drivers/input/touchscreen/resistive-adc-touch.c This two driver + the proposed one, will replace functionality of ads7846.c Regards, Oleksij -- 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- |
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Tue, Mar 9, 2021 at 1:42 PM Oleksij Rempel wrote: > On Tue, Mar 09, 2021 at 01:05:27PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron > > wrote: > > > > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > > Oleksij Rempel wrote: > > > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > > optimized for > > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > > > So far, this driver was tested with custom version of > > > > resistive-adc-touch driver, > > > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > > > are working without additional changes. > > > > > > > > Signed-off-by: Oleksij Rempel > > > > > > Hi Oleksij, > > > > > > To consider this as a possible long term route instead of just making this > > > a touchscreen driver, we'll want to see those mods to the > > > resistive-adc-touch. > > > Of course that doesn't stop review of this in the meantime. > > > > > > There are quite a few things in here that feel pretty specific to the > > > touchscreen > > > usecase. That makes me wonder if this is a sensible approach or not. > > > > I'm wondering if this has any similarities with existing drivers under > > drivers/input/touchscreen. > > Yes, for example: drivers/input/touchscreen/ads7846.c Then I have a few questions here: 1/ why the above mentioned driver can't be extended to cover this? 2/ or why is the proposed driver named after the touchscreen instead of the real AD/C chip behind it? 3/ maybe we can introduce a simple AD/C driver under IIO for that? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Tue, Mar 09, 2021 at 01:05:27PM +0200, Andy Shevchenko wrote: > On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron > wrote: > > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > Oleksij Rempel wrote: > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > optimized for > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > So far, this driver was tested with custom version of resistive-adc-touch > > > driver, > > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > > are working without additional changes. > > > > > > Signed-off-by: Oleksij Rempel > > > > Hi Oleksij, > > > > To consider this as a possible long term route instead of just making this > > a touchscreen driver, we'll want to see those mods to the > > resistive-adc-touch. > > Of course that doesn't stop review of this in the meantime. > > > > There are quite a few things in here that feel pretty specific to the > > touchscreen > > usecase. That makes me wonder if this is a sensible approach or not. > > I'm wondering if this has any similarities with existing drivers under > drivers/input/touchscreen. Yes, for example: drivers/input/touchscreen/ads7846.c Regards, Oleksij -- 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- |
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
Hi Jonathan, On Sat, Mar 06, 2021 at 02:59:59PM +, Jonathan Cameron wrote: > On Sat, 6 Mar 2021 14:28:52 +0100 > Oleksij Rempel wrote: > > > On Fri, Mar 05, 2021 at 07:02:39PM +, Jonathan Cameron wrote: > > > On Fri, 5 Mar 2021 14:38:13 +0100 > > > Oleksij Rempel wrote: > > > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > > optimized for > > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > > > So far, this driver was tested with custom version of > > > > resistive-adc-touch driver, > > > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > > > are working without additional changes. > > > > > > > > Signed-off-by: Oleksij Rempel > > > > > > Hi Oleksij, > > > > > > To consider this as a possible long term route instead of just making this > > > a touchscreen driver, we'll want to see those mods to the > > > resistive-adc-touch. > > > Of course that doesn't stop review of this in the meantime. > > > > ok. > > > > I had following issues with the existing resistive-adc-touch driver: > > - the buffer layout is not configurable over DT or i didn't understood > > how to properly configure it > > - the "pressure" channel provide pre processed data driver or > > controller, this information cannot be extracted directly from the > > touchscreen plates. > > > > I did following changes to make it work for my use case: > > > > --- a/drivers/input/touchscreen/resistive-adc-touch.c > > +++ b/drivers/input/touchscreen/resistive-adc-touch.c > > @@ -44,15 +44,34 @@ static int grts_cb(const void *data, void *private) > > { > > const u16 *touch_info = data; > > struct grts_state *st = private; > > - unsigned int x, y, press = 0x0; > > + unsigned int x, y, press = 0x0, z1, z2; > > + unsigned int Rt; > > > > /* channel data coming in buffer in the order below */ > > - x = touch_info[0]; > > - y = touch_info[1]; > > + // TODO: make sure we get buffers in proper order > > Ah. So to figure this out we'll need to read some more info about the > channels. The phandle order for the touchscreen binding > should probably be specified (if it's not already) and that should let > us establish the ordering of channels. Ack. So this should be done in the touchscreen driver and can be done later? > > + x = touch_info[3]; > > + z2 = touch_info[2]; > > + z1 = touch_info[1]; > > + y = touch_info[0]; > > + > > + if (z1) { > > + Rt = z2; > > So for this we are going to need to define it in a generic fashion - probably > via a mode + coefficients in DT? I assume, mode will be needed any way, coefficients can stay as is and if we get some different use case add an overwrite binding to the devicetree. > > + Rt -= z1; > > + Rt *= 800; > > + //Rt *= ts->x_plate_ohms; > > + Rt = DIV_ROUND_CLOSEST(Rt, 16); > > + Rt *= x; > > + Rt /= z1; > > + Rt = DIV_ROUND_CLOSEST(Rt, 256); > > + } else > > + Rt = 0x400; > > + > > if (st->pressure) > > - press = touch_info[2]; > > + press = Rt; > > > > - if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > > + //printk("%s:%i: x: %x, y %x, z1: %x, z2: %x, press: %x\n", __func__, > > __LINE__, x, y, z1, z2, press); > > + //if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > > + if ((!x && !y) || (st->pressure && (press > 0x350))) { > > /* report end of touch */ > > input_report_key(st->input, BTN_TOUCH, 0); > > input_sync(st->input); > > @@ -116,7 +135,7 @@ static int grts_probe(struct platform_device *pdev) > > } > > > > chan = >iio_chans[0]; > > - st->pressure = false; > > + st->pressure = true; > > while (chan && chan->indio_dev) { > > > > > > > > > There are quite a few things in here that feel pretty specific to the > > > touchscreen > > > usecase. That makes me wonder if this is a sensible approach or not. > > > > I'm sure it is the right way to go. Here is why: > > > > A typical resistive touchscreen can be described as 2 resistors (plates) > > shorted to each other on pressure: > > > > o Y+ > > | > > # > > # > > # / shorted on pressure > > |/ > > o---###---+---###--o > > X-|X+ > > # > > # > > # > > | > > o Y- > > > > > > To find the location of shorted circuit (finger position) we need to > > measure voltage on different points of the circuit: > > - to get X-position, apply voltage on X+/X- and measure voltage on Y+ > > - to get Y-position, apply voltage on Y+/Y- and measure voltage on X+ > > > > Measuring the "pressure" is a bit more tricky: > > - apply voltage on X-/Y+ and measure on X+ and Y-, so we will get Z1 and > > Z2 > >
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Sun, Mar 07, 2021 at 03:05:31PM -0800, Dmitry Torokhov wrote: > Hi Oleksij, > > On Fri, Mar 05, 2021 at 02:38:13PM +0100, Oleksij Rempel wrote: > > + > > + /* TODO: remove IRQ_NOAUTOEN after needed patches are mainline */ > > + irq_set_status_flags(spi->irq, IRQ_NOAUTOEN); > > + ret = devm_request_threaded_irq(dev, spi->irq, > > + NULL, > > + _adc_irq, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + name, indio_dev); > > I'd recommend dropping IRQF_TRIGGER_LOW and only using IRQF_ONESHOT and > rely on the platform (ACPI, DT) to specify trigger polarity according to > how device is wired in a given system. In general I believe newer > drivers should not specify interrupt triggers themselves. Ok, thx. Sounds good. Regards, Oleksij -- 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- |
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron wrote: > > On Fri, 5 Mar 2021 14:38:13 +0100 > Oleksij Rempel wrote: > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized > > for > > the touchscreen use case. By implementing it as IIO ADC device, we can > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > So far, this driver was tested with custom version of resistive-adc-touch > > driver, > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > are working without additional changes. > > > > Signed-off-by: Oleksij Rempel > > Hi Oleksij, > > To consider this as a possible long term route instead of just making this > a touchscreen driver, we'll want to see those mods to the resistive-adc-touch. > Of course that doesn't stop review of this in the meantime. > > There are quite a few things in here that feel pretty specific to the > touchscreen > usecase. That makes me wonder if this is a sensible approach or not. I'm wondering if this has any similarities with existing drivers under drivers/input/touchscreen. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
Hi Oleksij, On Fri, Mar 05, 2021 at 02:38:13PM +0100, Oleksij Rempel wrote: > + > + /* TODO: remove IRQ_NOAUTOEN after needed patches are mainline */ > + irq_set_status_flags(spi->irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(dev, spi->irq, > + NULL, > + _adc_irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + name, indio_dev); I'd recommend dropping IRQF_TRIGGER_LOW and only using IRQF_ONESHOT and rely on the platform (ACPI, DT) to specify trigger polarity according to how device is wired in a given system. In general I believe newer drivers should not specify interrupt triggers themselves. Thanks. -- Dmitry
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Sat, 6 Mar 2021 14:28:52 +0100 Oleksij Rempel wrote: > On Fri, Mar 05, 2021 at 07:02:39PM +, Jonathan Cameron wrote: > > On Fri, 5 Mar 2021 14:38:13 +0100 > > Oleksij Rempel wrote: > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > optimized for > > > the touchscreen use case. By implementing it as IIO ADC device, we can > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > So far, this driver was tested with custom version of resistive-adc-touch > > > driver, > > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > > are working without additional changes. > > > > > > Signed-off-by: Oleksij Rempel > > > > Hi Oleksij, > > > > To consider this as a possible long term route instead of just making this > > a touchscreen driver, we'll want to see those mods to the > > resistive-adc-touch. > > Of course that doesn't stop review of this in the meantime. > > ok. > > I had following issues with the existing resistive-adc-touch driver: > - the buffer layout is not configurable over DT or i didn't understood > how to properly configure it > - the "pressure" channel provide pre processed data driver or > controller, this information cannot be extracted directly from the > touchscreen plates. > > I did following changes to make it work for my use case: > > --- a/drivers/input/touchscreen/resistive-adc-touch.c > +++ b/drivers/input/touchscreen/resistive-adc-touch.c > @@ -44,15 +44,34 @@ static int grts_cb(const void *data, void *private) > { > const u16 *touch_info = data; > struct grts_state *st = private; > - unsigned int x, y, press = 0x0; > + unsigned int x, y, press = 0x0, z1, z2; > + unsigned int Rt; > > /* channel data coming in buffer in the order below */ > - x = touch_info[0]; > - y = touch_info[1]; > + // TODO: make sure we get buffers in proper order Ah. So to figure this out we'll need to read some more info about the channels. The phandle order for the touchscreen binding should probably be specified (if it's not already) and that should let us establish the ordering of channels. > + x = touch_info[3]; > + z2 = touch_info[2]; > + z1 = touch_info[1]; > + y = touch_info[0]; > + > + if (z1) { > + Rt = z2; So for this we are going to need to define it in a generic fashion - probably via a mode + coefficients in DT? > + Rt -= z1; > + Rt *= 800; > + //Rt *= ts->x_plate_ohms; > + Rt = DIV_ROUND_CLOSEST(Rt, 16); > + Rt *= x; > + Rt /= z1; > + Rt = DIV_ROUND_CLOSEST(Rt, 256); > + } else > + Rt = 0x400; > + > if (st->pressure) > - press = touch_info[2]; > + press = Rt; > > - if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > + //printk("%s:%i: x: %x, y %x, z1: %x, z2: %x, press: %x\n", __func__, > __LINE__, x, y, z1, z2, press); > + //if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { > + if ((!x && !y) || (st->pressure && (press > 0x350))) { > /* report end of touch */ > input_report_key(st->input, BTN_TOUCH, 0); > input_sync(st->input); > @@ -116,7 +135,7 @@ static int grts_probe(struct platform_device *pdev) > } > > chan = >iio_chans[0]; > - st->pressure = false; > + st->pressure = true; > while (chan && chan->indio_dev) { > > > > > There are quite a few things in here that feel pretty specific to the > > touchscreen > > usecase. That makes me wonder if this is a sensible approach or not. > > I'm sure it is the right way to go. Here is why: > > A typical resistive touchscreen can be described as 2 resistors (plates) > shorted to each other on pressure: > > o Y+ > | > # > # > # / shorted on pressure > |/ > o---###---+---###--o > X-|X+ > # > # > # > | > o Y- > > > To find the location of shorted circuit (finger position) we need to > measure voltage on different points of the circuit: > - to get X-position, apply voltage on X+/X- and measure voltage on Y+ > - to get Y-position, apply voltage on Y+/Y- and measure voltage on X+ > > Measuring the "pressure" is a bit more tricky: > - apply voltage on X-/Y+ and measure on X+ and Y-, so we will get Z1 and > Z2 > - will need to know real plate resistance to do following calculation: > Rtouch = Rx-plate * (X-position / 4096) * (Z2/Z1 - 1) > > There is is still more points which share all resistive touchscreens: > - they have parasitic capacitance, so it take some time between > switching to voltage on and usable measurements > - they act as antenna, so we measure different kind of electrical noise > - we have low-frequency mechanical waves on the plates which can trigger > some bounce
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Fri, Mar 05, 2021 at 07:02:39PM +, Jonathan Cameron wrote: > On Fri, 5 Mar 2021 14:38:13 +0100 > Oleksij Rempel wrote: > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized > > for > > the touchscreen use case. By implementing it as IIO ADC device, we can > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > So far, this driver was tested with custom version of resistive-adc-touch > > driver, > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > > are working without additional changes. > > > > Signed-off-by: Oleksij Rempel > > Hi Oleksij, > > To consider this as a possible long term route instead of just making this > a touchscreen driver, we'll want to see those mods to the resistive-adc-touch. > Of course that doesn't stop review of this in the meantime. ok. I had following issues with the existing resistive-adc-touch driver: - the buffer layout is not configurable over DT or i didn't understood how to properly configure it - the "pressure" channel provide pre processed data driver or controller, this information cannot be extracted directly from the touchscreen plates. I did following changes to make it work for my use case: --- a/drivers/input/touchscreen/resistive-adc-touch.c +++ b/drivers/input/touchscreen/resistive-adc-touch.c @@ -44,15 +44,34 @@ static int grts_cb(const void *data, void *private) { const u16 *touch_info = data; struct grts_state *st = private; - unsigned int x, y, press = 0x0; + unsigned int x, y, press = 0x0, z1, z2; + unsigned int Rt; /* channel data coming in buffer in the order below */ - x = touch_info[0]; - y = touch_info[1]; + // TODO: make sure we get buffers in proper order + x = touch_info[3]; + z2 = touch_info[2]; + z1 = touch_info[1]; + y = touch_info[0]; + + if (z1) { + Rt = z2; + Rt -= z1; + Rt *= 800; + //Rt *= ts->x_plate_ohms; + Rt = DIV_ROUND_CLOSEST(Rt, 16); + Rt *= x; + Rt /= z1; + Rt = DIV_ROUND_CLOSEST(Rt, 256); + } else + Rt = 0x400; + if (st->pressure) - press = touch_info[2]; + press = Rt; - if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { + //printk("%s:%i: x: %x, y %x, z1: %x, z2: %x, press: %x\n", __func__, __LINE__, x, y, z1, z2, press); + //if ((!x && !y) || (st->pressure && (press < st->pressure_min))) { + if ((!x && !y) || (st->pressure && (press > 0x350))) { /* report end of touch */ input_report_key(st->input, BTN_TOUCH, 0); input_sync(st->input); @@ -116,7 +135,7 @@ static int grts_probe(struct platform_device *pdev) } chan = >iio_chans[0]; - st->pressure = false; + st->pressure = true; while (chan && chan->indio_dev) { > There are quite a few things in here that feel pretty specific to the > touchscreen > usecase. That makes me wonder if this is a sensible approach or not. I'm sure it is the right way to go. Here is why: A typical resistive touchscreen can be described as 2 resistors (plates) shorted to each other on pressure: o Y+ | # # # / shorted on pressure |/ o---###---+---###--o X-|X+ # # # | o Y- To find the location of shorted circuit (finger position) we need to measure voltage on different points of the circuit: - to get X-position, apply voltage on X+/X- and measure voltage on Y+ - to get Y-position, apply voltage on Y+/Y- and measure voltage on X+ Measuring the "pressure" is a bit more tricky: - apply voltage on X-/Y+ and measure on X+ and Y-, so we will get Z1 and Z2 - will need to know real plate resistance to do following calculation: Rtouch = Rx-plate * (X-position / 4096) * (Z2/Z1 - 1) There is is still more points which share all resistive touchscreens: - they have parasitic capacitance, so it take some time between switching to voltage on and usable measurements - they act as antenna, so we measure different kind of electrical noise - we have low-frequency mechanical waves on the plates which can trigger some bounce artifacts - the results will change depending on the temperature and the supply voltage. So we need to monitor both of them to adjust our results. To handle this issues we need to skip some samples until voltage is settled, we need to apply some simple digital low-pass filter to reduce the noise and add some corrections if we are able to measure system temperature and voltage. All of described measurements are more or less touchscreen and not touch controller specific. IMO, resistive-adc-touch provide proper abstraction separation and should be a long therm - way to go :) Now is the
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Fri, 5 Mar 2021 14:38:13 +0100 Oleksij Rempel wrote: > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for > the touchscreen use case. By implementing it as IIO ADC device, we can > make use of resistive-adc-touch and iio-hwmon drivers. > > So far, this driver was tested with custom version of resistive-adc-touch > driver, > since it need to be extended to make use of Z1 and Z2 channels. The X/Y > are working without additional changes. > > Signed-off-by: Oleksij Rempel Hi Oleksij, To consider this as a possible long term route instead of just making this a touchscreen driver, we'll want to see those mods to the resistive-adc-touch. Of course that doesn't stop review of this in the meantime. There are quite a few things in here that feel pretty specific to the touchscreen usecase. That makes me wonder if this is a sensible approach or not. Jonathan > --- > MAINTAINERS | 8 + > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ti-tsc2046.c | 652 +++ > 4 files changed, 673 insertions(+) > create mode 100644 drivers/iio/adc/ti-tsc2046.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3fea1a934b32..2d33c6442a55 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17852,6 +17852,14 @@ S: Supported > F: Documentation/devicetree/bindings/net/nfc/trf7970a.txt > F: drivers/nfc/trf7970a.c > > +TI TSC2046 ADC DRIVER > +M: Oleksij Rempel > +R: ker...@pengutronix.de > +L: linux-...@vger.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 > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 15587a1bc80d..6ad6f04dfd20 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -1175,6 +1175,18 @@ config TI_TLC4541 > This driver can also be built as a module. If so, the module will be > called ti-tlc4541. > > +config TI_TSC2046 > + tristate "Texas Instruments TSC2046 ADC driver" > + depends on SPI > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say yes here to build support for ADC functionality of Texas > + Instruments TSC2046 touch screen controller. > + > + This driver can also be built as a module. If so, the module will be > + called ti-tsc2046. > + > config TWL4030_MADC > tristate "TWL4030 MADC (Monitoring A/D Converter)" > depends on TWL4030_CORE > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 5fca90ada0ec..440e18ac6780 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -105,6 +105,7 @@ obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o > obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o > obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o > +obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o > obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o > obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o > obj-$(CONFIG_VF610_ADC) += vf610_adc.o > diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c > new file mode 100644 > index ..e119e7c31fa7 > --- /dev/null > +++ b/drivers/iio/adc/ti-tsc2046.c > @@ -0,0 +1,652 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Texas Instruments TSC2046 SPI ADC driver > + * > + * Copyright (c) 2021 Oleksij Rempel , Pengutronix > + */ > + > +#include I'd prefer the more specific include after the generic linux ones. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define TI_TSC2046_NAME "tsc2046" > + > +/* this driver doesn't aim at the peak continuous sample rate */ > +#define TI_TSC2046_MAX_SAMPLE_RATE 125000 > +#define TI_TSC2046_SAMPLE_BITS (8 /*cmd*/ + 16 > /*sample*/) > +#define TI_TSC2046_MAX_CLK_FREQ \ > + (TI_TSC2046_MAX_SAMPLE_RATE * TI_TSC2046_SAMPLE_BITS) > + > +/* > + * Default settling time. This time depends on the: > + * - PCB design > + * - touch plates size, temperature, etc > + * - initial power state of the ADC > + * > + * Since most values higher than 100us seems to be good, it make sense to > + * have some default value. These values were measuring get by testing on a > + * PLYM2M board at 2MHz SPI CLK rate. > + * > + * Sometimes there are extra signal filter capacitors on the touchscreen > + * signals, which make it 10 or 100 times worse. Sounds like something that makes sense to expose in dt? > + */ > +#define TI_TSC2046_SETTLING_TIME_XYZ_DEF_US 700 > +/* Oversampling count for the low-pass filter */ > +#define TI_TSC2046_SAMPLES_XYZ_DEF 5 > +/* Default sample interval */ > +#define TI_TSC2046_SAMPLE_INTERVAL_US
[PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for the touchscreen use case. By implementing it as IIO ADC device, we can make use of resistive-adc-touch and iio-hwmon drivers. So far, this driver was tested with custom version of resistive-adc-touch driver, since it need to be extended to make use of Z1 and Z2 channels. The X/Y are working without additional changes. Signed-off-by: Oleksij Rempel --- MAINTAINERS | 8 + drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ti-tsc2046.c | 652 +++ 4 files changed, 673 insertions(+) create mode 100644 drivers/iio/adc/ti-tsc2046.c diff --git a/MAINTAINERS b/MAINTAINERS index 3fea1a934b32..2d33c6442a55 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17852,6 +17852,14 @@ S: Supported F: Documentation/devicetree/bindings/net/nfc/trf7970a.txt F: drivers/nfc/trf7970a.c +TI TSC2046 ADC DRIVER +M: Oleksij Rempel +R: ker...@pengutronix.de +L: linux-...@vger.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 L: alsa-de...@alsa-project.org (moderated for non-subscribers) diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 15587a1bc80d..6ad6f04dfd20 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1175,6 +1175,18 @@ config TI_TLC4541 This driver can also be built as a module. If so, the module will be called ti-tlc4541. +config TI_TSC2046 + tristate "Texas Instruments TSC2046 ADC driver" + depends on SPI + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + Say yes here to build support for ADC functionality of Texas + Instruments TSC2046 touch screen controller. + + This driver can also be built as a module. If so, the module will be + called ti-tsc2046. + config TWL4030_MADC tristate "TWL4030 MADC (Monitoring A/D Converter)" depends on TWL4030_CORE diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 5fca90ada0ec..440e18ac6780 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -105,6 +105,7 @@ obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o +obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o obj-$(CONFIG_VF610_ADC) += vf610_adc.o diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c new file mode 100644 index ..e119e7c31fa7 --- /dev/null +++ b/drivers/iio/adc/ti-tsc2046.c @@ -0,0 +1,652 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Texas Instruments TSC2046 SPI ADC driver + * + * Copyright (c) 2021 Oleksij Rempel , Pengutronix + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define TI_TSC2046_NAME"tsc2046" + +/* this driver doesn't aim at the peak continuous sample rate */ +#defineTI_TSC2046_MAX_SAMPLE_RATE 125000 +#defineTI_TSC2046_SAMPLE_BITS (8 /*cmd*/ + 16 /*sample*/) +#defineTI_TSC2046_MAX_CLK_FREQ \ + (TI_TSC2046_MAX_SAMPLE_RATE * TI_TSC2046_SAMPLE_BITS) + +/* + * Default settling time. This time depends on the: + * - PCB design + * - touch plates size, temperature, etc + * - initial power state of the ADC + * + * Since most values higher than 100us seems to be good, it make sense to + * have some default value. These values were measuring get by testing on a + * PLYM2M board at 2MHz SPI CLK rate. + * + * Sometimes there are extra signal filter capacitors on the touchscreen + * signals, which make it 10 or 100 times worse. + */ +#define TI_TSC2046_SETTLING_TIME_XYZ_DEF_US700 +/* Oversampling count for the low-pass filter */ +#define TI_TSC2046_SAMPLES_XYZ_DEF 5 +/* Default sample interval */ +#define TI_TSC2046_SAMPLE_INTERVAL_US 1 + +#define TI_TSC2046_START BIT(7) +#define TI_TSC2046_ADDRGENMASK(6, 4) +#define TI_TSC2046_ADDR_TEMP1 7 +#define TI_TSC2046_ADDR_AUX6 +#define TI_TSC2046_ADDR_X 5 +#define TI_TSC2046_ADDR_Z2 4 +#define TI_TSC2046_ADDR_Z1 3 +#define TI_TSC2046_ADDR_VBAT 2 +#define TI_TSC2046_ADDR_Y 1 +#define TI_TSC2046_ADDR_TEMP0 0 + +/* + * The mode bit sets the resolution of the ADC. With this bit low, the next + * conversion has 12-bit resolution, whereas with this bit high, the next + * conversion has 8-bit resolution. This driver is optimized