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 ?
> 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?
> +
> 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.
> 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.
> 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.
> 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?
> +#ifndef CONFIG_ACPI
> + .of_match_table = ktd20xx_match_table,
Typo: ktd20xx_match_table -> ktd202x_match_table
André
> +#endif
> },
> .probe = ktd202x_probe,
> .remove = ktd202x_remove,
> .shutdown = ktd202x_shutdown,
> + .id_table = ktd202x_id,
> };
> module_i2c_driver(ktd202x_driver);
>
