Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-28 Thread Greg Kroah-Hartman 
On Tue, Feb 28, 2017 at 01:59:53PM +0530, Arushi Singhal wrote:

Please fix your email client to not send html mail, otherwise the
mailing lists reject them, and your quoting is all messed up :(

> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
> gre...@linuxfoundation.org> wrote:
> 
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal 
> > ---
> > changes in v6
> >   - changes done such that no other errors can generate.
> >   - Improve the coding style.
> >   - Introduced new variables.
> >   - type of the variable is changed.
> >
> >  drivers/staging/xgifb/XGI_main_26.c | 29 ++---
> >  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
> >  2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/
> XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> xgifb_video_info *xgifb_info)
> >                       }
> >
> >                       if ((filter >= 0) && (filter <= 7)) {
> > +                             const u8 *f = XGI_TV_filter[filter_tb].
> filter[filter];
> >                               pr_debug("FilterTable[%d]-%d: %*ph\n",
> > -                                      filter_tb, filter,
> > -                                      4, XGI_TV_filter[filter_tb].
> > -                                                filter[filter]);
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x35,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][0]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x36,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][1]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x37,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][2]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x38,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][3]));
> > +                                      filter_tb, filter, 4, f);
> > +                             xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > +                             xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > +                             xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > +                             xgifb_reg_set(XGIPART2, 0x38, f[3]);
> >                       }
> >               }
> >       }
> > diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/
> vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> >
> >       for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> >              tempbx; (*i)--) {
> > -             infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > -                             Ext_InfoFlag;
> > +             unsigned short j;
> > +
> > +             j = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > +             infoflag = j;
> > +
> >               if (infoflag & tempax)
> >                       return 1;
> 
> 
> Why are you using a temporary variable 'j' here?  It's not needed at
> all, and just is confusing to read the code now, don't you agree?
> 
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a
> line will not increase by 80.

I know _why_ you are doing it, sorry, the point is that it makes no
sense at all to

Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-28 Thread Greg Kroah-Hartman 
On Tue, Feb 28, 2017 at 01:59:53PM +0530, Arushi Singhal wrote:

Please fix your email client to not send html mail, otherwise the
mailing lists reject them, and your quoting is all messed up :(

> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman <
> gre...@linuxfoundation.org> wrote:
> 
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
> >
> > Signed-off-by: Arushi Singhal 
> > ---
> > changes in v6
> >   - changes done such that no other errors can generate.
> >   - Improve the coding style.
> >   - Introduced new variables.
> >   - type of the variable is changed.
> >
> >  drivers/staging/xgifb/XGI_main_26.c | 29 ++---
> >  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
> >  2 files changed, 17 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/
> XGI_main_26.c
> > index 69ed137337ce..9870ea3b76b4 100644
> > --- a/drivers/staging/xgifb/XGI_main_26.c
> > +++ b/drivers/staging/xgifb/XGI_main_26.c
> > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct
> xgifb_video_info *xgifb_info)
> >                       }
> >
> >                       if ((filter >= 0) && (filter <= 7)) {
> > +                             const u8 *f = XGI_TV_filter[filter_tb].
> filter[filter];
> >                               pr_debug("FilterTable[%d]-%d: %*ph\n",
> > -                                      filter_tb, filter,
> > -                                      4, XGI_TV_filter[filter_tb].
> > -                                                filter[filter]);
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x35,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][0]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x36,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][1]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x37,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][2]));
> > -                             xgifb_reg_set(
> > -                                     XGIPART2,
> > -                                     0x38,
> > -                                     (XGI_TV_filter[filter_tb].
> > -                                             filter[filter][3]));
> > +                                      filter_tb, filter, 4, f);
> > +                             xgifb_reg_set(XGIPART2, 0x35, f[0]);
> > +                             xgifb_reg_set(XGIPART2, 0x36, f[1]);
> > +                             xgifb_reg_set(XGIPART2, 0x37, f[2]);
> > +                             xgifb_reg_set(XGIPART2, 0x38, f[3]);
> >                       }
> >               }
> >       }
> > diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/
> vb_setmode.c
> > index 7c7c8c8f1df3..249a32804c06 100644
> > --- a/drivers/staging/xgifb/vb_setmode.c
> > +++ b/drivers/staging/xgifb/vb_setmode.c
> > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned
> short ModeIdIndex,
> >
> >       for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
> >              tempbx; (*i)--) {
> > -             infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> > -                             Ext_InfoFlag;
> > +             unsigned short j;
> > +
> > +             j = XGI330_RefIndex[RefreshRateTableIndex +
> (*i)].Ext_InfoFlag;
> > +             infoflag = j;
> > +
> >               if (infoflag & tempax)
> >                       return 1;
> 
> 
> Why are you using a temporary variable 'j' here?  It's not needed at
> all, and just is confusing to read the code now, don't you agree?
> 
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a
> line will not increase by 80.

I know _why_ you are doing it, sorry, the point is that it makes no
sense at all to _do_ that.  Please don't just

Re: [Outreachy kernel] Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-28 Thread Julia Lawall 


On Tue, 28 Feb 2017, Arushi Singhal wrote:

>
>
> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman 
>  wrote:
>   On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
>   > Error reported by checkpatch.pl as "avoid multiple line dereference".
>   > Addition of new variables to make the code more readable and also to
>   > correct about mentioned error as by itroducing new variables line is
>   > not exceeding 80 characters.
>   >
>   > Signed-off-by: Arushi Singhal 
>   > ---
>   > changes in v6
>   >   - changes done such that no other errors can generate.
>   >   - Improve the coding style.
>   >   - Introduced new variables.
>   >   - type of the variable is changed.
>   >
>   >  drivers/staging/xgifb/XGI_main_26.c | 29 
> ++---
>   >  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
>   >  2 files changed, 17 insertions(+), 29 deletions(-)
>   >
>   > diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
>   > index 69ed137337ce..9870ea3b76b4 100644
>   > --- a/drivers/staging/xgifb/XGI_main_26.c
>   > +++ b/drivers/staging/xgifb/XGI_main_26.c
>   > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct 
> xgifb_video_info *xgifb_info)
>   >                       }
>   >
>   >                       if ((filter >= 0) && (filter <= 7)) {
>   > +                             const u8 *f = 
> XGI_TV_filter[filter_tb].filter[filter];
>   >                               pr_debug("FilterTable[%d]-%d: %*ph\n",
>   > -                                      filter_tb, filter,
>   > -                                      4, XGI_TV_filter[filter_tb].
>   > -                                                filter[filter]);
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x35,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][0]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x36,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][1]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x37,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][2]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x38,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][3]));
>   > +                                      filter_tb, filter, 4, f);
>   > +                             xgifb_reg_set(XGIPART2, 0x35, f[0]);
>   > +                             xgifb_reg_set(XGIPART2, 0x36, f[1]);
>   > +                             xgifb_reg_set(XGIPART2, 0x37, f[2]);
>   > +                             xgifb_reg_set(XGIPART2, 0x38, f[3]);
>   >                       }
>   >               }
>   >       }
>   > diff --git a/drivers/staging/xgifb/vb_setmode.c 
> b/drivers/staging/xgifb/vb_setmode.c
>   > index 7c7c8c8f1df3..249a32804c06 100644
>   > --- a/drivers/staging/xgifb/vb_setmode.c
>   > +++ b/drivers/staging/xgifb/vb_setmode.c
>   > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned 
> short ModeIdIndex,
>   >
>   >       for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
>   >              tempbx; (*i)--) {
>   > -             infoflag = XGI330_RefIndex[RefreshRateTableIndex + 
> (*i)].
>   > -                             Ext_InfoFlag;
>   > +             unsigned short j;
>   > +
>   > +             j = XGI330_RefIndex[RefreshRateTableIndex + 
> (*i)].Ext_InfoFlag;
>   > +             infoflag = j;
>   > +
>   >               if (infoflag & tempax)
>   >                       return 1;
>
>
> Why are you using a temporary variable 'j' here?  It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
>
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a 
> line will
> not increase by 80.

I agree with Greg that this is not a readable solution.  Putting the whole
thing on one line would be

Re: [Outreachy kernel] Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-28 Thread Julia Lawall 


On Tue, 28 Feb 2017, Arushi Singhal wrote:

>
>
> On Tue, Feb 28, 2017 at 11:21 AM, Greg Kroah-Hartman 
>  wrote:
>   On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
>   > Error reported by checkpatch.pl as "avoid multiple line dereference".
>   > Addition of new variables to make the code more readable and also to
>   > correct about mentioned error as by itroducing new variables line is
>   > not exceeding 80 characters.
>   >
>   > Signed-off-by: Arushi Singhal 
>   > ---
>   > changes in v6
>   >   - changes done such that no other errors can generate.
>   >   - Improve the coding style.
>   >   - Introduced new variables.
>   >   - type of the variable is changed.
>   >
>   >  drivers/staging/xgifb/XGI_main_26.c | 29 
> ++---
>   >  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
>   >  2 files changed, 17 insertions(+), 29 deletions(-)
>   >
>   > diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
>   > index 69ed137337ce..9870ea3b76b4 100644
>   > --- a/drivers/staging/xgifb/XGI_main_26.c
>   > +++ b/drivers/staging/xgifb/XGI_main_26.c
>   > @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct 
> xgifb_video_info *xgifb_info)
>   >                       }
>   >
>   >                       if ((filter >= 0) && (filter <= 7)) {
>   > +                             const u8 *f = 
> XGI_TV_filter[filter_tb].filter[filter];
>   >                               pr_debug("FilterTable[%d]-%d: %*ph\n",
>   > -                                      filter_tb, filter,
>   > -                                      4, XGI_TV_filter[filter_tb].
>   > -                                                filter[filter]);
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x35,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][0]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x36,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][1]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x37,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][2]));
>   > -                             xgifb_reg_set(
>   > -                                     XGIPART2,
>   > -                                     0x38,
>   > -                                     (XGI_TV_filter[filter_tb].
>   > -                                             filter[filter][3]));
>   > +                                      filter_tb, filter, 4, f);
>   > +                             xgifb_reg_set(XGIPART2, 0x35, f[0]);
>   > +                             xgifb_reg_set(XGIPART2, 0x36, f[1]);
>   > +                             xgifb_reg_set(XGIPART2, 0x37, f[2]);
>   > +                             xgifb_reg_set(XGIPART2, 0x38, f[3]);
>   >                       }
>   >               }
>   >       }
>   > diff --git a/drivers/staging/xgifb/vb_setmode.c 
> b/drivers/staging/xgifb/vb_setmode.c
>   > index 7c7c8c8f1df3..249a32804c06 100644
>   > --- a/drivers/staging/xgifb/vb_setmode.c
>   > +++ b/drivers/staging/xgifb/vb_setmode.c
>   > @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned 
> short ModeIdIndex,
>   >
>   >       for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
>   >              tempbx; (*i)--) {
>   > -             infoflag = XGI330_RefIndex[RefreshRateTableIndex + 
> (*i)].
>   > -                             Ext_InfoFlag;
>   > +             unsigned short j;
>   > +
>   > +             j = XGI330_RefIndex[RefreshRateTableIndex + 
> (*i)].Ext_InfoFlag;
>   > +             infoflag = j;
>   > +
>   >               if (infoflag & tempax)
>   >                       return 1;
>
>
> Why are you using a temporary variable 'j' here?  It's not needed at
> all, and just is confusing to read the code now, don't you agree?
>
>
> I am using temporary variable of small size(character) so that when
> I fixed the multiple line dereference then the line number of characters in a 
> line will
> not increase by 80.

I agree with Greg that this is not a readable solution.  Putting the whole
thing on one line would be better, even if it goes over 80 characters.
Having the field on a

Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Joe Perches 
On Tue, 2017-02-28 at 06:51 +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
[]
> Remember, coding style cleanups are to be done to make the code easier
> to understand and follow.  Not to blindly follow a perl script that
> can not think.  Sometimes it is not right...

Yeah, what Greg said.

Also very long identifier names like RefreshRateTableIndex
and simple dereferences like
XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag
make using 80 columns silly.

Just ignore 80 column limits when there are 20+ character
identifiers.  Ideally shorten the identifiers to something
less verbose and/or use temporaries where appropriate like
I showed in my reply to your suggested V5 patch.

Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Joe Perches 
On Tue, 2017-02-28 at 06:51 +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> > Error reported by checkpatch.pl as "avoid multiple line dereference".
> > Addition of new variables to make the code more readable and also to
> > correct about mentioned error as by itroducing new variables line is
> > not exceeding 80 characters.
[]
> Remember, coding style cleanups are to be done to make the code easier
> to understand and follow.  Not to blindly follow a perl script that
> can not think.  Sometimes it is not right...

Yeah, what Greg said.

Also very long identifier names like RefreshRateTableIndex
and simple dereferences like
XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag
make using 80 columns silly.

Just ignore 80 column limits when there are 20+ character
identifiers.  Ideally shorten the identifiers to something
less verbose and/or use temporaries where appropriate like
I showed in my reply to your suggested V5 patch.

Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Greg Kroah-Hartman 
On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> Error reported by checkpatch.pl as "avoid multiple line dereference".
> Addition of new variables to make the code more readable and also to
> correct about mentioned error as by itroducing new variables line is
> not exceeding 80 characters.
> 
> Signed-off-by: Arushi Singhal 
> ---
> changes in v6
>   - changes done such that no other errors can generate.
>   - Improve the coding style.
>   - Introduced new variables.
>   - type of the variable is changed.
> 
>  drivers/staging/xgifb/XGI_main_26.c | 29 ++---
>  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
>  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 69ed137337ce..9870ea3b76b4 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info 
> *xgifb_info)
>   }
>  
>   if ((filter >= 0) && (filter <= 7)) {
> + const u8 *f = 
> XGI_TV_filter[filter_tb].filter[filter];
>   pr_debug("FilterTable[%d]-%d: %*ph\n",
> -  filter_tb, filter,
> -  4, XGI_TV_filter[filter_tb].
> -filter[filter]);
> - xgifb_reg_set(
> - XGIPART2,
> - 0x35,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][0]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x36,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][1]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x37,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][2]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x38,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][3]));
> +  filter_tb, filter, 4, f);
> + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> + xgifb_reg_set(XGIPART2, 0x38, f[3]);
>   }
>   }
>   }
> diff --git a/drivers/staging/xgifb/vb_setmode.c 
> b/drivers/staging/xgifb/vb_setmode.c
> index 7c7c8c8f1df3..249a32804c06 100644
> --- a/drivers/staging/xgifb/vb_setmode.c
> +++ b/drivers/staging/xgifb/vb_setmode.c
> @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
> ModeIdIndex,
>  
>   for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
>  tempbx; (*i)--) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short j;
> +
> + j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = j;
> +
>   if (infoflag & tempax)
>   return 1;


Why are you using a temporary variable 'j' here?  It's not needed at
all, and just is confusing to read the code now, don't you agree?

> @@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
> ModeIdIndex,
>   }
>  
>   for ((*i) = 0;; (*i)++) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short m;
> +
> + m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = m;
> +
>   if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
>   != tempbx) {
>   return 0;

Same here, why add a new variable that isn't used more than once?  You
are trying to work around something that doesn't make sense to work
around.

Remember, coding style cleanups are to be done to make the code easier
to understand and follow.  Not to blindly follow a perl script that
can not think.  Sometimes it is not right...

thanks,

greg k-h

Re: [PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Greg Kroah-Hartman 
On Tue, Feb 28, 2017 at 10:35:30AM +0530, Arushi Singhal wrote:
> Error reported by checkpatch.pl as "avoid multiple line dereference".
> Addition of new variables to make the code more readable and also to
> correct about mentioned error as by itroducing new variables line is
> not exceeding 80 characters.
> 
> Signed-off-by: Arushi Singhal 
> ---
> changes in v6
>   - changes done such that no other errors can generate.
>   - Improve the coding style.
>   - Introduced new variables.
>   - type of the variable is changed.
> 
>  drivers/staging/xgifb/XGI_main_26.c | 29 ++---
>  drivers/staging/xgifb/vb_setmode.c  | 17 +++--
>  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/XGI_main_26.c 
> b/drivers/staging/xgifb/XGI_main_26.c
> index 69ed137337ce..9870ea3b76b4 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info 
> *xgifb_info)
>   }
>  
>   if ((filter >= 0) && (filter <= 7)) {
> + const u8 *f = 
> XGI_TV_filter[filter_tb].filter[filter];
>   pr_debug("FilterTable[%d]-%d: %*ph\n",
> -  filter_tb, filter,
> -  4, XGI_TV_filter[filter_tb].
> -filter[filter]);
> - xgifb_reg_set(
> - XGIPART2,
> - 0x35,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][0]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x36,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][1]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x37,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][2]));
> - xgifb_reg_set(
> - XGIPART2,
> - 0x38,
> - (XGI_TV_filter[filter_tb].
> - filter[filter][3]));
> +  filter_tb, filter, 4, f);
> + xgifb_reg_set(XGIPART2, 0x35, f[0]);
> + xgifb_reg_set(XGIPART2, 0x36, f[1]);
> + xgifb_reg_set(XGIPART2, 0x37, f[2]);
> + xgifb_reg_set(XGIPART2, 0x38, f[3]);
>   }
>   }
>   }
> diff --git a/drivers/staging/xgifb/vb_setmode.c 
> b/drivers/staging/xgifb/vb_setmode.c
> index 7c7c8c8f1df3..249a32804c06 100644
> --- a/drivers/staging/xgifb/vb_setmode.c
> +++ b/drivers/staging/xgifb/vb_setmode.c
> @@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
> ModeIdIndex,
>  
>   for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
>  tempbx; (*i)--) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short j;
> +
> + j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = j;
> +
>   if (infoflag & tempax)
>   return 1;


Why are you using a temporary variable 'j' here?  It's not needed at
all, and just is confusing to read the code now, don't you agree?

> @@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
> ModeIdIndex,
>   }
>  
>   for ((*i) = 0;; (*i)++) {
> - infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
> - Ext_InfoFlag;
> + unsigned short m;
> +
> + m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
> + infoflag = m;
> +
>   if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
>   != tempbx) {
>   return 0;

Same here, why add a new variable that isn't used more than once?  You
are trying to work around something that doesn't make sense to work
around.

Remember, coding style cleanups are to be done to make the code easier
to understand and follow.  Not to blindly follow a perl script that
can not think.  Sometimes it is not right...

thanks,

greg k-h

[PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Arushi Singhal 
Error reported by checkpatch.pl as "avoid multiple line dereference".
Addition of new variables to make the code more readable and also to
correct about mentioned error as by itroducing new variables line is
not exceeding 80 characters.

Signed-off-by: Arushi Singhal 
---
changes in v6
  - changes done such that no other errors can generate.
  - Improve the coding style.
  - Introduced new variables.
  - type of the variable is changed.

 drivers/staging/xgifb/XGI_main_26.c | 29 ++---
 drivers/staging/xgifb/vb_setmode.c  | 17 +++--
 2 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c 
b/drivers/staging/xgifb/XGI_main_26.c
index 69ed137337ce..9870ea3b76b4 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info 
*xgifb_info)
}
 
if ((filter >= 0) && (filter <= 7)) {
+   const u8 *f = 
XGI_TV_filter[filter_tb].filter[filter];
pr_debug("FilterTable[%d]-%d: %*ph\n",
-filter_tb, filter,
-4, XGI_TV_filter[filter_tb].
-  filter[filter]);
-   xgifb_reg_set(
-   XGIPART2,
-   0x35,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][0]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x36,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][1]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x37,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][2]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x38,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][3]));
+filter_tb, filter, 4, f);
+   xgifb_reg_set(XGIPART2, 0x35, f[0]);
+   xgifb_reg_set(XGIPART2, 0x36, f[1]);
+   xgifb_reg_set(XGIPART2, 0x37, f[2]);
+   xgifb_reg_set(XGIPART2, 0x38, f[3]);
}
}
}
diff --git a/drivers/staging/xgifb/vb_setmode.c 
b/drivers/staging/xgifb/vb_setmode.c
index 7c7c8c8f1df3..249a32804c06 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
ModeIdIndex,
 
for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
   tempbx; (*i)--) {
-   infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-   Ext_InfoFlag;
+   unsigned short j;
+
+   j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+   infoflag = j;
+
if (infoflag & tempax)
return 1;
 
@@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
ModeIdIndex,
}
 
for ((*i) = 0;; (*i)++) {
-   infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-   Ext_InfoFlag;
+   unsigned short m;
+
+   m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+   infoflag = m;
+
if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
!= tempbx) {
return 0;
@@ -5092,8 +5098,7 @@ unsigned short XGI_GetRatePtrCRT2(struct 
xgi_hw_device_info *pXGIHWDE,
 
i = 0;
do {
-   if (XGI330_RefIndex[RefreshRateTableIndex + i].
-   ModeID != ModeNo)
+   if (XGI330_RefIndex[RefreshRateTableIndex + i].ModeID != ModeNo)
break;
temp = XGI330_RefIndex[RefreshRateTableIndex + i].Ext_InfoFlag;
temp &= ModeTypeMask;
-- 
2.11.0

[PATCH v6] staging: xgifb: correct the multiple line dereference

2017-02-27 Thread Arushi Singhal 
Error reported by checkpatch.pl as "avoid multiple line dereference".
Addition of new variables to make the code more readable and also to
correct about mentioned error as by itroducing new variables line is
not exceeding 80 characters.

Signed-off-by: Arushi Singhal 
---
changes in v6
  - changes done such that no other errors can generate.
  - Improve the coding style.
  - Introduced new variables.
  - type of the variable is changed.

 drivers/staging/xgifb/XGI_main_26.c | 29 ++---
 drivers/staging/xgifb/vb_setmode.c  | 17 +++--
 2 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c 
b/drivers/staging/xgifb/XGI_main_26.c
index 69ed137337ce..9870ea3b76b4 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -878,30 +878,13 @@ static void XGIfb_post_setmode(struct xgifb_video_info 
*xgifb_info)
}
 
if ((filter >= 0) && (filter <= 7)) {
+   const u8 *f = 
XGI_TV_filter[filter_tb].filter[filter];
pr_debug("FilterTable[%d]-%d: %*ph\n",
-filter_tb, filter,
-4, XGI_TV_filter[filter_tb].
-  filter[filter]);
-   xgifb_reg_set(
-   XGIPART2,
-   0x35,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][0]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x36,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][1]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x37,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][2]));
-   xgifb_reg_set(
-   XGIPART2,
-   0x38,
-   (XGI_TV_filter[filter_tb].
-   filter[filter][3]));
+filter_tb, filter, 4, f);
+   xgifb_reg_set(XGIPART2, 0x35, f[0]);
+   xgifb_reg_set(XGIPART2, 0x36, f[1]);
+   xgifb_reg_set(XGIPART2, 0x37, f[2]);
+   xgifb_reg_set(XGIPART2, 0x38, f[3]);
}
}
}
diff --git a/drivers/staging/xgifb/vb_setmode.c 
b/drivers/staging/xgifb/vb_setmode.c
index 7c7c8c8f1df3..249a32804c06 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -221,8 +221,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
ModeIdIndex,
 
for (; XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID ==
   tempbx; (*i)--) {
-   infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-   Ext_InfoFlag;
+   unsigned short j;
+
+   j = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+   infoflag = j;
+
if (infoflag & tempax)
return 1;
 
@@ -231,8 +234,11 @@ static unsigned char XGI_AjustCRT2Rate(unsigned short 
ModeIdIndex,
}
 
for ((*i) = 0;; (*i)++) {
-   infoflag = XGI330_RefIndex[RefreshRateTableIndex + (*i)].
-   Ext_InfoFlag;
+   unsigned short m;
+
+   m = XGI330_RefIndex[RefreshRateTableIndex + (*i)].Ext_InfoFlag;
+   infoflag = m;
+
if (XGI330_RefIndex[RefreshRateTableIndex + (*i)].ModeID
!= tempbx) {
return 0;
@@ -5092,8 +5098,7 @@ unsigned short XGI_GetRatePtrCRT2(struct 
xgi_hw_device_info *pXGIHWDE,
 
i = 0;
do {
-   if (XGI330_RefIndex[RefreshRateTableIndex + i].
-   ModeID != ModeNo)
+   if (XGI330_RefIndex[RefreshRateTableIndex + i].ModeID != ModeNo)
break;
temp = XGI330_RefIndex[RefreshRateTableIndex + i].Ext_InfoFlag;
temp &= ModeTypeMask;
-- 
2.11.0