Hi, Thank you for your review. Sorry for the late reply. I came back from holiday. On Thu, Feb 8, 2024 at 5:03 AM André Apitzsch <git@xxxxxxxxxxx> wrote: > > Hi, > > Am Mittwoch, dem 07.02.2024 um 14:10 +0800 schrieb Kate Hsuan: > > This LED controller also installed on a Xiaomi pad2 and it is an x86 > > platform. The original driver is based on the 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 would affect the original > would -> won't ? won't > > > implementations. > > > > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx> > > --- > > drivers/leds/rgb/Kconfig | 1 - > > drivers/leds/rgb/leds-ktd202x.c | 68 ++++++++++++++++++++++--------- > > -- > > 2 files changed, 45 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig > > index a6a21f564673..f245dbd9a163 100644 > > --- a/drivers/leds/rgb/Kconfig > > +++ b/drivers/leds/rgb/Kconfig > > @@ -17,7 +17,6 @@ config LEDS_GROUP_MULTICOLOR > > config LEDS_KTD202X > > tristate "LED support for KTD202x Chips" > > depends on I2C > > - depends on OF > > select REGMAP_I2C > > help > > This option enables support for the Kinetic > > KTD2026/KTD2027 > > diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds- > > ktd202x.c > > index 514965795a10..a1aef62e3db5 100644 > > --- a/drivers/leds/rgb/leds-ktd202x.c > > +++ b/drivers/leds/rgb/leds-ktd202x.c > > @@ -112,6 +112,8 @@ static int ktd202x_chip_disable(struct ktd202x > > *chip) > > > > regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, > > KTD202X_ENABLE_CTRL_SLEEP); > > > > + usleep_range(500, 600); > Why have you added the sleep? When removing the module, a WARN_ON() shown as the following URL will be triggered. It happens randomly. I tried to mitigate it through a sleep. https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2396 > > > + > > ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), > > chip->regulators); > > if (ret) { > > dev_err(chip->dev, "Failed to disable regulators: > > %d\n", ret); > > @@ -381,16 +383,18 @@ static int ktd202x_blink_mc_set(struct > > led_classdev *cdev, > > mc->num_colors); > > } > > > > -static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct > > device_node *np, > > +static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct > > fwnode_handle *np, > > struct ktd202x_led *led, struct > > led_init_data *init_data) > > { > > struct led_classdev *cdev; > > - struct device_node *child; > > + struct fwnode_handle *child; > Nit, swap the above lines for reverse Christmas tree. okay > > > struct mc_subled *info; > > - int num_channels; > > + int num_channels = 0; > > int i = 0; > > > > - num_channels = of_get_available_child_count(np); > > + fwnode_for_each_available_child_node(np, child) { > > + num_channels++; > > + } > > if (!num_channels || num_channels > chip->num_leds) > > return -EINVAL; > > > > @@ -398,22 +402,22 @@ static int ktd202x_setup_led_rgb(struct ktd202x > > *chip, struct device_node *np, > > if (!info) > > return -ENOMEM; > > > > - 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); > > - of_node_put(child); > > + fwnode_handle_put(child); > > return -EINVAL; > > } > > > > - 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); > > - of_node_put(child); > > + fwnode_handle_put(child); > > return ret; > > } > > > > @@ -433,14 +437,14 @@ static int ktd202x_setup_led_rgb(struct ktd202x > > *chip, struct device_node *np, > > return devm_led_classdev_multicolor_register_ext(chip->dev, > > &led->mcdev, init_data); > > } > > > > -static int ktd202x_setup_led_single(struct ktd202x *chip, struct > > device_node *np, > > +static int ktd202x_setup_led_single(struct ktd202x *chip, struct > > fwnode_handle *np, > > struct ktd202x_led *led, struct > > led_init_data *init_data) > > { > > struct led_classdev *cdev; > > u32 reg; > > int 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); > > return -EINVAL; > > @@ -454,7 +458,7 @@ static int ktd202x_setup_led_single(struct > > ktd202x *chip, struct device_node *np > > return devm_led_classdev_register_ext(chip->dev, &led->cdev, > > init_data); > > } > > > > -static int ktd202x_add_led(struct ktd202x *chip, struct device_node > > *np, unsigned int index) > > +static int ktd202x_add_led(struct ktd202x *chip, struct > > fwnode_handle *np, unsigned int index) > > { > > struct ktd202x_led *led = &chip->leds[index]; > > struct led_init_data init_data = {}; > > @@ -463,14 +467,14 @@ static int ktd202x_add_led(struct ktd202x > > *chip, struct device_node *np, unsigne > > int ret; > > > > /* 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); > > return ret; > > } > > > > led->chip = chip; > > - init_data.fwnode = of_fwnode_handle(np); > > + init_data.fwnode = np; > > > > if (color == LED_COLOR_ID_RGB) { > > cdev = &led->mcdev.led_cdev; > > @@ -492,26 +496,30 @@ static int ktd202x_add_led(struct ktd202x > > *chip, struct device_node *np, unsigne > > > > static int ktd202x_probe_dt(struct ktd202x *chip) > > { > > - struct device_node *np = dev_of_node(chip->dev), *child; > > - int count; > > + struct device *dev = chip->dev; > > + struct fwnode_handle *np, *child; > Nit, swap the above lines. > > > + int count = 0; > Un-needed init. Okay > > > int i = 0; > > > > - chip->num_leds = (int)(unsigned > > long)of_device_get_match_data(chip->dev); > > + count = device_get_child_node_count(dev); > > > > - count = of_get_available_child_count(np); > > if (!count || count > chip->num_leds) > > return -EINVAL; > > > > + np = dev_fwnode(chip->dev); > > + if (!np) > > + return -ENODEV; > > + > > regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, > > KTD202X_RSTR_RESET); > > > > /* Allow the device to execute the complete reset */ > > usleep_range(200, 300); > > > > - for_each_available_child_of_node(np, child) { > > + fwnode_for_each_available_child_node(np, child) { > > int ret = ktd202x_add_led(chip, child, i); > > > > if (ret) { > > - of_node_put(child); > > + fwnode_handle_put(child); > > return ret; > > } > > i++; > > @@ -533,7 +541,7 @@ static int ktd202x_probe(struct i2c_client > > *client) > > { > > struct device *dev = &client->dev; > > struct ktd202x *chip; > > - int count; > > + int count = 0; > Un-needed init. Okay. > > > int ret; > > > > count = device_get_child_node_count(dev); > > @@ -568,6 +576,8 @@ static int ktd202x_probe(struct i2c_client > > *client) > > return ret; > > } > > > > + chip->num_leds = (int) (unsigned > > long)i2c_get_match_data(client); > > + > > ret = ktd202x_probe_dt(chip); > > if (ret < 0) { > > regulator_bulk_disable(ARRAY_SIZE(chip->regulators), > > chip->regulators); > > @@ -602,21 +612,33 @@ static void ktd202x_shutdown(struct i2c_client > > *client) > > regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, > > KTD202X_RSTR_RESET); > > } > > > > +static const struct i2c_device_id ktd202x_id[] = { > > + {"ktd2026", KTD2026_NUM_LEDS}, > > + {"ktd2027", KTD2027_NUM_LEDS}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(i2c, ktd202x_id); > > + > > +#ifndef CONFIG_ACPI > > 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", > > - .of_match_table = ktd202x_match_table, > > + .name = "leds-ktd202x", > Why was the space after "name" replaced by tab? I'll remove it. > > > +#ifndef CONFIG_ACPI > > + .of_match_table = ktd20xx_match_table, > Typo: ktd20xx_match_table -> ktd202x_match_table Ops. I'll fix it. > > André > > > +#endif > > }, > > .probe = ktd202x_probe, > > .remove = ktd202x_remove, > > .shutdown = ktd202x_shutdown, > > + .id_table = ktd202x_id, > > }; > > module_i2c_driver(ktd202x_driver); > > > I'll propose a v2 patch to improve the code according to your suggestions. -- BR, Kate
