* [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