| Message ID | 20231001-ktd202x-v5-0-f544a1d0510d@apitzsch.eu |
|---|---|
| Headers | show |
| Series | leds: Add a driver for KTD202x | expand |
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? > + > + ret = of_property_read_u32(child, "reg", ®); > + if (ret != 0 || reg >= chip->num_leds) { > + dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np); Mossing of_node_put(np);? > + 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", ®); > + 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] > + 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) > + 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; > +} ...
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", ®); > > + 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", ®); > > + 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é
Hello André, On Sun, Oct 01, 2023 at 06:56:20PM +0200, André Apitzsch wrote: > Am Sonntag, dem 01.10.2023 um 17:15 +0200 schrieb Christophe JAILLET: > > Le 01/10/2023 à 15:52, André Apitzsch a écrit : > > > + 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). In my book a variable with a narrow scope is better. I didn't check, but if you can restrict both variables to the for loop, that's nicer. Best regards Uwe
Le 01/10/2023 à 18:56, André Apitzsch a écrit : > 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", ®); >>> + 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? How can the caller do this? The goal of this of_node_put() is to release a reference taken by the for_each_available_child_of_node() loop, in case of early exit. The caller can't know if np needs to be released or not. An error code is returned either if an error occurs within the for_each loop, or if devm_led_classdev_multicolor_register_ext() fails. More over, in your case the caller is ktd202x_add_led(). From there either ktd202x_setup_led_rgb() or ktd202x_setup_led_single() is called. ktd202x_setup_led_single() does not take any reference to np. But if it fails, of_node_put() would still be called. > >> >>> + 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", ®); >>> + 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]. Why [2]? It does not seem needed here. of_get_available_child_count() does not keep any reference. CJ > >> >>> + 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é >
Am Sonntag, dem 01.10.2023 um 22:46 +0200 schrieb Christophe JAILLET: > Le 01/10/2023 à 18:56, André Apitzsch a écrit : > > 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", ®); > > > > + 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? > > How can the caller do this? > > The goal of this of_node_put() is to release a reference taken by the > for_each_available_child_of_node() loop, in case of early exit. > > The caller can't know if np needs to be released or not. An error > code > is returned either if an error occurs within the for_each loop, or if > devm_led_classdev_multicolor_register_ext() fails. > > More over, in your case the caller is ktd202x_add_led(). > From there either ktd202x_setup_led_rgb() or > ktd202x_setup_led_single() > is called. > > ktd202x_setup_led_single() does not take any reference to np. > But if it fails, of_node_put() would still be called. > > Hello Christophe, It seems I misunderstood when of_node_put() is used. Thanks for the explanation. While checking the usage of of_node_put(), I noticed that dev_err() (and of_node_put()) should take "child" and not "np", here. André > > > > > > > + 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", ®); > > > > + 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]. > > Why [2]? > It does not seem needed here. > > of_get_available_child_count() does not keep any reference. > > CJ > > > > > > > > > > + 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é > > >
Add the binding description and the corresponding driver for the Kinetic KTD2026 and KTD2027. Signed-off-by: André Apitzsch <git@apitzsch.eu> --- Changes in v5: - Restructure brightness_set() + add comments to it to be easier understandable - Add some line breaks + remove little line-wraps to improve readability - Move parts of add_led() to setup_led_{rgb,single}() - Move mutex_init() to the end of probe to omit gotos - Fix grammar - Set initial intensity to max brightness to avoid LED staying off when brightness is changed after switching to timer trigger, because of zero intensity - Link to v4: https://lore.kernel.org/r/20230923-ktd202x-v4-0-14f724f6d43b@apitzsch.eu Changes in v4: - Annotate struct ktd202x with __counted_by - Link to v3: https://lore.kernel.org/r/20230906-ktd202x-v3-0-7fcb91c65d3a@apitzsch.eu Changes in v3: - Add r-b to bindings patch - Replace .probe_new by .probe - Link to v2: https://lore.kernel.org/r/20230901-ktd202x-v2-0-3cb8b0ca02ed@apitzsch.eu Changes in v2: - Make binding description filename match compatible - Address comments by Lee Jones - Extend driver description in Kconfig - Add copyright + link to datasheet - Add unit to definition/variable names, where needed - Define magic numbers - Remove forward declaration of 'struct ktd202x' - Remove superfluous comments - Get rid of struct ktd202x_info - Join ktd202x_chip_init() with ktd202x_chip_enable() - Return the error on ktd202x_chip_disable() - Remove unreachable case from chip_in_use() - Rename ktd202x_brightness_set() argument from num_colors to num_channels - Forward errors received in ktd202x_brightness_set() - Remove variable for 'num_channels = 1' - Add some explanations to blink time calculation - Remove unneeded lcdev from ktd202x_blink_*_set() - Add define for max brightness and replace deprecated LED_FULL by it - Move setting led_classdev.brightness to ktd202x_brightness_*_set() - Move mutex_lock inside ktd202x_blink_set() - Add comment that 'color' property is optional (allow EINVAL) - Replace escaped double quotes by single quotes - Avoid overloading variable 'color' - Do not lock during probe - Remove usage of 'of_match_ptr' - Document interrupt and pull-up supply, like done for aw2013[1] - Fix error in num_steps calculation - Link to v1: https://lore.kernel.org/r/20230618-ktd202x-v1-0-fc182fefadd7@apitzsch.eu [1] https://lore.kernel.org/linux-leds/20230815-aw2013-vio-v3-0-2505296b0856@gerhold.net/ --- André Apitzsch (2): dt-bindings: leds: Add Kinetic KTD2026/2027 LED leds: add ktd202x driver .../devicetree/bindings/leds/kinetic,ktd202x.yaml | 171 ++++++ drivers/leds/rgb/Kconfig | 13 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-ktd202x.c | 619 +++++++++++++++++++++ 4 files changed, 804 insertions(+) --- base-commit: 165adeea3617ea22dc49f8880474ebf3a98b696d change-id: 20230618-ktd202x-546b2a7d240b Best regards,