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?
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
>
>
