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

[Andy Shevchenko]

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