diff mbox

[1/4] staging: omap-thermal: Correct checkpatch.pl warnings

Message ID 1347379615-6881-2-git-send-email-eduardo.valentin@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eduardo Valentin Sept. 11, 2012, 4:06 p.m. UTC 
From: J Keerthy <j-keerthy@ti.com>

Removes checkpatch warnings on omap-bandgap.c.

Signed-off-by: J Keerthy <j-keerthy@ti.com>
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/staging/omap-thermal/omap-bandgap.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

Comments

Dan Carpenter Sept. 12, 2012, 8:11 a.m. UTC | #1 
On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> From: J Keerthy <j-keerthy@ti.com>
> 
> Removes checkpatch warnings on omap-bandgap.c.
> 

Which checkpatch.pl warnings?

> +				omap_bandgap_writel(bg_ptr,
> +					rval->tshut_threshold,
> +						   tsr->tshut_threshold);

That's just whacky.

Personally, I've never cared much about long lines, so I'd prefer
to leave these as is until someone can break the functions up and
remove them in a proper way instead of just shifting everything
randomly to the left.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Sept. 12, 2012, 8:26 a.m. UTC | #2 
On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> > From: J Keerthy <j-keerthy@ti.com>
> > 
> > Removes checkpatch warnings on omap-bandgap.c.
> > 
> 
> Which checkpatch.pl warnings?
> 
> > +				omap_bandgap_writel(bg_ptr,
> > +					rval->tshut_threshold,
> > +						   tsr->tshut_threshold);
> 
> That's just whacky.
> 
> Personally, I've never cared much about long lines, so I'd prefer
> to leave these as is until someone can break the functions up and
> remove them in a proper way instead of just shifting everything
> randomly to the left.
> 

Sorry, that was my default response without looking at the code.

This is already broken up into small functions pretty nicely.  You
might want to consider using shorter names.  For example
omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
could be changed to just "bg" because it's obviously a pointer.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin Sept. 12, 2012, 9:19 a.m. UTC | #3 
Hello Dan,

On Wed, Sep 12, 2012 at 11:26 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
>> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
>> > From: J Keerthy <j-keerthy@ti.com>
>> >
>> > Removes checkpatch warnings on omap-bandgap.c.
>> >
>>
>> Which checkpatch.pl warnings?
>>
>> > +                           omap_bandgap_writel(bg_ptr,
>> > +                                   rval->tshut_threshold,
>> > +                                              tsr->tshut_threshold);
>>
>> That's just whacky.
>>
>> Personally, I've never cared much about long lines, so I'd prefer
>> to leave these as is until someone can break the functions up and
>> remove them in a proper way instead of just shifting everything
>> randomly to the left.
>>
>
> Sorry, that was my default response without looking at the code.
>
> This is already broken up into small functions pretty nicely.  You
> might want to consider using shorter names.  For example
> omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
> could be changed to just "bg" because it's obviously a pointer.

Yeah, that's one option. Of course the deal is to find the proper
balance between cryptic symbol names and code mangled / shifted to the
left.

>
> regards,
> dan carpenter
Dan Carpenter Sept. 12, 2012, 2:18 p.m. UTC | #4 
On Wed, Sep 12, 2012 at 12:19:00PM +0300, Valentin, Eduardo wrote:
> Hello Dan,
> 
> On Wed, Sep 12, 2012 at 11:26 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
> > On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> >> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> >> > From: J Keerthy <j-keerthy@ti.com>
> >> >
> >> > Removes checkpatch warnings on omap-bandgap.c.
> >> >
> >>
> >> Which checkpatch.pl warnings?
> >>
> >> > +                           omap_bandgap_writel(bg_ptr,
> >> > +                                   rval->tshut_threshold,
> >> > +                                              tsr->tshut_threshold);
> >>
> >> That's just whacky.
> >>
> >> Personally, I've never cared much about long lines, so I'd prefer
> >> to leave these as is until someone can break the functions up and
> >> remove them in a proper way instead of just shifting everything
> >> randomly to the left.
> >>
> >
> > Sorry, that was my default response without looking at the code.
> >
> > This is already broken up into small functions pretty nicely.  You
> > might want to consider using shorter names.  For example
> > omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
> > could be changed to just "bg" because it's obviously a pointer.
> 
> Yeah, that's one option. Of course the deal is to find the proper
> balance between cryptic symbol names and code mangled / shifted to the
> left.
> 

Another option would be to just let checkpatch complain.  It's a
perl script, not the king of us.  Do what looks the nicest to human
beings.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/omap-thermal/omap-bandgap.c b/drivers/staging/omap-thermal/omap-bandgap.c
index c556abb..9ef44ea 100644
--- a/drivers/staging/omap-thermal/omap-bandgap.c
+++ b/drivers/staging/omap-thermal/omap-bandgap.c
@@ -1037,20 +1037,20 @@  static int omap_bandgap_save_ctxt(struct omap_bandgap *bg_ptr)
 
 		if (OMAP_BANDGAP_HAS(bg_ptr, MODE_CONFIG))
 			rval->bg_mode_ctrl = omap_bandgap_readl(bg_ptr,
-								tsr->bgap_mode_ctrl);
+							tsr->bgap_mode_ctrl);
 		if (OMAP_BANDGAP_HAS(bg_ptr, COUNTER))
 			rval->bg_counter = omap_bandgap_readl(bg_ptr,
-							      tsr->bgap_counter);
+							tsr->bgap_counter);
 		if (OMAP_BANDGAP_HAS(bg_ptr, TALERT)) {
 			rval->bg_threshold = omap_bandgap_readl(bg_ptr,
-								tsr->bgap_threshold);
+							tsr->bgap_threshold);
 			rval->bg_ctrl = omap_bandgap_readl(bg_ptr,
-							   tsr->bgap_mask_ctrl);
+						   tsr->bgap_mask_ctrl);
 		}
 
 		if (OMAP_BANDGAP_HAS(bg_ptr, TSHUT_CONFIG))
 			rval->tshut_threshold = omap_bandgap_readl(bg_ptr,
-								   tsr->tshut_threshold);
+						   tsr->tshut_threshold);
 	}
 
 	return 0;
@@ -1074,8 +1074,9 @@  static int omap_bandgap_restore_ctxt(struct omap_bandgap *bg_ptr)
 
 		if (val == 0) {
 			if (OMAP_BANDGAP_HAS(bg_ptr, TSHUT_CONFIG))
-				omap_bandgap_writel(bg_ptr, rval->tshut_threshold,
-							   tsr->tshut_threshold);
+				omap_bandgap_writel(bg_ptr,
+					rval->tshut_threshold,
+						   tsr->tshut_threshold);
 			/* Force immediate temperature measurement and update
 			 * of the DTEMP field
 			 */