Re: [PATCH 13/14] HID: lg: constify fixed up report descriptor
From: Benjamin Tissoires
Date: Thu Sep 05 2024 - 10:54:12 EST
As you can see in the b4 reply, I've now applied all of the patches but
this one. Please see below.
On Aug 28 2024, Thomas Weißschuh wrote:
> Now that the HID core can handle const report descriptors,
> constify them where possible.
>
> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> ---
> drivers/hid/hid-lg.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> index a9be918e2b5c..c1feeb1dd077 100644
> --- a/drivers/hid/hid-lg.c
> +++ b/drivers/hid/hid-lg.c
> @@ -58,7 +58,7 @@
> * These descriptors remove the combined Y axis and instead report
> * separate throttle (Y) and brake (RZ).
> */
> -static __u8 df_rdesc_fixed[] = {
> +static const __u8 df_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -124,7 +124,7 @@ static __u8 df_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 dfp_rdesc_fixed[] = {
> +static const __u8 dfp_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -172,7 +172,7 @@ static __u8 dfp_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 fv_rdesc_fixed[] = {
> +static const __u8 fv_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -239,7 +239,7 @@ static __u8 fv_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 momo_rdesc_fixed[] = {
> +static const __u8 momo_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -285,7 +285,7 @@ static __u8 momo_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 momo2_rdesc_fixed[] = {
> +static const __u8 momo2_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -333,7 +333,7 @@ static __u8 momo2_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 ffg_rdesc_fixed[] = {
> +static const __u8 ffg_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystik), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -379,7 +379,7 @@ static __u8 ffg_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 fg_rdesc_fixed[] = {
> +static const __u8 fg_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystik), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -431,6 +431,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
> + const __u8 *ret = NULL;
Not really happy about this, usually "ret" is an int, and this makes
things slightly harder to read.
>
> if ((drv_data->quirks & LG_RDESC) && *rsize >= 91 && rdesc[83] == 0x26 &&
> rdesc[84] == 0x8c && rdesc[85] == 0x02) {
> @@ -453,7 +454,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FG_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Wingman Formula GP report descriptor\n");
> - rdesc = fg_rdesc_fixed;
> + ret = fg_rdesc_fixed;
can't you just return fg_rdesc_fixed after setting *rsize, like you did
in the other patches?
(same for all of the other branches)
> *rsize = sizeof(fg_rdesc_fixed);
> } else {
> hid_info(hdev,
> @@ -466,7 +467,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FFG_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Wingman Formula Force GP report descriptor\n");
> - rdesc = ffg_rdesc_fixed;
> + ret = ffg_rdesc_fixed;
> *rsize = sizeof(ffg_rdesc_fixed);
> }
> break;
> @@ -476,7 +477,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == DF_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Driving Force report descriptor\n");
> - rdesc = df_rdesc_fixed;
> + ret = df_rdesc_fixed;
> *rsize = sizeof(df_rdesc_fixed);
> }
> break;
> @@ -485,7 +486,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == MOMO_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Momo Force (Red) report descriptor\n");
> - rdesc = momo_rdesc_fixed;
> + ret = momo_rdesc_fixed;
> *rsize = sizeof(momo_rdesc_fixed);
> }
> break;
> @@ -494,7 +495,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == MOMO2_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Momo Racing Force (Black) report descriptor\n");
> - rdesc = momo2_rdesc_fixed;
> + ret = momo2_rdesc_fixed;
> *rsize = sizeof(momo2_rdesc_fixed);
> }
> break;
> @@ -503,7 +504,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FV_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Formula Vibration report descriptor\n");
> - rdesc = fv_rdesc_fixed;
> + ret = fv_rdesc_fixed;
> *rsize = sizeof(fv_rdesc_fixed);
> }
> break;
> @@ -512,7 +513,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == DFP_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Driving Force Pro report descriptor\n");
> - rdesc = dfp_rdesc_fixed;
> + ret = dfp_rdesc_fixed;
> *rsize = sizeof(dfp_rdesc_fixed);
> }
> break;
> @@ -529,6 +530,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> break;
> }
>
> + if (ret)
> + return ret;
> return rdesc;
> }
>
>
> --
> 2.46.0
>
Cheers,
Benjamin