Re: [PATCH v5 2/2] leds: add ktd202x driver

["André Apitzsch" <git@apitzsch.eu>]

Hi Christophe,

Am Sonntag, dem 01.10.2023 um 17:15 +0200 schrieb Christophe JAILLET:
> Le 01/10/2023 à 15:52, André Apitzsch a écrit :
> > This commit adds support for Kinetic KTD2026/7 RGB/White LED
> > driver.
> > 
> > Signed-off-by: André Apitzsch
> > <git-AtRKszJ1oGPsq35pWSNszA@public.gmane.org>
> 
> ...
> 
> > +static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct
> > device_node *np,
> > +                                struct ktd202x_led *led, struct
> > led_init_data *init_data)
> > +{
> > +       struct led_classdev *cdev;
> > +       struct device_node *child;
> > +       struct mc_subled *info;
> > +       int num_channels;
> > +       int i = 0;
> > +       u32 reg;
> > +       int ret;
> > +
> > +       num_channels = of_get_available_child_count(np);
> > +       if (!num_channels || num_channels > chip->num_leds)
> > +               return -EINVAL;
> > +
> > +       info = devm_kcalloc(chip->dev, num_channels, sizeof(*info),
> > GFP_KERNEL);
> > +       if (!info)
> > +               return -ENOMEM;
> > +
> > +       for_each_available_child_of_node(np, child) {
> > +               u32 mono_color = 0;
> 
> Un-needed init.
> And, why is it defined here, while reg is defined out-side the loop?

I'll move it out-side the loop (without initialization).

> 
> > +
> > +               ret = of_property_read_u32(child, "reg", &reg);
> > +               if (ret != 0 || reg >= chip->num_leds) {
> > +                       dev_err(chip->dev, "invalid 'reg' of
> > %pOFn\n", np);
> 
> Mossing of_node_put(np);?

It shouldn't be needed here if handled in the calling function, right? 

> 
> > +                       return -EINVAL;
> > +               }
> > +
> > +               ret = of_property_read_u32(child, "color",
> > &mono_color);
> > +               if (ret < 0 && ret != -EINVAL) {
> > +                       dev_err(chip->dev, "failed to parse 'color'
> > of %pOF\n", np);
> 
> Mossing of_node_put(np);?
> 
> > +                       return ret;
> > +               }
> > +
> > +               info[i].color_index = mono_color;
> > +               info[i].channel = reg;
> > +               info[i].intensity = KTD202X_MAX_BRIGHTNESS;
> > +               i++;
> > +       }
> > +
> > +       led->mcdev.subled_info = info;
> > +       led->mcdev.num_colors = num_channels;
> > +
> > +       cdev = &led->mcdev.led_cdev;
> > +       cdev->brightness_set_blocking = ktd202x_brightness_mc_set;
> > +       cdev->blink_set = ktd202x_blink_mc_set;
> > +
> > +       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,
> > +                                   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);
> > +       if (ret != 0 || reg >= chip->num_leds) {
> > +               dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
> > +               return -EINVAL;
> > +       }
> > +       led->index = reg;
> > +
> > +       cdev = &led->cdev;
> > +       cdev->brightness_set_blocking =
> > ktd202x_brightness_single_set;
> > +       cdev->blink_set = ktd202x_blink_single_set;
> > +
> > +       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)
> > +{
> > +       struct ktd202x_led *led = &chip->leds[index];
> > +       struct led_init_data init_data = {};
> > +       struct led_classdev *cdev;
> > +       u32 color = 0;
> Un-needed init.
> 
> > +       int ret;
> > +
> > +       /* Color property is optional in single color case */
> > +       ret = of_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);
> > +
> > +       if (color == LED_COLOR_ID_RGB) {
> > +               cdev = &led->mcdev.led_cdev;
> > +               ret = ktd202x_setup_led_rgb(chip, np, led,
> > &init_data);
> > +       } else {
> > +               cdev = &led->cdev;
> > +               ret = ktd202x_setup_led_single(chip, np, led,
> > &init_data);
> > +       }
> > +
> > +       if (ret) {
> > +               dev_err(chip->dev, "unable to register %s\n", cdev-
> > >name);
> > +               of_node_put(np);
> 
> This is strange to have it here.
> Why not above after "if (ret < 0 && ret != -EINVAL) {"?
> 
> It would look much more natural to have it a few lines below, ... [1]

Good catch. I'll move of_node_put(np); to [1] and [2].

> 
> > +               return ret;
> > +       }
> > +
> > +       cdev->max_brightness = KTD202X_MAX_BRIGHTNESS;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ktd202x_probe_dt(struct ktd202x *chip)
> > +{
> > +       struct device_node *np = dev_of_node(chip->dev), *child;
> > +       unsigned int i;
> > +       int count, ret;
> > +
> > +       chip->num_leds = (int)(unsigned
> > long)of_device_get_match_data(chip->dev);
> > +
> > +       count = of_get_available_child_count(np);
> > +       if (!count || count > chip->num_leds)

[2].

> > +               return -EINVAL;
> > +
> > +       regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL,
> > KTD202X_RSTR_RESET);
> > +
> > +       /* Allow the device to execute the complete reset */
> > +       usleep_range(200, 300);
> > +
> > +       i = 0;
> > +       for_each_available_child_of_node(np, child) {
> > +               ret = ktd202x_add_led(chip, child, i);
> > +               if (ret)
> 
> [1] ... here.
> 
> Otherwise, it is likely that, thanks to a static checker, an
> additionnal 
> of_node_put() will be added on early exit of the loop.
> 
> CJ
> 
> > +                       return ret;
> > +               i++;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 

Best regards,
André