* [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] 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
* 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
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.