Re: [PATCH v4 2/2] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Andy Shevchenko <andy.shevchenko@gmail.com>]

Wed, Mar 06, 2024 at 10:58:01AM +0800, Kate Hsuan kirjoitti:
> This LED controller also installed on a Xiaomi pad2 and it is a x86
> platform. The original driver is based on device tree and can't be
> used for this ACPI based system. This patch migrated the driver to
> use fwnode to access the properties. Moreover, the fwnode API
> supports device tree so this work won't effect the original
> implementations.

...

> +	fwnode_for_each_available_child_node(np, child) {

Please, rename np to fwnode to avoid confusion.

> +		num_channels++;
> +	}

...

> -	for_each_available_child_of_node(np, child) {
> +	fwnode_for_each_available_child_node(np, child) {
>  		u32 mono_color;
>  		u32 reg;
>  		int ret;
>  
> -		ret = of_property_read_u32(child, "reg", &reg);
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>  		if (ret != 0 || reg >= chip->num_leds) {
>  			dev_err(chip->dev, "invalid 'reg' of %pOFn\n", child);

Must be %pfw now.

> -			of_node_put(child);
> +			fwnode_handle_put(child);

>  			return -EINVAL;

Side note: This shouldn't shadow error code when ret != 0.

>  		}

...

> -		ret = of_property_read_u32(child, "color", &mono_color);
> +		ret = fwnode_property_read_u32(child, "color", &mono_color);
>  		if (ret < 0 && ret != -EINVAL) {
>  			dev_err(chip->dev, "failed to parse 'color' of %pOF\n", child);

Must be %pfw now.

> -			of_node_put(child);
> +			fwnode_handle_put(child);
>  			return ret;
>  		}

...

> -	ret = of_property_read_u32(np, "reg", &reg);
> +	ret = fwnode_property_read_u32(np, "reg", &reg);
>  	if (ret != 0 || reg >= chip->num_leds) {
>  		dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);

Must be %pfw now.

>  		return -EINVAL;

>  	/* Color property is optional in single color case */
> -	ret = of_property_read_u32(np, "color", &color);
> +	ret = fwnode_property_read_u32(np, "color", &color);
>  	if (ret < 0 && ret != -EINVAL) {
>  		dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);

Must be %pfw now.

>  		return ret;
>  	}

...

> +	struct fwnode_handle *child, *np;

Do not use np for sturct fwnode_handle. It will be quite confusing.

...

> -	chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
> +	count = device_get_child_node_count(dev);

>  

Redundant blank line.

> -	count = of_get_available_child_count(np);
>  	if (!count || count > chip->num_leds)
>  		return -EINVAL;

...

> +	chip->num_leds = (unsigned long)i2c_get_match_data(client);

No warnings during compilation?

...

> +static const struct i2c_device_id ktd202x_id[] = {
> +	{"ktd2026", KTD2026_NUM_LEDS},
> +	{"ktd2027", KTD2027_NUM_LEDS},
> +	{},

N ocomma for the terminator entry.

> +};
> +MODULE_DEVICE_TABLE(i2c, ktd202x_id);

...

> +#ifndef CONFIG_ACPI

Please, no. Drop them.

>  static const struct of_device_id ktd202x_match_table[] = {
>  	{ .compatible = "kinetic,ktd2026", .data = (void *)KTD2026_NUM_LEDS },
>  	{ .compatible = "kinetic,ktd2027", .data = (void *)KTD2027_NUM_LEDS },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ktd202x_match_table);
> +#endif
>  
>  static struct i2c_driver ktd202x_driver = {
>  	.driver = {
>  		.name = "leds-ktd202x",
> +#ifndef CONFIG_ACPI
>  		.of_match_table = ktd202x_match_table,
> +#endif

This is quite unusual besides being ugly.

>  	},
>  	.probe = ktd202x_probe,
>  	.remove = ktd202x_remove,
>  	.shutdown = ktd202x_shutdown,
> +	.id_table = ktd202x_id,
>  };

-- 
With Best Regards,
Andy Shevchenko