Received-SPF: pass (google.com: domain of linux-kernel+bounces-91617-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 4 Mar 2024 22:29:41 -0500
From: Abdel Alkuor <alkuor@gmail.com>
To: Lee Jones <lee@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Alice Chen <alice_chen@richtek.com>,
	Jean-Jacques Hiblot <jjhiblot@traphandler.com>,
	ChiYuan Huang <cy_huang@richtek.com>,
	ChiaEn Wu <chiaen_wu@richtek.com>,
	Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	=?utf-8?B?QW5kcsOp?= Apitzsch <git@apitzsch.eu>,
	linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add NCP5623 multi-led driver
Message-ID: <ZeaRpSrkeFKAXIlq@abdel>
References: <20240217230956.630522-1-alkuor@gmail.com>
 <20240217230956.630522-2-alkuor@gmail.com>
 <20240301085046.GE1209090@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240301085046.GE1209090@google.com>

On Fri, Mar 01, 2024 at 08:50:46AM +0000, Lee Jones wrote:

Hi Lee,
> > +#define NCP5623_REG(x)			((x) << 0x5)
> 
> What's 0x5?  Probably worth defining.
This is a function offset. I'll add a define.

> 
> > +	guard(mutex)(&ncp->lock);
> 
> Are these self-unlocking?
Correct. Here is a short introduction about it
https://www.marcusfolkesson.se/blog/mutex-guards-in-the-linux-kernel/
> 
> > +	ncp->old_brightness = brightness;
> 
> The nomenclature is confusing here.
> 
> For the most part, this will carry the present value, no?
>
Yes, I'll change it to current_brightness instead

> > +	ret = ncp5623_write(ncp->client,
> > +			    NCP5623_DIMMING_TIME_REG, pattern[0].delta_t / 8);
> 
> Why 8?  Magic numbers should be replaced with #defines.
> 
This is dim step in ms. I'll add a define for it.

> > +static int ncp5623_pattern_clear(struct led_classdev *led_cdev)
> > +{
> > +	return 0;
> > +}
> 
> Not sure I see the point in this.
> 
> Is the .pattern_clear() compulsorily?
>
Unfortunately, it is. For example, in pattern_trig_store_patterns, when
hw pattern is used, it is expected to have pattern_clear implemented.

static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
                                            const char *buf, const u32 *buf_int,
                                            size_t count, bool hw_pattern)
{
	...
         if (data->is_hw_pattern)
                 led_cdev->pattern_clear(led_cdev);
 	...
}

> > +	init_data.fwnode = mc_node;
> > +
> > +	ncp->mc_dev.led_cdev.max_brightness = NCP5623_MAX_BRIGHTNESS;
> > +	ncp->mc_dev.subled_info = subled_info;
> > +	ncp->mc_dev.led_cdev.brightness_set_blocking = ncp5623_brightness_set;
> > +	ncp->mc_dev.led_cdev.pattern_set = ncp5623_pattern_set;
> > +	ncp->mc_dev.led_cdev.pattern_clear = ncp5623_pattern_clear;
> > +	ncp->mc_dev.led_cdev.default_trigger = "pattern";
> > +
> > +	mutex_init(&ncp->lock);
> > +	i2c_set_clientdata(client, ncp);
> > +
> > +	ret = led_classdev_multicolor_register_ext(dev, &ncp->mc_dev, &init_data);
> > +	if (ret)
> > +		goto destroy_lock;
> > +
> > +	fwnode_handle_put(mc_node);
> 
> Didn't you just store this ~16 lines up?
> 
Ops! I'll remove it.

Thanks,
Abdel