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", ®);
> + ret = fwnode_property_read_u32(child, "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", ®);
> + ret = fwnode_property_read_u32(np, "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
