Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode

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

On Fri, Jan 31, 2020 at 10:28:23AM -0800, Andrew Duggan wrote:
> 
> On 1/28/20 9:22 AM, Christopher Heiny wrote:
> > On Tue, 2020-01-28 at 10:41 +0100, Lucas Stach wrote:
> > > CAUTION: Email originated externally, do not click links or open
> > > attachments unless you recognize the sender and know the content is
> > > safe.
> > > 
> > > 
> > > Hi Christopher,
> > > 
> > > On Di, 2020-01-28 at 07:02 +0000, Christopher Heiny wrote:
> > > > On Mon, 2020-01-27 at 11:21 -0800, Andrew Duggan wrote:
> > > > > Hi Dmitry,
> > > > > 
> > > > > On 1/26/20 6:24 PM, Dmitry Torokhov wrote:
> > > > > > On Mon, Jan 20, 2020 at 12:16:28PM +0100, Lucas Stach wrote:
> > > > > > > When the distance thresholds are set the controller must be
> > > > > > > in
> > > > > > > reduced
> > > > > > > reporting mode for them to have any effect on the interrupt
> > > > > > > generation.
> > > > > > > This has a potentially large impact on the number of events
> > > > > > > the
> > > > > > > host
> > > > > > > needs to process.
> > > > > > > 
> > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > > > ---
> > > > > > > I'm not sure if we want a separate DT property to allow the
> > > > > > > use
> > > > > > > of
> > > > > > > reduced reporting mode, as this change might lead to problems
> > > > > > > for
> > > > > > > controllers with unreasonably large threshold values. I'm not
> > > > > > > sure if
> > > > > > > any controller with bogus threshold values exist in the wild.
> > > > > > > ---
> > > > > > Andrew, any feedback on this patch?
> > > > > > 
> > > > > > Thanks!
> > > > > The RMI4 spec does say that delta X/Y thresholds are only used in
> > > > > reduced reporting mode, so this patch is needed for the threshold
> > > > > values
> > > > > to have an effect.
> > > > > 
> > > > > Reviewed-by: Andrew Duggan <aduggan@synaptics.com>
> > > > > 
> > > > > Because reduced reporting mode is so dependent on these
> > > > > thresholds,
> > > > > my
> > > > > opinion is that it is better not to add a separate DT property,
> > > > > but
> > > > > to
> > > > > instead control reduced reporting mode through the setting of
> > > > > these
> > > > > thresholds. My guess is that the if reduced reporting is not
> > > > > enabled
> > > > > in
> > > > > the firmware by default, then the thresholds may not be valid.
> > > > > Setting
> > > > > the thresholds to 0 will essentially disable reduced reporting
> > > > > mode.
> > > > > So
> > > > > that would be how a user could disable reduced reporting mode
> > > > > without
> > > > > a
> > > > > separate DT property. Chris, do you have an opinion on this?
> > > > > Anything
> > > > > I
> > > > > overlooked?
> > > > Hi Dmitry, Andrew, Lucas,
> > > > 
> > > > I'm in agreement with Andrew on this.  Having two ways to
> > > > enable/disable reduced reporting (that is, both the DT property and
> > > > the
> > > > thresholds) could lead to confusion and unexpected
> > > > behavior.  Simpler
> > > > is better, in my opinion.
> > > > 
> > > > However, in that case I'm a little concerned about the logic in the
> > > > patch below.  When either of the thresholds are set to non-zero, we
> > > > clear the report mask and then enable the reduced reporting bit.
> > > > Subsequently setting both thresholds to zero will disable reduced
> > > > reporting, but will not enable another report mode.  Unless there
> > > > is
> > > > code elsewhere to catch this case (and if there is, it seems like a
> > > > bad
> > > > idea to be handling this in two different places), it could result
> > > > in
> > > > the touchpad being disabled.
> > > > 
> > > > As a hypothetical instance of this, imagine a user using the
> > > > touchpad
> > > > to manipulate report threshold sliders in a GUI.  Setting both
> > > > sliders
> > > > to zero would disable the touch sensor reporting, potentially
> > > > leaving
> > > > the user with a dead touch sensor and no easy way to recover from
> > > > that.
> > > I can see how this would be a problem, but then I see no interface in
> > > the driver to actually change the threshold values on the fly. They
> > > are
> > > either set by firmware or specified via DT properties. So I don't see
> > > how the threshold values would change on an active device. Anything
> > > i'm
> > > overlooking?
> > Hi Lucas,
> > 
> > You're not overlooking anything.  Mainly it's me being a worry-wart,
> > and assuming that if something can be adjusted, someone will eventually
> > come along and add a sysfs interface to adjust it, and then someone
> > else will create a userspace tool to adjust it, and things will break.
> > 
> > If you guys are OK with Andrew's original evaluation, then you can
> > ignore my worry (which is, as admitted, entirely a hypothetical).
> > 
> > 					Cheers,
> > 						Chris
> 
> Currently, the driver only sets the thresholds in rmi_f11_initialize(). If
> someone were to decide to add another method for setting the thresholds they
> would probably remove the logic from rmi_f11_initialize() and put it in a
> new function so that the code can be called from multiple places. In that
> case, they should also include the code in this patch in the new function. I
> think the comment above the new code makes it clear that setting the
> reporting mode to reduced reporting is needed for the threshold values to be
> used by the firmware. I don't see a way to handle potential future changes
> without adding what may be unnecessary complexity. I reaffirm my review sign
> off.

Applied, thank you everyone.

-- 
Dmitry