From: "André Apitzsch" <git@apitzsch.eu>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: conor+dt@kernel.org, devicetree@vger.kernel.org,
krzysztof.kozlowski+dt@linaro.org, lee@kernel.org,
linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
pavel@ucw.cz, phone-devel@vger.kernel.org, robh+dt@kernel.org,
u.kleine-koenig@pengutronix.de,
~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH v5 2/2] leds: add ktd202x driver
Date: Mon, 02 Oct 2023 08:09:59 +0200 [thread overview]
Message-ID: <15b8ecff56393a6249fe1956689940a238e3824e.camel@apitzsch.eu> (raw)
In-Reply-To: <82e9cffc-472a-b725-1a12-de8aade67189@wanadoo.fr>
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é
> >
>
prev parent reply other threads:[~2023-10-02 6:10 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-01 13:52 [PATCH v5 0/2] leds: Add a driver for KTD202x André Apitzsch
2023-10-01 13:52 ` [PATCH v5 1/2] dt-bindings: leds: Add Kinetic KTD2026/2027 LED André Apitzsch
2023-10-01 13:52 ` [PATCH v5 2/2] leds: add ktd202x driver André Apitzsch
2023-10-01 15:15 ` Christophe JAILLET
2023-10-01 16:56 ` André Apitzsch
2023-10-01 17:05 ` Uwe Kleine-König
2023-10-01 20:46 ` Christophe JAILLET
2023-10-02 6:09 ` André Apitzsch [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15b8ecff56393a6249fe1956689940a238e3824e.camel@apitzsch.eu \
--to=git@apitzsch.eu \
--cc=christophe.jaillet@wanadoo.fr \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=u.kleine-koenig@pengutronix.de \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).