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@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. Applied, thank you everyone. -- Dmitry
