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@xxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxxxx>
> > >
> > > 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
>
> Regards,
> Lucas
>
> > > > > drivers/input/rmi4/rmi_f11.c | 14 ++++++++++++++
> > > > > 1 file changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/drivers/input/rmi4/rmi_f11.c
> > > > > b/drivers/input/rmi4/rmi_f11.c
> > > > > index bbf9ae9f3f0c..6adea8a3e8fb 100644
> > > > > --- a/drivers/input/rmi4/rmi_f11.c
> > > > > +++ b/drivers/input/rmi4/rmi_f11.c
> > > > > @@ -412,6 +412,10 @@ struct f11_2d_sensor_queries {
> > > > >
> > > > > /* Defs for Ctrl0. */
> > > > > #define RMI_F11_REPORT_MODE_MASK 0x07
> > > > > +#define RMI_F11_REPORT_MODE_CONTINUOUS (0 << 0)
> > > > > +#define RMI_F11_REPORT_MODE_REDUCED (1 << 0)
> > > > > +#define RMI_F11_REPORT_MODE_FS_CHANGE (2 << 0)
> > > > > +#define RMI_F11_REPORT_MODE_FP_CHANGE (3 << 0)
> > > > > #define RMI_F11_ABS_POS_FILT (1 << 3)
> > > > > #define RMI_F11_REL_POS_FILT (1 << 4)
> > > > > #define RMI_F11_REL_BALLISTICS (1 << 5)
> > > > > @@ -1195,6 +1199,16 @@ static int rmi_f11_initialize(struct
> > > > > rmi_function *fn)
> > > > > ctrl->ctrl0_11[RMI_F11_DELTA_Y_THRESHOLD] =
> > > > > sensor->axis_align.delta_y_threshold;
> > > > >
> > > > > + /*
> > > > > + * If distance threshold values are set, switch to
> > > > > reduced
> > > > > reporting
> > > > > + * mode so they actually get used by the controller.
> > > > > + */
> > > > > + if (ctrl->ctrl0_11[RMI_F11_DELTA_X_THRESHOLD] ||
> > > > > + ctrl->ctrl0_11[RMI_F11_DELTA_Y_THRESHOLD]) {
> > > > > + ctrl->ctrl0_11[0] &= ~RMI_F11_REPORT_MODE_MASK;
> > > > > + ctrl->ctrl0_11[0] |=
> > > > > RMI_F11_REPORT_MODE_REDUCED;
> > > > > + }
> > > > > +
> > > > > if (f11->sens_query.has_dribble) {
> > > > > switch (sensor->dribble) {
> > > > > case RMI_REG_STATE_OFF:
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > --
> > > > Dmitry
