Re: [PATCH 2/2] staging: sm750fb: Remove unused return value in program_mode_registers()

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Dorcas AnonoLitunya <anonolitunya@xxxxxxxxx>
> > > ---
> > >  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