Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Danny Kaehn <kaehndan@gmail.com>
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	jikos@kernel.org, benjamin.tissoires@redhat.com,
	bartosz.golaszewski@linaro.org, dmitry.torokhov@gmail.com,
	devicetree@vger.kernel.org, linux-input@vger.kernel.org,
	ethan.twardy@plexus.com
Subject: Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support
Date: Mon, 20 Mar 2023 15:00:20 +0200	[thread overview]
Message-ID: <ZBhY5Gp88gVK7q/g@smile.fi.intel.com> (raw)
In-Reply-To: <ZBhYXwjPeRiZwxMT@smile.fi.intel.com>

On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:

...

> > +	device_for_each_child_node(&hdev->dev, child) {
> > +		name = fwnode_get_name(child);
> > +		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +
> > +		if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > +			device_set_node(&dev->adap.dev, child);
> > +		else if ((name && strcmp("gpio", name)) == 0 ||
> > +					(!ret && addr == 1))
> > +			dev->gc.fwnode = child;
> > +	}
> 
> Please, make addresses defined explicitly. You may also do it with node naming
> schema:
> 
> #define CP2112_I2C_ADR		0
> #define CP2112_GPIO_ADR		1
> 
> static const char * const cp2112_cell_names[] = {
> 	[CP2112_I2C_ADR]	= "i2c",
> 	[CP2112_GPIO_ADR]	= "gpio",
> };
> 
> 	device_for_each_child_node(&hdev->dev, child) {
> 		name = fwnode_get_name(child);
> 		if (name) {
> 			ret = match_string(cp2112_cell_names, ARRAY_SIZE(cp2112_cell_names), name);
> 			if (ret >= 0)
> 				addr = ret;
> 		} else
> 			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> 		if (ret < 0)
> 			...error handling if needed...
> 
> 		switch (addr) {
> 		case CP2112_I2C_ADR:
> 			device_set_node(&dev->adap.dev, child);
> 			break;
> 		case CP2112_GPIO_ADR:
> 			dev->gc.fwnode = child;
> 			break;
> 		default:
> 			...error handling...
> 		}
> 	}

Btw, don't you use "reg" property for the child nodes? It would be better from
de facto used patterns (we have a couple of mode drivers that have a common
code to read "reg" or _ADR() and that code can be split into a helper and used
here).

-- 
With Best Regards,
Andy Shevchenko