Re: [PATCH v5 2/2] leds: add ktd202x driver
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
> > > >
> > >
> > > ...
> > >
> > > > +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",
> > > > _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 = >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,
> > > > >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 = >cdev;
> > > > + cdev->brightness_set_blocking =
> > > > ktd202x_brightness_single_set;
> > > > + cdev->blink_set = ktd202x_blink_single_set;
> > > > +
> > > > + return devm_led_classdev_register_ext(chip->dev,
> > > > > cdev, init_data);
> > > > +}
> > > > +
> > > > +static int ktd202x_add_led(struct ktd202x *chip, struct
> > > > device_node *np, unsigned
Re: [PATCH v5 2/2] leds: add ktd202x driver
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
...
+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",
_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 = >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,
>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 = >cdev;
+ cdev->brightness_set_blocking =
ktd202x_brightness_single_set;
+ cdev->blink_set = ktd202x_blink_single_set;
+
+ return devm_led_classdev_register_ext(chip->dev,
cdev, init_data);
+}
+
+static int ktd202x_add_led(struct ktd202x *chip, struct
device_node *np, unsigned int index)
+{
+ struct ktd202x_led *led = >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", );
+ 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 = >mcdev.led_cdev;
+ ret = ktd202x_setup_led_rgb(chip, np, led,
_data);
+ } else {
+ cdev = >cdev;
+ ret = ktd202x_setup_led_single(chip, np, led,
_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.
Re: [PATCH v5 2/2] leds: add ktd202x driver
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
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/2] leds: add ktd202x driver
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
> >
>
> ...
>
> > +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",
> > _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 = >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,
> > >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 = >cdev;
> > + cdev->brightness_set_blocking =
> > ktd202x_brightness_single_set;
> > + cdev->blink_set = ktd202x_blink_single_set;
> > +
> > + return devm_led_classdev_register_ext(chip->dev,
> > >cdev, init_data);
> > +}
> > +
> > +static int ktd202x_add_led(struct ktd202x *chip, struct
> > device_node *np, unsigned int index)
> > +{
> > + struct ktd202x_led *led = >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", );
> > + 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 = >mcdev.led_cdev;
> > + ret = ktd202x_setup_led_rgb(chip, np, led,
> > _data);
> > + } else {
> > + cdev = >cdev;
> > + ret = ktd202x_setup_led_single(chip, np, led,
> > _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)
> > +{
> > +
Re: [PATCH v5 2/2] leds: add ktd202x driver
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
...
+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", _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 = >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,
>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 = >cdev;
+ cdev->brightness_set_blocking = ktd202x_brightness_single_set;
+ cdev->blink_set = ktd202x_blink_single_set;
+
+ return devm_led_classdev_register_ext(chip->dev, >cdev, init_data);
+}
+
+static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np,
unsigned int index)
+{
+ struct ktd202x_led *led = >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", );
+ 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 = >mcdev.led_cdev;
+ ret = ktd202x_setup_led_rgb(chip, np, led, _data);
+ } else {
+ cdev = >cdev;
+ ret = ktd202x_setup_led_single(chip, np, led, _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
+
