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.
