| Message ID | 20230921205025.20303-1-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | leds: turris-omnia: Fix unused variable | expand |
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?
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/
On Fri, 22 Sep 2023, Marek Behún wrote: > 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)? As I said before, I'm not going to force you into anything. Fire away.
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c index e1a4629479c5..f27241896970 100644 --- a/drivers/leds/leds-turris-omnia.c +++ b/drivers/leds/leds-turris-omnia.c @@ -60,7 +60,6 @@ struct omnia_leds { static int omnia_cmd_write_u8(const struct i2c_client *client, u8 cmd, u8 val) { u8 buf[2] = { cmd, val }; - int ret; return i2c_master_send(client, buf, sizeof(buf)); }
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(-)