On Tue, 9 Mar 2021 12:40:46 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> On Sat, Mar 06, 2021 at 02:59:59PM +0000, Jonathan Cameron wrote:
> > On Sat, 6 Mar 2021 14:28:52 +0100
> > Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> >
> > > On Fri, Mar 05, 2021 at 07:02:39PM +0000, Jonathan Cameron wrote:
> > > > On Fri, 5 Mar 2021 14:38:13 +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 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 <o.rempel@xxxxxxxxxxxxxx>
> > > >
> > > > 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 = &st->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 question, what is TSC2046? Please see the block diagram on
> > > page 10:
> > > https://www.ti.com/lit/ds/symlink/tsc2046.pdf
> > >
> > > If I oversimplify this diagram, this controller is an ADC with 3 pin
> > > muxes:
> > > - ADC input mux
> > > - termistor mux chained after ADC mux
> > > - voltage output mux
> > >
> > > all touchscreen specific code within this driver can be moved to the
> > > resistive-adc-touch, but need some optimization or discussion on how
> > > this should be done properly.
> >
> > ok. There is definitely an argument in favour of generic code. Whether
> > the method of IIO provider and touch screen driver consumer makes sense
> > is a little less clear but it's certainly a reasonable option.
> >
> > > > > ---
> > > > > +/*
> > > > > + * 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?
> > >
> > > yes. we need to discuss what is the proper place. Is it possible to
> > > grub enough buffers in CPU effective way withing one SPI transfer and
> > > then hand over it to the resistive-adc-touch driver?
> > >
> > > I placed it within ADC driver for optimization. IMO, proper place should
> > > be resistive-adc-touch driver.
> >
> > It isn't that unusual to have a generic ADC that needs some settling time
> > before a good result is obtained, so it might be fine to keep an element of
> > that in the ADC driver, perhaps even defining a standard interface to do
> > it. It is a somewhat similar requirement to oversampling, just that you
> > drop the first N samples rather than averaging them.
>
> ok, sounds good.
>
>
> > > > > + */
> > > > > + if (ret < 0 && ret != -EBUSY) {
> > > > > + dev_err_ratelimited(dev, "%s: Filed to push scan buffer %pe\n",
> > > > failed
> > > > > + __func__, ERR_PTR(ret));
> > > > > +
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + return valid ? 0 : -EAGAIN;
> > > >
> > > > If not valid, why are we pushing it to the buffer?
> > >
> > > The touchscreen driver will classify this buffer as finger-up event.
> > > and the ADC driver need to know it to stop sampling.
> >
> > That is a fairly specific thing to do. Does the existing
> > touch screen driver do that, or it that a proposed change?
>
> existing driver is already doing it, the proposed change change only
> change the way how this information is extracted. The ADC driver needs
> only provide enough samples, so that touchscreen driver can detect it.
>
Ok. I'm fine with that in the touchscreen side of things.
