| Message ID | 20161020195917.20051-5-fcooper@ti.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote: > On systems with a fixed display/touchscreen orientation it is important to > pass in the "correct" x and y coordinates based on the orientation. > Currently, to support landscape and portrait touchscreen-swapped-x-y > simply does the following: > > Assuming touchscreen is as follows: > X: 1280 Y:800 programmed in touchscreen controller and also interchange > bit cleared. Assuming ts mounted in portrait mode. > > 1280 (X) > ------ > | | > | | 800 (Y) > | | > | | > ------ > > 800 (Y) > ------ > | | > | | 1280 (X) > | | > | | > ------ > > However, the above isn't really what we want especially in distros that > assumes a fixed orientation. In this case what we really want is to > interchange the x and y coordinates so the Y coordinate can return a max > value of 1280 and X can return a max value of 800. > > 800 (X) > ------ > | | > | | 1280 (Y) > | | > | | > ------ > > Since the driver is limited to the value reported by the touchscreen > controller this issue can't be fixed purely in the driver. Therefore, > add a new DT property that supports interchanging X and Y coordinates > internally within the hardware. I'm not sure I follow why existing properties don't cover this. > > Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> > --- > .../devicetree/bindings/input/touchscreen/goodix.txt | 2 ++ > drivers/input/touchscreen/goodix.c | 13 +++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt > index ebc7cb7..b8be2ab 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt > @@ -25,6 +25,8 @@ Optional properties: > - touchscreen-inverted-y : Y axis is inverted (boolean) > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean) > (swapping is done after inverting the axis) > + - touchscreen-inter-x-y : X and Y maximum values programmed in the device > + are interchanged internally in hardware. (boolean) Minimally this should be vendor specific and have a vendor prefix I think. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote: > On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote: > > <snip> > I'm not sure I follow why existing properties don't cover this. Me neither. I certainly don't understand why the driver can't mangle the data from the touchscreen all it wants. It's not like user-space is talking to the touchscreen directly. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/27/2016 05:34 AM, Bastien Nocera wrote: > On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote: >> On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote: >>> > <snip> >> I'm not sure I follow why existing properties don't cover this. > > Me neither. I certainly don't understand why the driver can't mangle > the data from the touchscreen all it wants. It's not like user-space is > talking to the touchscreen directly. > Sorry the above could of been clearer. Lets ignore talking about X and Y axis for a little bit since that really depends on the default touchscreen config values and the way it is mounted on the touchscreen. Now lets say when your interacting with the touchscreen the touchscreen controller outputs a max value of 1280 when moving your finger horizontally and 800 when moving your finger vertically. <-1280-> ------ | | ^ | | | | | 800 | | | ------ V So no matter what your horizontal range is 0-1280 and your vertical range is 0-800. Now based on the above diagram you can see that usually you want the longer side to have a higher resolution. So you may want a vertical range of 0-1280 and a horizontal range from 0-800 instead. So lets add labels to the original diagram and assume that the x and y axis from the driver/user-space perspective is as follows. <-1280-> (X) ------ | | ^ | | | | | 800 (Y) | | | ------ V The only thing the driver (software) has the ability to do is change the "orientation". <-1280-> (Y) ------ | | ^ | | | | | 800 (X) | | | ------ V However, this doesn't change the resolution ie range of values in the horizontal and vertical direction the touch screen controller will report. Only the hardware can determine the resolution it will use. The interchange bit I set essentially swaps the range that the controller is currently programmed to use which in my first diagram the horizontal range was 0-1280 and my vertical range is 0-800. So by setting this interchange bit in hardware the horizontal range will now be 0-800 while the vertical range will be 0-1280 which is what we want. Does this clarify things? I messed up the second diagram in my commit message which is probably what caused the confusion. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/26/2016 06:18 PM, Rob Herring wrote: > On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote: >> On systems with a fixed display/touchscreen orientation it is important to >> pass in the "correct" x and y coordinates based on the orientation. >> Currently, to support landscape and portrait touchscreen-swapped-x-y >> simply does the following: >> >> Assuming touchscreen is as follows: >> X: 1280 Y:800 programmed in touchscreen controller and also interchange >> bit cleared. Assuming ts mounted in portrait mode. >> >> 1280 (X) >> ------ >> | | >> | | 800 (Y) >> | | >> | | >> ------ >> >> 800 (Y) >> ------ >> | | >> | | 1280 (X) >> | | >> | | >> ------ >> >> However, the above isn't really what we want especially in distros that >> assumes a fixed orientation. In this case what we really want is to >> interchange the x and y coordinates so the Y coordinate can return a max >> value of 1280 and X can return a max value of 800. >> >> 800 (X) >> ------ >> | | >> | | 1280 (Y) >> | | >> | | >> ------ >> >> Since the driver is limited to the value reported by the touchscreen >> controller this issue can't be fixed purely in the driver. Therefore, >> add a new DT property that supports interchanging X and Y coordinates >> internally within the hardware. > > I'm not sure I follow why existing properties don't cover this. > >> >> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> >> --- >> .../devicetree/bindings/input/touchscreen/goodix.txt | 2 ++ >> drivers/input/touchscreen/goodix.c | 13 +++++++++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> index ebc7cb7..b8be2ab 100644 >> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt >> @@ -25,6 +25,8 @@ Optional properties: >> - touchscreen-inverted-y : Y axis is inverted (boolean) >> - touchscreen-swapped-x-y : X and Y axis are swapped (boolean) >> (swapping is done after inverting the axis) >> + - touchscreen-inter-x-y : X and Y maximum values programmed in the device >> + are interchanged internally in hardware. (boolean) > > Minimally this should be vendor specific and have a vendor prefix I > think. Would "goodix,inter-x-y" work? > > Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-10-27 at 12:42 -0500, Franklin S Cooper Jr wrote: > > On 10/27/2016 05:34 AM, Bastien Nocera wrote: > > On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote: > > > On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr > > > wrote: > > > > > > > > <snip> > > > I'm not sure I follow why existing properties don't cover this. > > > > Me neither. I certainly don't understand why the driver can't > > mangle > > the data from the touchscreen all it wants. It's not like user- > > space is > > talking to the touchscreen directly. > > > > Sorry the above could of been clearer. > > Lets ignore talking about X and Y axis for a little bit since that > really depends on the default touchscreen config values and the way > it > is mounted on the touchscreen. Now lets say when your interacting > with > the touchscreen the touchscreen controller outputs a max value of > 1280 > when moving your finger horizontally and 800 when moving your finger > vertically. > > <-1280-> > ------ > > | ^ > > | | > > | 800 > > | | > > ------ V > > So no matter what your horizontal range is 0-1280 and your vertical > range is 0-800. Now based on the above diagram you can see that > usually > you want the longer side to have a higher resolution. So you may want > a > vertical range of 0-1280 and a horizontal range from 0-800 instead. > > So lets add labels to the original diagram and assume that the x and > y > axis from the driver/user-space perspective is as follows. > <-1280-> (X) > ------ > > | ^ > > | | > > | 800 (Y) > > | | > > ------ V > > The only thing the driver (software) has the ability to do is change > the > "orientation". > > <-1280-> (Y) > ------ > > | ^ > > | | > > | 800 (X) > > | | > > ------ V > > However, this doesn't change the resolution ie range of values in the > horizontal and vertical direction the touch screen controller will > report. Only the hardware can determine the resolution it will use. > The > interchange bit I set essentially swaps the range that the controller > is > currently programmed to use which in my first diagram the horizontal > range was 0-1280 and my vertical range is 0-800. So by setting this > interchange bit in hardware the horizontal range will now be 0-800 > while > the vertical range will be 0-1280 which is what we want. > > Does this clarify things? I messed up the second diagram in my commit > message which is probably what caused the confusion. Looks to me that this should be fixed in the firmware configuration, which is what Irina's patches allow doing. If the goal of this series is implementing this, I wouldn't take any of those patches until we figure out why the firmware config in those devices isn't set properly. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt index ebc7cb7..b8be2ab 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt @@ -25,6 +25,8 @@ Optional properties: - touchscreen-inverted-y : Y axis is inverted (boolean) - touchscreen-swapped-x-y : X and Y axis are swapped (boolean) (swapping is done after inverting the axis) + - touchscreen-inter-x-y : X and Y maximum values programmed in the device + are interchanged internally in hardware. (boolean) Example: diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index c2428e1..3f93375 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -39,6 +39,7 @@ struct goodix_ts_data { bool swapped_x_y; bool inverted_x; bool inverted_y; + bool interchange_x_y; unsigned int max_touch_num; unsigned int int_trigger_type; int cfg_len; @@ -76,6 +77,9 @@ struct goodix_ts_data { #define MAX_CONTACTS_LOC 5 #define TRIGGER_LOC 6 +/* Register Bits */ +#define XY_COORD_INTER 3 + static const unsigned long goodix_irq_flags[] = { IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, @@ -500,6 +504,10 @@ static void goodix_tweak_config(struct goodix_ts_data *ts) put_unaligned_le16(ts->abs_x_max, &config[RESOLUTION_LOC]); put_unaligned_le16(ts->abs_y_max, &config[RESOLUTION_LOC + 2]); + if (ts->interchange_x_y) + config[TRIGGER_LOC] = config[TRIGGER_LOC] | + (1 << XY_COORD_INTER); + check_sum = goodix_calculate_checksum(ts->cfg_len, config); config[raw_cfg_len] = check_sum; @@ -700,6 +708,11 @@ static int goodix_configure_dev(struct goodix_ts_data *ts) } } + ts->interchange_x_y = device_property_read_bool(&ts->client->dev, + "touchscreen-inter-x-y"); + if (ts->interchange_x_y) + alter_config = true; + if (alter_config) goodix_tweak_config(ts);
On systems with a fixed display/touchscreen orientation it is important to pass in the "correct" x and y coordinates based on the orientation. Currently, to support landscape and portrait touchscreen-swapped-x-y simply does the following: Assuming touchscreen is as follows: X: 1280 Y:800 programmed in touchscreen controller and also interchange bit cleared. Assuming ts mounted in portrait mode. 1280 (X) ------ | | | | 800 (Y) | | | | ------ 800 (Y) ------ | | | | 1280 (X) | | | | ------ However, the above isn't really what we want especially in distros that assumes a fixed orientation. In this case what we really want is to interchange the x and y coordinates so the Y coordinate can return a max value of 1280 and X can return a max value of 800. 800 (X) ------ | | | | 1280 (Y) | | | | ------ Since the driver is limited to the value reported by the touchscreen controller this issue can't be fixed purely in the driver. Therefore, add a new DT property that supports interchanging X and Y coordinates internally within the hardware. Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> --- .../devicetree/bindings/input/touchscreen/goodix.txt | 2 ++ drivers/input/touchscreen/goodix.c | 13 +++++++++++++ 2 files changed, 15 insertions(+)