>
> Also include linux/device.h as you are using functions from there.
> Like devm_kcalloc().
Alright, I’ll add that
>
>>>
>>>> +#include <linux/hid.h>
>>>> +#include <linux/backlight.h>
>>>> +
>>>> +#include "hid-ids.h"
>>>> +
>>>> +#define APPLETB_BL_ON 1
>>>> +#define APPLETB_BL_DIM 3
>>>> +#define APPLETB_BL_OFF 4
>>>> +
>>>> +#define HID_UP_APPLEVENDOR_TB_BL 0xff120000
>>>> +
>>>> +#define HID_VD_APPLE_TB_BRIGHTNESS 0xff120001
>>>> +#define HID_USAGE_AUX1 0xff120020
>>>> +#define HID_USAGE_BRIGHTNESS 0xff120021
>>>> +
>>>> +static int appletb_bl_def_brightness = 2;
>>>> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
>>>> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
>>>> + " 0 - Touchbar is off\n"
>>>> + " 1 - Dim brightness\n"
>>>> + " [2] - Full brightness");
>>>> +
>>>> +struct appletb_bl {
>>>> + struct hid_field *aux1_field, *brightness_field;
>>>> + struct backlight_device *bdev;
>>>> +
>>>> + bool full_on;
>>>> +};
>>>> +
>>>> +const u8 appletb_bl_brightness_map[] = {
>>>
>>> static?
>>>
>>>> + APPLETB_BL_OFF,
>>>> + APPLETB_BL_DIM,
>>>> + APPLETB_BL_ON
>>>
>>> The last element is not a sentinel element, so it should have comma.
>>
>> static const u8 appletb_bl_brightness_map[] = {
>> APPLETB_BL_OFF,
>> APPLETB_BL_DIM,
>> APPLETB_BL_ON,
>> };
>>
>> This?
>
> Yes.
>
>>>
>>>> +};
>>>> +
>>>> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
>>>> +{
>>>> + struct hid_report *report = bl->brightness_field->report;
>>>> + struct hid_device *hdev = report->device;
>>>> + int ret;
>>>> +
>>>> + ret = hid_set_field(bl->aux1_field, 0, 1);
>>>> + if (ret) {
>>>> + hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = hid_set_field(bl->brightness_field, 0, brightness);
>>>> + if (ret) {
>>>> + hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (!bl->full_on) {
>>>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
>>>> + if (ret < 0) {
>>>> + hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + bl->full_on = true;
>>>> + }
>>>> +
>>>> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>>>> +
>>>> + if (brightness == APPLETB_BL_OFF) {
>>>> + hid_hw_power(hdev, PM_HINT_NORMAL);
>>>> + bl->full_on = false;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int appletb_bl_update_status(struct backlight_device *bdev)
>>>> +{
>>>> + struct appletb_bl *bl = bl_get_data(bdev);
>>>> + u16 brightness;
>>>> +
>>>> + if (bdev->props.state & BL_CORE_SUSPENDED)
>>>> + brightness = 0;
>>>
>>> From backlight.h:
>>>
>>> * backlight drivers are expected to use backlight_is_blank()
>>> * in their update_status() operation rather than reading the
>>> * state property.
>>>
>>> Seems to be applicable here.
>>>
>>> Also the hardcoded "0" as index into appletb_bl_brightness_map could be
>>> avoided by some restructuring.
>>
>> static int appletb_bl_update_status(struct backlight_device *bdev)
>> {
>> struct appletb_bl *bl = bl_get_data(bdev);
>> u16 brightness;
>>
>> if (backlight_is_blank(bdev))
>> brightness = APPLETB_BL_OFF;
>> else
>> brightness = backlight_get_brightness(bdev);
>>
>> return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
>> }
>>
>> This?
>
> This now looks to be a different logic than before.
> Below should be the same as in the original patch.
>
> static int appletb_bl_update_status(struct backlight_device *bdev)
> {
> struct appletb_bl *bl = bl_get_data(bdev);
> u8 brightness;
>
> if (backlight_is_blank(bdev))
> brightness = APPLETB_BL_OFF;
> else
> brightness = appletb_bl_brightness_map[backlight_get_brightness(bdev)]);
>
> return appletb_bl_set_brightness(bl, brightness);
> }
I’ll replace the original code with this
>
> Maybe it's worth to make APPLETB_BL_* an enum for more clarity.
I don’t think there is a specific need for an enum here
If you are fine with these changes, I’ll send a v5
>
>>>> + else
>>>> + brightness = backlight_get_brightness(bdev);
>>>> +
>>>> + return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
>>>> +}
>>>> +
>
> <snip>
Thanks
Aditya
