Modifies the return type of different static functions to void as the return value is being ignored
in all funtion calls thus not useful to have a return type.This improves
code readability and maintainability.
Dorcas AnonoLitunya (2):
staging: sm750fb: Remove unused return value in
display_control_adjust_sm750le()
staging: sm750fb: Remove unused return value in
program_mode_registers()
drivers/staging/sm750fb/ddk750_mode.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--
2.42.0.345.gaab89be2eb
Modifies the return type of display_control_adjust_sm750le()
to void from unsigned long as the return value is being ignored in
all subsequent function calls.
This improves code readability and maintainability.
Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Dorcas AnonoLitunya <[email protected]>
---
drivers/staging/sm750fb/ddk750_mode.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_mode.c b/drivers/staging/sm750fb/ddk750_mode.c
index bcdd291d25c9..83ace6cc9583 100644
--- a/drivers/staging/sm750fb/ddk750_mode.c
+++ b/drivers/staging/sm750fb/ddk750_mode.c
@@ -13,7 +13,7 @@
* HW only supports 7 predefined pixel clocks, and clock select is
* in bit 29:27 of Display Control register.
*/
-static unsigned long
+static void
display_control_adjust_sm750le(struct mode_parameter *mode_param,
unsigned long disp_control)
{
@@ -70,8 +70,6 @@ display_control_adjust_sm750le(struct mode_parameter *mode_param,
disp_control |= DISPLAY_CTRL_CLOCK_PHASE;
poke32(CRT_DISPLAY_CTRL, disp_control);
-
- return disp_control;
}
/* only timing related registers will be programed */
--
2.42.0.345.gaab89be2eb
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 <[email protected]>
Signed-off-by: Dorcas AnonoLitunya <[email protected]>
---
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;
}
- 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 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 <[email protected]>
> Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> ---
> 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 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, 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 <[email protected]>
> > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > ---
> > 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, 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 <[email protected]>
> Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> ---
> 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, 18 Oct 2023, Dorcas AnonoLitunya wrote:
> Modifies the return type of different static functions to void as the return value is being ignored
> in all funtion calls thus not useful to have a return type.This improves
> code readability and maintainability.
Remember to put a space between a period and the start of the next
sentence.
julia
>
> Dorcas AnonoLitunya (2):
> staging: sm750fb: Remove unused return value in
> display_control_adjust_sm750le()
> staging: sm750fb: Remove unused return value in
> program_mode_registers()
>
> drivers/staging/sm750fb/ddk750_mode.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> --
> 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 <[email protected]>
> > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > ---
> > 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 <[email protected]>
> > > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > > ---
> > > 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 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 <[email protected]>
> > > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > Signed-off-by: Dorcas AnonoLitunya <[email protected]>
> > > > ---
> > > > 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