Re: [PATCH 13/14] HID: lg: constify fixed up report descriptor

["Thomas Weißschuh" <linux@weissschuh.net>]

On 2024-09-05 16:51:48+0000, Benjamin Tissoires wrote:
> As you can see in the b4 reply, I've now applied all of the patches but
> this one. Please see below.

Thanks!

> 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@weissschuh.net>
> > ---
> >  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.

Ack.

> 
> >  
> >  	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?

The code looked like it wanted to avoid multiple return sites.
I tried to preserve that style, but indeed it looks wonky.
I'll resend it with the changes.