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
