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] [day] [month] [year] [list]

Message-ID: <aXxjYtiZnSX-wEUh@smile.fi.intel.com>
Date: Fri, 30 Jan 2026 09:53:06 +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 Thu, Jan 29, 2026 at 12:36:50PM -0600, Danny Kaehn wrote:
> On Tue, Jan 27, 2026 at 10:06:27PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 27, 2026 at 08:47:49AM -0600, Danny Kaehn wrote:

...

> > I'm wondering if we can avoid this (additional) check and use the result of one
> > of the branches.
> 
> Meaning something like using the result of acpi_get_local_address() to
> determine whether the node is ACPI vs. not? That is what it used to do,
> before I needed to switch to different schemas for DT vs. ACPI. Now, it
> doesn't really make sense to use the child node types to determine
> whether the GPIO node is shared, but still possible if we store a bool
> result from the *_for_each_child_node() loop, but needs more complex
> logic to store that based on each child's type (and the loop is fully
> unnecessary for the non-ACPI case anyways).
> 
> Following the discussion on the DT binding thread, do you still want
> ACPI to follow this different schema with the separate GPIO child node,
> or would you prefer to unify them?

Wouldn't it be a bit messy if we combine main Device object with the GPIO
and leave I²C as a separate node? Besides that it seems already established
practice to have GPIO + I²C controllers separated based on _ADR (see Intel
Galileo case, drivers/mfd/intel_quark_i2c_gpio.c). Even if we can combine
I prefer to use the existing schema to have less animals in the zoo, for
the consistency's sake.

...

> > > +		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()
> 
> 
> Makes sense, will update. I initially assumed we wanted to use the
> "device_*" API wherever possible.

Yes, but use a common sense. If we have fwnode already available, why should we
still use device_*()?

> > > +			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)) {
> 
> Wouldn't it be valid to use software nodes to describe the
> CP2112's functions? Is there any reason to intentionally prevent that?

swnode:s are for quirks. I hope in this case we won't see them IRL.
In any case, let's enable them when we will have the case.

> > > +		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