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:

quoted

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.

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

quoted

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