On Wed, May 29, 2013 at 07:09:53PM +0200, Sebastian Andrzej Siewior wrote: > The driver programs a threshold of "steps-to-configure" say 5. The > REG_FIFO0THR registers says it should it be programmed to "threshold > minus one". The driver does not expect just 5 coordinates but 5 * 2 + 2. > Multiplied by two because 5 for X and 5 for Y. Plus 2 because we have > two Z. > The whole thing works because It reads the 5 coordinates for X and Y and > split over FIFO0 and FIFO1 and the last element in each FIFO is ignored > within the loop and read later. > Nothing guaranties that FIFO1 is ready by the time it is read. In fact I > could see that that FIFO1 reaturns for Y channels 8,9, 10, 12, 6 and for > Y channel 7 for Z. The problem is that channel 7 and channel 12 got > somehow mixed up. The variance filter here maybe tries to get rid of the > wrong Y values, dunno. > The other Problem is that FIFO1 is also used by the IIO part leading to > not wrong results if both (tsc & adc) are used. > > The patch tries to clean up the whole thing a little: > - Name it "coordiante-readouts" and not "steps-to-configure" and > document it properly. Point out that it the hardware does a total of > "5 * 2 + 2" reads / steps and not just what is configured. > > - Remove the +1 and -1 in REG_STEPCONFIG, REG_STEPDELAY and its counter > part in the for loop. This is just confusing. > > - Use only FIFO0 in TSC. The fifo has space for 64 entries so should be > fine. > > - Read the whole FIFO in one function and check the channel. > > - in case we dawdle around, make sure we only read a multiple of our > coordinate set. On the second interrupt we will cleanup the remaining > enties. > > Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Acked-by: Dmitry Torokhov <[email protected]> > --- > .../bindings/input/touchscreen/ti-tsc-adc.txt | 15 ++-- > arch/arm/boot/dts/am335x-evm.dts | 2 +- > drivers/iio/adc/ti_am335x_adc.c | 2 +- > drivers/input/touchscreen/ti_am335x_tsc.c | 87 > ++++++++++---------- > include/linux/mfd/ti_am335x_tscadc.h | 4 +- > 5 files changed, 58 insertions(+), 52 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt > b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt > index e533e9d..80e04e3 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt > @@ -6,10 +6,15 @@ > ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen > support on the platform. > ti,x-plate-resistance: X plate resistance > - ti,steps-to-configure: The sequencer supports a total of 16 > - programmable steps. A step configured to read a > - single co-ordinate value. Can be applied more > - number of times for better results. > + ti,coordiante-readouts: The sequencer supports a total of 16 > + programmable steps each step is used to > + read a single coordinate. A single > + readout is enough but multiple reads can > + increase the quality. > + A value of 5 means, 5 reads for X, 5 for > + Y and 2 for Z (always). This utilises 12 > + of the 16 software steps available. The > + remaining 4 can be used by the ADC. > ti,wire-config: Different boards could have a different order for > connecting wires on touchscreen. We need to provide an > 8 bit number where in the 1st four bits represent the > @@ -28,7 +33,7 @@ > tsc { > ti,wires = <4>; > ti,x-plate-resistance = <200>; > - ti,steps-to-configure = <5>; > + ti,coordiante-readouts = <5>; > ti,wire-config = <0x00 0x11 0x22 0x33>; > }; > > diff --git a/arch/arm/boot/dts/am335x-evm.dts > b/arch/arm/boot/dts/am335x-evm.dts > index be6a5b2..26fea97 100644 > --- a/arch/arm/boot/dts/am335x-evm.dts > +++ b/arch/arm/boot/dts/am335x-evm.dts > @@ -250,7 +250,7 @@ > tsc { > ti,wires = <4>; > ti,x-plate-resistance = <200>; > - ti,steps-to-configure = <5>; > + ti,coordiante-readouts = <5>; > ti,wire-config = <0x00 0x11 0x22 0x33>; > }; > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index b2f27de..2988840 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -72,7 +72,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev) > > stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1; > > - for (i = (steps + 1); i <= TOTAL_STEPS; i++) { > + for (i = steps; i < TOTAL_STEPS; i++) { > tiadc_writel(adc_dev, REG_STEPCONFIG(i), > stepconfig | STEPCONFIG_INP(channels)); > tiadc_writel(adc_dev, REG_STEPDELAY(i), > diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c > b/drivers/input/touchscreen/ti_am335x_tsc.c > index 2921163..7c97fc7 100644 > --- a/drivers/input/touchscreen/ti_am335x_tsc.c > +++ b/drivers/input/touchscreen/ti_am335x_tsc.c > @@ -58,7 +58,7 @@ struct titsc { > unsigned int bckup_x; > unsigned int bckup_y; > bool pen_down; > - int steps_to_configure; > + int coordiante_readouts; > u32 config_inp[4]; > int bit_xp, bit_xn, bit_yp, bit_yn; > int inp_xp, inp_xn, inp_yp, inp_yn; > @@ -168,11 +168,8 @@ static int titsc_config_wires(struct titsc *ts_dev) > static void titsc_step_config(struct titsc *ts_dev) > { > unsigned int config; > - unsigned int stepenable = 0; > - int i, total_steps; > - > - /* Configure the Step registers */ > - total_steps = 2 * ts_dev->steps_to_configure; > + int i; > + int end_step; > > config = STEPCONFIG_MODE_HWSYNC | > STEPCONFIG_AVG_16 | ts_dev->bit_xp; > @@ -190,7 +187,9 @@ static void titsc_step_config(struct titsc *ts_dev) > break; > } > > - for (i = 1; i <= ts_dev->steps_to_configure; i++) { > + /* 1 … coordiante_readouts is for X */ > + end_step = ts_dev->coordiante_readouts; > + for (i = 0; i < end_step; i++) { > titsc_writel(ts_dev, REG_STEPCONFIG(i), config); > titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY); > } > @@ -198,7 +197,7 @@ static void titsc_step_config(struct titsc *ts_dev) > config = 0; > config = STEPCONFIG_MODE_HWSYNC | > STEPCONFIG_AVG_16 | ts_dev->bit_yn | > - STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1; > + STEPCONFIG_INM_ADCREFM; > switch (ts_dev->wires) { > case 4: > config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp); > @@ -212,12 +211,13 @@ static void titsc_step_config(struct titsc *ts_dev) > break; > } > > - for (i = (ts_dev->steps_to_configure + 1); i <= total_steps; i++) { > + /* coordiante_readouts … coordiante_readouts * 2 is for Y */ > + end_step = ts_dev->coordiante_readouts * 2; > + for (i = ts_dev->coordiante_readouts; i < end_step; i++) { > titsc_writel(ts_dev, REG_STEPCONFIG(i), config); > titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY); > } > > - config = 0; > /* Charge step configuration */ > config = ts_dev->bit_xp | ts_dev->bit_yn | > STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR | > @@ -226,37 +226,40 @@ static void titsc_step_config(struct titsc *ts_dev) > titsc_writel(ts_dev, REG_CHARGECONFIG, config); > titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY); > > - config = 0; > - /* Configure to calculate pressure */ > + /* coordiante_readouts * 2 … coordiante_readouts * 2 + 2 is for Z */ > config = STEPCONFIG_MODE_HWSYNC | > STEPCONFIG_AVG_16 | ts_dev->bit_yp | > ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM | > STEPCONFIG_INP(ts_dev->inp_xp); > - titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config); > - titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1), > + titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config); > + titsc_writel(ts_dev, REG_STEPDELAY(end_step), > STEPCONFIG_OPENDLY); > > - config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1; > - titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config); > - titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2), > + end_step++; > + config |= STEPCONFIG_INP(ts_dev->inp_yn); > + titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config); > + titsc_writel(ts_dev, REG_STEPDELAY(end_step), > STEPCONFIG_OPENDLY); > > - for (i = 0; i <= (total_steps + 2); i++) > - stepenable |= 1 << i; > - ts_dev->enable_bits = stepenable; > + /* The steps1 … end and bit 0 for TS_Charge */ > + ts_dev->enable_bits = (1 << (end_step + 2)) - 1; > > titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits); > } > > static void titsc_read_coordinates(struct titsc *ts_dev, > - unsigned int *x, unsigned int *y) > + u32 *x, u32 *y, u32 *z1, u32 *z2) > { > unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT); > unsigned int prev_val_x = ~0, prev_val_y = ~0; > unsigned int prev_diff_x = ~0, prev_diff_y = ~0; > unsigned int read, diff; > unsigned int i, channel; > + unsigned int creads = ts_dev->coordiante_readouts; > > + *z1 = *z2 = 0; > + if (fifocount % (creads * 2 + 2)) > + fifocount -= fifocount % (creads * 2 + 2); > /* > * Delta filter is used to remove large variations in sampled > * values from ADC. The filter tries to predict where the next > @@ -265,32 +268,31 @@ static void titsc_read_coordinates(struct titsc *ts_dev, > * algorithm compares the difference with that of a present value, > * if true the value is reported to the sub system. > */ > - for (i = 0; i < fifocount - 1; i++) { > + for (i = 0; i < fifocount; i++) { > read = titsc_readl(ts_dev, REG_FIFO0); > - channel = read & 0xf0000; > - channel = channel >> 0x10; > - if ((channel >= 0) && (channel < ts_dev->steps_to_configure)) { > - read &= 0xfff; > + channel = (read & 0xf0000) >> 16; > + read &= 0xfff; > + if (channel < creads) { > diff = abs(read - prev_val_x); > if (diff < prev_diff_x) { > prev_diff_x = diff; > *x = read; > } > prev_val_x = read; > - } > > - read = titsc_readl(ts_dev, REG_FIFO1); > - channel = read & 0xf0000; > - channel = channel >> 0x10; > - if ((channel >= ts_dev->steps_to_configure) && > - (channel < (2 * ts_dev->steps_to_configure - 1))) { > - read &= 0xfff; > + } else if (channel < creads * 2) { > diff = abs(read - prev_val_y); > if (diff < prev_diff_y) { > prev_diff_y = diff; > *y = read; > } > prev_val_y = read; > + > + } else if (channel < creads * 2 + 1) { > + *z1 = read; > + > + } else if (channel < creads * 2 + 2) { > + *z2 = read; > } > } > } > @@ -307,26 +309,24 @@ static irqreturn_t titsc_irq(int irq, void *dev) > > status = titsc_readl(ts_dev, REG_IRQSTATUS); > if (status & IRQENB_FIFO0THRES) { > - titsc_read_coordinates(ts_dev, &x, &y); > + > + titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2); > > diffx = abs(x - (ts_dev->bckup_x)); > diffy = abs(y - (ts_dev->bckup_y)); > ts_dev->bckup_x = x; > ts_dev->bckup_y = y; > > - z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff; > - z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff; > - > if (ts_dev->pen_down && z1 != 0 && z2 != 0) { > /* > * Calculate pressure using formula > * Resistance(touch) = x plate resistance * > * x postion/4096 * ((z2 / z1) - 1) > */ > - z = z2 - z1; > + z = z1 - z2; > z *= x; > z *= ts_dev->x_plate_resistance; > - z /= z1; > + z /= z2; > z = (z + 2047) >> 12; > > if ((diffx < TSCADC_DELTA_X) && > @@ -399,8 +399,8 @@ static int titsc_parse_dt(struct ti_tscadc_dev > *tscadc_dev, > if (err < 0) > return err; > > - err = of_property_read_u32(node, "ti,steps-to-configure", > - &ts_dev->steps_to_configure); > + err = of_property_read_u32(node, "ti,coordiante-readouts", > + &ts_dev->coordiante_readouts); > if (err < 0) > return err; > > @@ -454,7 +454,8 @@ static int titsc_probe(struct platform_device *pdev) > goto err_free_irq; > } > titsc_step_config(ts_dev); > - titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure); > + titsc_writel(ts_dev, REG_FIFO0THR, > + ts_dev->coordiante_readouts * 2 + 2 - 1); > > input_dev->name = "ti-tsc"; > input_dev->dev.parent = &pdev->dev; > @@ -526,7 +527,7 @@ static int titsc_resume(struct device *dev) > } > titsc_step_config(ts_dev); > titsc_writel(ts_dev, REG_FIFO0THR, > - ts_dev->steps_to_configure); > + ts_dev->coordiante_readouts * 2 + 2 - 1); > return 0; > } > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h > b/include/linux/mfd/ti_am335x_tscadc.h > index c985262..4ec52b0 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -30,8 +30,8 @@ > #define REG_IDLECONFIG 0x058 > #define REG_CHARGECONFIG 0x05C > #define REG_CHARGEDELAY 0x060 > -#define REG_STEPCONFIG(n) (0x64 + ((n - 1) * 8)) > -#define REG_STEPDELAY(n) (0x68 + ((n - 1) * 8)) > +#define REG_STEPCONFIG(n) (0x64 + ((n) * 8)) > +#define REG_STEPDELAY(n) (0x68 + ((n) * 8)) > #define REG_FIFO0CNT 0xE4 > #define REG_FIFO0THR 0xE8 > #define REG_FIFO1CNT 0xF0 > -- > 1.7.10.4 > -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
