* [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
@ 2020-01-20 11:16 Lucas Stach
2020-01-27 2:24 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Lucas Stach @ 2020-01-20 11:16 UTC (permalink / raw)
To: Dmitry Torokhov, Andrew Duggan; +Cc: linux-input, kernel, patchwork-lst
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.
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-20 11:16 [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode Lucas Stach
@ 2020-01-27 2:24 ` Dmitry Torokhov
2020-01-27 19:21 ` Andrew Duggan
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-01-27 2:24 UTC (permalink / raw)
To: Lucas Stach, Andrew Duggan; +Cc: linux-input, kernel, patchwork-lst
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!
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-27 2:24 ` Dmitry Torokhov
@ 2020-01-27 19:21 ` Andrew Duggan
2020-01-28 7:02 ` Christopher Heiny
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Duggan @ 2020-01-27 19:21 UTC (permalink / raw)
To: Dmitry Torokhov, Lucas Stach, Christopher Heiny
Cc: linux-input, kernel, patchwork-lst
Hi Dmitry,
On 1/26/20 6:24 PM, Dmitry Torokhov wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> 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?
Andrew
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-27 19:21 ` Andrew Duggan
@ 2020-01-28 7:02 ` Christopher Heiny
2020-01-28 9:41 ` Lucas Stach
0 siblings, 1 reply; 11+ messages in thread
From: Christopher Heiny @ 2020-01-28 7:02 UTC (permalink / raw)
To: Andrew Duggan, Dmitry Torokhov, Lucas Stach
Cc: linux-input, kernel, patchwork-lst
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.
Cheers,
Chris
>
> Andrew
>
> > > 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-28 7:02 ` Christopher Heiny
@ 2020-01-28 9:41 ` Lucas Stach
2020-01-28 17:22 ` Christopher Heiny
0 siblings, 1 reply; 11+ messages in thread
From: Lucas Stach @ 2020-01-28 9:41 UTC (permalink / raw)
To: Christopher Heiny, Andrew Duggan, Dmitry Torokhov
Cc: linux-input, kernel, patchwork-lst
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?
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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-28 9:41 ` Lucas Stach
@ 2020-01-28 17:22 ` Christopher Heiny
2020-01-31 18:28 ` Andrew Duggan
0 siblings, 1 reply; 11+ messages in thread
From: Christopher Heiny @ 2020-01-28 17:22 UTC (permalink / raw)
To: Lucas Stach, Andrew Duggan, Dmitry Torokhov
Cc: linux-input, kernel, patchwork-lst
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
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-28 17:22 ` Christopher Heiny
@ 2020-01-31 18:28 ` Andrew Duggan
2020-02-01 1:38 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Duggan @ 2020-01-31 18:28 UTC (permalink / raw)
To: Christopher Heiny, Lucas Stach, Dmitry Torokhov
Cc: linux-input, kernel, patchwork-lst
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.
Andrew
>> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-01-31 18:28 ` Andrew Duggan
@ 2020-02-01 1:38 ` Dmitry Torokhov
2020-02-19 3:01 ` Paul Hollinsky
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-02-01 1:38 UTC (permalink / raw)
To: Andrew Duggan
Cc: Christopher Heiny, Lucas Stach, linux-input, kernel, patchwork-lst
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-02-01 1:38 ` Dmitry Torokhov
@ 2020-02-19 3:01 ` Paul Hollinsky
2020-02-21 2:36 ` Andrew Duggan
0 siblings, 1 reply; 11+ messages in thread
From: Paul Hollinsky @ 2020-02-19 3:01 UTC (permalink / raw)
To: Dmitry Torokhov, Andrew Duggan
Cc: Christopher Heiny, Lucas Stach, linux-input, kernel, patchwork-lst
On 1/31/2020 8:38 PM, Dmitry Torokhov wrote:
> 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.
>
Hi everyone,
I believe there may be an issue with the reduced reporting mode, at
least on my machine. I have a Lenovo ThinkPad X250 with the Synaptics
TM3075-002 trackpad.
With this patch, the trackpad becomes unusable. On a reboot, my control
register values are [38 00 19 19 00 10 90 06 ea 03 0f 01]. This
corresponds to a delta X/Y threshold of 25 and a palm rejection value of
0. The protocol documentation mentions that the palm rejection value
becomes active when in reduced reporting mode, hence its inclusion here.
Sometimes I could get the mouse to move very sporadically if I used my
entire hand on the trackpad. I instrumented the f11_attention IRQ
handler and found that it was not being called except for these sporadic
movements. I tried a few different combinations of these three values,
but they had no effect. This included setting all three values to 0,
which in theory will yield the same behavior as continuous reporting
mode, but it did not on my machine.
This leads me to suspect a hardware/firmware bug.
I believe the idea was that the platform/configuration could set
sensor->axis_align.delta_x(/y)_threshold to 0 in order to disable the
reduced reporting mode. This is the case on my laptop, but only a
nonzero value will override the value from the firmware, so the reduced
reporting mode still gets enabled.
All the best,
Paul
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-02-19 3:01 ` Paul Hollinsky
@ 2020-02-21 2:36 ` Andrew Duggan
2020-03-07 22:34 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Duggan @ 2020-02-21 2:36 UTC (permalink / raw)
To: Paul Hollinsky, Dmitry Torokhov
Cc: Christopher Heiny, Lucas Stach, linux-input, kernel, patchwork-lst
On 2/18/20 7:01 PM, Paul Hollinsky wrote:
>
>
> On 1/31/2020 8:38 PM, Dmitry Torokhov wrote:
>> 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.
>>
>
> Hi everyone,
>
> I believe there may be an issue with the reduced reporting mode, at
> least on my machine. I have a Lenovo ThinkPad X250 with the Synaptics
> TM3075-002 trackpad.
>
> With this patch, the trackpad becomes unusable. On a reboot, my control
> register values are [38 00 19 19 00 10 90 06 ea 03 0f 01]. This
> corresponds to a delta X/Y threshold of 25 and a palm rejection value of
> 0. The protocol documentation mentions that the palm rejection value
> becomes active when in reduced reporting mode, hence its inclusion here.
>
Hmm, it looks like the firmware was configured with non-zero Delta X/Y
Position thresholds. But, the firmware does not enable reduced reporting
mode by default so those thresholds have no effect. However, this patch
will now enable reduced reporting mode since it sees the non-zero
threshold which were read from the firmware. I did not consider the case
where the firmware would have thresholds set, but not enabled when I
reviewed this patch initially. Based on this new info I would suggest we
change the if statement to check sensor->axis_align.delta_x_threshold ||
sensor->axis_align.delta_y_threshold. Then we would only change the
reporting mode if the driver is explicitly setting the thresholds.
> Sometimes I could get the mouse to move very sporadically if I used my
> entire hand on the trackpad. I instrumented the f11_attention IRQ
> handler and found that it was not being called except for these sporadic
> movements. I tried a few different combinations of these three values,
> but they had no effect. This included setting all three values to 0,
> which in theory will yield the same behavior as continuous reporting
> mode, but it did not on my machine.
>
> This leads me to suspect a hardware/firmware bug.
>
> I believe the idea was that the platform/configuration could set
> sensor->axis_align.delta_x(/y)_threshold to 0 in order to disable the
> reduced reporting mode. This is the case on my laptop, but only a
> nonzero value will override the value from the firmware, so the reduced
> reporting mode still gets enabled.
>
The patch is meant to enable reduced reporting mode if the driver sets
the delta thresholds. Otherwise, the newly set thresholds will have no
effect. Generally, I don't think reduced reporting mode is enabled by
default. At least not on the various touchpads I looked at. I think the
issue you are seeing is that reduced reporting mode is being enabled
when it should not be.
Andrew
> All the best,
> Paul
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode
2020-02-21 2:36 ` Andrew Duggan
@ 2020-03-07 22:34 ` Dmitry Torokhov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-03-07 22:34 UTC (permalink / raw)
To: Andrew Duggan
Cc: Paul Hollinsky, Christopher Heiny, Lucas Stach, linux-input,
kernel, patchwork-lst
On Thu, Feb 20, 2020 at 06:36:59PM -0800, Andrew Duggan wrote:
>
> On 2/18/20 7:01 PM, Paul Hollinsky wrote:
> >
> > Hi everyone,
> >
> > I believe there may be an issue with the reduced reporting mode, at
> > least on my machine. I have a Lenovo ThinkPad X250 with the Synaptics
> > TM3075-002 trackpad.
> >
> > With this patch, the trackpad becomes unusable. On a reboot, my control
> > register values are [38 00 19 19 00 10 90 06 ea 03 0f 01]. This
> > corresponds to a delta X/Y threshold of 25 and a palm rejection value of
> > 0. The protocol documentation mentions that the palm rejection value
> > becomes active when in reduced reporting mode, hence its inclusion here.
> >
> Hmm, it looks like the firmware was configured with non-zero Delta X/Y
> Position thresholds. But, the firmware does not enable reduced reporting
> mode by default so those thresholds have no effect. However, this patch will
> now enable reduced reporting mode since it sees the non-zero threshold which
> were read from the firmware. I did not consider the case where the firmware
> would have thresholds set, but not enabled when I reviewed this patch
> initially. Based on this new info I would suggest we change the if statement
> to check sensor->axis_align.delta_x_threshold ||
> sensor->axis_align.delta_y_threshold. Then we would only change the
> reporting mode if the driver is explicitly setting the thresholds.
Any chance I could get a patch implementing this?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-03-07 22:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 11:16 [PATCH] Input: synaptics-rmi4 - switch to reduced reporting mode Lucas Stach
2020-01-27 2:24 ` Dmitry Torokhov
2020-01-27 19:21 ` Andrew Duggan
2020-01-28 7:02 ` Christopher Heiny
2020-01-28 9:41 ` Lucas Stach
2020-01-28 17:22 ` Christopher Heiny
2020-01-31 18:28 ` Andrew Duggan
2020-02-01 1:38 ` Dmitry Torokhov
2020-02-19 3:01 ` Paul Hollinsky
2020-02-21 2:36 ` Andrew Duggan
2020-03-07 22:34 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).