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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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