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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
> Support describing the CP2112's I2C and GPIO interfaces in firmware.
> 
> I2C and GPIO child nodes can either be children with names "i2c" and
> "gpio", or, for ACPI, device nodes with _ADR Zero and One,
> respectively.
> 
> Additionally, support configuring the I2C bus speed from the
> clock-frequency device property.

Thank you for the update, my comments below.

...

> +	struct i2c_timings timings;
> +	struct fwnode_handle *child;

Longer line first easier to read.

> +	u32 addr;
> +	const char *name;

Ditto.

...

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

-- 
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux