Re: [PATCH] leds: rgb: leds-qcom-lpg: Add support for 6-bit PWM resolution

[Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>]

On 2/20/2025 6:55 AM, Lee Jones wrote:
> On Wed, 12 Feb 2025, Anjelique Melendez wrote:
> 
>> Currently, driver only allows for PWM modules to use 9-bit resolution.
>> However, PWM modules can support 6-bit and 9-bit resolution. Add support
>> for 6-bit resolution.
>>
>> Suggested-by: Zejiong Huang <zejiongh@qti.qualcomm.com>
>> Signed-off-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
>> ---
>>   drivers/leds/rgb/leds-qcom-lpg.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
>> index f3c9ef2bfa57..4e5c56ded1f0 100644
>> --- a/drivers/leds/rgb/leds-qcom-lpg.c
>> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
>> @@ -24,6 +24,7 @@
>>   #define LPG_PATTERN_CONFIG_REG	0x40
>>   #define LPG_SIZE_CLK_REG	0x41
>>   #define  PWM_CLK_SELECT_MASK	GENMASK(1, 0)
>> +#define  PWM_SIZE_SELECT_MASK	BIT(2)
> 
> Are you sure you want to shove this between 2 seemingly related defines?
> 
We placed the PWM_SIZE_SELECT_MASK here so we could group all the masks 
used for PWM together then the masks used for Hi resolution PWM together
i.e
   1. pwm clk mask
   2. pwm size mask
   3. hi resolution pwm clk mask
   4. hi resolution pwm size mask

Would you rather have definitions grouped based on mask type?
i.e
   1. pwm clk mask
   2. hi resolution pwm clk mask
   3. pwm size mask
   4. hi resolution pwm size mask


>>   #define  PWM_CLK_SELECT_HI_RES_MASK	GENMASK(2, 0)
>>   #define  PWM_SIZE_HI_RES_MASK	GENMASK(6, 4)
>>   #define LPG_PREDIV_CLK_REG	0x42

Thanks,
Anjelique