Re: [PATCH v3 1/9] HID: hid-appletb-bl: add driver for the backlight of Apple Touch Bars

[Aditya Garg <gargaditya08@live.com>]

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