| Message ID | 1453777477-29706-2-git-send-email-stefan@agner.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 01/26/2016 04:04 AM, Stefan Agner wrote: > The measurements read from the controller which are temporary stored > in conversion_data, are interpreted wrong. The first measurement X+ > contains the Y position, and the second measurement Y+ the X position > (see also Table 11 Register Table in the data sheet). > > The problem is already known and a swap option has been introduced: > commit 6680884a4420 ("Input: ad7879 - add option to correct xy axis") > > However, with that the meaning of the new boolean is inverted since > the underlying values are already swapped. With this change, a true > in swap_xy actually swaps the two axis. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > Hi Michael, > > It seems that swap_xy is not used in any board which is in mainline, > hence swap_xy is always false. Therefore, up until now all boards > actually used swapped axis. However, I doubt that the blackfin boards > really have those axis swapped, it is probably more likely that the > userspace calibration took care of it. > > However, if they are really swapped, we should set the swap_xy flag > to 1 for those board... > > Do you happen to now what is the case with those boards? > Hi Stefan, I would be hesitant to invert the default behaviour of the driver. Too many people in the field already using it as it is. A XY swap can have multiple reasons. Lot's of small VGA/QVGA TFTs have the option to switch the scan direction from Landscape to Portrait. In addition you can also rotate and flip or mirror using VDMA options. So it really depends on the use case, how the touch panel is mounted to the screen or how it is wired. Regards, Michael > -- > Stefan > > drivers/input/touchscreen/ad7879.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c > index a73934b..e290e7b 100644 > --- a/drivers/input/touchscreen/ad7879.c > +++ b/drivers/input/touchscreen/ad7879.c > @@ -94,8 +94,8 @@ > #define AD7879_TEMP_BIT (1<<1) > > enum { > - AD7879_SEQ_XPOS = 0, > - AD7879_SEQ_YPOS = 1, > + AD7879_SEQ_YPOS = 0, > + AD7879_SEQ_XPOS = 1, > AD7879_SEQ_Z1 = 2, > AD7879_SEQ_Z2 = 3, > AD7879_NR_SENSE = 4, >
On 2016-01-25 23:58, Michael Hennerich wrote: > On 01/26/2016 04:04 AM, Stefan Agner wrote: >> The measurements read from the controller which are temporary stored >> in conversion_data, are interpreted wrong. The first measurement X+ >> contains the Y position, and the second measurement Y+ the X position >> (see also Table 11 Register Table in the data sheet). >> >> The problem is already known and a swap option has been introduced: >> commit 6680884a4420 ("Input: ad7879 - add option to correct xy axis") >> >> However, with that the meaning of the new boolean is inverted since >> the underlying values are already swapped. With this change, a true >> in swap_xy actually swaps the two axis. >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> Hi Michael, >> >> It seems that swap_xy is not used in any board which is in mainline, >> hence swap_xy is always false. Therefore, up until now all boards >> actually used swapped axis. However, I doubt that the blackfin boards >> really have those axis swapped, it is probably more likely that the >> userspace calibration took care of it. >> >> However, if they are really swapped, we should set the swap_xy flag >> to 1 for those board... >> >> Do you happen to now what is the case with those boards? >> > > > Hi Stefan, > > I would be hesitant to invert the default behaviour of the driver. > Too many people in the field already using it as it is. Afaik, we should be able to change in-kernel API's (especially if they are wrong) since we do not guarantee any API... > > A XY swap can have multiple reasons. > > Lot's of small VGA/QVGA TFTs have the option to switch the scan > direction from Landscape to Portrait. In addition you can also rotate > and flip or mirror using VDMA options. So it really depends on the use > case, how the touch panel is mounted to the screen or how it is wired. Ok, I see the reason for that functionality. I am mainly concerned about the new DT bindings. The touchscreen binding documents specify touchscreen-swapped-x-y, see: https://www.kernel.org/doc/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt I would like to make sure that this property is really swapping axis (and not necessary if the hardware is implemented according to the datasheet...) We could also implement a workaround to keep the platform data behavior as is (invert the swap_xy flag)... -- Stefan >> -- >> Stefan >> >> drivers/input/touchscreen/ad7879.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c >> index a73934b..e290e7b 100644 >> --- a/drivers/input/touchscreen/ad7879.c >> +++ b/drivers/input/touchscreen/ad7879.c >> @@ -94,8 +94,8 @@ >> #define AD7879_TEMP_BIT (1<<1) >> >> enum { >> - AD7879_SEQ_XPOS = 0, >> - AD7879_SEQ_YPOS = 1, >> + AD7879_SEQ_YPOS = 0, >> + AD7879_SEQ_XPOS = 1, >> AD7879_SEQ_Z1 = 2, >> AD7879_SEQ_Z2 = 3, >> AD7879_NR_SENSE = 4, >> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 26, 2016 at 09:04:20AM -0800, Stefan Agner wrote: > On 2016-01-25 23:58, Michael Hennerich wrote: > > On 01/26/2016 04:04 AM, Stefan Agner wrote: > >> The measurements read from the controller which are temporary stored > >> in conversion_data, are interpreted wrong. The first measurement X+ > >> contains the Y position, and the second measurement Y+ the X position > >> (see also Table 11 Register Table in the data sheet). > >> > >> The problem is already known and a swap option has been introduced: > >> commit 6680884a4420 ("Input: ad7879 - add option to correct xy axis") > >> > >> However, with that the meaning of the new boolean is inverted since > >> the underlying values are already swapped. With this change, a true > >> in swap_xy actually swaps the two axis. > >> > >> Signed-off-by: Stefan Agner <stefan@agner.ch> > >> --- > >> Hi Michael, > >> > >> It seems that swap_xy is not used in any board which is in mainline, > >> hence swap_xy is always false. Therefore, up until now all boards > >> actually used swapped axis. However, I doubt that the blackfin boards > >> really have those axis swapped, it is probably more likely that the > >> userspace calibration took care of it. > >> > >> However, if they are really swapped, we should set the swap_xy flag > >> to 1 for those board... > >> > >> Do you happen to now what is the case with those boards? > >> > > > > > > Hi Stefan, > > > > I would be hesitant to invert the default behaviour of the driver. > > Too many people in the field already using it as it is. > > Afaik, we should be able to change in-kernel API's (especially if they > are wrong) since we do not guarantee any API... > > > > > A XY swap can have multiple reasons. > > > > Lot's of small VGA/QVGA TFTs have the option to switch the scan > > direction from Landscape to Portrait. In addition you can also rotate > > and flip or mirror using VDMA options. So it really depends on the use > > case, how the touch panel is mounted to the screen or how it is wired. > > Ok, I see the reason for that functionality. > > I am mainly concerned about the new DT bindings. The touchscreen binding > documents specify touchscreen-swapped-x-y, see: > https://www.kernel.org/doc/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt > > I would like to make sure that this property is really swapping axis > (and not necessary if the hardware is implemented according to the > datasheet...) > > We could also implement a workaround to keep the platform data behavior > as is (invert the swap_xy flag)... That is probably the best option. Thanks.
diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c index a73934b..e290e7b 100644 --- a/drivers/input/touchscreen/ad7879.c +++ b/drivers/input/touchscreen/ad7879.c @@ -94,8 +94,8 @@ #define AD7879_TEMP_BIT (1<<1) enum { - AD7879_SEQ_XPOS = 0, - AD7879_SEQ_YPOS = 1, + AD7879_SEQ_YPOS = 0, + AD7879_SEQ_XPOS = 1, AD7879_SEQ_Z1 = 2, AD7879_SEQ_Z2 = 3, AD7879_NR_SENSE = 4,
The measurements read from the controller which are temporary stored in conversion_data, are interpreted wrong. The first measurement X+ contains the Y position, and the second measurement Y+ the X position (see also Table 11 Register Table in the data sheet). The problem is already known and a swap option has been introduced: commit 6680884a4420 ("Input: ad7879 - add option to correct xy axis") However, with that the meaning of the new boolean is inverted since the underlying values are already swapped. With this change, a true in swap_xy actually swaps the two axis. Signed-off-by: Stefan Agner <stefan@agner.ch> --- Hi Michael, It seems that swap_xy is not used in any board which is in mainline, hence swap_xy is always false. Therefore, up until now all boards actually used swapped axis. However, I doubt that the blackfin boards really have those axis swapped, it is probably more likely that the userspace calibration took care of it. However, if they are really swapped, we should set the swap_xy flag to 1 for those board... Do you happen to now what is the case with those boards? -- Stefan drivers/input/touchscreen/ad7879.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)