Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

From: Jacek Anaszewski <j.anaszewski@samsung.com>
To: Florian Vaussard <florian.vaussard@heig-vd.ch>
Cc: Pavel Machek <pavel@ucw.cz>,
	Florian Vaussard <florian.vaussard@gmail.com>,
	devicetree@vger.kernel.org, Richard Purdie <rpurdie@rpsys.net>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver
Date: Mon, 27 Jun 2016 09:09:08 +0200	[thread overview]
Message-ID: <5770D114.70909@samsung.com> (raw)
In-Reply-To: <51f6c4d4-e5af-97b6-e7d9-61f0093de134@heig-vd.ch>

Hi Florian and Pavel,

On 06/27/2016 07:46 AM, Florian Vaussard wrote:
> Hi Pavel,
>
> Le 26. 06. 16 à 23:49, Pavel Machek a écrit :
>> Hi!
>>
>>>> +struct ncp5623_led {
>>>> +	bool active;
>>>> +	unsigned int led_no;
>>>> +	struct led_classdev ldev;
>>>> +	struct work_struct work;
>>>> +	struct ncp5623_priv *priv;
>>>> +};
>>>> +
>>>> +struct ncp5623_priv {
>>>> +	struct ncp5623_led leds[NCP5623_MAX_LEDS];
>>>
>>> Please allocate memory dynamically, depending on the number
>>> of LEDs defined in a Device Tree.
>>
>> MAX_LEDs is three. Are you sure overhead of dynamic allocation is
>> worth it?
>>
>> And if this is for RGB leds... very probably device will want to use
>> all 3 channels.
>>
>
> I was about to raise the same question during the v2 of this patch. In addition
> to your arguments, this also changes the way this array is indexed.
>
> Currently the LED number is used as index, but with dynamic allocation I have to
> use an abstract index. This makes some logic a bit harder, especially to check
> if the same LED is declared twice in the device tree (duplicated 'reg' property).

Fair enough. Please ignore my remark then.

-- 
Best regards,
Jacek Anaszewski