Date:   Wed, 24 Nov 2021 20:53:15 +0100
From:   =?iso-8859-1?Q?Jos=E9_Exp=F3sito?= <jose.exposito89@gmail.com>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Peter Hutterer <peter.hutterer@who-t.net>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads
Message-ID: <20211124195315.GA9441@elementary>
References: <20211123191238.12472-1-jose.exposito89@gmail.com>
 <CAO-hwJLB8h6fQRF8UjN3rER_6xS2Shi3ffEr92PhkVCijtYRpQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAO-hwJLB8h6fQRF8UjN3rER_6xS2Shi3ffEr92PhkVCijtYRpQ@mail.gmail.com>
Precedence: bulk

Hi Benjamin,

Thank you very much for your quick answer.

On Wed, Nov 24, 2021 at 10:39:02AM +0100, Benjamin Tissoires wrote:
> As long as udev intrinsic is happy with it (and it correctly tags the
> touchpad as ID_INPUT_something), I'm fine with it.

Yes, the device is still tagged correctly. For example, this is the original
output for "libinput record" (libinput issue 674):

  Supported Events:
  Event type 0 (EV_SYN)
    Event type 1 (EV_KEY)
    Event code 272 (BTN_LEFT)
    Event code 273 (BTN_RIGHT)
    Event code 325 (BTN_TOOL_FINGER)
  [...]
  udev:
    properties:
    - ID_INPUT=1
    - ID_INPUT_HEIGHT_MM=61
    - ID_INPUT_TOUCHPAD=1
    - ID_INPUT_WIDTH_MM=93

And the same output after applying the patch:

  Supported Events:
  Event type 0 (EV_SYN)
    Event type 1 (EV_KEY)
    Event code 272 (BTN_LEFT)
    Event code 325 (BTN_TOOL_FINGER)
  [...]
  udev:
    properties:
    - ID_INPUT=1
    - ID_INPUT_HEIGHT_MM=61
    - ID_INPUT_TOUCHPAD=1
    - ID_INPUT_WIDTH_MM=93

Notice that BTN_RIGHT is not present but the udev tags are the same.
I don't have access to that specific touchpad, but I own a Magic
Trackpad 1 and 2 -whose driver clears the BTN_RIGHT bit- and they
are properly tagged as well.

> I think it depends if you plan on fixing just hid-multitouch or the others.
> If you have more than one driver, then yes, adding a new symbol in
> hid-input.c makes sense. If not, then you are just exposing a new
> function we won't know if there are users and we won't be able to
> change without care.

I'd like to fix the issue on every driver. It is not a big amount of
duplicated code, just a couple of lines on drivers that don't already
clear the BTN_RIGHT/MIDDLE bit, but I agree with you, moving into a
common function is cleaner.

Also, the "input_set_property" function would allow us to add more
conditions associated with other properties in case we wanted to.

Thanks again for your input, I'll send the patchset for review as soon as
possible.

Jose