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