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

["Thomas Weißschuh" <thomas@t-8ch.de>]

On 2024-08-10 15:30:58+0000, Aditya Garg wrote:
> 
> > 
> > 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

Sounds good.

FYI I'll also review the other patches soonish.
So sending a v5 right away is not necessary.