Re: [PATCH v2 1/2] leds: tlc591xx: simplify driver by using the managed led API

[Jean-Jacques Hiblot]

Hi Dan,

On 08/07/2019 20:08, Dan Murphy wrote:
> JJ
>
> On 7/8/19 5:06 AM, Jean-Jacques Hiblot wrote:
> > Use the managed API of the LED class (devm_led_classdev_register()
> > instead of led_classdev_register()).
> > This allows us to remove the code used to track-and-destroy the LED 
> > devices
>
> What changed from v1?

What changed is a bug fix in the loop: replaced led = &priv->leds[idx] 
with led = &priv->leds[reg];


> I don't see any changes especially the bounds on the reg property.

It is not quite apparent in the diff, but there is such a check. Here is 
what we have in the probe():

ÂÂÂ for_each_child_of_node(np, child) {
ÂÂÂ ÂÂÂ struct tlc591xx_led *led;

ÂÂÂ ÂÂÂ err = of_property_read_u32(child, "reg", &reg);
ÂÂÂ ÂÂÂ if (err) {
ÂÂÂ ÂÂÂ ÂÂÂ of_node_put(child);
ÂÂÂ ÂÂÂ ÂÂÂ return err;
ÂÂÂ ÂÂÂ }
ÂÂÂ ÂÂÂ if (reg < 0 || reg >= tlc591xx->max_leds ||
ÂÂÂ ÂÂÂ ÂÂÂ priv->leds[reg].active) {
ÂÂÂ ÂÂÂ ÂÂÂ of_node_put(child);
ÂÂÂ ÂÂÂ ÂÂÂ return -EINVAL;
ÂÂÂ ÂÂÂ }
ÂÂÂ ÂÂÂ led = &priv->leds[reg];

[...]
ÂÂÂ ÂÂÂ }
ÂÂÂ }

> Dan
>
>
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxx>
> > ---
> > Â drivers/leds/leds-tlc591xx.c | 79 +++++++++---------------------------
> > Â 1 file changed, 20 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> > index 59ff088c7d75..3d5a4b92f016 100644
> > --- a/drivers/leds/leds-tlc591xx.c
> > +++ b/drivers/leds/leds-tlc591xx.c
> > @@ -128,51 +128,6 @@ tlc591xx_brightness_set(struct led_classdev 
> > *led_cdev,
> > ÂÂÂÂÂ return err;
> > Â }
> > Â -static void
> > -tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
> > -{
> > -ÂÂÂ int i = j;
> > -
> > -ÂÂÂ while (--i >= 0) {
> > -ÂÂÂÂÂÂÂ if (priv->leds[i].active)
> > - led_classdev_unregister(&priv->leds[i].ldev);
> > -ÂÂÂ }
> > -}
> > -
> > -static int
> > -tlc591xx_configure(struct device *dev,
> > -ÂÂÂÂÂÂÂÂÂÂ struct tlc591xx_priv *priv,
> > -ÂÂÂÂÂÂÂÂÂÂ const struct tlc591xx *tlc591xx)
> > -{
> > -ÂÂÂ unsigned int i;
> > -ÂÂÂ int err = 0;
> > -
> > -ÂÂÂ tlc591xx_set_mode(priv->regmap, MODE2_DIM);
> > -ÂÂÂ for (i = 0; i < TLC591XX_MAX_LEDS; i++) {
> > -ÂÂÂÂÂÂÂ struct tlc591xx_led *led = &priv->leds[i];
> > -
> > -ÂÂÂÂÂÂÂ if (!led->active)
> > -ÂÂÂÂÂÂÂÂÂÂÂ continue;
> > -
> > -ÂÂÂÂÂÂÂ led->priv = priv;
> > -ÂÂÂÂÂÂÂ led->led_no = i;
> > -ÂÂÂÂÂÂÂ led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
> > -ÂÂÂÂÂÂÂ led->ldev.max_brightness = LED_FULL;
> > -ÂÂÂÂÂÂÂ err = led_classdev_register(dev, &led->ldev);
> > -ÂÂÂÂÂÂÂ if (err < 0) {
> > -ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "couldn't register LED %s\n",
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ led->ldev.name);
> > -ÂÂÂÂÂÂÂÂÂÂÂ goto exit;
> > -ÂÂÂÂÂÂÂ }
> > -ÂÂÂ }
> > -
> > -ÂÂÂ return 0;
> > -
> > -exit:
> > -ÂÂÂ tlc591xx_destroy_devices(priv, i);
> > -ÂÂÂ return err;
> > -}
> > -
> > Â static const struct regmap_config tlc591xx_regmap = {
> > ÂÂÂÂÂ .reg_bits = 8,
> > ÂÂÂÂÂ .val_bits = 8,
> > @@ -225,7 +180,11 @@ tlc591xx_probe(struct i2c_client *client,
> > Â ÂÂÂÂÂ i2c_set_clientdata(client, priv);
> > Â +ÂÂÂ tlc591xx_set_mode(priv->regmap, MODE2_DIM);
> > +
> > ÂÂÂÂÂ for_each_child_of_node(np, child) {
> > +ÂÂÂÂÂÂÂ struct tlc591xx_led *led;
> > +
> > ÂÂÂÂÂÂÂÂÂ err = of_property_read_u32(child, "reg", &reg);
> > ÂÂÂÂÂÂÂÂÂ if (err) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ of_node_put(child);
> > @@ -236,22 +195,25 @@ tlc591xx_probe(struct i2c_client *client,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ of_node_put(child);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > ÂÂÂÂÂÂÂÂÂ }
> > -ÂÂÂÂÂÂÂ priv->leds[reg].active = true;
> > -ÂÂÂÂÂÂÂ priv->leds[reg].ldev.name =
> > +ÂÂÂÂÂÂÂ led = &priv->leds[reg];
> > +
> > +ÂÂÂÂÂÂÂ led->active = true;
> > +ÂÂÂÂÂÂÂ led->ldev.name =
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ of_get_property(child, "label", NULL) ? : child->name;
> > -ÂÂÂÂÂÂÂ priv->leds[reg].ldev.default_trigger =
> > +ÂÂÂÂÂÂÂ led->ldev.default_trigger =
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ of_get_property(child, "linux,default-trigger", NULL);
> > -ÂÂÂ }
> > -ÂÂÂ return tlc591xx_configure(dev, priv, tlc591xx);
> > -}
> > -
> > -static int
> > -tlc591xx_remove(struct i2c_client *client)
> > -{
> > -ÂÂÂ struct tlc591xx_priv *priv = i2c_get_clientdata(client);
> > -
> > -ÂÂÂ tlc591xx_destroy_devices(priv, TLC591XX_MAX_LEDS);
> > Â +ÂÂÂÂÂÂÂ led->priv = priv;
> > +ÂÂÂÂÂÂÂ led->led_no = reg;
> > +ÂÂÂÂÂÂÂ led->ldev.brightness_set_blocking = tlc591xx_brightness_set;
> > +ÂÂÂÂÂÂÂ led->ldev.max_brightness = LED_FULL;
> > +ÂÂÂÂÂÂÂ err = devm_led_classdev_register(dev, &led->ldev);
> > +ÂÂÂÂÂÂÂ if (err < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "couldn't register LED %s\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ led->ldev.name);
> > +ÂÂÂÂÂÂÂÂÂÂÂ return err;
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > ÂÂÂÂÂ return 0;
> > Â }
> > Â @@ -268,7 +230,6 @@ static struct i2c_driver tlc591xx_driver = {
> > ÂÂÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(of_tlc591xx_leds_match),
> > ÂÂÂÂÂ },
> > ÂÂÂÂÂ .probe = tlc591xx_probe,
> > -ÂÂÂ .remove = tlc591xx_remove,
> > ÂÂÂÂÂ .id_table = tlc591xx_id,
> > Â };