Re: [PATCH 2/3] HID: apple-touchbar: Add driver for the Touch Bar on MacBook Pros

From: Andy Shevchenko
Date: Sun Feb 12 2023 - 06:56:31 EST


On Fri, Feb 10, 2023 at 03:44:26AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
>
> This driver enables basic touch bar functionality: enabling it, switching
> between modes on FN key press, and dimming and turning the display
> off/on when idle/active.

...

> Signed-off-by: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
> [Kerem Karabay: use USB product IDs from hid-ids.h]
> [Kerem Karabay: use hid_hw_raw_request except when setting the touchbar mode on T1 Macs]
> [Kerem Karabay: update Kconfig description]
> Signed-off-by: Kerem Karabay <kekrby@xxxxxxxxx>
> [Orlando Chamberlain: add usage check to not bind to keyboard backlight interface]
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@xxxxxxxxx>
> [Aditya Garg: check if apple-touchbar is enabled in the special driver list]
> [Aditya Garg: fix suspend on T2 Macs]

Are you going to use this as a changelog? Not okay for a list of changes.
Consider Co-developed-by and proper Changelog in the cover letter.

> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>

...

> +config HID_APPLE_TOUCHBAR
> + tristate "Apple Touch Bar"
> + depends on USB_HID
> + help

> + Say Y here if you want support for the Touch Bar on x86
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-touchbar.

Wrong indentation for the help description. Missing two spaces.

...

> +#define dev_fmt(fmt) "tb: " fmt

Do we really need this?

...


> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);

> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);

> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size);

Make sure you will have no unnecessary forward declarations.

...

> +static struct attribute *appletb_attrs[] = {
> + &dev_attr_idle_timeout.attr,
> + &dev_attr_dim_timeout.attr,
> + &dev_attr_fnmode.attr,

> + NULL,

No comma for terminator entry.

> +};

...

> +static struct appletb_device *appletb_dev;

Why is it global?

...

> +static bool appletb_disable_autopm(struct hid_device *hdev)
> +{
> + int rc;
> +
> + rc = hid_hw_power(hdev, PM_HINT_FULLON);

> +

Redundant blank line and see below.

> + if (rc == 0)
> + return true;
> +
> + hid_err(hdev,
> + "Failed to disable auto-pm on touch bar device (%d)\n", rc);
> + return false;


if (rc)
hid_err(...);

return rc == 0;

> +}

...

> +static bool appletb_any_tb_key_pressed(struct appletb_device *tb_dev)
> +{
> + return !!memchr_inv(tb_dev->last_tb_keys_pressed, 0,
> + sizeof(tb_dev->last_tb_keys_pressed));

Sounds like last_tb_keys_pressed should be declared as a bitmap and hence

return !bitmap_empty(...);

> +}

...

> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", tb_dev->idle_timeout);

sysfs_emit().

> +}

...

> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> + long new;
> + int rc;
> +
> + rc = kstrtol(buf, 0, &new);
> + if (rc || new > INT_MAX || new < -2)
> + return -EINVAL;

Do not shadow the error code.

if (rc)
return rc;

Why do you use INT_MAX check with to-long conversion instead of simply calling
kstrtoint()?


> + appletb_set_idle_timeout(tb_dev, new);
> + appletb_update_touchbar(tb_dev, true);
> +
> + return size;
> +}

...

> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + tb_dev->dim_to_is_calc ? -2 : tb_dev->dim_timeout);

sysfs_emit()

> +}
> +
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{

As per above.

> +}
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{

As per above.

> +}
> +
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{

As per above.

> +}

...

> +static int appletb_tb_key_to_slot(unsigned int code)
> +{
> + switch (code) {
> + case KEY_ESC:
> + return 0;
> + case KEY_F1:
> + case KEY_F2:
> + case KEY_F3:
> + case KEY_F4:
> + case KEY_F5:
> + case KEY_F6:
> + case KEY_F7:
> + case KEY_F8:
> + case KEY_F9:
> + case KEY_F10:
> + return code - KEY_F1 + 1;
> + case KEY_F11:
> + case KEY_F12:
> + return code - KEY_F11 + 11;
> + default:
> + return -1;

Use appropriate error code from errno.h.

> + }
> +}

...

> + { }, /* Terminating zero entry */

No comma.

...

> +static bool appletb_match_internal_device(struct input_handler *handler,
> + struct input_dev *inp_dev)
> +{
> + struct device *dev = &inp_dev->dev;
> +
> + if (inp_dev->id.bustype == BUS_SPI)
> + return true;
> +
> + /* in kernel: dev && !is_usb_device(dev) */
> + while (dev && !(dev->type && dev->type->name &&
> + !strcmp(dev->type->name, "usb_device")))
> + dev = dev->parent;

I believe we have some helpers to mach like this.

> + /*
> + * Apple labels all their internal keyboards and trackpads as such,
> + * instead of maintaining an ever expanding list of product-id's we
> + * just look at the device's product name.
> + */
> + if (dev)
> + return !!strstr(to_usb_device(dev)->product, "Internal Keyboard");
> +
> + return false;
> +}

...

> +static int appletb_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)

Can be a single line.

...

> + rc = hid_parse(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hid parse failed (%d)\n", rc);
> + goto error;

return dev_err_probe(...);

(error label seems useless)

> + }

...

> + if ((hdev->product == USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) &&
> + !(hdev->collection && hdev->collection[0].usage ==
> + HID_USAGE_APPLE_APP)) {

Broken indentation.

> + return -ENODEV;
> + }

...

> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw start failed (%d)\n", rc);

dev_err_probe()

It will unite the style of error messaging.

> + goto clear_iface_info;
> + }

> + rc = hid_hw_open(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw open failed (%d)\n", rc);

Ditto. And so on.

> + goto stop_hid;
> + }

...

> + /* initialize sysfs attributes */
> + rc = sysfs_create_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to create sysfs attributes (%d)\n", rc);
> + goto unreg_handler;
> + }

Can't you use .dev_groups?

> + }

...

> + /* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
> + USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> + { },

No comma.

...

> +

Redundant blank line.

> +MODULE_DEVICE_TABLE(hid, appletb_hid_ids);

...

> +#ifdef CONFIG_PM
> + .suspend = appletb_suspend,
> + .reset_resume = appletb_reset_resume,
> +#endif

Why not using .driver.pm ?

...

> +module_init(appletb_init);
> +module_exit(appletb_exit);

Just move them closer to the implementation.

--
With Best Regards,
Andy Shevchenko