Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Tobita, Tatsunosuke" <tatsunosuke.tobita@wacom.com>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Ping Cheng <pinglinux@gmail.com>
CC:     Alistair Francis <alistair@alistair23.me>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "linux-imx@nxp.com" <linux-imx@nxp.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input <linux-input@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "alistair23@gmail.com" <alistair23@gmail.com>
Subject: RE: [PATCH v11 1/4] HID: wacom_sys: Add support for flipping the data
 values
Thread-Topic: [PATCH v11 1/4] HID: wacom_sys: Add support for flipping the
 data values
Thread-Index: AQHXvQLkRs45KqGVP02BAT9/lvnGjavZnY8FgAFeRDA=
Date:   Tue, 19 Oct 2021 22:57:36 +0000
Message-ID: <AM8PR07MB739659D037054A3B1EB2647087BD9@AM8PR07MB7396.eurprd07.prod.outlook.com>
References: <20211009114313.17967-1-alistair@alistair23.me>
 <CAF8JNh+OUzvAHA9tBrH2d_WxWPXRgiunhGO5KV4-fqVG+tUOyQ@mail.gmail.com>
 <YW4kgnI0DQHj4sw4@google.com>
In-Reply-To: <YW4kgnI0DQHj4sw4@google.com>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?PTxBnpWtz//eYVtzaLMaDHbwjUS1JTnfBwyGM2nlWFuCDN6TWkPtdBxABqQJ?=
 =?us-ascii?Q?JrorHJX16/QWe8mzF0Py0/jdrd2zpNfwHxLpSjioaEoVwHjRq2lpP6wCdQZX?=
 =?us-ascii?Q?tg1yCYwpsO12xHm+Dn7Tcx+MQOf9PT4q7WoRxaM5woB94uIqBO3tM54epmch?=
 =?us-ascii?Q?CO+/ASFU8LjRdBk7n0HL2ANNTZwb6l9Jlzh8FQFjhMxZzDtuqWoW8TJiYa9X?=
 =?us-ascii?Q?GutK6vJMVdF9rya0jFuBF5T6r14UOA/qTgLgyzOWk/tjBrNEOBqyLxkuG+EO?=
 =?us-ascii?Q?r2MB2ERkU3Iw/CoVvIlQCt+JUfoNSD3FOqUCddk9Lc/g5WvFKguYxC6p1oBR?=
 =?us-ascii?Q?K+ZNbp6p3XjMy1ub+6KO5wMP1iy3JYpshlpDdfue0m1uzTD7zkqsLkvjlhcn?=
 =?us-ascii?Q?udiLgp5XI+KOKa29bBJSZ+24/ow0R7qlvQaDCIlfU8AF+DzA3xL7/EgKWttU?=
 =?us-ascii?Q?wXatJJKCdvEcmRoHXdpGzxfd9q9afVv6pq+8e30fMxVcm2J++zczq21FobH9?=
 =?us-ascii?Q?KQo/t/FsOC8A79qYK7kxjbp2N7sN99AG8dhQtl3dl+k8to+TWgVqV4fr4LNL?=
 =?us-ascii?Q?iASiO/vju0UMNqNOFR50/hmgrMPHracfJQ3diuGhBml5fhsepL9/6o9NwzPf?=
 =?us-ascii?Q?ERNHu87EeVoD0eUYKlLFo2WkYomLGlReCma1hW89jhIa3Qv3nuK59KHlpxLi?=
 =?us-ascii?Q?YFuJ5SDaXbW7DlPNHdrdiC0Vm8GcjXdYdv4tRAMAHFSe9mqiVRhXOGWRTsH9?=
 =?us-ascii?Q?GE493p0zeYp2jQvbHFv4gp3+Y0u4mVz56dI1H/fjz1INZADMvuMC3RatF2Wr?=
 =?us-ascii?Q?yv5U7ph2s6v61frR7GAJrG5cCFRnQ+/cVzCfucvnZ086EQFQrSgc4jUFKebs?=
 =?us-ascii?Q?ur0/rgOzzv8TkXs2ZPq7DSzHWN6dj2sZQ/TyKcYh+fcsLRdcIjsiNuNMj00A?=
 =?us-ascii?Q?Tdm0Wrgkpvb3eOmmd9tMb8cfrckEZbGtsyCR1LAp7QJ5NjWazJgeEDuQDm/B?=
 =?us-ascii?Q?6DZV/z5YtdAcDvisKTzWJy7tFmgoy8fFvifZVMZJC/AI/9uC+ww6vEZjimFv?=
 =?us-ascii?Q?58aSN4WiTJKFeoiK6eJxOsHYJS3wguBFUoa4+QFWUEUSfLmmBnV5okSjSsK/?=
 =?us-ascii?Q?IaLnYPi3F6/J7FK4zAD+YtjnZ3SN29JZ4jn247FzqMFJC/8JZ8Dnvqrw4+7C?=
 =?us-ascii?Q?bf7QFU43UCmwyXks5WNSJ3vY9+mvV0aLCpUCVRENnR4jgcF6+Tc5PvRUgAm+?=
 =?us-ascii?Q?4dE7Jk+O4UTKNKrLQj9ZNgLKN70y6MivjM6HvzX5jxaTjHF6fJHucB/1Wr7N?=
 =?us-ascii?Q?lrhnLMiBjwBBUm9U24CkYUrvSRLTdV2tOL/vVR6TxHBei4u+GO98+MtvRSlj?=
 =?us-ascii?Q?Dkk/QQgRUHABaEnkJ6ehQHkiYFnoMu97sAsd7msOY2OgxjVto2i/Afb7oBRk?=
 =?us-ascii?Q?1zGoHkdTGl0x/OYDCf7KXHgxiu3CC2w5?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wacom.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM8PR07MB7396.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c76167c2-a57c-412e-1576-08d99353d7cd
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Oct 2021 22:57:36.0671
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 9655a91b-107e-4537-834e-d15e84872626
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: tatsunosuke.tobita@wacom.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR07MB7057
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dmitry,

>I am really confused about this distinction. Could you please elaborate wh=
y wacom driver only supports 0x056a (and, curiously, some Lenovo) devices.
We want 0x2d1F to work with ***ONLY*** with the generic HID driver because =
the every recent firmware is designed for that; no help is needed from othe=
r sides.
In contrast, the most of the devices is required help from other sides. The=
refore, the devices with VID 0x2D1F devices are supposed to run alone and s=
eparated out from our brand shipped products.=20
I don't know how much we can go further to tell you about this -because of =
our business, but 0x2D1F was obtained such purpose. Your understanding is m=
uch appreciated.

Thanks,

Tats

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>=20
Sent: Tuesday, October 19, 2021 10:51 AM
To: Ping Cheng <pinglinux@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>; shawnguo@kernel.org; s.hauer=
@pengutronix.de; linux-imx@nxp.com; Jiri Kosina <jikos@kernel.org>; Benjami=
n Tissoires <benjamin.tissoires@redhat.com>; linux-input <linux-input@vger.=
kernel.org>; devicetree@vger.kernel.org; LKML <linux-kernel@vger.kernel.org=
>; linux-arm-kernel@lists.infradead.org; alistair23@gmail.com
Subject: Re: [PATCH v11 1/4] HID: wacom_sys: Add support for flipping the d=
ata values

[EXTERNAL]

Hi Ping,

On Mon, Oct 18, 2021 at 10:41:55AM -0700, Ping Cheng wrote:
> Hi Alistair,
>
> On Sat, Oct 9, 2021, 4:44 AM Alistair Francis <alistair@alistair23.me>
> wrote:
>
> > Add support to the Wacom HID device for flipping the values based on=20
> > device tree settings. This allows us to support devices where the=20
> > panel is installed in a different orientation, such as the reMarkable2.
> >
>
> This device was designed for hid-generic driver, if it's not driven by=20
> wacom_i2c.c or an out of tree driver.
>
> wacom.ko doesn't support vid 0x2d1f devices.

I am really confused about this distinction. Could you please elaborate why=
 wacom driver only supports 0x056a (and, curiously, some Lenovo) devices.

Thanks.


>
> Nacked-by: Ping Cheng <Ping.Cheng@wacom.com>
>
> Sorry about that,
> Ping
>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  .../bindings/input/hid-over-i2c.txt           | 20 ++++++
> >  drivers/hid/wacom_sys.c                       | 25 ++++++++
> >  drivers/hid/wacom_wac.c                       | 61 +++++++++++++++++++
> >  drivers/hid/wacom_wac.h                       | 13 ++++
> >  4 files changed, 119 insertions(+)
> >
> > diff --git=20
> > a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > index c76bafaf98d2..16ebd7c46049 100644
> > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > @@ -33,6 +33,26 @@ device-specific compatible properties, which=20
> > should be used in addition to the
> >  - post-power-on-delay-ms: time required by the device after=20
> > enabling its regulators
> >    or powering it on, before it is ready for communication.
> >
> > +  flip-tilt-x:
> > +    type: boolean
> > +    description: Flip the tilt X values read from device
> > +
> > +  flip-tilt-y:
> > +    type: boolean
> > +    description: Flip the tilt Y values read from device

Do these really need to be controlled separately from the main touchcsreen =
orientation?

> > +
> > +  flip-pos-x:
> > +    type: boolean
> > +    description: Flip the X position values read from device
> > +
> > +  flip-pos-y:
> > +    type: boolean
> > +    description: Flip the Y position values read from device

We already have touchscreen-inverted-x/y defined in Documentation/devicetre=
e/bindings/input/touchscreen/touchscreen.yaml,
why are they not sufficient?

> > +
> > +  flip-distance:
> > +    type: boolean
> > +    description: Flip the distance values read from device

I am still confused of the notion of flipped distance.

> > +
> >  Example:
> >
> >         i2c-hid-dev@2c {
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index=20
> > 93f49b766376..47d9590b10bd 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -10,6 +10,7 @@
> >
> >  #include "wacom_wac.h"
> >  #include "wacom.h"
> > +#include <linux/of.h>
> >  #include <linux/input/mt.h>
> >
> >  #define WAC_MSG_RETRIES                5
> > @@ -2730,6 +2731,28 @@ static void wacom_mode_change_work(struct=20
> > work_struct *work)
> >         return;
> >  }
> >
> > +static void wacom_of_read(struct hid_device *hdev, struct wacom_wac
> > *wacom_wac)
> > +{
> > +       if (IS_ENABLED(CONFIG_OF)) {
> > +               wacom_wac->flip_tilt_x =3D
> > of_property_read_bool(hdev->dev.parent->of_node,
> > +                                                       "flip-tilt-x");
> > +               wacom_wac->flip_tilt_y =3D
> > of_property_read_bool(hdev->dev.parent->of_node,
> > +                                                       "flip-tilt-y");
> > +               wacom_wac->flip_pos_x =3D
> > of_property_read_bool(hdev->dev.parent->of_node,
> > +                                                       "flip-pos-x");
> > +               wacom_wac->flip_pos_y =3D
> > of_property_read_bool(hdev->dev.parent->of_node,
> > +                                                       "flip-pos-y");
> > +               wacom_wac->flip_distance =3D
> > of_property_read_bool(hdev->dev.parent->of_node,
> > +                                                       "flip-distance"=
);
> > +       } else {
> > +               wacom_wac->flip_tilt_x =3D false;
> > +               wacom_wac->flip_tilt_y =3D false;
> > +               wacom_wac->flip_pos_x =3D false;
> > +               wacom_wac->flip_pos_y =3D false;
> > +               wacom_wac->flip_distance =3D false;
> > +       }
> > +}
> > +
> >  static int wacom_probe(struct hid_device *hdev,
> >                 const struct hid_device_id *id)  { @@ -2797,6=20
> > +2820,8 @@ static int wacom_probe(struct hid_device *hdev,
> >                                  error);
> >         }
> >
> > +       wacom_of_read(hdev, wacom_wac);
> > +
> >         wacom_wac->probe_complete =3D true;
> >         return 0;
> >  }
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index=20
> > 33a6908995b1..c01f683e23fa 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -3261,6 +3261,63 @@ static int wacom_status_irq(struct wacom_wac=20
> > *wacom_wac, size_t len)
> >         return 0;
> >  }
> >
> > +static int wacom_of_irq(struct wacom_wac *wacom_wac, size_t len) {
> > +       const struct wacom_features *features =3D &wacom_wac->features;
> > +       unsigned char *data =3D wacom_wac->data;
> > +       struct input_dev *input =3D wacom_wac->pen_input;
> > +       unsigned int x, y, pressure;
> > +       unsigned char tsw, f1, f2, ers;
> > +       short tilt_x, tilt_y, distance;
> > +
> > +       if (!IS_ENABLED(CONFIG_OF))
> > +               return 0;
> > +
> > +       tsw =3D data[1] & WACOM_TIP_SWITCH_bm;
> > +       ers =3D data[1] & WACOM_ERASER_bm;
> > +       f1 =3D data[1] & WACOM_BARREL_SWITCH_bm;
> > +       f2 =3D data[1] & WACOM_BARREL_SWITCH_2_bm;
> > +       x =3D le16_to_cpup((__le16 *)&data[2]);
> > +       y =3D le16_to_cpup((__le16 *)&data[4]);
> > +       pressure =3D le16_to_cpup((__le16 *)&data[6]);
> > +
> > +       /* Signed */
> > +       tilt_x =3D get_unaligned_le16(&data[9]);
> > +       tilt_y =3D get_unaligned_le16(&data[11]);
> > +
> > +       distance =3D get_unaligned_le16(&data[13]);

You are still parsing raw data. The point of HID is to provide common frame=
work for scaling raw values.

Thanks.

--
Dmitry