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

[Antonio Ospite <ao2@ao2.it>]

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().

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.
 */

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

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?