linux-kernel - Re: [PATCH v32 2/6] leds: lp50xx: Add the LP50XX family of the RGB LED driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <935119fa-6d1f-8e99-51f9-87966b4d03ad@ti.com>
Date:   Tue, 11 Aug 2020 12:27:21 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Pavel Machek <pavel@....cz>
CC:     <jacek.anaszewski@...il.com>, <robh@...nel.org>,
        <marek.behun@....cz>, <devicetree@...r.kernel.org>,
        <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v32 2/6] leds: lp50xx: Add the LP50XX family of the RGB
 LED driver

Hello

On 8/11/20 5:54 AM, Pavel Machek wrote:
> Hi!
>
>> +/* LP5009 and LP5012 registers */
>> +#define LP5012_BNK_BRT		0x03
>> +#define LP5012_BNKA_CLR		0x04
>> +#define LP5012_BNKB_CLR		0x05
>> +#define LP5012_BNKC_CLR		0x06
>> +#define LP5012_LED0_BRT		0x07
>> +#define LP5012_LED1_BRT		0x08
>> +#define LP5012_LED2_BRT		0x09
>> +#define LP5012_LED3_BRT		0x0a
>> +#define LP5012_OUT0_CLR		0x0b
>> +#define LP5012_OUT1_CLR		0x0c
>> +#define LP5012_OUT2_CLR		0x0d
>> +#define LP5012_OUT3_CLR		0x0e
>> +#define LP5012_OUT4_CLR		0x0f
>> +#define LP5012_OUT5_CLR		0x10
>> +#define LP5012_OUT6_CLR		0x11
>> +#define LP5012_OUT7_CLR		0x12
>> +#define LP5012_OUT8_CLR		0x13
>> +#define LP5012_OUT9_CLR		0x14
>> +#define LP5012_OUT10_CLR	0x15
>> +#define LP5012_OUT11_CLR	0x16
>> +#define LP5012_RESET		0x17
>> +
>> +/* LP5018 and LP5024 registers */
>> +#define LP5024_BNK_BRT		0x03
>> +#define LP5024_BNKA_CLR		0x04
>> +#define LP5024_BNKB_CLR		0x05
>> +#define LP5024_BNKC_CLR		0x06
>> +#define LP5024_LED0_BRT		0x07
>> +#define LP5024_LED1_BRT		0x08
>> +#define LP5024_LED2_BRT		0x09
>> +#define LP5024_LED3_BRT		0x0a
>> +#define LP5024_LED4_BRT		0x0b
>> +#define LP5024_LED5_BRT		0x0c
>> +#define LP5024_LED6_BRT		0x0d
>> +#define LP5024_LED7_BRT		0x0e
>> +
>> +#define LP5024_OUT0_CLR		0x0f
>> +#define LP5024_OUT1_CLR		0x10
>> +#define LP5024_OUT2_CLR		0x11
>> +#define LP5024_OUT3_CLR		0x12
>> +#define LP5024_OUT4_CLR		0x13
>> +#define LP5024_OUT5_CLR		0x14
>> +#define LP5024_OUT6_CLR		0x15
>> +#define LP5024_OUT7_CLR		0x16
>> +#define LP5024_OUT8_CLR		0x17
>> +#define LP5024_OUT9_CLR		0x18
>> +#define LP5024_OUT10_CLR	0x19
>> +#define LP5024_OUT11_CLR	0x1a
>> +#define LP5024_OUT12_CLR	0x1b
>> +#define LP5024_OUT13_CLR	0x1c
>> +#define LP5024_OUT14_CLR	0x1d
>> +#define LP5024_OUT15_CLR	0x1e
>> +#define LP5024_OUT16_CLR	0x1f
>> +#define LP5024_OUT17_CLR	0x20
>> +#define LP5024_OUT18_CLR	0x21
>> +#define LP5024_OUT19_CLR	0x22
>> +#define LP5024_OUT20_CLR	0x23
>> +#define LP5024_OUT21_CLR	0x24
>> +#define LP5024_OUT22_CLR	0x25
>> +#define LP5024_OUT23_CLR	0x26
>> +#define LP5024_RESET		0x27
>> +
>> +/* LP5030 and LP5036 registers */
>> +#define LP5036_LED_CFG1		0x03
>> +#define LP5036_BNK_BRT		0x04
>> +#define LP5036_BNKA_CLR		0x05
>> +#define LP5036_BNKB_CLR		0x06
>> +#define LP5036_BNKC_CLR		0x07
>> +#define LP5036_LED0_BRT		0x08
>> +#define LP5036_LED1_BRT		0x09
>> +#define LP5036_LED2_BRT		0x0a
>> +#define LP5036_LED3_BRT		0x0b
>> +#define LP5036_LED4_BRT		0x0c
>> +#define LP5036_LED5_BRT		0x0d
>> +#define LP5036_LED6_BRT		0x0e
>> +#define LP5036_LED7_BRT		0x0f
>> +#define LP5036_LED8_BRT		0x10
>> +#define LP5036_LED9_BRT		0x11
>> +#define LP5036_LED10_BRT	0x12
>> +#define LP5036_LED11_BRT	0x13
>> +
>> +#define LP5036_OUT0_CLR		0x14
>> +#define LP5036_OUT1_CLR		0x15
>> +#define LP5036_OUT2_CLR		0x16
>> +#define LP5036_OUT3_CLR		0x17
>> +#define LP5036_OUT4_CLR		0x18
>> +#define LP5036_OUT5_CLR		0x19
>> +#define LP5036_OUT6_CLR		0x1a
>> +#define LP5036_OUT7_CLR		0x1b
>> +#define LP5036_OUT8_CLR		0x1c
>> +#define LP5036_OUT9_CLR		0x1d
>> +#define LP5036_OUT10_CLR	0x1e
>> +#define LP5036_OUT11_CLR	0x1f
>> +#define LP5036_OUT12_CLR	0x20
>> +#define LP5036_OUT13_CLR	0x21
>> +#define LP5036_OUT14_CLR	0x22
>> +#define LP5036_OUT15_CLR	0x23
>> +#define LP5036_OUT16_CLR	0x24
>> +#define LP5036_OUT17_CLR	0x25
>> +#define LP5036_OUT18_CLR	0x26
>> +#define LP5036_OUT19_CLR	0x27
>> +#define LP5036_OUT20_CLR	0x28
>> +#define LP5036_OUT21_CLR	0x29
>> +#define LP5036_OUT22_CLR	0x2a
>> +#define LP5036_OUT23_CLR	0x2b
>> +#define LP5036_OUT24_CLR	0x2c
>> +#define LP5036_OUT25_CLR	0x2d
>> +#define LP5036_OUT26_CLR	0x2e
>> +#define LP5036_OUT27_CLR	0x2f
>> +#define LP5036_OUT28_CLR	0x30
>> +#define LP5036_OUT29_CLR	0x31
>> +#define LP5036_OUT30_CLR	0x32
>> +#define LP5036_OUT31_CLR	0x33
>> +#define LP5036_OUT32_CLR	0x34
>> +#define LP5036_OUT33_CLR	0x35
>> +#define LP5036_OUT34_CLR	0x36
>> +#define LP5036_OUT35_CLR	0x37
>> +#define LP5036_RESET		0x38
>> +static const struct reg_default lp5012_reg_defs[] = {
> ...
>> +	{LP5012_OUT1_CLR, 0x00},
>> +	{LP5012_OUT2_CLR, 0x00},
>> +	{LP5012_OUT3_CLR, 0x00},
>> +	{LP5012_OUT4_CLR, 0x00},
>> +	{LP5012_OUT5_CLR, 0x00},
>> +	{LP5012_OUT6_CLR, 0x00},
>> +	{LP5012_OUT7_CLR, 0x00},
>> +	{LP5012_OUT8_CLR, 0x00},
>> +	{LP5012_OUT9_CLR, 0x00},
>> +	{LP5012_OUT10_CLR, 0x00},
>> +	{LP5012_OUT11_CLR, 0x00},
>> +	{LP5012_RESET, 0x00}
>> +};
>> +
>> +static const struct reg_default lp5024_reg_defs[] = {
> ...
>> +	{LP5024_OUT1_CLR, 0x00},
>> +	{LP5024_OUT2_CLR, 0x00},
>> +	{LP5024_OUT3_CLR, 0x00},
>> +	{LP5024_OUT4_CLR, 0x00},
>> +	{LP5024_OUT5_CLR, 0x00},
>> +	{LP5024_OUT6_CLR, 0x00},
>> +	{LP5024_OUT7_CLR, 0x00},
>> +	{LP5024_OUT8_CLR, 0x00},
>> +	{LP5024_OUT9_CLR, 0x00},
>> +	{LP5024_OUT10_CLR, 0x00},
>> +	{LP5024_OUT11_CLR, 0x00},
>> +	{LP5024_OUT12_CLR, 0x00},
>> +	{LP5024_OUT13_CLR, 0x00},
>> +	{LP5024_OUT14_CLR, 0x00},
>> +	{LP5024_OUT15_CLR, 0x00},
>> +	{LP5024_OUT16_CLR, 0x00},
>> +	{LP5024_OUT17_CLR, 0x00},
>> +	{LP5024_OUT18_CLR, 0x00},
>> +	{LP5024_OUT19_CLR, 0x00},
>> +	{LP5024_OUT20_CLR, 0x00},
>> +	{LP5024_OUT21_CLR, 0x00},
>> +	{LP5024_OUT22_CLR, 0x00},
>> +	{LP5024_OUT23_CLR, 0x00},
>> +	{LP5024_RESET, 0x00}
>> +};
>> +
>> +static const struct reg_default lp5036_reg_defs[] = {
>> +	{LP5036_OUT1_CLR, 0x00},
>> +	{LP5036_OUT2_CLR, 0x00},
>> +	{LP5036_OUT3_CLR, 0x00},
>> +	{LP5036_OUT4_CLR, 0x00},
>> +	{LP5036_OUT5_CLR, 0x00},
>> +	{LP5036_OUT6_CLR, 0x00},
>> +	{LP5036_OUT7_CLR, 0x00},
>> +	{LP5036_OUT8_CLR, 0x00},
>> +	{LP5036_OUT9_CLR, 0x00},
>> +	{LP5036_OUT10_CLR, 0x00},
>> +	{LP5036_OUT11_CLR, 0x00},
>> +	{LP5036_OUT12_CLR, 0x00},
>> +	{LP5036_OUT13_CLR, 0x00},
>> +	{LP5036_OUT14_CLR, 0x00},
>> +	{LP5036_OUT15_CLR, 0x00},
>> +	{LP5036_OUT16_CLR, 0x00},
>> +	{LP5036_OUT17_CLR, 0x00},
>> +	{LP5036_OUT18_CLR, 0x00},
>> +	{LP5036_OUT19_CLR, 0x00},
>> +	{LP5036_OUT20_CLR, 0x00},
>> +	{LP5036_OUT21_CLR, 0x00},
>> +	{LP5036_OUT22_CLR, 0x00},
>> +	{LP5036_OUT23_CLR, 0x00},
>> +	{LP5036_OUT24_CLR, 0x00},
>> +	{LP5036_OUT25_CLR, 0x00},
>> +	{LP5036_OUT26_CLR, 0x00},
>> +	{LP5036_OUT27_CLR, 0x00},
>> +	{LP5036_OUT28_CLR, 0x00},
>> +	{LP5036_OUT29_CLR, 0x00},
>> +	{LP5036_OUT30_CLR, 0x00},
>> +	{LP5036_OUT31_CLR, 0x00},
>> +	{LP5036_OUT32_CLR, 0x00},
>> +	{LP5036_OUT33_CLR, 0x00},
>> +	{LP5036_OUT34_CLR, 0x00},
>> +	{LP5036_OUT35_CLR, 0x00},
>> +	{LP5036_RESET, 0x00}
>> +};
> Actually... This is quite impressive ammount of code to
> zero-initialize few registers. Could the regmap be told to set the
> range to zero, or use loops to reduce ammount of code?

I am not aware of any regmap calls that will set a range of registers to 
a certain value.

Well it depends on where we want to create the default cache values.

Either we run through a for..loop during driver probe and delay device 
start up or we keep the simple arrays and increase the driver total size.

I am not seeing an advantage or disadvantage of one over the other.  
This is a simpler and more clear way to init the registers versus a loop.

Dan

> Thanks,
> 								Pavel
>
>