linux-kernel - Re: [PATCH v13 2/3] HID: cp2112: Fwnode Support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aXkawwpmLW7m4Fu8@smile.fi.intel.com>
Date: Tue, 27 Jan 2026 22:06:27 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Danny Kaehn <danny.kaehn@...xus.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
	Andi Shyti <andi.shyti@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	devicetree@...r.kernel.org, linux-input@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Ethan Twardy <ethan.twardy@...xus.com>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, Leo Huang <leohu@...dia.com>,
	Arun D Patil <arundp@...dia.com>, Willie Thai <wthai@...dia.com>,
	Ting-Kai Chen <tingkaic@...dia.com>
Subject: Re: [PATCH v13 2/3] HID: cp2112: Fwnode Support

On Tue, Jan 27, 2026 at 08:47:49AM -0600, Danny Kaehn wrote:
> Support describing the CP2112's I2C and GPIO interfaces in firmware.
> 
> Bindings between the firmware nodes and the functions of the device
> are distinct between ACPI and DeviceTree.
> 
> For ACPI, the i2c_adapter will use the child with _ADR Zero and the

_ADR equals to Zero

> gpio_chip will use the child with _ADR One. For DeviceTree, the

_ADR equals to One

> i2c_adapter will use the child with name "i2c", but the gpio_chip
> will share a firmware node with the CP2112.

...

Also it's interesting choice of capital letters in the Subject.

I would expect "...: Add fwnode support"

...

> +/**
> + * enum cp2112_child_acpi_cell_addrs - Child ACPI addresses for CP2112 sub-functions
> + * @CP2112_I2C_ADR: Address for I2C node
> + * @CP2112_GPIO_ADR: Address for GPIO node

Probably you want to mention in the description of the enum (here) that
the assigned values are explicit since this is basically a protocol between
FW and OS. That's why we may not change this values without breaking
older firmware descriptions.

> + */
> +enum cp2112_child_acpi_cell_addrs {
> +	CP2112_I2C_ADR = 0,
> +	CP2112_GPIO_ADR = 1,
> +};

...

> +	if (is_acpi_device_node(dev_fwnode(&hdev->dev))) {

I'm wondering if we can avoid this (additional) check and use the result of one
of the branches.

> +		device_for_each_child_node(&hdev->dev, child) {

If we are still use the above check it will be dev_fwnode() duplication call,
so perhaps a temporary variable to collect the device's fwnode and use it
there, below (see below), and here as for

		fwnode_for_each_child_node()

> +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +			if (ret)
> +				continue;
> +
> +			switch (addr) {
> +			case CP2112_I2C_ADR:
> +				device_set_node(&dev->adap.dev, child);
> +				break;
> +			case CP2112_GPIO_ADR:
> +				dev->gc.fwnode = child;
> +				break;
> +			}
> +		}
> +	} else {

I would still check if this is a proper (OF) node, in case we stick with the
ACPI check above. Because we might have swnode and if it triggers, it will be
really something unexpected.

	} else if (is_of_node(fwnode)) {


> +		child = device_get_named_child_node(&hdev->dev, "i2c");
> +		device_set_node(&dev->adap.dev, child);
> +		fwnode_handle_put(child);
> +	}

-- 
With Best Regards,
Andy Shevchenko