* [PATCH] leds: lp3944: improve wording and formatting in a comment
@ 2016-01-27 14:31 Antonio Ospite
2016-01-27 14:56 ` Jacek Anaszewski
0 siblings, 1 reply; 6+ messages in thread
From: Antonio Ospite @ 2016-01-27 14:31 UTC (permalink / raw)
To: linux-leds; +Cc: Antonio Ospite, Richard Purdie, Jacek Anaszewski
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.
*/
if (led->type == LP3944_LED_TYPE_LED_INVERTED && status < 2)
status = 1 - status;
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: lp3944: improve wording and formatting in a comment
2016-01-27 14:31 [PATCH] leds: lp3944: improve wording and formatting in a comment Antonio Ospite
@ 2016-01-27 14:56 ` Jacek Anaszewski
2016-01-27 16:31 ` Antonio Ospite
0 siblings, 1 reply; 6+ messages in thread
From: Jacek Anaszewski @ 2016-01-27 14:56 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-leds, Richard Purdie
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.
> if (led->type == LP3944_LED_TYPE_LED_INVERTED && status < 2)
> status = 1 - status;
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: lp3944: improve wording and formatting in a comment
2016-01-27 14:56 ` Jacek Anaszewski
@ 2016-01-27 16:31 ` Antonio Ospite
2016-01-28 7:35 ` Jacek Anaszewski
0 siblings, 1 reply; 6+ messages in thread
From: Antonio Ospite @ 2016-01-27 16:31 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Richard Purdie
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] leds: lp3944: improve wording and formatting in a comment
2016-01-27 16:31 ` Antonio Ospite
@ 2016-01-28 7:35 ` Jacek Anaszewski
2016-01-28 15:49 ` [PATCHv2] " Antonio Ospite
0 siblings, 1 reply; 6+ messages in thread
From: Jacek Anaszewski @ 2016-01-28 7:35 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-leds, Richard Purdie
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2] leds: lp3944: improve wording and formatting in a comment
2016-01-28 7:35 ` Jacek Anaszewski
@ 2016-01-28 15:49 ` Antonio Ospite
2016-01-29 8:34 ` Jacek Anaszewski
0 siblings, 1 reply; 6+ messages in thread
From: Antonio Ospite @ 2016-01-28 15:49 UTC (permalink / raw)
To: linux-leds; +Cc: Antonio Ospite, Richard Purdie, Jacek Anaszewski
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>
---
Changes since v1:
- make the comment more explicit as suggested by Jacek
drivers/leds/leds-lp3944.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/leds-lp3944.c b/drivers/leds/leds-lp3944.c
index 6c758ae..be60c18 100644
--- a/drivers/leds/leds-lp3944.c
+++ b/drivers/leds/leds-lp3944.c
@@ -199,8 +199,11 @@ 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 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;
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2] leds: lp3944: improve wording and formatting in a comment
2016-01-28 15:49 ` [PATCHv2] " Antonio Ospite
@ 2016-01-29 8:34 ` Jacek Anaszewski
0 siblings, 0 replies; 6+ messages in thread
From: Jacek Anaszewski @ 2016-01-29 8:34 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-leds, Richard Purdie
On 01/28/2016 04:49 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>
> ---
>
> Changes since v1:
> - make the comment more explicit as suggested by Jacek
>
> drivers/leds/leds-lp3944.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp3944.c b/drivers/leds/leds-lp3944.c
> index 6c758ae..be60c18 100644
> --- a/drivers/leds/leds-lp3944.c
> +++ b/drivers/leds/leds-lp3944.c
> @@ -199,8 +199,11 @@ 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 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;
>
Applied, thanks.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-29 8:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-27 14:31 [PATCH] leds: lp3944: improve wording and formatting in a comment Antonio Ospite
2016-01-27 14:56 ` Jacek Anaszewski
2016-01-27 16:31 ` Antonio Ospite
2016-01-28 7:35 ` Jacek Anaszewski
2016-01-28 15:49 ` [PATCHv2] " Antonio Ospite
2016-01-29 8:34 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox