Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fafac053-6009-562e-8e29-ee6435a3c8d1@gmail.com>
Date:   Fri, 7 Apr 2023 12:56:50 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
To:     Andreas Kemnade <andreas@kemnade.info>
Cc:     pavel@ucw.cz, lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-leds@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        hns@goldelico.com
References: <20230406060825.103187-1-andreas@kemnade.info>
 <20230406060825.103187-3-andreas@kemnade.info>
 <7d8c558f-0d21-91ed-ecd0-cac079d366ee@gmail.com>
 <20230406214539.59dfaac7@aktux>
Content-Language: en-US, en-GB
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c
 LED driver
In-Reply-To: <20230406214539.59dfaac7@aktux>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/6/23 22:45, Andreas Kemnade wrote:
> On Thu, 6 Apr 2023 11:57:15 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> [...]
> 
>>
>>> + */
>>> +
>>> +#include <linux/i2c.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#define BD2606_MAX_LEDS 6
>>> +#define BD2606_MAX_BRIGHTNESS 63
>>> +#define BD2606_REG_PWRCNT 3
>>> +#define ldev_to_led(c)	container_of(c, struct bd2606mvv_led, ldev)
>>> +
>>> +struct bd2606mvv_led {
>>> +	bool active;
>>
>> I didn't spot where this 'active' was used?
>>
> [..]
> 
>>> +		if (reg < 0 || reg >= BD2606_MAX_LEDS ||
>>> +		    priv->leds[reg].active) {
> 
> here
> 
>>> +			of_node_put(child);
>>> +			return -EINVAL;
>>> +		}
>>> +		led = &priv->leds[reg];
>>> +
>>> +		led->active = true;
> 
> and here

Oh, right. So, if I read this correctly, "active" is only used in the 
probe for checking if same 'reg' is given for mone than one LEDs.

If the 'active' is not used after probe then I'd prefer limiting the 
life-time to probe. Perhaps drop this from the allocated private data 
and just take it from the stack and let it go when probe is done?

This is a minor thing but if there will be other reason(s) to re-spin, 
then this might be changed?

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~