Re: [Outreachy kernel] Re: [PATCH] Staging: sm750fb: Remove unnecessary local variables

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 12, 2019 at 03:03:20PM -0700, Greg KH wrote:
> On Tue, Mar 12, 2019 at 04:57:37PM -0500, Madhumitha Prabakaran wrote:
> > Remove unnecessary local variables in function get_mxclk_freq.
> > Issue found by Coccinelle using ret.cocci.
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > ---
> >  drivers/staging/sm750fb/ddk750_chip.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
> > index 90f5480304f4..d0462f21fe36 100644
> > --- a/drivers/staging/sm750fb/ddk750_chip.c
> > +++ b/drivers/staging/sm750fb/ddk750_chip.c
> > @@ -33,19 +33,16 @@ void sm750_set_chip_type(unsigned short dev_id, u8 rev_id)
> >  
> >  static unsigned int get_mxclk_freq(void)
> >  {
> > -	unsigned int pll_reg;
> > -	unsigned int M, N, OD, POD;
> > -
> >  	if (sm750_get_chip_type() == SM750LE)
> >  		return MHz(130);
> >  
> > -	pll_reg = peek32(MXCLK_PLL_CTRL);
> > -	M = (pll_reg & PLL_CTRL_M_MASK) >> PLL_CTRL_M_SHIFT;
> > -	N = (pll_reg & PLL_CTRL_N_MASK) >> PLL_CTRL_N_SHIFT;
> > -	OD = (pll_reg & PLL_CTRL_OD_MASK) >> PLL_CTRL_OD_SHIFT;
> > -	POD = (pll_reg & PLL_CTRL_POD_MASK) >> PLL_CTRL_POD_SHIFT;
> > -
> > -	return DEFAULT_INPUT_CLOCK * M / N / (1 << OD) / (1 << POD);
> > +	return DEFAULT_INPUT_CLOCK * (peek32(MXCLK_PLL_CTRL) &
> > +	       PLL_CTRL_M_MASK) >> PLL_CTRL_M_SHIFT /
> > +	       (peek32(MXCLK_PLL_CTRL) & PLL_CTRL_N_MASK) >>
> > +	       PLL_CTRL_N_SHIFT / (1 << (peek32(MXCLK_PLL_CTRL) &
> > +	       PLL_CTRL_OD_MASK) >> PLL_CTRL_OD_SHIFT) / (1 <<
> > +	       (peek32(MXCLK_PLL_CTRL) & PLL_CTRL_POD_MASK) >>
> > +	       PLL_CTRL_POD_SHIFT);
> 
> Oh wow, that's almost impossible to now read.
> 
> Sometimes you want intermediate variables for the programmer, not for
> the compiler.  The end result is the same, but please tell me that you
> could figure out what that one single return line actually does :(
> 
> Remember, we write C code for people to understand first, and the CPU to
> understand second as we have to maintain it for the future.

Also, this change might be wrong, and at the least, will be slower as
you now call peek() a lot more times.  Is that a function that has no
side-affects on the hardware?  At the least it is doing a device access,
so you only want to do that once if possible.

Remember, there is real hardware behind the driver, that's why the
driver is written in the first place :)

thanks,

greg k-h