Re: [PATCH V3 4/7] backlight: qcom-wled: Rename PM8941* to WLED3

kgunda
2018-06-20T06:26:43Z

On 2018-06-20 04:41, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
>
>> Rename the PM8941* references as WLED3 to make the
>> driver generic and have WLED support for other PMICs.
>>
>
> Looks good, just three minor comments below.
>
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>> drivers/video/backlight/qcom-wled.c | 248
>> ++++++++++++++++++------------------
>> 1 file changed, 125 insertions(+), 123 deletions(-)
>>
>> diff --git a/drivers/video/backlight/qcom-wled.c
>> b/drivers/video/backlight/qcom-wled.c
>> index 0b6d219..812f3cb 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -18,77 +18,79 @@
>> #include <linux/regmap.h>
>>
>> /* From DT binding */
>> -#define PM8941_WLED_DEFAULT_BRIGHTNESS 2048
>> +#define WLED_DEFAULT_BRIGHTNESS 2048
>>
>> -#define PM8941_WLED_REG_VAL_BASE 0x40
>> -#define PM8941_WLED_REG_VAL_MAX 0xFFF
>> +#define WLED3_SINK_REG_BRIGHT_MAX 0xFFF
>> +#define WLED3_CTRL_REG_VAL_BASE 0x40
>>
>> -#define PM8941_WLED_REG_MOD_EN 0x46
>> -#define PM8941_WLED_REG_MOD_EN_BIT BIT(7)
>> -#define PM8941_WLED_REG_MOD_EN_MASK BIT(7)
>> +/* WLED3 control registers */
>> +#define WLED3_CTRL_REG_MOD_EN 0x46
>> +#define WLED3_CTRL_REG_MOD_EN_BIT BIT(7)
>> +#define WLED3_CTRL_REG_MOD_EN_MASK BIT(7)
>>
>> -#define PM8941_WLED_REG_SYNC 0x47
>
> These are in address order, any reason why WLED3_SINK_REG_SYNC moved
> down?
>
Actually, this is a sink specific register. That's why moved it to
under the /* WLED3 sink specific registers */
>> -#define PM8941_WLED_REG_SYNC_MASK 0x07
>> -#define PM8941_WLED_REG_SYNC_LED1 BIT(0)
>> -#define PM8941_WLED_REG_SYNC_LED2 BIT(1)
>> -#define PM8941_WLED_REG_SYNC_LED3 BIT(2)
>> -#define PM8941_WLED_REG_SYNC_ALL 0x07
>> -#define PM8941_WLED_REG_SYNC_CLEAR 0x00
>> +#define WLED3_CTRL_REG_FREQ 0x4c
>> +#define WLED3_CTRL_REG_FREQ_MASK 0x0f
>>
>> -#define PM8941_WLED_REG_FREQ 0x4c
>> -#define PM8941_WLED_REG_FREQ_MASK 0x0f
>> +#define WLED3_CTRL_REG_OVP 0x4d
>> +#define WLED3_CTRL_REG_OVP_MASK 0x03
>>
>> -#define PM8941_WLED_REG_OVP 0x4d
>> -#define PM8941_WLED_REG_OVP_MASK 0x03
>> +#define WLED3_CTRL_REG_ILIMIT 0x4e
>> +#define WLED3_CTRL_REG_ILIMIT_MASK 0x07
>>
>> -#define PM8941_WLED_REG_BOOST 0x4e
>> -#define PM8941_WLED_REG_BOOST_MASK 0x07
>> +/* WLED3 sink registers */
>> +#define WLED3_SINK_REG_SYNC 0x47
>> +#define WLED3_SINK_REG_SYNC_MASK 0x07
>> +#define WLED3_SINK_REG_SYNC_LED1 BIT(0)
>> +#define WLED3_SINK_REG_SYNC_LED2 BIT(1)
>> +#define WLED3_SINK_REG_SYNC_LED3 BIT(2)
>> +#define WLED3_SINK_REG_SYNC_ALL 0x07
>> +#define WLED3_SINK_REG_SYNC_CLEAR 0x00
>>
> [..]
>> -struct pm8941_wled_config {
>> - u32 i_boost_limit;
>> +struct wled_config {
>> + u32 boost_i_limit;
>> u32 ovp;
>> u32 switch_freq;
>> u32 num_strings;
>> - u32 i_limit;
>> + u32 string_i_limit;
>
> Changing the members in this struct seems unrelated.
>
Changed the members, to have resemblance with the IPCAT and
better readability .
>> bool cs_out_en;
>> bool ext_gen;
>> bool cabc_en;
>> };
>>
> [..]
>> -MODULE_DESCRIPTION("pm8941 wled driver");
>> +MODULE_DESCRIPTION("qcom wled driver");
>
> I would prefer if this was "Qualcomm WLED driver".
>
Sure. I will change it in the next series.
>> MODULE_LICENSE("GPL v2");
>
> Regards,
> Bjorn