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 [off-list ref] wrote:

quoted

On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote:

quoted

From: Orlando Chamberlain <redacted>
+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.

quoted

quoted

+
+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); +}
+