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

[Andrew Duggan <aduggan@synaptics.com>]

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
>