* [PATCH] Staging: sm750fb: Remove unnecessary local variables
@ 2019-03-12 21:57 Madhumitha Prabakaran
2019-03-12 22:03 ` Greg KH
2019-03-12 22:09 ` [Outreachy kernel] " Julia Lawall
0 siblings, 2 replies; 6+ messages in thread
From: Madhumitha Prabakaran @ 2019-03-12 21:57 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh, outreachy-kernel
Cc: Madhumitha Prabakaran
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);
}
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: sm750fb: Remove unnecessary local variables
2019-03-12 21:57 [PATCH] Staging: sm750fb: Remove unnecessary local variables Madhumitha Prabakaran
@ 2019-03-12 22:03 ` Greg KH
2019-03-12 22:08 ` [Outreachy kernel] " Greg KH
2019-03-12 22:09 ` [Outreachy kernel] " Julia Lawall
1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-03-12 22:03 UTC (permalink / raw)
To: Madhumitha Prabakaran; +Cc: sudipm.mukherjee, teddy.wang, outreachy-kernel
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.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] Staging: sm750fb: Remove unnecessary local variables
2019-03-12 22:03 ` Greg KH
@ 2019-03-12 22:08 ` Greg KH
2019-03-12 22:26 ` Madhumthia Prabakaran
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-03-12 22:08 UTC (permalink / raw)
To: Madhumitha Prabakaran; +Cc: sudipm.mukherjee, teddy.wang, outreachy-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: sm750fb: Remove unnecessary local variables
2019-03-12 21:57 [PATCH] Staging: sm750fb: Remove unnecessary local variables Madhumitha Prabakaran
2019-03-12 22:03 ` Greg KH
@ 2019-03-12 22:09 ` Julia Lawall
2019-03-12 22:23 ` Madhumthia Prabakaran
1 sibling, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2019-03-12 22:09 UTC (permalink / raw)
To: Madhumitha Prabakaran; +Cc: outreachy-kernel
On Tue, 12 Mar 2019, 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);
I'm not sure what you are doing here. The proposed semantic patch doesn't
do this. It only matches cases where there is an assignment to a variable
and then a return of exactly that variable.
I have the impression that you are looking at the files that are handled
by Coccinelle, then searching in those files for something that looks
relevant, and then making the transformation by hand. You shouldn't do
that. Coccinelle will find the proper places and make the transformation
for you. You will have an appropriate patchon standard output. You can
then apply it to your code, check that it compiles, check that the format
is OK and checkpatch is happy, etc.
julia
> }
>
> /*
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190312215737.29200-1-madhumithabiw%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: sm750fb: Remove unnecessary local variables
2019-03-12 22:09 ` [Outreachy kernel] " Julia Lawall
@ 2019-03-12 22:23 ` Madhumthia Prabakaran
0 siblings, 0 replies; 6+ messages in thread
From: Madhumthia Prabakaran @ 2019-03-12 22:23 UTC (permalink / raw)
To: Julia Lawall, outreachy-kernel
On Tue, Mar 12, 2019 at 11:09:30PM +0100, Julia Lawall wrote:
>
>
> On Tue, 12 Mar 2019, 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);
>
> I'm not sure what you are doing here. The proposed semantic patch doesn't
> do this. It only matches cases where there is an assignment to a variable
> and then a return of exactly that variable.
>
> I have the impression that you are looking at the files that are handled
> by Coccinelle, then searching in those files for something that looks
> relevant, and then making the transformation by hand. You shouldn't do
> that. Coccinelle will find the proper places and make the transformation
> for you. You will have an appropriate patchon standard output. You can
> then apply it to your code, check that it compiles, check that the format
> is OK and checkpatch is happy, etc.
>
> julia
>
>
>
> > }
> >
> > /*
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190312215737.29200-1-madhumithabiw%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
Yeah, you are right. I have a misunderstanding about working of Coccinelle. I'm trying to understand it. I will drop this patch.
Thanks,
Madhumitha
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] Staging: sm750fb: Remove unnecessary local variables
2019-03-12 22:08 ` [Outreachy kernel] " Greg KH
@ 2019-03-12 22:26 ` Madhumthia Prabakaran
0 siblings, 0 replies; 6+ messages in thread
From: Madhumthia Prabakaran @ 2019-03-12 22:26 UTC (permalink / raw)
To: Greg KH, outreachy-kernel
On Tue, Mar 12, 2019 at 03:08:40PM -0700, Greg KH wrote:
> 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
Even I thought the same, when I was editing it but I was unsure about Coccinelle.
Thanks,
Madhumitha
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-12 22:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-12 21:57 [PATCH] Staging: sm750fb: Remove unnecessary local variables Madhumitha Prabakaran
2019-03-12 22:03 ` Greg KH
2019-03-12 22:08 ` [Outreachy kernel] " Greg KH
2019-03-12 22:26 ` Madhumthia Prabakaran
2019-03-12 22:09 ` [Outreachy kernel] " Julia Lawall
2019-03-12 22:23 ` Madhumthia Prabakaran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.