[PATCH 0/2] Staging: iio: adc: Checkpatch.pl warning cleanups

[PATCH 0/2] Staging: iio: adc: Checkpatch.pl warning cleanups

[Brian Vandre]

This patch-set fixes 2 checkpatch.pl warnings
dealing with lines over 80 characters.

Brian Vandre (2):
  Staging: iio: adc: fix line over 80 characters
  Staging: iio: adc: fix line over 80 characters

 drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

This fixes the checkpatch.pl warning:
WARNING: line over 80 characters

Signed-off-by: Brian Vandre <bvandre@gmail.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 32a1926..7d4b068 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
 	 * SoC's delay unit and start the conversion later
 	 * and automatically.
 	 */
-	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
+	mxs_lradc_reg_wrt(lradc,
+		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
 		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
 		LRADC_DELAY_KICK |
 		LRADC_DELAY_DELAY(lradc->settling_delay),
-- 
1.9.1

Re: [PATCH 1/2] Staging: iio: adc: fix line over 80 characters

[Daniel Baluta]

On Tue, Oct 21, 2014 at 5:48 AM, Brian Vandre <bvandre@gmail.com> wrote:
> This fixes the checkpatch.pl warning:
> WARNING: line over 80 characters
>
> Signed-off-by: Brian Vandre <bvandre@gmail.com>

As a part of OPW [1] IIO cleanup project [2] we analyzed all checkpatch.pl
warnings / errors and we decided not to fix some of them as there is no
obvious improvement.

This "line over 80 characters" warning was one of them. In my opinion
it doesn't make code easier to read.

thanks,
Daniel.

[1] http://kernelnewbies.org/OPWIntro
[2] http://kernelnewbies.org/IIO_cleanup

[PATCH 2/2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

This fixes the checkpatch.pl warning:
WARNING: line over 80 characters

Signed-off-by: Brian Vandre <bvandre@gmail.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 7d4b068..62449a6 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -514,7 +514,8 @@ static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
 	 * SoC's delay unit and start the conversion later
 	 * and automatically.
 	 */
-	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
+	mxs_lradc_reg_wrt(lradc,
+		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
 		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
 		LRADC_DELAY_KICK |
 		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
-- 
1.9.1

Re: [PATCH 0/2] Staging: iio: adc: Checkpatch.pl warning cleanups

[Alexandre Belloni]

Hi,

These are two patches with the same commit log, in the same file. You
can probably squash them.

On 20/10/2014 at 21:48:47 -0500, Brian Vandre wrote :
> This patch-set fixes 2 checkpatch.pl warnings
> dealing with lines over 80 characters.
> 
> Brian Vandre (2):
>   Staging: iio: adc: fix line over 80 characters
>   Staging: iio: adc: fix line over 80 characters
> 
>  drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2] Staging: iio: adc: Checkpatch.pl warning cleanups

[Brian Vandre]

This patch-set fixes 2 checkpatch.pl warnings
dealing with lines over 80 characters.

Changes from v1:
-squashed 2 patches into 1 as suggested by Alexandre Belloni


Brian Vandre (1):
  Staging: iio: adc: fix line over 80 characters

 drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH v2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

This fixes the 2 checkpatch.pl warnings:
WARNING: line over 80 characters

Signed-off-by: Brian Vandre <bvandre@gmail.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 32a1926..62449a6 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
 	 * SoC's delay unit and start the conversion later
 	 * and automatically.
 	 */
-	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
+	mxs_lradc_reg_wrt(lradc,
+		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
 		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
 		LRADC_DELAY_KICK |
 		LRADC_DELAY_DELAY(lradc->settling_delay),
@@ -513,7 +514,8 @@ static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
 	 * SoC's delay unit and start the conversion later
 	 * and automatically.
 	 */
-	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
+	mxs_lradc_reg_wrt(lradc,
+		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
 		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
 		LRADC_DELAY_KICK |
 		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
-- 
1.9.1

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Sudip Mukherjee]

On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
> This fixes the 2 checkpatch.pl warnings:
> WARNING: line over 80 characters
> 
please check your patch with --strict option of checkpatch.pl , and you will get :
"Alignment should match open parenthesis" .

thanks
sudip

> Signed-off-by: Brian Vandre <bvandre@gmail.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 32a1926..62449a6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
>  	 * SoC's delay unit and start the conversion later
>  	 * and automatically.
>  	 */
> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> +	mxs_lradc_reg_wrt(lradc,
> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
>  		LRADC_DELAY_KICK |
>  		LRADC_DELAY_DELAY(lradc->settling_delay),
> @@ -513,7 +514,8 @@ static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
>  	 * SoC's delay unit and start the conversion later
>  	 * and automatically.
>  	 */
> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> +	mxs_lradc_reg_wrt(lradc,
> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
>  		LRADC_DELAY_KICK |
>  		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Hartmut Knaack]

Sudip Mukherjee schrieb am 22.10.2014 06:21:
> On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
>> This fixes the 2 checkpatch.pl warnings:
>> WARNING: line over 80 characters
>>
> please check your patch with --strict option of checkpatch.pl , and you will get :
> "Alignment should match open parenthesis" .
> 
Good point, but what solution would you propose?
> thanks
> sudip
> 
>> Signed-off-by: Brian Vandre <bvandre@gmail.com>
>> ---
>>  drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>> index 32a1926..62449a6 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
>>  	 * SoC's delay unit and start the conversion later
>>  	 * and automatically.
>>  	 */
>> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>> +	mxs_lradc_reg_wrt(lradc,
>> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
>>  		LRADC_DELAY_KICK |
>>  		LRADC_DELAY_DELAY(lradc->settling_delay),
>> @@ -513,7 +514,8 @@ static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
>>  	 * SoC's delay unit and start the conversion later
>>  	 * and automatically.
>>  	 */
>> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>> +	mxs_lradc_reg_wrt(lradc,
>> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
>>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
>>  		LRADC_DELAY_KICK |
>>  		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

On Thu, Oct 23, 2014 at 01:47:37AM +0200, Hartmut Knaack wrote:
> Sudip Mukherjee schrieb am 22.10.2014 06:21:
> > On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
> >> This fixes the 2 checkpatch.pl warnings:
> >> WARNING: line over 80 characters
> >>
> > please check your patch with --strict option of checkpatch.pl , and you will get :
> > "Alignment should match open parenthesis" .
> > 
> Good point, but what solution would you propose?

Hey All,

Thanks for all the feedback.  This is my first attempt at a patch so I thank you all
for helping me through it.  I have a question about the strict option on checkpatch.pl.
I thought that the stict option was not necessarily part of the coding standards but
more of a nice to have.  Should I be always using the strict option?

On this particular patch if I were to align to the open parenthesis it would push the
comment "/* trigger DELAY unit#3 */" off to the next line which I thought we be less
clear.  If --strict is optional then I would argue to leave it the way it is, but again
this is my first time and I am learning.

If I were to align to the parenthesis should I just move the comment to the next line
or possibly delete the comment altogether?  The code is clear and the comment might not
be necessary but I didn't want to remove anything the original author wrote.

Thanks,
Brian Vandre

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Joe Perches]

On Thu, 2014-10-23 at 01:47 +0200, Hartmut Knaack wrote:
> Sudip Mukherjee schrieb am 22.10.2014 06:21:
> > On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
> >> This fixes the 2 checkpatch.pl warnings:
> >> WARNING: line over 80 characters
> >>
> > please check your patch with --strict option of checkpatch.pl , and you will get :
> > "Alignment should match open parenthesis" .
> > 
> Good point, but what solution would you propose?

> >> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c

> >> @@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> >>  	 * SoC's delay unit and start the conversion later
> >>  	 * and automatically.
> >>  	 */
> >> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >> +	mxs_lradc_reg_wrt(lradc,
> >> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> >>  		LRADC_DELAY_KICK |
> >>  		LRADC_DELAY_DELAY(lradc->settling_delay),

Maybe something like:

	mxs_lradc_reg_wrt(lradc,
			  LRADC_DELAY_TRIGGER(0) |
						/* don't trigger ADC */
			  LRADC_DELAY_TRIGGER_DELAYS(1 << 3) |
						/* trigger DELAY unit#3 */
			  LRADC_DELAY_KICK |
			  LRADC_DELAY_DELAY(lradc->settling_delay),

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Sudip Mukherjee]

On Thu, Oct 23, 2014 at 01:47:37AM +0200, Hartmut Knaack wrote:
> Sudip Mukherjee schrieb am 22.10.2014 06:21:
> > On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
> >> This fixes the 2 checkpatch.pl warnings:
> >> WARNING: line over 80 characters
> >>
> > please check your patch with --strict option of checkpatch.pl , and you will get :
> > "Alignment should match open parenthesis" .
> > 
> Good point, but what solution would you propose?
when you are breaking the line , make a point to align the next line with the open parenthesis.
like : 
-  mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
+  mxs_lradc_reg_wrt(lradc,
+          	     LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */

I have seen in many patches Greg commenting to resend the patch after aligning it.
so my opinion , even though it is optional , better to use it while sending a patch , so that no one can say about that.

happened to me in few of my initial patches. :) 


> > thanks
> > sudip
> > 
> >> Signed-off-by: Brian Vandre <bvandre@gmail.com>
> >> ---
> >>  drivers/staging/iio/adc/mxs-lradc.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> >> index 32a1926..62449a6 100644
> >> --- a/drivers/staging/iio/adc/mxs-lradc.c
> >> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> >> @@ -455,7 +455,8 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> >>  	 * SoC's delay unit and start the conversion later
> >>  	 * and automatically.
> >>  	 */
> >> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >> +	mxs_lradc_reg_wrt(lradc,
> >> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> >>  		LRADC_DELAY_KICK |
> >>  		LRADC_DELAY_DELAY(lradc->settling_delay),
> >> @@ -513,7 +514,8 @@ static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
> >>  	 * SoC's delay unit and start the conversion later
> >>  	 * and automatically.
> >>  	 */
> >> -	mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >> +	mxs_lradc_reg_wrt(lradc,
> >> +		LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> >>  		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> >>  		LRADC_DELAY_KICK |
> >>  		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
> >> -- 
> >> 1.9.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Lars-Peter Clausen]

On 10/22/2014 06:21 AM, Sudip Mukherjee wrote:
> On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
>> This fixes the 2 checkpatch.pl warnings:
>> WARNING: line over 80 characters
>>
> please check your patch with --strict option of checkpatch.pl , and you will get :
> "Alignment should match open parenthesis" .

Those checkpatch warnings are suggestions, not hard requirements. The idea 
is to improve code legibility, but if the change has the adverse effect the 
warning can and should be ignored. Also when making a change you should keep 
the existing indention style of a file.

- Lars

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

On Thu, Oct 23, 2014 at 12:50:00PM +0200, Lars-Peter Clausen wrote:
> On 10/22/2014 06:21 AM, Sudip Mukherjee wrote:
> >On Tue, Oct 21, 2014 at 05:56:47PM -0500, Brian Vandre wrote:
> >>This fixes the 2 checkpatch.pl warnings:
> >>WARNING: line over 80 characters
> >>
> >please check your patch with --strict option of checkpatch.pl , and you will get :
> >"Alignment should match open parenthesis" .
> 
> Those checkpatch warnings are suggestions, not hard requirements.
> The idea is to improve code legibility, but if the change has the
> adverse effect the warning can and should be ignored. Also when
> making a change you should keep the existing indention style of a
> file.
> 
> - Lars

I think my patch does follow the style of the original file.  If you run checkpatch.pl --strict
on the whole file you will get many open parenthesis warnings. I believe it does make it slightly
more legibile.

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Brian Vandre]

With all the replies I have gotten it seems like there might not be a good path forward
with this patch.  I am starting to agree with what Daniel Baluta said above that this doesn't
make the code easier to read.  All the other suggestions don't quite fit the same style
as the rest of the file so it might just be better to leave it.

This being my first try I thank you all for your input.  It has helped me learn quite a bit.

Hopefully on the next patch I can fix something a little more meaningful!

Thanks,
Brian Vandre

Re: [PATCH v2] Staging: iio: adc: fix line over 80 characters

[Alexandre Belloni]

Hi,

On 23/10/2014 at 07:53:36 -0500, Brian Vandre wrote :
> With all the replies I have gotten it seems like there might not be a good path forward
> with this patch.  I am starting to agree with what Daniel Baluta said above that this doesn't
> make the code easier to read.  All the other suggestions don't quite fit the same style
> as the rest of the file so it might just be better to leave it.
> 
> This being my first try I thank you all for your input.  It has helped me learn quite a bit.
> 
> Hopefully on the next patch I can fix something a little more meaningful!
> 

Thank you for your effort anyway. My last comment would be that you
don't need a cover letter when sending only one patch.

There are plenty of things to fix in the kernel, maybe we can help you
find something.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com