All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Marek Behún" <kabel@kernel.org>
To: Lee Jones <lee@kernel.org>
Cc: linux-leds@vger.kernel.org
Subject: Re: [PATCH] leds: turris-omnia: Fix unused variable
Date: Fri, 22 Sep 2023 13:16:54 +0200	[thread overview]
Message-ID: <20230922131654.2172a184@dellmb> (raw)
In-Reply-To: <20230922065919.GA3660432@google.com>

On Fri, 22 Sep 2023 07:59:19 +0100
Lee Jones <lee@kernel.org> wrote:

> On Thu, 21 Sep 2023, Marek Behún wrote:
> 
> > The variable ret is not used in this function.
> > 
> > Fixes: 28350bc0ac77 ("leds: turris-omnia: Do not use SMBUS calls")
> > Closes: https://lore.kernel.org/linux-leds/202309212215.Yl5VQaSm-lkp@intel.com/T/#u
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/leds/leds-turris-omnia.c | 1 -
> >  1 file changed, 1 deletion(-)  
> 
> I already fixed and squashed this.
> 
> How was this missed when you tested the set?

I am not sure, but it is possible that I've refactored that function
from my original (return 0 on success) to your proposed (return number
of written bytes on success) and did not notice the warning. I am sure
I tested that the result works. Maybe I switched to another terminal
where I was testing it too fast and did not notice the warning.

Sorry about this.

Anyway, I have a question. Several days ago I also sent for review
a new driver for other feautres the Turris Omnia MCU provides (GPIO,
watchdog, wakeup+poweroff).
There, I also refactored the _write and _read functions as you
suggested (to return the number of bytes written/read).
On review, Andy Shevchenko requested [1] to refactor it to my original
(return 0 on success). I mentioned to him [2] your request, to which he
replied [3]:
  This is strange. For example, regmap APIs never returns amount of
  data written or read. I think it's solely depends on the API. It might
  be useful for i²c APIs, in case you can do something about it. but if
  you have wrappers on top of that already (meaning not using directly
  the i2c_*() calls, I dunno the positive return is anyhow useful.
Since I agree with him, taking this into account, would you accept a
patch that returns those function to how I originally wrote them
(return 0 on success)?

Thanks.

Marek

[1]
https://lore.kernel.org/linux-gpio/ZQmUFPvIx91+ps6k@smile.fi.intel.com/
[2]
https://lore.kernel.org/linux-gpio/ZQnn+Gi0xVlsGCYA@smile.fi.intel.com/
[3]
https://lore.kernel.org/linux-gpio/ZQnn+Gi0xVlsGCYA@smile.fi.intel.com/

  reply	other threads:[~2023-09-22 11:17 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-21 20:50 [PATCH] leds: turris-omnia: Fix unused variable Marek Behún
2023-09-22  6:59 ` Lee Jones
2023-09-22 11:16   ` Marek Behún [this message]
2023-09-25  7:53     ` Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230922131654.2172a184@dellmb \
    --to=kabel@kernel.org \
    --cc=lee@kernel.org \
    --cc=linux-leds@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.