On Sat, Feb 11, 2023 at 02:23:42AM +0000, Thomas Weißschuh wrote: > On Sat, Feb 11, 2023 at 10:24:25AM +1100, Orlando Chamberlain wrote: > > On Fri, 10 Feb 2023 16:25:18 +0000 > > Thomas Weißschuh <thomas@xxxxxxxx> wrote: > > > > > On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > > > > From: Orlando Chamberlain <orlandoch.dev@xxxxxxxxx> > > > > +static void apple_magic_backlight_power_set(struct > > > > apple_magic_backlight *backlight, > > > > + char power, char rate) > > > > +{ > > > > + struct hid_report *rep = backlight->power; > > > > + > > > > + rep->field[0]->value[0] = power ? 1 : 0; > > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > > + rep->field[1]->value[0] |= rate << 8; > > > > + > > > > + hid_hw_request(backlight->hdev, backlight->power, > > > > HID_REQ_SET_REPORT); +} > > > > + > > > > +static void apple_magic_backlight_brightness_set(struct > > > > apple_magic_backlight *backlight, > > > > + int brightness, > > > > char rate) +{ > > > > + struct hid_report *rep = backlight->brightness; > > > > + > > > > + rep->field[0]->value[0] = brightness; > > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > > + rep->field[1]->value[0] |= rate << 8; > > > > + > > > > + hid_hw_request(backlight->hdev, backlight->brightness, > > > > HID_REQ_SET_REPORT); > > > > + > > > > > > The two functions above are nearly identical. > > > > They are indeed quite similar, and I can turn the backlight off with the > > brightness one, but when I logged the usb packets Windows used, it used > > both so I've done the same in the Linux driver to (hopefully) ensure it > > works with any other models or firmware updates that the Windows driver > > works on. > > I didn't mean to suggest changing the logic, just the way the code is > organized: > > static void apple_magic_backlight_report_set(struct apple_magic_backlight *backlight, > struct hid_report *rep, char value, char rate) > { > rep->field[0]->value[0] = value; > rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > rep->field[1]->value[0] |= rate << 8; > > hid_hw_request(backlight->hdev, rep, HID_REQ_SET_REPORT); > } > > static void apple_magic_backlight_set(struct apple_magic_backlight *backlight, > int brightness, char rate) > { > apple_magic_backlight_report_set(backlight, backlight->power, !!brightness, rate); > if (brightness) > apple_magic_backlight_report_set(backlight, backlight->brightness, brightness, rate); > } > > This way you can get rid of the duplicated code. Or even better, get rid of the struct apple_magic_backlight parameter altogether: static void apple_magic_backlight_report_set(struct hid_report *rep, char value, char rate) { rep->field[0]->value[0] = value; rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ rep->field[1]->value[0] |= rate << 8; hid_hw_request(rep->device, rep, HID_REQ_SET_REPORT); } > > > > > > > > + > > > > +static void apple_magic_backlight_set(struct apple_magic_backlight > > > > *backlight, > > > > + int brightness, char rate) > > > > +{ > > > > + apple_magic_backlight_power_set(backlight, brightness, > > > > rate); > > > > + if (brightness) > > > > + apple_magic_backlight_brightness_set(backlight, > > > > brightness, rate); +} > > > > +