| Message ID | 492e63bbc58147fb534930ef9e1fb5d844ae8769.1697619623.git.anonolitunya@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Remove unused return values from functions | expand |
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. thanks, greg k-h
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? thanks, Dorcas > thanks, > > greg k-h
On Wed, Oct 18, 2023 at 12:07:38PM +0300, Dorcas AnonoLitunya wrote: > -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; No need to set ret here. > } > - return ret; > } regards, dan carpenter
On Wed, 18 Oct 2023, 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; Is it still useful to have ret = -1? Maybe the ret variable is not useful at all any more, but one would have to check the parts of the function that aren't shown. julia > } > - return ret; > } > > int ddk750_set_mode_timing(struct mode_parameter *parm, enum clock_type clock) > -- > 2.42.0.345.gaab89be2eb > >
On Wed, Oct 18, 2023 at 02:06:41PM +0200, Julia Lawall wrote: > > > On Wed, 18 Oct 2023, 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; > > Is it still useful to have ret = -1? Maybe the ret variable is not useful > at all any more, but one would have to check the parts of the function > that aren't shown. > I agree Julia. I will remove the setting part for ret = -1 but keep the ret variable just in case it is being used by parts of the function not shown. Thanks for the feedback. Dorcas > julia > > > } > > - return ret; > > } > > > > int ddk750_set_mode_timing(struct mode_parameter *parm, enum clock_type clock) > > -- > > 2.42.0.345.gaab89be2eb > > > >
On Wed, 18 Oct 2023, Dorcas Litunya wrote: > On Wed, Oct 18, 2023 at 02:06:41PM +0200, Julia Lawall wrote: > > > > > > On Wed, 18 Oct 2023, 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; > > > > Is it still useful to have ret = -1? Maybe the ret variable is not useful > > at all any more, but one would have to check the parts of the function > > that aren't shown. > > > I agree Julia. I will remove the setting part for ret = -1 but keep the > ret variable just in case it is being used by parts of the function not > shown. Thanks for the feedback. You can check the rest of the function code and see if ret is still useful. julia > > Dorcas > > julia > > > > > } > > > - return ret; > > > } > > > > > > int ddk750_set_mode_timing(struct mode_parameter *parm, enum clock_type clock) > > > -- > > > 2.42.0.345.gaab89be2eb > > > > > > >
On Wed, Oct 18, 2023 at 03:59:27PM +0300, Dorcas Litunya wrote: > On Wed, Oct 18, 2023 at 02:06:41PM +0200, Julia Lawall wrote: > > > > > > On Wed, 18 Oct 2023, 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; > > > > Is it still useful to have ret = -1? Maybe the ret variable is not useful > > at all any more, but one would have to check the parts of the function > > that aren't shown. > > > I agree Julia. I will remove the setting part for ret = -1 but keep the > ret variable just in case it is being used by parts of the function not > shown. No, don't do that, you will trip other static checkers if you do so. Remove it entirely as it is obviously not needed anymore. thanks, greg k-h
On Wed, Oct 18, 2023 at 03:25:05PM +0200, Greg Kroah-Hartman wrote: > 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 Makes sense. I am not sure whether there exists a function that cannot fail. But for this patch I will start by assuming that it cannot fail and remove the error return variable altogether. Then after submission of the patch, I will look at whether the function can fail and see how to propagate the error. I think this should work? thanks, Dorcas
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
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; } - return ret; } int ddk750_set_mode_timing(struct mode_parameter *parm, enum clock_type clock)
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(-)