From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Dorcas Litunya <anonolitunya@gmail.com>
Cc: outreachy@lists.linux.dev, julia.lawall@inria.fr,
dan.carpenter@linaro.org, andi.shyti@linux.intel.com,
Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
Teddy Wang <teddy.wang@siliconmotion.com>,
linux-fbdev@vger.kernel.org, linux-staging@lists.linux.dev,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] staging: sm750fb: Remove unused return value in program_mode_registers()
Date: Wed, 18 Oct 2023 15:25:05 +0200 [thread overview]
Message-ID: <2023101830-janitor-trapezoid-bb48@gregkh> (raw)
In-Reply-To: <ZS+moqE585lPBN3m@dorcaslitunya-virtual-machine>
On Wed, Oct 18, 2023 at 12:34:26PM +0300, Dorcas Litunya wrote:
> On Wed, Oct 18, 2023 at 11:26:33AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Oct 18, 2023 at 12:07:38PM +0300, Dorcas AnonoLitunya wrote:
> > > Modifies the return type of program_mode_registers()
> > > to void from int as the return value is being ignored in
> > > all subsequent function calls.
> > >
> > > This improves code readability and maintainability.
> > >
> > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Dorcas AnonoLitunya <anonolitunya@gmail.com>
> > > ---
> > > drivers/staging/sm750fb/ddk750_mode.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/sm750fb/ddk750_mode.c b/drivers/staging/sm750fb/ddk750_mode.c
> > > index 83ace6cc9583..e15039238232 100644
> > > --- a/drivers/staging/sm750fb/ddk750_mode.c
> > > +++ b/drivers/staging/sm750fb/ddk750_mode.c
> > > @@ -73,8 +73,8 @@ display_control_adjust_sm750le(struct mode_parameter *mode_param,
> > > }
> > >
> > > /* only timing related registers will be programed */
> > > -static int program_mode_registers(struct mode_parameter *mode_param,
> > > - struct pll_value *pll)
> > > +static void program_mode_registers(struct mode_parameter *mode_param,
> > > + struct pll_value *pll)
> > > {
> > > int ret = 0;
> > > int cnt = 0;
> > > @@ -202,7 +202,6 @@ static int program_mode_registers(struct mode_parameter *mode_param,
> > > } else {
> > > ret = -1;
> >
> > Why are you still setting the 'ret' variable if you are not doing
> > anything with it anymore?
> >
> > > }
> > > - return ret;
> >
> > Are you sure that the caller shouldn't be checking for errors instead of
> > dropping the return value? If so, document that in the changelog too.
> >
> Seems like the caller doesn't use the function to check for errors as in
> the code below:
>
> int ddk750_set_mode_timing(struct mode_parameter *parm, enum clock_type clock)
> {
> struct pll_value pll;
>
> pll.input_freq = DEFAULT_INPUT_CLOCK;
> pll.clock_type = clock;
>
> sm750_calc_pll_value(parm->pixel_clock, &pll);
> if (sm750_get_chip_type() == SM750LE) {
> /* set graphic mode via IO method */
> outb_p(0x88, 0x3d4);
> outb_p(0x06, 0x3d5);
> }
> program_mode_registers(parm, &pll);
> return 0;
>
> It will still return 0 regardless of whether there is an error or not.
> Since I am not sure how the two functions relate to one another, is
> there need to check error in the caller function?
That is correct, it is not checking for errors, but shouldn't it? If
the function can fail, then it should have proper error handling so
return the correct error (hint -1 is not a valid error), and then
propagate it up the call chain correctly as well.
For doing this type of work, either the function can not fail so there
can not be an error return value, or it can, and then the error should
be propagated correctly. Sorry for not spelling that out earlier.
thanks,
greg k-h
next prev parent reply other threads:[~2023-10-18 13:25 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 9:07 [PATCH 0/2] Remove unused return values from functions Dorcas AnonoLitunya
2023-10-18 9:07 ` [PATCH 1/2] staging: sm750fb: Remove unused return value in display_control_adjust_sm750le() Dorcas AnonoLitunya
2023-10-18 9:07 ` [PATCH 2/2] staging: sm750fb: Remove unused return value in program_mode_registers() Dorcas AnonoLitunya
2023-10-18 9:26 ` Greg Kroah-Hartman
2023-10-18 9:34 ` Dorcas Litunya
2023-10-18 13:25 ` Greg Kroah-Hartman [this message]
2023-10-18 13:24 ` Dorcas Litunya
2023-10-18 9:34 ` Dan Carpenter
2023-10-18 12:06 ` Julia Lawall
2023-10-18 12:59 ` Dorcas Litunya
2023-10-18 13:15 ` Julia Lawall
2023-10-18 13:23 ` Greg Kroah-Hartman
2023-10-18 12:07 ` [PATCH 0/2] Remove unused return values from functions Julia Lawall
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=2023101830-janitor-trapezoid-bb48@gregkh \
--to=gregkh@linuxfoundation.org \
--cc=andi.shyti@linux.intel.com \
--cc=anonolitunya@gmail.com \
--cc=dan.carpenter@linaro.org \
--cc=julia.lawall@inria.fr \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=outreachy@lists.linux.dev \
--cc=sudipm.mukherjee@gmail.com \
--cc=teddy.wang@siliconmotion.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).