linux-kernel - Re: [PATCH V5 5/8] backlight: qcom-wled: Restructure the driver for WLED3

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]

Date:   Wed, 29 Aug 2018 12:40:24 +0530
From:   kgunda@...eaurora.org
To:     Pavel Machek <pavel@....cz>
Cc:     bjorn.andersson@...aro.org, jingoohan1@...il.com,
        lee.jones@...aro.org, b.zolnierkie@...sung.com,
        dri-devel@...ts.freedesktop.org, daniel.thompson@...aro.org,
        jacek.anaszewski@...il.com, robh+dt@...nel.org,
        mark.rutland@....com, linux-leds@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fbdev@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH V5 5/8] backlight: qcom-wled: Restructure the driver for
 WLED3

On 2018-08-27 15:39, Pavel Machek wrote:
> On Fri 2018-08-24 15:57:44, Kiran Gunda wrote:
>> Restructure the driver to add the support for new WLED
>> peripherals.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@...eaurora.org>
>> Acked-by: Daniel Thompson <daniel.thompson@...aro.org>
>> ---
>> Changes from V3:
>>     - This is the new patch after splitting the
>>       "backlight: qcom-wled: Add support for WLED4 peripheral" patch
>>       to seperate the WLED3 specific restructure.
>> 
>> Changes from V4:
>>     - Initialize wled->cfg.enabled_strings to 0,1,2,3.
>>     - Replaced the WLED3 macro with 3.
>> 
>>  drivers/video/backlight/qcom-wled.c | 395 
>> ++++++++++++++++++++++--------------
>>  1 file changed, 245 insertions(+), 150 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index 3cd6e75..a746bec 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -15,59 +15,71 @@
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/of_address.h>
>>  #include <linux/regmap.h>
>> 
>>  /* From DT binding */
>> +#define WLED_MAX_STRINGS				4
>> +
>>  #define WLED_DEFAULT_BRIGHTNESS				2048
>> 
>> -#define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>> -#define WLED3_CTRL_REG_VAL_BASE				0x40
>> +#define WLED_SINK_REG_BRIGHT_MAX			0xFFF
> 
> Stop this, no. In previous patch you renamed from ABC123_ to WLED3_,
> now you are renaming back to WLED?
> 
> Stop messing with the names. I'd actually prefer you to stick with
> original driver name, and just add note that this now supports more
> hardware.
> 
> But yes, _one_ rename is okay. I guess. But renaming it twice in one
> series is not acceptable.
> 
ok. I will stop renaming from WLED3 to WLED in this patch. I did it 
because these registers
are common for both WLED3 and WLED4 and Bjorn also suggested the same. 
Anyways I will stop
this renaming in this patch from WLED3 to WLED.

>> @@ -365,6 +433,15 @@ static int wled_configure(struct wled *wled, 
>> struct device *dev)
>> 
>>  	cfg->num_strings = cfg->num_strings + 1;
>> 
>> +	string_len = of_property_count_elems_of_size(dev->of_node,
>> +						     "qcom,enabled-strings",
>> +						     sizeof(u32));
>> +	if (string_len > 0)
>> +		rc = of_property_read_u32_array(dev->of_node,
>> +						"qcom,enabled-strings",
>> +						wled->cfg.enabled_strings,
>> +						sizeof(u32));
>> +
>>  	return 0;
>>  }
> 
> rc is assigned but never used.
Will address it next series.
> 									Pavel