Received-SPF: pass (google.com: domain of linux-kernel+bounces-171275-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240502042335.15611-1-mmyangfl@gmail.com> <nycvar.YFH.7.76.2405062329320.16865@cbobk.fhfr.pm>
In-Reply-To: <nycvar.YFH.7.76.2405062329320.16865@cbobk.fhfr.pm>
From: =?UTF-8?B?TWlsYW4gUGzFvsOtaw==?= <milan.plzik@gmail.com>
Date: Tue, 7 May 2024 14:29:25 +0200
Message-ID: <CAN-R+gxhdSB2qsLhx8-g02OuQkTmJ2khLOvi9LKpzJgzP1Sc3Q@mail.gmail.com>
Subject: Re: [PATCH] HID: kye: Change Device Usage from Puck to Mouse
To: Jiri Kosina <jikos@kernel.org>
Cc: David Yang <mmyangfl@gmail.com>, linux-input@vger.kernel.org, 
	Benjamin Tissoires <bentiss@kernel.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello Jiri, David,

no objections here. Unfortunately, I can't test the change at the
moment (I barely remember there was a mouse that could be used with
that tablet), but the change sounds good to me. Also, thanks a lot for
keeping the HID drivers up-to-date even for such old hardware :)

Best,
  Milan


On Mon, May 6, 2024 at 11:30=E2=80=AFPM Jiri Kosina <jikos@kernel.org> wrot=
e:
>
> On Thu, 2 May 2024, David Yang wrote:
>
> > Change device type because
> > a. it is exactly a mouse, with left/right buttons and scroll wheel;
> > b. it does not have visible marks or crosshairs, thus does not provide
> > higher accuracy than stylus.
>
> Let's CC Milan, who originally added all this in feb6faf1e5d46 ("HID: kye=
:
> Fix report descriptor for Genius PenSketch M912") ... Milan, any concerns
> about the below?
>
> Thanks.
>
> >
> > Signed-off-by: David Yang <mmyangfl@gmail.com>
> > ---
> >  drivers/hid/hid-kye.c | 75 +++++++++++++++++++++++++------------------
> >  1 file changed, 44 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/hid/hid-kye.c b/drivers/hid/hid-kye.c
> > index eb9bf2829937..70ceb9437332 100644
> > --- a/drivers/hid/hid-kye.c
> > +++ b/drivers/hid/hid-kye.c
> > @@ -209,7 +209,7 @@ static const __u8 pensketch_t609a_control_rdesc[] =
=3D {
> >       0xC0               /*  End Collection            */
> >  };
> >
> > -/* Fix indexes in kye_tablet_fixup if you change this */
> > +/* Fix indexes in kye_tablet_fixup() if you change this */
> >  static const __u8 kye_tablet_rdesc[] =3D {
> >       0x06, 0x00, 0xFF,             /*  Usage Page (FF00h),            =
 */
> >       0x09, 0x01,                   /*  Usage (01h),                   =
 */
> > @@ -262,12 +262,16 @@ static const __u8 kye_tablet_rdesc[] =3D {
> >       0x27, 0xFF, 0x07, 0x00, 0x00, /*      Logical Maximum (2047),    =
 */
> >       0x81, 0x02,                   /*      Input (Variable),          =
 */
> >       0xC0,                         /*    End Collection,              =
 */
> > -     0xC0,                         /*  End Collection,                =
 */
> > -     0x05, 0x0D,                   /*  Usage Page (Digitizer),        =
 */
> > -     0x09, 0x21,                   /*  Usage (Puck),                  =
 */
> > +     0xC0                          /*  End Collection,                =
 */
> > +};
> > +
> > +/* Fix indexes in kye_tablet_fixup() if you change this */
> > +static const __u8 kye_tablet_mouse_rdesc[] =3D {
> > +     0x05, 0x01,                   /*  Usage Page (Desktop),          =
 */
> > +     0x09, 0x02,                   /*  Usage (Mouse),                 =
 */
> >       0xA1, 0x01,                   /*  Collection (Application),      =
 */
> >       0x85, 0x11,                   /*    Report ID (17),              =
 */
> > -     0x09, 0x21,                   /*    Usage (Puck),                =
 */
> > +     0x09, 0x01,                   /*    Usage (Pointer),             =
 */
> >       0xA0,                         /*    Collection (Physical),       =
 */
> >       0x05, 0x09,                   /*      Usage Page (Button),       =
 */
> >       0x19, 0x01,                   /*      Usage Minimum (01h),       =
 */
> > @@ -280,7 +284,7 @@ static const __u8 kye_tablet_rdesc[] =3D {
> >       0x95, 0x04,                   /*      Report Count (4),          =
 */
> >       0x81, 0x01,                   /*      Input (Constant),          =
 */
> >       0x05, 0x0D,                   /*      Usage Page (Digitizer),    =
 */
> > -     0x09, 0x32,                   /*      Usage (In Range),          =
 */
> > +     0x09, 0x37,                   /*      Usage (Data Valid),        =
 */
> >       0x95, 0x01,                   /*      Report Count (1),          =
 */
> >       0x81, 0x02,                   /*      Input (Variable),          =
 */
> >       0x05, 0x01,                   /*      Usage Page (Desktop),      =
 */
> > @@ -317,7 +321,7 @@ static const struct kye_tablet_info {
> >       __s32 y_physical_maximum;
> >       __s8 unit_exponent;
> >       __s8 unit;
> > -     bool has_punk;
> > +     bool has_mouse;
> >       unsigned int control_rsize;
> >       const __u8 *control_rdesc;
> >  } kye_tablets_info[] =3D {
> > @@ -402,7 +406,7 @@ static __u8 *kye_consumer_control_fixup(struct hid_=
device *hdev, __u8 *rdesc,
> >  static __u8 *kye_tablet_fixup(struct hid_device *hdev, __u8 *rdesc, un=
signed int *rsize)
> >  {
> >       const struct kye_tablet_info *info;
> > -     unsigned int newsize;
> > +     __u8 *newdesc =3D rdesc;
> >
> >       if (*rsize < sizeof(kye_tablet_rdesc)) {
> >               hid_warn(hdev,
> > @@ -420,36 +424,45 @@ static __u8 *kye_tablet_fixup(struct hid_device *=
hdev, __u8 *rdesc, unsigned int
> >               return rdesc;
> >       }
> >
> > -     newsize =3D info->has_punk ? sizeof(kye_tablet_rdesc) : 112;
> > -     memcpy(rdesc, kye_tablet_rdesc, newsize);
> > -
> > -     put_unaligned_le32(info->x_logical_maximum, rdesc + 66);
> > -     put_unaligned_le32(info->x_physical_maximum, rdesc + 72);
> > -     rdesc[77] =3D info->unit;
> > -     rdesc[79] =3D info->unit_exponent;
> > -     put_unaligned_le32(info->y_logical_maximum, rdesc + 87);
> > -     put_unaligned_le32(info->y_physical_maximum, rdesc + 92);
> > -     put_unaligned_le32(info->pressure_logical_maximum, rdesc + 104);
> > -
> > -     if (info->has_punk) {
> > -             put_unaligned_le32(info->x_logical_maximum, rdesc + 156);
> > -             put_unaligned_le32(info->x_physical_maximum, rdesc + 162)=
;
> > -             rdesc[167] =3D info->unit;
> > -             rdesc[169] =3D info->unit_exponent;
> > -             put_unaligned_le32(info->y_logical_maximum, rdesc + 177);
> > -             put_unaligned_le32(info->y_physical_maximum, rdesc + 182)=
;
> > +     memcpy(newdesc, kye_tablet_rdesc, sizeof(kye_tablet_rdesc));
> > +
> > +     put_unaligned_le32(info->x_logical_maximum, newdesc + 66);
> > +     put_unaligned_le32(info->x_physical_maximum, newdesc + 72);
> > +     newdesc[77] =3D info->unit;
> > +     newdesc[79] =3D info->unit_exponent;
> > +     put_unaligned_le32(info->y_logical_maximum, newdesc + 87);
> > +     put_unaligned_le32(info->y_physical_maximum, newdesc + 92);
> > +     put_unaligned_le32(info->pressure_logical_maximum, newdesc + 104)=
;
> > +
> > +     newdesc +=3D sizeof(kye_tablet_rdesc);
> > +
> > +     if (info->has_mouse) {
> > +             if (newdesc + sizeof(kye_tablet_mouse_rdesc) > rdesc + *r=
size)
> > +                     hid_err(hdev, "control desc unexpectedly large\n"=
);
> > +             else {
> > +                     memcpy(newdesc, kye_tablet_mouse_rdesc, sizeof(ky=
e_tablet_mouse_rdesc));
> > +
> > +                     put_unaligned_le32(info->x_logical_maximum, newde=
sc + 44);
> > +                     put_unaligned_le32(info->x_physical_maximum, newd=
esc + 50);
> > +                     newdesc[55] =3D info->unit;
> > +                     newdesc[57] =3D info->unit_exponent;
> > +                     put_unaligned_le32(info->y_logical_maximum, newde=
sc + 65);
> > +                     put_unaligned_le32(info->y_physical_maximum, newd=
esc + 70);
> > +
> > +                     newdesc +=3D sizeof(kye_tablet_mouse_rdesc);
> > +             }
> >       }
> >
> >       if (info->control_rsize) {
> > -             if (newsize + info->control_rsize > *rsize)
> > -                     hid_err(hdev, "control rdesc unexpectedly large")=
;
> > +             if (newdesc + info->control_rsize > rdesc + *rsize)
> > +                     hid_err(hdev, "control desc unexpectedly large\n"=
);
> >               else {
> > -                     memcpy(rdesc + newsize, info->control_rdesc, info=
->control_rsize);
> > -                     newsize +=3D info->control_rsize;
> > +                     memcpy(newdesc, info->control_rdesc, info->contro=
l_rsize);
> > +                     newdesc +=3D info->control_rsize;
> >               }
> >       }
> >
> > -     *rsize =3D newsize;
> > +     *rsize =3D newdesc - rdesc;
> >       return rdesc;
> >  }
> >
> > --
> > 2.43.0
> >
>
> --
> Jiri Kosina
> SUSE Labs
>