Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431591 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver Pavel Machek <pavel@ucw.cz> - 2016-06-26 23:50 +0200
    Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver Florian Vaussard <florian.vaussard@heig-vd.ch> - 2016-06-27 08:00 +0200
      Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-06-27 09:10 +0200

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

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.

Best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1431732

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).

Best,
Florian

[toc] | [prev] | [next] | [standalone]

#1431765

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web