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

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

 



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", &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);
> > -                     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", &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);
> >               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






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux