[1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.

  From: Ronald Tschalär <ronald@innovation.ch>

The iBridge device provides access to several devices, including:
- the Touch Bar
- the iSight webcam
- the light sensor
- the fingerprint sensor

This driver provides the core support for managing the iBridge device
and the access to the underlying devices. In particular, the
functionality for the touch bar and light sensor is exposed via USB HID
interfaces, and on devices with the T1 chip one of the HID devices is
used for both functions. So this driver creates virtual HID devices, one
per top-level report collection on each HID device (for a total of 3
virtual HID devices). The sub-drivers then bind to these virtual HID
devices.

This way the Touch Bar and ALS drivers can be kept in their own modules,
while at the same time making them look very much like as if they were
connected to the real HID devices. And those drivers then work (mostly)
without further changes on MacBooks with the T2 chip that don't need
this driver.

Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
[Kerem Karabay: convert to a platform driver]
[Kerem Karabay: fix appleib_forward_int_op]
[Kerem Karabay: rely on HID core's parsing in appleib_add_device]
Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 drivers/hid/Kconfig         |  15 +
 drivers/hid/Makefile        |   1 +
 drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
 drivers/hid/apple-ibridge.h |  15 +
 drivers/hid/hid-ids.h       |   1 +
 drivers/hid/hid-quirks.c    |   3 +
 6 files changed, 645 insertions(+)
 create mode 100644 drivers/hid/apple-ibridge.c
 create mode 100644 drivers/hid/apple-ibridge.h

-- 
2.37.2
  

Hi,

some comments inline.

On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@innovation.ch>
> 
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
> 
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and on devices with the T1 chip one of the HID devices is
> used for both functions. So this driver creates virtual HID devices, one
> per top-level report collection on each HID device (for a total of 3
> virtual HID devices). The sub-drivers then bind to these virtual HID
> devices.
> 
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices. And those drivers then work (mostly)
> without further changes on MacBooks with the T2 chip that don't need
> this driver.
> 
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> [Kerem Karabay: convert to a platform driver]
> [Kerem Karabay: fix appleib_forward_int_op]
> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>  drivers/hid/Kconfig         |  15 +
>  drivers/hid/Makefile        |   1 +
>  drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
>  drivers/hid/apple-ibridge.h |  15 +
>  drivers/hid/hid-ids.h       |   1 +
>  drivers/hid/hid-quirks.c    |   3 +
>  6 files changed, 645 insertions(+)
>  create mode 100644 drivers/hid/apple-ibridge.c
>  create mode 100644 drivers/hid/apple-ibridge.h
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index e2a5d30c8..e69afa5f4 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -130,6 +130,21 @@ config HID_APPLE
>  	Say Y here if you want support for keyboards of	Apple iBooks, PowerBooks,
>  	MacBooks, MacBook Pros and Apple Aluminum.
>  
> +config HID_APPLE_IBRIDGE
> +	tristate "Apple iBridge"
> +	depends on USB_HID
> +	depends on (X86 && ACPI) || COMPILE_TEST
> +	imply HID_SENSOR_HUB
> +	imply HID_SENSOR_ALS
> +	help
> +	This module provides the core support for the Apple T1 chip found
> +	on 2016 and 2017 MacBookPro's, also known as the iBridge. The drivers
> +	for the Touch Bar (apple-touchbar) and light sensor (hid-sensor-hub
> +	and hid-sensor-als) need to be enabled separately.
> +
> +	To compile this driver as a module, choose M here: the
> +	module will be called apple-ibridge.
> +
>  config HID_APPLEIR
>  	tristate "Apple infrared receiver"
>  	depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index e8014c1a2..b61373cd8 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH)	+= hid-accutouch.o
>  obj-$(CONFIG_HID_ALPS)		+= hid-alps.o
>  obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
>  obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
> +obj-$(CONFIG_HID_APPLE_IBRIDGE)	+= apple-ibridge.o
>  obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
>  obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
>  obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
> diff --git a/drivers/hid/apple-ibridge.c b/drivers/hid/apple-ibridge.c
> new file mode 100644
> index 000000000..4d26f8d66
> --- /dev/null
> +++ b/drivers/hid/apple-ibridge.c
> @@ -0,0 +1,610 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +/**
> + * DOC: Overview
> + *
> + * 2016 and 2017 MacBookPro models with a Touch Bar (MacBookPro13,[23] and
> + * MacBookPro14,[23]) have an Apple iBridge chip (also known as T1 chip) which
> + * exposes the touch bar, built-in webcam (iSight), ambient light sensor, and
> + * Secure Enclave Processor (SEP) for TouchID. It shows up in the system as a
> + * USB device with 3 configurations: 'Default iBridge Interfaces', 'Default
> + * iBridge Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'.
> + *
> + * In the first (default after boot) configuration, 4 usb interfaces are
> + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing
> + * the touch bar and the ambient light sensor. The webcam interfaces are
> + * already handled by the uvcvideo driver. However, there is a problem with
> + * the other two interfaces: one of them contains functionality (HID reports)
> + * used by both the touch bar and the ALS, which is an issue because the kernel
> + * allows only one driver to be attached to a given device. This driver exists
> + * to solve this issue.
> + *
> + * This driver is implemented as a HID driver that attaches to both HID
> + * interfaces and in turn creates several virtual child HID devices, one for
> + * each top-level collection found in each interfaces report descriptor. The
> + * touch bar and ALS drivers then attach to these virtual HID devices, and this
> + * driver forwards the operations between the real and virtual devices.
> + *
> + * One important aspect of this approach is that resulting (virtual) HID
> + * devices look much like the HID devices found on the later MacBookPro models
> + * which have a T2 chip, where there are separate USB interfaces for the touch
> + * bar and ALS functionality, which means that the touch bar and ALS drivers
> + * work (mostly) the same on both types of models.
> + *
> + * Lastly, this driver also takes care of the power-management for the
> + * iBridge when suspending and resuming.
> + */

Maybe add a pr_fmt definition here?

> +
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"
> +#include "apple-ibridge.h"
> +
> +#define APPLEIB_BASIC_CONFIG	1
> +
> +static struct hid_device_id appleib_sub_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_TB) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_ALS) },
> +};

Structs like this should be "const".

> +
> +static struct {
> +	unsigned int usage;
> +	struct hid_device_id *dev_id;
> +} appleib_usage_map[] = {
> +	/* Default iBridge configuration, key inputs and mode settings */
> +	{ 0x00010006, &appleib_sub_hid_ids[0] },
> +	/* OS X iBridge configuration, digitizer inputs */
> +	{ 0x000D0005, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, display/DFR settings */
> +	{ 0xFF120001, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, ALS */
> +	{ 0x00200041, &appleib_sub_hid_ids[1] },
> +};

const

> +
> +struct appleib_device {
> +	acpi_handle asoc_socw;
> +};
> +
> +struct appleib_hid_dev_info {
> +	struct hid_device	*hdev;
> +	struct hid_device	*sub_hdevs[ARRAY_SIZE(appleib_sub_hid_ids)];
> +	bool			sub_open[ARRAY_SIZE(appleib_sub_hid_ids)];
> +};
> +
> +static int appleib_hid_raw_event(struct hid_device *hdev,
> +				 struct hid_report *report, u8 *data, int size)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		if (READ_ONCE(hdev_info->sub_open[i]))
> +			hid_input_report(hdev_info->sub_hdevs[i], report->type,
> +					 data, size, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> +				  unsigned int *rsize)
> +{
> +	/* Some fields have a size of 64 bits, which according to HID 1.11
> +	 * Section 8.4 is not valid ("An item field cannot span more than 4
> +	 * bytes in a report"). Furthermore, hid_field_extract() complains
> +	 * when encountering such a field. So turn them into two 32-bit fields
> +	 * instead.
> +	 */
> +
> +	if (*rsize == 634 &&
> +	    /* Usage Page 0xff12 (vendor defined) */
> +	    rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> +	    /* Usage 0x51 */
> +	    rdesc[416] == 0x09 && rdesc[417] == 0x51 &&
> +	    /* report size 64 */
> +	    rdesc[432] == 0x75 && rdesc[433] == 64 &&
> +	    /* report count 1 */
> +	    rdesc[434] == 0x95 && rdesc[435] == 1) {
> +		rdesc[433] = 32;
> +		rdesc[435] = 2;
> +		hid_dbg(hdev, "Fixed up first 64-bit field\n");
> +	}
> +
> +	if (*rsize == 634 &&
> +	    /* Usage Page 0xff12 (vendor defined) */
> +	    rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> +	    /* Usage 0x51 */
> +	    rdesc[611] == 0x09 && rdesc[612] == 0x51 &&
> +	    /* report size 64 */
> +	    rdesc[627] == 0x75 && rdesc[628] == 64 &&
> +	    /* report count 1 */
> +	    rdesc[629] == 0x95 && rdesc[630] == 1) {
> +		rdesc[628] = 32;
> +		rdesc[630] = 2;
> +		hid_dbg(hdev, "Fixed up second 64-bit field\n");
> +	}
> +
> +	return rdesc;
> +}
> +
> +#ifdef CONFIG_PM
> +/**
> + * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
> + * all virtual HID devices attached to the given real HID device.
> + * @hdev the real hid-device
> + * @forward a function that calls the callback on the given driver
> + * @args arguments for the forward function
> + *
> + * This is for callbacks that return a status as an int.
> + *
> + * Returns: 0 on success, or the first error returned by the @forward function.
> + */
> +static int appleib_forward_int_op(struct hid_device *hdev,
> +				  int (*forward)(struct hid_driver *,
> +						 struct hid_device *, void *),
> +				  void *args)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> +	struct hid_device *sub_hdev;
> +	int rc;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		sub_hdev = hdev_info->sub_hdevs[i];
> +		if (sub_hdev->driver) {
> +			rc = forward(sub_hdev->driver, sub_hdev, args);
> +			if (rc)
> +				return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int appleib_hid_suspend_fwd(struct hid_driver *drv,
> +				   struct hid_device *hdev, void *args)
> +{
> +	int rc = 0;
> +
> +	if (drv->suspend)
> +		rc = drv->suspend(hdev, *(pm_message_t *)args);
> +
> +	return rc;
> +}
> +
> +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> +	return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message);
> +}
> +
> +static int appleib_hid_resume_fwd(struct hid_driver *drv,
> +				  struct hid_device *hdev, void *args)
> +{
> +	int rc = 0;
> +
> +	if (drv->resume)
> +		rc = drv->resume(hdev);
> +
> +	return rc;
> +}
> +
> +static int appleib_hid_resume(struct hid_device *hdev)
> +{
> +	return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL);
> +}
> +
> +static int appleib_hid_reset_resume_fwd(struct hid_driver *drv,
> +					struct hid_device *hdev, void *args)
> +{
> +	int rc = 0;
> +
> +	if (drv->reset_resume)
> +		rc = drv->reset_resume(hdev);
> +
> +	return rc;
> +}
> +
> +static int appleib_hid_reset_resume(struct hid_device *hdev)
> +{
> +	return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL);
> +}
> +#endif /* CONFIG_PM */
> +
> +static int appleib_ll_start(struct hid_device *hdev)
> +{
> +	return 0;
> +}
> +
> +static void appleib_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int appleib_set_open(struct hid_device *hdev, bool open)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		/*
> +		 * hid_hw_open(), and hence appleib_ll_open(), is called
> +		 * from the driver's probe function, which in turn is called
> +		 * while adding the sub-hdev; but at this point we haven't yet
> +		 * added the sub-hdev to our list. So if we don't find the
> +		 * sub-hdev in our list assume it's in the process of being
> +		 * added and set the flag on the first unset sub-hdev.
> +		 */
> +		if (hdev_info->sub_hdevs[i] == hdev ||
> +		    !hdev_info->sub_hdevs[i]) {
> +			WRITE_ONCE(hdev_info->sub_open[i], open);
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int appleib_ll_open(struct hid_device *hdev)
> +{
> +	return appleib_set_open(hdev, true);
> +}
> +
> +static void appleib_ll_close(struct hid_device *hdev)
> +{
> +	appleib_set_open(hdev, false);
> +}
> +
> +static int appleib_ll_power(struct hid_device *hdev, int level)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> +	return hid_hw_power(hdev_info->hdev, level);
> +}
> +
> +static int appleib_ll_parse(struct hid_device *hdev)
> +{
> +	/* we've already called hid_parse_report() */
> +	return 0;
> +}
> +
> +static void appleib_ll_request(struct hid_device *hdev,
> +			       struct hid_report *report, int reqtype)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> +	hid_hw_request(hdev_info->hdev, report, reqtype);
> +}
> +
> +static int appleib_ll_wait(struct hid_device *hdev)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> +	hid_hw_wait(hdev_info->hdev);
> +	return 0;
> +}
> +
> +static int appleib_ll_raw_request(struct hid_device *hdev,
> +				  unsigned char reportnum, __u8 *buf,
> +				  size_t len, unsigned char rtype, int reqtype)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> +	return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
> +				  reqtype);
> +}
> +
> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
> +				    size_t len)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> +
> +	return hid_hw_output_report(hdev_info->hdev, buf, len);
> +}
> +
> +static struct hid_ll_driver appleib_ll_driver = {
> +	.start = appleib_ll_start,
> +	.stop = appleib_ll_stop,
> +	.open = appleib_ll_open,
> +	.close = appleib_ll_close,
> +	.power = appleib_ll_power,
> +	.parse = appleib_ll_parse,
> +	.request = appleib_ll_request,
> +	.wait = appleib_ll_wait,
> +	.raw_request = appleib_ll_raw_request,
> +	.output_report = appleib_ll_output_report,
> +};

const

> +
> +static struct hid_device_id *appleib_find_dev_id_for_usage(unsigned int usage)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(appleib_usage_map); i++) {
> +		if (appleib_usage_map[i].usage == usage)
> +			return appleib_usage_map[i].dev_id;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct hid_device *
> +appleib_add_sub_dev(struct appleib_hid_dev_info *hdev_info,
> +		    struct hid_device_id *dev_id)
> +{
> +	struct hid_device *sub_hdev;
> +	int rc;
> +
> +	sub_hdev = hid_allocate_device();
> +	if (IS_ERR(sub_hdev))
> +		return sub_hdev;
> +
> +	sub_hdev->dev.parent = &hdev_info->hdev->dev;
> +
> +	sub_hdev->bus = dev_id->bus;
> +	sub_hdev->group = dev_id->group;
> +	sub_hdev->vendor = dev_id->vendor;
> +	sub_hdev->product = dev_id->product;
> +
> +	sub_hdev->ll_driver = &appleib_ll_driver;
> +
> +	snprintf(sub_hdev->name, sizeof(sub_hdev->name),
> +		 "iBridge Virtual HID %s/%04x:%04x",
> +		 dev_name(sub_hdev->dev.parent), sub_hdev->vendor,
> +		 sub_hdev->product);
> +
> +	sub_hdev->driver_data = hdev_info;
> +
> +	rc = hid_add_device(sub_hdev);
> +	if (rc) {
> +		hid_destroy_device(sub_hdev);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return sub_hdev;
> +}
> +
> +static struct appleib_hid_dev_info *appleib_add_device(struct hid_device *hdev)
> +{
> +	struct appleib_hid_dev_info *hdev_info;
> +	struct hid_device_id *dev_id;
> +	unsigned int usage;
> +	int i;
> +
> +	hdev_info = devm_kzalloc(&hdev->dev, sizeof(*hdev_info), GFP_KERNEL);
> +	if (!hdev_info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hdev_info->hdev = hdev;
> +
> +	for (i = 0; i < hdev->maxcollection; i++) {
> +		usage = hdev->collection[i].usage;
> +		dev_id = appleib_find_dev_id_for_usage(usage);
> +
> +		if (!dev_id) {
> +			hid_warn(hdev, "Unknown collection encountered with usage %x\n",
> +				 usage);
> +		} else {
> +			hdev_info->sub_hdevs[i] = appleib_add_sub_dev(hdev_info, dev_id);
> +
> +			if (IS_ERR(hdev_info->sub_hdevs[i])) {
> +				while (i-- > 0)
> +					hid_destroy_device(hdev_info->sub_hdevs[i]);
> +				return (void *)hdev_info->sub_hdevs[i];
> +			}
> +		}
> +	}
> +
> +	return hdev_info;
> +}
> +
> +static void appleib_remove_device(struct hid_device *hdev)
> +{
> +	struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		if (hdev_info->sub_hdevs[i])
> +			hid_destroy_device(hdev_info->sub_hdevs[i]);
> +	}
> +
> +	hid_set_drvdata(hdev, NULL);
> +}
> +
> +static int appleib_hid_probe(struct hid_device *hdev,
> +			     const struct hid_device_id *id)
> +{
> +	struct appleib_hid_dev_info *hdev_info;
> +	struct usb_device *udev;
> +	int rc;
> +
> +	/* check and set usb config first */
> +	udev = hid_to_usb_dev(hdev);

I don't think hid_to_usb_dev() does any check that hdev is connected via
USB and will produce undefined behavior when used on a non-USB device.

Use hid_is_usb() first.

> +
> +	if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
> +		rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
> +		return rc ? rc : -ENODEV;
> +	}
> +
> +	rc = hid_parse(hdev);
> +	if (rc) {
> +		hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> +		goto error;
> +	}
> +
> +	rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> +	if (rc) {
> +		hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> +		goto error;
> +	}
> +
> +	hdev_info = appleib_add_device(hdev);
> +	if (IS_ERR(hdev_info)) {
> +		rc = PTR_ERR(hdev_info);
> +		goto stop_hw;
> +	}
> +
> +	hid_set_drvdata(hdev, hdev_info);
> +
> +	rc = hid_hw_open(hdev);
> +	if (rc) {
> +		hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> +		goto remove_dev;
> +	}
> +
> +	return 0;
> +
> +remove_dev:
> +	appleib_remove_device(hdev);
> +stop_hw:
> +	hid_hw_stop(hdev);
> +error:
> +	return rc;
> +}
> +
> +static void appleib_hid_remove(struct hid_device *hdev)
> +{
> +	hid_hw_close(hdev);
> +	appleib_remove_device(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appleib_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> +	{ },
> +};
> +
> +static struct hid_driver appleib_hid_driver = {
> +	.name = "apple-ibridge-hid",
> +	.id_table = appleib_hid_ids,
> +	.probe = appleib_hid_probe,
> +	.remove = appleib_hid_remove,
> +	.raw_event = appleib_hid_raw_event,
> +	.report_fixup = appleib_report_fixup,
> +#ifdef CONFIG_PM
> +	.suspend = appleib_hid_suspend,
> +	.resume = appleib_hid_resume,
> +	.reset_resume = appleib_hid_reset_resume,
> +#endif
> +};

const

> +
> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
> +{
> +	struct appleib_device *ib_dev;
> +	acpi_status sts;
> +
> +	ib_dev = devm_kzalloc(&pdev->dev, sizeof(*ib_dev), GFP_KERNEL);
> +	if (!ib_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* get iBridge acpi power control method for suspend/resume */
> +	sts = acpi_get_handle(ACPI_HANDLE(&pdev->dev), "SOCW", &ib_dev->asoc_socw);
> +	if (ACPI_FAILURE(sts)) {
> +		dev_err(&pdev->dev,
> +			"Error getting handle for ASOC.SOCW method: %s\n",
> +			acpi_format_exception(sts));
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	/* ensure iBridge is powered on */
> +	sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> +	if (ACPI_FAILURE(sts))
> +		dev_warn(&pdev->dev, "SOCW(1) failed: %s\n",
> +			 acpi_format_exception(sts));
> +
> +	return ib_dev;
> +}
> +
> +static int appleib_probe(struct platform_device *pdev)
> +{
> +	struct appleib_device *ib_dev;
> +	int ret;
> +
> +	ib_dev = appleib_alloc_device(pdev);
> +	if (IS_ERR(ib_dev))
> +		return PTR_ERR(ib_dev);
> +
> +	ret = hid_register_driver(&appleib_hid_driver);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error registering hid driver: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, ib_dev);
> +
> +	return 0;
> +}
> +
> +static int appleib_remove(struct platform_device *pdev)
> +{
> +	hid_unregister_driver(&appleib_hid_driver);
> +
> +	return 0;
> +}
> +
> +static int appleib_suspend(struct platform_device *pdev, pm_message_t message)
> +{
> +	struct appleib_device *ib_dev;
> +	int rc;
> +
> +	ib_dev = platform_get_drvdata(pdev);
> +
> +	rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> +	if (ACPI_FAILURE(rc))
> +		dev_warn(&pdev->dev, "SOCW(0) failed: %s\n",
> +			 acpi_format_exception(rc));
> +
> +	return 0;
> +}
> +
> +static int appleib_resume(struct platform_device *pdev)
> +{
> +	struct appleib_device *ib_dev;
> +	int rc;
> +
> +	ib_dev = platform_get_drvdata(pdev);
> +
> +	rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> +	if (ACPI_FAILURE(rc))
> +		dev_warn(&pdev->dev, "SOCW(1) failed: %s\n",
> +			 acpi_format_exception(rc));
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id appleib_acpi_match[] = {
> +	{ "APP7777", 0 },
> +	{ },

No trailing comma after end-of-array marker.

> +};
> +
> +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match);
> +
> +static struct platform_driver appleib_driver = {
> +	.probe		= appleib_probe,
> +	.remove		= appleib_remove,
> +	.suspend	= appleib_suspend,
> +	.resume		= appleib_resume,
> +	.driver		= {
> +		.name		  = "apple-ibridge",
> +		.acpi_match_table = appleib_acpi_match,
> +	},
> +};
> +
> +module_platform_driver(appleib_driver);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/apple-ibridge.h b/drivers/hid/apple-ibridge.h
> new file mode 100644
> index 000000000..8aefcf615
> --- /dev/null
> +++ b/drivers/hid/apple-ibridge.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +#ifndef __LINUX_APPLE_IBRDIGE_H
> +#define __LINUX_APPLE_IBRDIGE_H
> +
> +#define USB_VENDOR_ID_LINUX_FOUNDATION	0x1d6b
> +#define USB_DEVICE_ID_IBRIDGE_TB	0x0301
> +#define USB_DEVICE_ID_IBRIDGE_ALS	0x0302
> +
> +#endif
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 0f8c11842..0c62e6280 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -187,6 +187,7 @@
>  #define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_NUMPAD_2021   0x029f
>  #define USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT 0x8102
>  #define USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY 0x8302
> +#define USB_DEVICE_ID_APPLE_IBRIDGE	0x8600
>  
>  #define USB_VENDOR_ID_ASUS		0x0486
>  #define USB_DEVICE_ID_ASUS_T91MT	0x0185
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index be3ad0257..c03535c4b 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -319,6 +319,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>  #endif
> +#if IS_ENABLED(CONFIG_HID_APPLE_IBRIDGE)
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> +#endif
>  #if IS_ENABLED(CONFIG_HID_APPLEIR)
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL2) },
> -- 
> 2.37.2
>
  

> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> 
> Hi,
> 
> some comments inline.
> 
> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>> From: Ronald Tschalär <ronald@innovation.ch>
>> 
>> The iBridge device provides access to several devices, including:
>> - the Touch Bar
>> - the iSight webcam
>> - the light sensor
>> - the fingerprint sensor
>> 
>> This driver provides the core support for managing the iBridge device
>> and the access to the underlying devices. In particular, the
>> functionality for the touch bar and light sensor is exposed via USB HID
>> interfaces, and on devices with the T1 chip one of the HID devices is
>> used for both functions. So this driver creates virtual HID devices, one
>> per top-level report collection on each HID device (for a total of 3
>> virtual HID devices). The sub-drivers then bind to these virtual HID
>> devices.
>> 
>> This way the Touch Bar and ALS drivers can be kept in their own modules,
>> while at the same time making them look very much like as if they were
>> connected to the real HID devices. And those drivers then work (mostly)
>> without further changes on MacBooks with the T2 chip that don't need
>> this driver.
>> 
>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>> [Kerem Karabay: convert to a platform driver]
>> [Kerem Karabay: fix appleib_forward_int_op]
>> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>> drivers/hid/Kconfig         |  15 +
>> drivers/hid/Makefile        |   1 +
>> drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
>> drivers/hid/apple-ibridge.h |  15 +
>> drivers/hid/hid-ids.h       |   1 +
>> drivers/hid/hid-quirks.c    |   3 +
>> 6 files changed, 645 insertions(+)
>> create mode 100644 drivers/hid/apple-ibridge.c
>> create mode 100644 drivers/hid/apple-ibridge.h
>> 
> 
>> 
>> +}
>> +
>> +static int appleib_ll_raw_request(struct hid_device *hdev,
>> +   unsigned char reportnum, __u8 *buf,
>> +   size_t len, unsigned char rtype, int reqtype)
>> +{
>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>> +
>> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
>> +   reqtype);
>> +}
>> +
>> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
>> +     size_t len)
>> +{
>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>> +
>> + return hid_hw_output_report(hdev_info->hdev, buf, len);
>> +}
>> +
>> +static struct hid_ll_driver appleib_ll_driver = {
>> + .start = appleib_ll_start,
>> + .stop = appleib_ll_stop,
>> + .open = appleib_ll_open,
>> + .close = appleib_ll_close,
>> + .power = appleib_ll_power,
>> + .parse = appleib_ll_parse,
>> + .request = appleib_ll_request,
>> + .wait = appleib_ll_wait,
>> + .raw_request = appleib_ll_raw_request,
>> + .output_report = appleib_ll_output_report,
>> +};
> 
> const

Are you sure about const here?

> 
>> +
>> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
>> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
>> + return rc ? rc : -ENODEV;
>> + }
>> +
>> + rc = hid_parse(hdev);
>> + if (rc) {
>> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
>> + goto error;
>> + }
>> +
>> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
>> + if (rc) {
>> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
>> + goto error;
>> + }
>> +
>> + hdev_info = appleib_add_device(hdev);
>> + if (IS_ERR(hdev_info)) {
>> + rc = PTR_ERR(hdev_info);
>> + goto stop_hw;
>> + }
>> +
>> + hid_set_drvdata(hdev, hdev_info);
>> +
>> + rc = hid_hw_open(hdev);
>> + if (rc) {
>> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
>> + goto remove_dev;
>> + }
>> +
>> + return 0;
>> +
>> +remove_dev:
>> + appleib_remove_device(hdev);
>> +stop_hw:
>> + hid_hw_stop(hdev);
>> +error:
>> + return rc;
>> +}
>> +
>> +static void appleib_hid_remove(struct hid_device *hdev)
>> +{
>> + hid_hw_close(hdev);
>> + appleib_remove_device(hdev);
>> + hid_hw_stop(hdev);
>> +}
>> +
>> +static const struct hid_device_id appleib_hid_ids[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
>> + { },
>> +};
>> +
>> +static struct hid_driver appleib_hid_driver = {
>> + .name = "apple-ibridge-hid",
>> + .id_table = appleib_hid_ids,
>> + .probe = appleib_hid_probe,
>> + .remove = appleib_hid_remove,
>> + .raw_event = appleib_hid_raw_event,
>> + .report_fixup = appleib_report_fixup,
>> +#ifdef CONFIG_PM
>> + .suspend = appleib_hid_suspend,
>> + .resume = appleib_hid_resume,
>> + .reset_resume = appleib_hid_reset_resume,
>> +#endif
>> +};
> 
> const

Are you sure you want to do const here, cause other hid-drivers are NOT using const.

>> +
>> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
>> +{
>> + struct appleib_device *ib_dev;
>> + acpi_status sts;
  

On Fri, Feb 10, 2023 at 9:30 AM Aditya Garg <gargaditya08@live.com> wrote:
>
>
>
> > On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >
> > Hi,
> >
> > some comments inline.
> >
> > On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> >> From: Ronald Tschalär <ronald@innovation.ch>
> >>
> >> The iBridge device provides access to several devices, including:
> >> - the Touch Bar
> >> - the iSight webcam
> >> - the light sensor
> >> - the fingerprint sensor
> >>
> >> This driver provides the core support for managing the iBridge device
> >> and the access to the underlying devices. In particular, the
> >> functionality for the touch bar and light sensor is exposed via USB HID
> >> interfaces, and on devices with the T1 chip one of the HID devices is
> >> used for both functions. So this driver creates virtual HID devices, one
> >> per top-level report collection on each HID device (for a total of 3
> >> virtual HID devices). The sub-drivers then bind to these virtual HID
> >> devices.
> >>
> >> This way the Touch Bar and ALS drivers can be kept in their own modules,
> >> while at the same time making them look very much like as if they were
> >> connected to the real HID devices. And those drivers then work (mostly)
> >> without further changes on MacBooks with the T2 chip that don't need
> >> this driver.
> >>
> >> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> >> [Kerem Karabay: convert to a platform driver]
> >> [Kerem Karabay: fix appleib_forward_int_op]
> >> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
> >> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> >> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> >> ---
> >> drivers/hid/Kconfig         |  15 +
> >> drivers/hid/Makefile        |   1 +
> >> drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
> >> drivers/hid/apple-ibridge.h |  15 +
> >> drivers/hid/hid-ids.h       |   1 +
> >> drivers/hid/hid-quirks.c    |   3 +
> >> 6 files changed, 645 insertions(+)
> >> create mode 100644 drivers/hid/apple-ibridge.c
> >> create mode 100644 drivers/hid/apple-ibridge.h
> >>
> >
> >>
> >> +}
> >> +
> >> +static int appleib_ll_raw_request(struct hid_device *hdev,
> >> +   unsigned char reportnum, __u8 *buf,
> >> +   size_t len, unsigned char rtype, int reqtype)
> >> +{
> >> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> >> +
> >> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
> >> +   reqtype);
> >> +}
> >> +
> >> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
> >> +     size_t len)
> >> +{
> >> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> >> +
> >> + return hid_hw_output_report(hdev_info->hdev, buf, len);
> >> +}
> >> +
> >> +static struct hid_ll_driver appleib_ll_driver = {
> >> + .start = appleib_ll_start,
> >> + .stop = appleib_ll_stop,
> >> + .open = appleib_ll_open,
> >> + .close = appleib_ll_close,
> >> + .power = appleib_ll_power,
> >> + .parse = appleib_ll_parse,
> >> + .request = appleib_ll_request,
> >> + .wait = appleib_ll_wait,
> >> + .raw_request = appleib_ll_raw_request,
> >> + .output_report = appleib_ll_output_report,
> >> +};
> >
> > const
>
> Are you sure about const here?
>
> >
> >> +
> >> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
> >> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
> >> + return rc ? rc : -ENODEV;
> >> + }
> >> +
> >> + rc = hid_parse(hdev);
> >> + if (rc) {
> >> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> >> + goto error;
> >> + }
> >> +
> >> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> >> + if (rc) {
> >> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> >> + goto error;
> >> + }
> >> +
> >> + hdev_info = appleib_add_device(hdev);
> >> + if (IS_ERR(hdev_info)) {
> >> + rc = PTR_ERR(hdev_info);
> >> + goto stop_hw;
> >> + }
> >> +
> >> + hid_set_drvdata(hdev, hdev_info);
> >> +
> >> + rc = hid_hw_open(hdev);
> >> + if (rc) {
> >> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> >> + goto remove_dev;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +remove_dev:
> >> + appleib_remove_device(hdev);
> >> +stop_hw:
> >> + hid_hw_stop(hdev);
> >> +error:
> >> + return rc;
> >> +}
> >> +
> >> +static void appleib_hid_remove(struct hid_device *hdev)
> >> +{
> >> + hid_hw_close(hdev);
> >> + appleib_remove_device(hdev);
> >> + hid_hw_stop(hdev);
> >> +}
> >> +
> >> +static const struct hid_device_id appleib_hid_ids[] = {
> >> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> >> + { },
> >> +};
> >> +
> >> +static struct hid_driver appleib_hid_driver = {
> >> + .name = "apple-ibridge-hid",
> >> + .id_table = appleib_hid_ids,
> >> + .probe = appleib_hid_probe,
> >> + .remove = appleib_hid_remove,
> >> + .raw_event = appleib_hid_raw_event,
> >> + .report_fixup = appleib_report_fixup,
> >> +#ifdef CONFIG_PM
> >> + .suspend = appleib_hid_suspend,
> >> + .resume = appleib_hid_resume,
> >> + .reset_resume = appleib_hid_reset_resume,
> >> +#endif
> >> +};
> >
> > const
>
> Are you sure you want to do const here, cause other hid-drivers are NOT using const.

It is scheduled for 6.3:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core

So yes, please make them const.

Cheers,
Benjamin

>
> >> +
> >> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
> >> +{
> >> + struct appleib_device *ib_dev;
> >> + acpi_status sts;
>
  

> On 10-Feb-2023, at 2:09 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> 
> On Fri, Feb 10, 2023 at 9:30 AM Aditya Garg <gargaditya08@live.com> wrote:
>> 
>> 
>> 
>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>> 
>>> Hi,
>>> 
>>> some comments inline.
>>> 
>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>>> From: Ronald Tschalär <ronald@innovation.ch>
>>>> 
>>>> The iBridge device provides access to several devices, including:
>>>> - the Touch Bar
>>>> - the iSight webcam
>>>> - the light sensor
>>>> - the fingerprint sensor
>>>> 
>>>> This driver provides the core support for managing the iBridge device
>>>> and the access to the underlying devices. In particular, the
>>>> functionality for the touch bar and light sensor is exposed via USB HID
>>>> interfaces, and on devices with the T1 chip one of the HID devices is
>>>> used for both functions. So this driver creates virtual HID devices, one
>>>> per top-level report collection on each HID device (for a total of 3
>>>> virtual HID devices). The sub-drivers then bind to these virtual HID
>>>> devices.
>>>> 
>>>> This way the Touch Bar and ALS drivers can be kept in their own modules,
>>>> while at the same time making them look very much like as if they were
>>>> connected to the real HID devices. And those drivers then work (mostly)
>>>> without further changes on MacBooks with the T2 chip that don't need
>>>> this driver.
>>>> 
>>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>>>> [Kerem Karabay: convert to a platform driver]
>>>> [Kerem Karabay: fix appleib_forward_int_op]
>>>> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
>>>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>>>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>>>> ---
>>>> drivers/hid/Kconfig         |  15 +
>>>> drivers/hid/Makefile        |   1 +
>>>> drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
>>>> drivers/hid/apple-ibridge.h |  15 +
>>>> drivers/hid/hid-ids.h       |   1 +
>>>> drivers/hid/hid-quirks.c    |   3 +
>>>> 6 files changed, 645 insertions(+)
>>>> create mode 100644 drivers/hid/apple-ibridge.c
>>>> create mode 100644 drivers/hid/apple-ibridge.h
>>>> 
>>> 
>>>> 
>>>> +}
>>>> +
>>>> +static int appleib_ll_raw_request(struct hid_device *hdev,
>>>> +   unsigned char reportnum, __u8 *buf,
>>>> +   size_t len, unsigned char rtype, int reqtype)
>>>> +{
>>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>>>> +
>>>> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
>>>> +   reqtype);
>>>> +}
>>>> +
>>>> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
>>>> +     size_t len)
>>>> +{
>>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>>>> +
>>>> + return hid_hw_output_report(hdev_info->hdev, buf, len);
>>>> +}
>>>> +
>>>> +static struct hid_ll_driver appleib_ll_driver = {
>>>> + .start = appleib_ll_start,
>>>> + .stop = appleib_ll_stop,
>>>> + .open = appleib_ll_open,
>>>> + .close = appleib_ll_close,
>>>> + .power = appleib_ll_power,
>>>> + .parse = appleib_ll_parse,
>>>> + .request = appleib_ll_request,
>>>> + .wait = appleib_ll_wait,
>>>> + .raw_request = appleib_ll_raw_request,
>>>> + .output_report = appleib_ll_output_report,
>>>> +};
>>> 
>>> const
>> 
>> Are you sure about const here?
>> 
>>> 
>>>> +
>>>> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
>>>> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
>>>> + return rc ? rc : -ENODEV;
>>>> + }
>>>> +
>>>> + rc = hid_parse(hdev);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
>>>> + goto error;
>>>> + }
>>>> +
>>>> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
>>>> + goto error;
>>>> + }
>>>> +
>>>> + hdev_info = appleib_add_device(hdev);
>>>> + if (IS_ERR(hdev_info)) {
>>>> + rc = PTR_ERR(hdev_info);
>>>> + goto stop_hw;
>>>> + }
>>>> +
>>>> + hid_set_drvdata(hdev, hdev_info);
>>>> +
>>>> + rc = hid_hw_open(hdev);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
>>>> + goto remove_dev;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +remove_dev:
>>>> + appleib_remove_device(hdev);
>>>> +stop_hw:
>>>> + hid_hw_stop(hdev);
>>>> +error:
>>>> + return rc;
>>>> +}
>>>> +
>>>> +static void appleib_hid_remove(struct hid_device *hdev)
>>>> +{
>>>> + hid_hw_close(hdev);
>>>> + appleib_remove_device(hdev);
>>>> + hid_hw_stop(hdev);
>>>> +}
>>>> +
>>>> +static const struct hid_device_id appleib_hid_ids[] = {
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
>>>> + { },
>>>> +};
>>>> +
>>>> +static struct hid_driver appleib_hid_driver = {
>>>> + .name = "apple-ibridge-hid",
>>>> + .id_table = appleib_hid_ids,
>>>> + .probe = appleib_hid_probe,
>>>> + .remove = appleib_hid_remove,
>>>> + .raw_event = appleib_hid_raw_event,
>>>> + .report_fixup = appleib_report_fixup,
>>>> +#ifdef CONFIG_PM
>>>> + .suspend = appleib_hid_suspend,
>>>> + .resume = appleib_hid_resume,
>>>> + .reset_resume = appleib_hid_reset_resume,
>>>> +#endif
>>>> +};
>>> 
>>> const
>> 
>> Are you sure you want to do const here, cause other hid-drivers are NOT using const.
> 
> It is scheduled for 6.3:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
> 
> So yes, please make them const.

Makes sense for `hid_ll_driver` as per the tree you send me, but I don’t see such a thing for
`static struct hid_driver`. Is that scheduled as well?

> 
> Cheers,
> Benjamin
> 
>> 
>>>> +
>>>> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
>>>> +{
>>>> + struct appleib_device *ib_dev;
>>>> + acpi_status sts;
  

On Fri, Feb 10, 2023 at 9:54 AM Aditya Garg <gargaditya08@live.com> wrote:
>
>
>
> > On 10-Feb-2023, at 2:09 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >
> > On Fri, Feb 10, 2023 at 9:30 AM Aditya Garg <gargaditya08@live.com> wrote:
> >>
> >>
> >>
> >>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >>>
> >>> Hi,
> >>>
> >>> some comments inline.
> >>>
> >>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> >>>> From: Ronald Tschalär <ronald@innovation.ch>
> >>>>
> >>>> The iBridge device provides access to several devices, including:
> >>>> - the Touch Bar
> >>>> - the iSight webcam
> >>>> - the light sensor
> >>>> - the fingerprint sensor
> >>>>
> >>>> This driver provides the core support for managing the iBridge device
> >>>> and the access to the underlying devices. In particular, the
> >>>> functionality for the touch bar and light sensor is exposed via USB HID
> >>>> interfaces, and on devices with the T1 chip one of the HID devices is
> >>>> used for both functions. So this driver creates virtual HID devices, one
> >>>> per top-level report collection on each HID device (for a total of 3
> >>>> virtual HID devices). The sub-drivers then bind to these virtual HID
> >>>> devices.
> >>>>
> >>>> This way the Touch Bar and ALS drivers can be kept in their own modules,
> >>>> while at the same time making them look very much like as if they were
> >>>> connected to the real HID devices. And those drivers then work (mostly)
> >>>> without further changes on MacBooks with the T2 chip that don't need
> >>>> this driver.
> >>>>
> >>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> >>>> [Kerem Karabay: convert to a platform driver]
> >>>> [Kerem Karabay: fix appleib_forward_int_op]
> >>>> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
> >>>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> >>>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> >>>> ---
> >>>> drivers/hid/Kconfig         |  15 +
> >>>> drivers/hid/Makefile        |   1 +
> >>>> drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
> >>>> drivers/hid/apple-ibridge.h |  15 +
> >>>> drivers/hid/hid-ids.h       |   1 +
> >>>> drivers/hid/hid-quirks.c    |   3 +
> >>>> 6 files changed, 645 insertions(+)
> >>>> create mode 100644 drivers/hid/apple-ibridge.c
> >>>> create mode 100644 drivers/hid/apple-ibridge.h
> >>>>
> >>>
> >>>>
> >>>> +}
> >>>> +
> >>>> +static int appleib_ll_raw_request(struct hid_device *hdev,
> >>>> +   unsigned char reportnum, __u8 *buf,
> >>>> +   size_t len, unsigned char rtype, int reqtype)
> >>>> +{
> >>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> >>>> +
> >>>> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
> >>>> +   reqtype);
> >>>> +}
> >>>> +
> >>>> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
> >>>> +     size_t len)
> >>>> +{
> >>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
> >>>> +
> >>>> + return hid_hw_output_report(hdev_info->hdev, buf, len);
> >>>> +}
> >>>> +
> >>>> +static struct hid_ll_driver appleib_ll_driver = {
> >>>> + .start = appleib_ll_start,
> >>>> + .stop = appleib_ll_stop,
> >>>> + .open = appleib_ll_open,
> >>>> + .close = appleib_ll_close,
> >>>> + .power = appleib_ll_power,
> >>>> + .parse = appleib_ll_parse,
> >>>> + .request = appleib_ll_request,
> >>>> + .wait = appleib_ll_wait,
> >>>> + .raw_request = appleib_ll_raw_request,
> >>>> + .output_report = appleib_ll_output_report,
> >>>> +};
> >>>
> >>> const
> >>
> >> Are you sure about const here?
> >>
> >>>
> >>>> +
> >>>> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
> >>>> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
> >>>> + return rc ? rc : -ENODEV;
> >>>> + }
> >>>> +
> >>>> + rc = hid_parse(hdev);
> >>>> + if (rc) {
> >>>> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> >>>> + goto error;
> >>>> + }
> >>>> +
> >>>> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> >>>> + if (rc) {
> >>>> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> >>>> + goto error;
> >>>> + }
> >>>> +
> >>>> + hdev_info = appleib_add_device(hdev);
> >>>> + if (IS_ERR(hdev_info)) {
> >>>> + rc = PTR_ERR(hdev_info);
> >>>> + goto stop_hw;
> >>>> + }
> >>>> +
> >>>> + hid_set_drvdata(hdev, hdev_info);
> >>>> +
> >>>> + rc = hid_hw_open(hdev);
> >>>> + if (rc) {
> >>>> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> >>>> + goto remove_dev;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +
> >>>> +remove_dev:
> >>>> + appleib_remove_device(hdev);
> >>>> +stop_hw:
> >>>> + hid_hw_stop(hdev);
> >>>> +error:
> >>>> + return rc;
> >>>> +}
> >>>> +
> >>>> +static void appleib_hid_remove(struct hid_device *hdev)
> >>>> +{
> >>>> + hid_hw_close(hdev);
> >>>> + appleib_remove_device(hdev);
> >>>> + hid_hw_stop(hdev);
> >>>> +}
> >>>> +
> >>>> +static const struct hid_device_id appleib_hid_ids[] = {
> >>>> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> >>>> + { },
> >>>> +};
> >>>> +
> >>>> +static struct hid_driver appleib_hid_driver = {
> >>>> + .name = "apple-ibridge-hid",
> >>>> + .id_table = appleib_hid_ids,
> >>>> + .probe = appleib_hid_probe,
> >>>> + .remove = appleib_hid_remove,
> >>>> + .raw_event = appleib_hid_raw_event,
> >>>> + .report_fixup = appleib_report_fixup,
> >>>> +#ifdef CONFIG_PM
> >>>> + .suspend = appleib_hid_suspend,
> >>>> + .resume = appleib_hid_resume,
> >>>> + .reset_resume = appleib_hid_reset_resume,
> >>>> +#endif
> >>>> +};
> >>>
> >>> const
> >>
> >> Are you sure you want to do const here, cause other hid-drivers are NOT using const.
> >
> > It is scheduled for 6.3:
> > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
> >
> > So yes, please make them const.
>
> Makes sense for `hid_ll_driver` as per the tree you send me, but I don’t see such a thing for
> `static struct hid_driver`. Is that scheduled as well?

Good point. I don't recall having seen such series now that you pinpoint this.

Thomas, is this something you have planned to submit?

Cheers,
Benjamin

>
> >
> > Cheers,
> > Benjamin
> >
> >>
> >>>> +
> >>>> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
> >>>> +{
> >>>> + struct appleib_device *ib_dev;
> >>>> + acpi_status sts;
>
>
  

> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> 
> Hi,
> 
> some comments inline.
> 
> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> 
>> +
>> +static struct {
>> + unsigned int usage;
>> + struct hid_device_id *dev_id;
>> +} appleib_usage_map[] = {
>> + /* Default iBridge configuration, key inputs and mode settings */
>> + { 0x00010006, &appleib_sub_hid_ids[0] },
>> + /* OS X iBridge configuration, digitizer inputs */
>> + { 0x000D0005, &appleib_sub_hid_ids[0] },
>> + /* All iBridge configurations, display/DFR settings */
>> + { 0xFF120001, &appleib_sub_hid_ids[0] },
>> + /* All iBridge configurations, ALS */
>> + { 0x00200041, &appleib_sub_hid_ids[1] },
>> +};
> 
> const
> 

Constantifying this results in compiler giving warnings

drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
   78 |         { 0x00200041, &appleib_sub_hid_ids[1] },
      |                       ^
drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  363 |         sub_hdev->ll_driver = &appleib_ll_driver;
      |                             ^
drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
  436 |         if hid_is_usb(hdev)
      |            ^~~~~~~~~~
      |            (
In file included from drivers/hid/apple-ibridge.c:48:
drivers/hid/apple-ibridge.c: In function 'appleib_probe':
drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  544 |         ret = hid_register_driver(&appleib_hid_driver);
      |                                   ^~~~~~~~~~~~~~~~~~~
./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
  898 |         __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
      |                               ^~~~~~
./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
  893 | extern int __must_check __hid_register_driver(struct hid_driver *,
      |                                               ^~~~~~~~~~~~~~~~~~~
drivers/hid/apple-ibridge.c: In function 'appleib_remove':
drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  558 |         hid_unregister_driver(&appleib_hid_driver);
      |                               ^~~~~~~~~~~~~~~~~~~
./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
  900 | extern void hid_unregister_driver(struct hid_driver *);
      |                                   ^~~~~~~~~~~~~~~~~~~
make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
make[5]: *** Waiting for unfinished jobs….

Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
be fixed.

In short, Thomas, do you really want me to constantify the structure I am talking about in this email, as well `static struct hid_driver`?
  

On Fri, 10 Feb 2023 12:05:13 +0000
Aditya Garg <gargaditya08@live.com> wrote:

> > On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de>
> > wrote:
> > 
> > Hi,
> > 
> > some comments inline.
> > 
> > On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> >   
> >> +
> >> +static struct {
> >> + unsigned int usage;
> >> + struct hid_device_id *dev_id;
> >> +} appleib_usage_map[] = {
> >> + /* Default iBridge configuration, key inputs and mode settings */
> >> + { 0x00010006, &appleib_sub_hid_ids[0] },
> >> + /* OS X iBridge configuration, digitizer inputs */
> >> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, display/DFR settings */
> >> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, ALS */
> >> + { 0x00200041, &appleib_sub_hid_ids[1] },
> >> +};  
> > 
> > const
> >   
> 
> Constantifying this results in compiler giving warnings
> 
> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards
> 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 78 |         { 0x00200041, &appleib_sub_hid_ids[1] }, |
>         ^ drivers/hid/apple-ibridge.c: In function
> 'appleib_add_sub_dev': drivers/hid/apple-ibridge.c:363:29: warning:
> assignment discards 'const' qualifier from pointer target type
> [-Wdiscarded-qualifiers] 363 |         sub_hdev->ll_driver =
> &appleib_ll_driver; |                             ^
> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before
> 'hid_is_usb' 436 |         if hid_is_usb(hdev) |            ^~~~~~~~~~
>       |            (
> In file included from drivers/hid/apple-ibridge.c:48:
> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of
> '__hid_register_driver' discards 'const' qualifier from pointer
> target type [-Wdiscarded-qualifiers] 544 |         ret =
> hid_register_driver(&appleib_hid_driver); |
>         ^~~~~~~~~~~~~~~~~~~ ./include/linux/hid.h:898:31: note: in
> definition of macro 'hid_register_driver' 898 |
> __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME) |
>                      ^~~~~~ ./include/linux/hid.h:893:47: note:
> expected 'struct hid_driver *' but argument is of type 'const struct
> hid_driver *' 893 | extern int __must_check
> __hid_register_driver(struct hid_driver *, |
>                      ^~~~~~~~~~~~~~~~~~~ drivers/hid/apple-ibridge.c:
> In function 'appleib_remove': drivers/hid/apple-ibridge.c:558:31:
> warning: passing argument 1 of 'hid_unregister_driver' discards
> 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 558 |         hid_unregister_driver(&appleib_hid_driver); |
>                     ^~~~~~~~~~~~~~~~~~~ ./include/linux/hid.h:900:35:
> note: expected 'struct hid_driver *' but argument is of type 'const
> struct hid_driver *' 900 | extern void hid_unregister_driver(struct
> hid_driver *); |
> ^~~~~~~~~~~~~~~~~~~ make[6]: *** [scripts/Makefile.build:250:
> drivers/hid/apple-ibridge.o] Error 1 make[5]: ***
> [scripts/Makefile.build:500: drivers/hid] Error 2 make[5]: ***
> Waiting for unfinished jobs….
> 
> Some warnings are also due to a typo in if and constantifying `static
> struct hid_driver`, although they probably can be fixed.
> 
> In short, Thomas, do you really want me to constantify the structure
> I am talking about in this email, as well `static struct hid_driver`?
> 

Were the changes needed for these structs to be const in the
linux-input tree for 6.3? If so then if you're applying the patches
onto linus' tree that might be why there are errors about consts.
  

> Were the changes needed for these structs to be const in the
> linux-input tree for 6.3? If so then if you're applying the patches
> onto linus' tree that might be why there are errors about consts.

I’d want the maintainers comment on this. Imo, these 2 structures needn’t be constantified.

Also, it would be nice if we could get a review on the other 2 patches, so that a v2 can be prepared.
  

On Feb 10 2023, Aditya Garg wrote:
> 
> > Were the changes needed for these structs to be const in the
> > linux-input tree for 6.3? If so then if you're applying the patches
> > onto linus' tree that might be why there are errors about consts.
> 
> I’d want the maintainers comment on this. Imo, these 2 structures needn’t be constantified.

The struct hid_ll_driver has to be constified, because otherwise it will
introduce an error/warning when this patch is merged in the hid tree.

For the struct hid_driver, as mentioned previously I don't think we have
the hid-core changes for that, and so you can't really constify them.

Cheers,
Benjamin

> 
> Also, it would be nice if we could get a review on the other 2 patches, so that a v2 can be prepared.
  

Responses inline

On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
> > On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> > 
> > Hi,
> > 
> > some comments inline.
> > 
> > On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> > 
> >> +
> >> +static struct {
> >> + unsigned int usage;
> >> + struct hid_device_id *dev_id;
> >> +} appleib_usage_map[] = {
> >> + /* Default iBridge configuration, key inputs and mode settings */
> >> + { 0x00010006, &appleib_sub_hid_ids[0] },
> >> + /* OS X iBridge configuration, digitizer inputs */
> >> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, display/DFR settings */
> >> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, ALS */
> >> + { 0x00200041, &appleib_sub_hid_ids[1] },
> >> +};
> > 
> > const
> > 
> 
> Constantifying this results in compiler giving warnings
> 
> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>    78 |         { 0x00200041, &appleib_sub_hid_ids[1] },

For this you also have to constify the hid_device_id *dev_id in
appleib_usage_map. And then propagate this change to some functions and
variables.

>       |                       ^
> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   363 |         sub_hdev->ll_driver = &appleib_ll_driver;

As Benjamin said this is because your changes are based on Linus' tree
but they will break as soon as they will be merged into the HID tree.
You should base your changes off of the HID tree:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core

This issue is essentially unlucky timing.

>       |                             ^
> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>   436 |         if hid_is_usb(hdev)
>       |            ^~~~~~~~~~
>       |            (

As the error message indicates, this is invalid syntax and missing a
'('.
What you want to do is to check for 

  if (!hid_is_usb(hdev))
    return -ENODEV;

*before* calling hid_to_usb_dev(hdev);

> In file included from drivers/hid/apple-ibridge.c:48:
> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   544 |         ret = hid_register_driver(&appleib_hid_driver);
>       |                                   ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>   898 |         __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>       |                               ^~~~~~
> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>   893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>       |                                               ^~~~~~~~~~~~~~~~~~~
> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   558 |         hid_unregister_driver(&appleib_hid_driver);
>       |                               ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>   900 | extern void hid_unregister_driver(struct hid_driver *);
>       |                                   ^~~~~~~~~~~~~~~~~~~

These are all because applib_hid_driver can not be const.
Sorry for the wrong advice.

Benjamin:
HID drivers can not be const because they embed a 'struct driver' that
is needed by the driver core to be mutable.
Fixing this is probably a larger enterprise.

> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
> make[5]: *** Waiting for unfinished jobs….
> 
> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
> be fixed.
> 
> In short, Thomas, do you really want me to constantify the structure I
> am talking about in this email, as well `static struct hid_driver`?

struct hid_driver: Don't constify
all others: Do constify

Thomas
  

> On 10-Feb-2023, at 9:04 PM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> 
> Responses inline
> 
> On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
>>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>> 
>>> Hi,
>>> 
>>> some comments inline.
>>> 
>>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>> 
>>>> +
>>>> +static struct {
>>>> + unsigned int usage;
>>>> + struct hid_device_id *dev_id;
>>>> +} appleib_usage_map[] = {
>>>> + /* Default iBridge configuration, key inputs and mode settings */
>>>> + { 0x00010006, &appleib_sub_hid_ids[0] },
>>>> + /* OS X iBridge configuration, digitizer inputs */
>>>> + { 0x000D0005, &appleib_sub_hid_ids[0] },
>>>> + /* All iBridge configurations, display/DFR settings */
>>>> + { 0xFF120001, &appleib_sub_hid_ids[0] },
>>>> + /* All iBridge configurations, ALS */
>>>> + { 0x00200041, &appleib_sub_hid_ids[1] },
>>>> +};
>>> 
>>> const
>>> 
>> 
>> Constantifying this results in compiler giving warnings
>> 
>> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>   78 |         { 0x00200041, &appleib_sub_hid_ids[1] },
> 
> For this you also have to constify the hid_device_id *dev_id in
> appleib_usage_map. And then propagate this change to some functions and
> variables.
> 
>>      |                       ^
>> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
>> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>  363 |         sub_hdev->ll_driver = &appleib_ll_driver;
> 
> As Benjamin said this is because your changes are based on Linus' tree
> but they will break as soon as they will be merged into the HID tree.
> You should base your changes off of the HID tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
> 
> This issue is essentially unlucky timing.
> 
>>      |                             ^
>> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
>> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>>  436 |         if hid_is_usb(hdev)
>>      |            ^~~~~~~~~~
>>      |            (
> 
> As the error message indicates, this is invalid syntax and missing a
> '('.
> What you want to do is to check for 
> 
>  if (!hid_is_usb(hdev))
>    return -ENODEV;

It was a typo on my part

+	/* check and set usb config first */
+	if (hid_is_usb(hdev))
+		udev = hid_to_usb_dev(hdev);
+	else
+		return -EINVAL;

This is what I have in my patch set now.

If there is something wrong with this, then do tell me

Thanks
> 
> *before* calling hid_to_usb_dev(hdev);
> 
>> In file included from drivers/hid/apple-ibridge.c:48:
>> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
>> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>  544 |         ret = hid_register_driver(&appleib_hid_driver);
>>      |                                   ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>>  898 |         __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>>      |                               ^~~~~~
>> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>  893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>>      |                                               ^~~~~~~~~~~~~~~~~~~
>> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
>> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>  558 |         hid_unregister_driver(&appleib_hid_driver);
>>      |                               ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>  900 | extern void hid_unregister_driver(struct hid_driver *);
>>      |                                   ^~~~~~~~~~~~~~~~~~~
> 
> These are all because applib_hid_driver can not be const.
> Sorry for the wrong advice.
> 
> Benjamin:
> HID drivers can not be const because they embed a 'struct driver' that
> is needed by the driver core to be mutable.
> Fixing this is probably a larger enterprise.
> 
>> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
>> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
>> make[5]: *** Waiting for unfinished jobs….
>> 
>> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
>> be fixed.
>> 
>> In short, Thomas, do you really want me to constantify the structure I
>> am talking about in this email, as well `static struct hid_driver`?
> 
> struct hid_driver: Don't constify
> all others: Do constify
> 
> Thomas
  

Feb 10, 2023 08:50:12 Aditya Garg <gargaditya08@live.com>:

>
>
>> On 10-Feb-2023, at 9:04 PM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>
>> Responses inline
>>
>> On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
>>>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> some comments inline.
>>>>
>>>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>>>
>>>>> +
>>>>> +static struct {
>>>>> + unsigned int usage;
>>>>> + struct hid_device_id *dev_id;
>>>>> +} appleib_usage_map[] = {
>>>>> + /* Default iBridge configuration, key inputs and mode settings */
>>>>> + { 0x00010006, &appleib_sub_hid_ids[0] },
>>>>> + /* OS X iBridge configuration, digitizer inputs */
>>>>> + { 0x000D0005, &appleib_sub_hid_ids[0] },
>>>>> + /* All iBridge configurations, display/DFR settings */
>>>>> + { 0xFF120001, &appleib_sub_hid_ids[0] },
>>>>> + /* All iBridge configurations, ALS */
>>>>> + { 0x00200041, &appleib_sub_hid_ids[1] },
>>>>> +};
>>>>
>>>> const
>>>>
>>>
>>> Constantifying this results in compiler giving warnings
>>>
>>> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>>   78 |         { 0x00200041, &appleib_sub_hid_ids[1] },
>>
>> For this you also have to constify the hid_device_id *dev_id in
>> appleib_usage_map. And then propagate this change to some functions and
>> variables.
>>
>>>      |                       ^
>>> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
>>> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 363 |         sub_hdev->ll_driver = &appleib_ll_driver;
>>
>> As Benjamin said this is because your changes are based on Linus' tree
>> but they will break as soon as they will be merged into the HID tree.
>> You should base your changes off of the HID tree:
>> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
>>
>> This issue is essentially unlucky timing.
>>
>>>      |                             ^
>>> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
>>> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>>> 436 |         if hid_is_usb(hdev)
>>>      |            ^~~~~~~~~~
>>>      |            (
>>
>> As the error message indicates, this is invalid syntax and missing a
>> '('.
>> What you want to do is to check for
>>
>> if (!hid_is_usb(hdev))
>>    return -ENODEV;
>
> It was a typo on my part
>
> +   /* check and set usb config first */
> +   if (hid_is_usb(hdev))
> +       udev = hid_to_usb_dev(hdev);
> +   else
> +       return -EINVAL;

I would prefer

if (!hid_is_usb(hdev))
    return -EINVAL;

udev = hid_to_usb_dev(hdev);

>
> This is what I have in my patch set now.
>
> If there is something wrong with this, then do tell me
>
> Thanks
>>
>> *before* calling hid_to_usb_dev(hdev);
>>
>>> In file included from drivers/hid/apple-ibridge.c:48:
>>> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
>>> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 544 |         ret = hid_register_driver(&appleib_hid_driver);
>>>      |                                   ^~~~~~~~~~~~~~~~~~~
>>> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>>> 898 |         __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>>>      |                               ^~~~~~
>>> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>> 893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>>>      |                                               ^~~~~~~~~~~~~~~~~~~
>>> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
>>> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 558 |         hid_unregister_driver(&appleib_hid_driver);
>>>      |                               ^~~~~~~~~~~~~~~~~~~
>>> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>> 900 | extern void hid_unregister_driver(struct hid_driver *);
>>>      |                                   ^~~~~~~~~~~~~~~~~~~
>>
>> These are all because applib_hid_driver can not be const.
>> Sorry for the wrong advice.
>>
>> Benjamin:
>> HID drivers can not be const because they embed a 'struct driver' that
>> is needed by the driver core to be mutable.
>> Fixing this is probably a larger enterprise.
>>
>>> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
>>> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
>>> make[5]: *** Waiting for unfinished jobs….
>>>
>>> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
>>> be fixed.
>>>
>>> In short, Thomas, do you really want me to constantify the structure I
>>> am talking about in this email, as well `static struct hid_driver`?
>>
>> struct hid_driver: Don't constify
>> all others: Do constify
>>
>> Thomas
  

On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@innovation.ch>
> 
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
> 
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and on devices with the T1 chip one of the HID devices is
> used for both functions. So this driver creates virtual HID devices, one
> per top-level report collection on each HID device (for a total of 3
> virtual HID devices). The sub-drivers then bind to these virtual HID
> devices.
> 
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices. And those drivers then work (mostly)
> without further changes on MacBooks with the T2 chip that don't need
> this driver.

...

> [Kerem Karabay: convert to a platform driver]
> [Kerem Karabay: fix appleib_forward_int_op]
> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]

If somebody is going to update this (and update seems required for upstreaming)
the list of changes will grow. I suggest to consider Co-developed-by and move
these lines to cover-letter changelog.

> Signed-off-by: Kerem Karabay <kekrby@gmail.com>

...

> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>

Can we keep it sorted?

> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"

+ Blank line?

> +#include "apple-ibridge.h"

...

> +static struct hid_device_id appleib_sub_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_TB) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> +			 USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +static struct {
> +	unsigned int usage;
> +	struct hid_device_id *dev_id;
> +} appleib_usage_map[] = {
> +	/* Default iBridge configuration, key inputs and mode settings */
> +	{ 0x00010006, &appleib_sub_hid_ids[0] },
> +	/* OS X iBridge configuration, digitizer inputs */
> +	{ 0x000D0005, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, display/DFR settings */
> +	{ 0xFF120001, &appleib_sub_hid_ids[0] },
> +	/* All iBridge configurations, ALS */
> +	{ 0x00200041, &appleib_sub_hid_ids[1] },
> +};

Shouldn't be other way around, i.e. via driver_data?

...

> +struct appleib_device {
> +	acpi_handle asoc_socw;
> +};

What's the point of having struct out of a single member? Can you use it directly?
(you can try and see if it's not ugly, in some cases struct can be justified)

...

> +	bool			sub_open[ARRAY_SIZE(appleib_sub_hid_ids)];

Why not using bitmap?

	DECLARE_BITMAP(sub_open, ARRAY_SIZE(...));

...

> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> +				  unsigned int *rsize)

Why __ types are in use? Is it part of ABI?

...

> +static int appleib_forward_int_op(struct hid_device *hdev,

> +				  int (*forward)(struct hid_driver *,
> +						 struct hid_device *, void *),

This can be on one line

> +				  void *args)

...

> +	if (drv->suspend)
> +		rc = drv->suspend(hdev, *(pm_message_t *)args);

This looks like a hack. What's going on here and why the pm_message_t is in
use? All new PM callbacks do not use it.

...

> +	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> +		/*
> +		 * hid_hw_open(), and hence appleib_ll_open(), is called
> +		 * from the driver's probe function, which in turn is called
> +		 * while adding the sub-hdev; but at this point we haven't yet
> +		 * added the sub-hdev to our list. So if we don't find the
> +		 * sub-hdev in our list assume it's in the process of being
> +		 * added and set the flag on the first unset sub-hdev.
> +		 */
> +		if (hdev_info->sub_hdevs[i] == hdev ||
> +		    !hdev_info->sub_hdevs[i]) {

Unusual order of || operator arguments.

This will have a side effect, i.e. if hdev is equal to NULL it will go to the
true branch. Is it by design?

> +			WRITE_ONCE(hdev_info->sub_open[i], open);
> +			return 0;
> +		}
> +	}

...

> +				while (i-- > 0)

while (i--) ?

> +					hid_destroy_device(hdev_info->sub_hdevs[i]);

> +				return (void *)hdev_info->sub_hdevs[i];

This casting is strange. And entire code piece. You will always return 0
element as a pointer here, why 'i'? Needs a lot of explanation.

...

> +static const struct hid_device_id appleib_hid_ids[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },

> +	{ },

No comma for the terminator entry.

> +};

...

> +#ifdef CONFIG_PM
> +	.suspend = appleib_hid_suspend,
> +	.resume = appleib_hid_resume,
> +	.reset_resume = appleib_hid_reset_resume,
> +#endif

Why not using

	.driver = {
		.pm = ...;
	},

?

...

> +	ret = hid_register_driver(&appleib_hid_driver);
> +	if (ret) {

> +		dev_err(&pdev->dev, "Error registering hid driver: %d\n",
> +			ret);
> +		return ret;

	return dev_err_probe(...);

> +	}

...

> +static int appleib_suspend(struct platform_device *pdev, pm_message_t message)
> +{
> +	struct appleib_device *ib_dev;
> +	int rc;

> +	ib_dev = platform_get_drvdata(pdev);

Just unite it with the definition above.
Ditto for the similar cases here and there.

> +	rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> +	if (ACPI_FAILURE(rc))
> +		dev_warn(&pdev->dev, "SOCW(0) failed: %s\n",
> +			 acpi_format_exception(rc));
> +
> +	return 0;
> +}

...

> +static const struct acpi_device_id appleib_acpi_match[] = {
> +	{ "APP7777", 0 },
> +	{ },

No comma for terminator entry.

> +};