Re: [PATCH] leds: lp3944: improve wording and formatting in a comment

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 01/27/2016 05:31 PM, Antonio Ospite wrote:
> On Wed, 27 Jan 2016 15:56:21 +0100
> Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>
>> Hi Antonio,
>>
>> Thanks for the patch.
>>
>> On 01/27/2016 03:31 PM, Antonio Ospite wrote:
>>> Improve the wording for the comment about the led status, and while at
>>> it also use the standard formatting for multi-line comments.
>>>
>>> Signed-off-by: Antonio Ospite <ao2@ao2.it>
>>> Cc: Richard Purdie <rpurdie@rpsys.net>
>>> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
>>> ---
>>>    drivers/leds/leds-lp3944.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-lp3944.c b/drivers/leds/leds-lp3944.c
>>> index 6c758ae..793838e 100644
>>> --- a/drivers/leds/leds-lp3944.c
>>> +++ b/drivers/leds/leds-lp3944.c
>>> @@ -199,8 +199,10 @@ static int lp3944_led_set(struct lp3944_led_data *led, u8 status)
>>>    	if (status > LP3944_LED_STATUS_DIM1)
>>>    		return -EINVAL;
>>>
>>> -	/* invert only 0 and 1, leave unchanged the other values,
>>> -	 * remember we are abusing status to set blink patterns
>>> +	/*
>>> +	 * Invert status only if it is < 2 (i.e. 0 or 1), leave it unchanged
>>> +	 * otherwise because we are abusing the status to set blink patterns
>>> +	 * too.
>>>    	 */
>>
>> The "too" at the end doesn't improve the comment. It adds some
>> incomplete information. Could you explain what does it mean that status
>> is abused to set blink patterns, please? And usually "too" means
>> "in addition to something", but here we don't know what we are referring
>> to.
>>
>
> Having both the words "abusing" and "too" in the same sentence can be
> confusing, indeed, I see your point.
>
> It could have been either:
>    "abusing the status to set blink patterns."
> or
>    "using the status to set blink patterns too."
>
> The point being that the lp3944_led_set() functions uses the status
> argument not only for the on/off state, but also as a mechanism to
> enable the hardware blinking functionality.
> So when status >= 2 we don't want to invert the status value.
>
> The inversion of the actual duty cycle for inverted LEDs is properly
> handled in lp3944_led_set_blink(), so it's unrelated to
> lp3944_led_set().

Thanks for the explanation.

>
> This is not great, I know, but it's an hack internal to the driver to
> reuse some code, it's not exposed to userspace.
>
> This was one of my first drivers, maybe I'd do things differently today.
>
> Anyhow, do you prefer a more explicit explanation like this one below?
>
> /*
>   * Invert status only when it's < 2 (i.e. 0 or 1) which means it's
>   * controlling the on/off state directly.
>   * When, instead, status is >= 2 don't invert it because it would mean
>   * to mess with the hardware blinking mode.
>   */

It's more informative now. Please modify the patch accordingly.

>>>    	if (led->type == LP3944_LED_TYPE_LED_INVERTED && status < 2)
>>>    		status = 1 - status; >
>>
>
> Thanks,
>     Antonio
>


-- 
Best Regards,
Jacek Anaszewski