Re: [PATCH resend v2 1/2] Input: touchscreen DT binding - add touchscreen-min-x and -min-y properties

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Thu, Oct 11, 2018 at 11:09:49AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 11-10-18 02:52, Dmitry Torokhov wrote:
> > Hi Hans,
> > 
> > Sorry, now I was being slow as well.
> 
> No problem.
> 
> > On Thu, Sep 20, 2018 at 07:31:43PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > I completely missed this mail earlier, sorry.
> > > 
> > > Thank you Benjamin for pointing this out to me.
> > > 
> > > On 03-08-18 02:31, Dmitry Torokhov wrote:
> > > > Hi Hans,
> > > > 
> > > > On Tue, Jul 31, 2018 at 01:19:57PM +0200, Hans de Goede wrote:
> > > > > Some touchscreens, depending on the firmware and/or the digitizer report
> > > > > coordinates which never reach 0 along one or both of their axis.
> > > > > 
> > > > > This has been seen for example on the Silead touchscreens on a Onda V891w
> > > > > and a Point of View mobii TAB-P800w(v2.0).
> > > > > 
> > > > > This commits documents 2 new touchscreen properties for communicating
> > > > > the minimum reported values to the OS: touchscreen-min-x and -min-y.
> > > > > 
> > > > > This commit also drop the (in pixels) comment from the documentation
> > > > > of the touchscreen-size-x and touchscreen-size-y properties. This comment
> > > > > suggests that there is a relation between the range of reported
> > > > > coordinates and the display resolution, which is only true for some
> > > > > devices. The (in pixels) comment is replaced with "(maximum x coordinate
> > > > > reported + 1)" to mirror the language describing the new touchscreen-min-x
> > > > > and -min-y properties.
> > > > 
> > > > I am concerned that people will not read the documentation carefully and
> > > > will treat it as true size, since it is what in the name. Maybe we
> > > > should say that it is size of usable area, in device units, and that
> > > > maximum reported coordinate is "touchscreen-min-x + touchscreen-size-x -
> > > > 1"?
> > > 
> > > Not sure what you mean with "true size" but in the implementation
> > > from this series, the maximum coordinated reported is (touchscreen-size-x - 1)
> > > not (touchscreen-min-x + touchscreen-size-x - 1) as you suggest.
> > > 
> > > Basically what this series does is set:
> > > 
> > > input_absinfo.minimum to the new touchscreen-min-x value (or 0 if not specified)
> > > input_absinfo.maximum to touchscreen-size-x - 1 as we've always done.
> > > 
> > > So the usable range / the range mapping from one screen edge to the other is:
> > > 
> > > touchscreen-min-x - (touchscreen-size-x - 1)
> > > 
> > > Which matches with the dt bindings doc after this patch, which
> > > reads after this patch:
> > > 
> > >   - touchscreen-min-x		: minimum x coordinate reported (0 if not set)
> > >   - touchscreen-min-y		: minimum y coordinate reported (0 if not set)
> > >   - touchscreen-size-x		: horizontal resolution of touchscreen
> > > 				  (maximum x coordinate reported + 1)
> > >   - touchscreen-size-y		: vertical resolution of touchscreen
> > > 				  (maximum y coordinate reported + 1)
> > > 
> > > I hope this clarifies things and if you want to change anything let
> > > me know.
> > 
> > Right, except that my concern is that people do not read documentation,
> > and therefore will not realize that touchscreen-size-x is not the "true"
> > what I called it, or what you call usable range, but rather maximum
> > coordinate (-1).  IOW I am concerned that if we have a device with
> > 640x480 screen for example, and touch controller reporting coordinates
> > with offset of 20, someone will specify:
> > 
> > touchscreen-min-x = 20
> > touchscreen-size-x = 640 (because that's their screen size)
> > 
> > and will not notice for some reason and later quirk it in their
> > software.
> 
> Ah I see.
> 
> > So I was asking if we should accommodate this, and actually set up max
> > on axis as "touchscreen-min-x + touchscreen-size-x - 1". It will still
> > be compatible with current bindings (having effectively min of 0), but
> > to me better reflects the name of the parameter - size of the screen.
> > 
> > Please let me know if this makes any sense to you.
> 
> I understand what you want now and why you want it.
> 
> But I'm not sure I agree with you. Some pre-cursor to this patch series
> actually had something like touchscreen-offset-x (or some-such I don't
> remember) which actually subtracted the specified value from the coordinates
> reported to userspace (clamping to 0).
> 
> In that setup I think setting:
> 
> touchscreen-size-x = (maximum x coordinate reported + 1) -
>                      (minimum x coordinate reported.
> 
> Makes sense, but since now we are not doing that and just copying the
> values over to input_absinfo.minimum/maximum I think a 1:1 mapping
> (with the - 1 adjustment for size) makes more sense.
> 
> The way I'm currently using this is with touchscreens where we cannot
> read this info from the hardware, so I repeatedly move my finger over
> each edge noting down the min / max value for e.g. the left/right
> edge and then directly putting these into the properties.
> 
> IMHO not having to do some math here to calculate the right value
> for touchscreen-size-x shows that treating touchscreen-size-x as
> (touchscreen-max-x + 1) is the right thing to do.
> 
> I'm actually worried that if we follow your suggestion people will
> indeed not read the docs and thus not do the math. I think they will
> just copy over the min / max readings and we and up with an
> input_absinfo.maximum value which is input_absinfo.minimum
> units too big.

I see. OK, let's keep it your way. Applied.

Thanks.

-- 
Dmitry