[PATCH] staging: speakup: Fix warning of line over 80 characters.

[PATCH] staging: speakup: Fix warning of line over 80 characters.

[Shirish Gajera]

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

All line over 80 characters in driver/staging/speakup/* are fixed.

Signed-off-by: Shirish Gajera <gshirishfree@gmail.com>
---
 drivers/staging/speakup/main.c           | 9 ++++++---
 drivers/staging/speakup/serialio.h       | 3 ++-
 drivers/staging/speakup/speakup.h        | 6 ++++--
 drivers/staging/speakup/speakup_decext.c | 6 ++++--
 drivers/staging/speakup/speakup_decpc.c  | 6 ++++--
 drivers/staging/speakup/spk_types.h      | 3 ++-
 6 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index 1249f91..c955976 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -423,7 +423,8 @@ static void announce_edge(struct vc_data *vc, int msg_id)
 	if (spk_bleeps & 1)
 		bleep(spk_y);
 	if ((spk_bleeps & 2) && (msg_id < edge_quiet))
-		synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
+		synth_printf("%s\n",
+			spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
 }
 
 static void speak_char(u_char ch)
@@ -1131,7 +1132,8 @@ static void spkup_write(const char *in_buf, int count)
 	if (in_count > 2 && rep_count > 2) {
 		if (last_type & CH_RPT) {
 			synth_printf(" ");
-			synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
+			synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
+					++rep_count);
 			synth_printf(" ");
 		}
 		rep_count = 0;
@@ -1847,7 +1849,8 @@ static void speakup_win_set(struct vc_data *vc)
 			win_right = spk_x;
 		}
 		snprintf(info, sizeof(info), spk_msg_get(MSG_WINDOW_BOUNDARY),
-			 (win_start) ? spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
+			 (win_start) ?
+				spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
 			 (int)spk_y + 1, (int)spk_x + 1);
 	}
 	synth_printf("%s\n", info);
diff --git a/drivers/staging/speakup/serialio.h b/drivers/staging/speakup/serialio.h
index 317bb84..1b39921 100644
--- a/drivers/staging/speakup/serialio.h
+++ b/drivers/staging/speakup/serialio.h
@@ -34,6 +34,7 @@ struct old_serial_port {
 #define SPK_TIMEOUT 100
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
-#define spk_serial_tx_busy() ((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
+#define spk_serial_tx_busy() \
+	((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
 
 #endif
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index 898dce5..a7f4962 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -61,10 +61,12 @@ extern struct st_var_header *spk_get_var_header(enum var_id_t var_id);
 extern struct st_var_header *spk_var_header_by_name(const char *name);
 extern struct punc_var_t *spk_get_punc_var(enum var_id_t var_id);
 extern int spk_set_num_var(int val, struct st_var_header *var, int how);
-extern int spk_set_string_var(const char *page, struct st_var_header *var, int len);
+extern int spk_set_string_var(const char *page, struct st_var_header *var,
+				int len);
 extern int spk_set_mask_bits(const char *input, const int which, const int how);
 extern special_func spk_special_handler;
-extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key);
+extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch,
+				u_short key);
 extern int synth_init(char *name);
 extern void synth_release(void);
 
diff --git a/drivers/staging/speakup/speakup_decext.c b/drivers/staging/speakup/speakup_decext.c
index 2b772f8..e0b5db9 100644
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/drivers/staging/speakup/speakup_decext.c
@@ -207,10 +207,12 @@ static void do_catch_up(struct spk_synth *synth)
 			if (time_after_eq(jiffies, jiff_max)) {
 				if (!in_escape)
 					spk_serial_out(PROCSPEECH);
-				spin_lock_irqsave(&speakup_info.spinlock, flags);
+				spin_lock_irqsave(&speakup_info.spinlock,
+							flags);
 				jiffy_delta_val = jiffy_delta->u.n.value;
 				delay_time_val = delay_time->u.n.value;
-				spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+				spin_unlock_irqrestore(&speakup_info.spinlock,
+							flags);
 				schedule_timeout(msecs_to_jiffies
 						 (delay_time_val));
 				jiff_max = jiffies + jiffy_delta_val;
diff --git a/drivers/staging/speakup/speakup_decpc.c b/drivers/staging/speakup/speakup_decpc.c
index f7b9c8a..437e13a 100644
--- a/drivers/staging/speakup/speakup_decpc.c
+++ b/drivers/staging/speakup/speakup_decpc.c
@@ -423,10 +423,12 @@ static void do_catch_up(struct spk_synth *synth)
 			if (time_after_eq(jiffies, jiff_max)) {
 				if (!in_escape)
 					dt_sendchar(PROCSPEECH);
-				spin_lock_irqsave(&speakup_info.spinlock, flags);
+				spin_lock_irqsave(&speakup_info.spinlock,
+							flags);
 				jiffy_delta_val = jiffy_delta->u.n.value;
 				delay_time_val = delay_time->u.n.value;
-				spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+				spin_unlock_irqrestore(&speakup_info.spinlock,
+							flags);
 				schedule_timeout(msecs_to_jiffies
 						 (delay_time_val));
 				jiff_max = jiffies + jiffy_delta_val;
diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 55d6c9b..e8ff5d7 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -168,7 +168,8 @@ struct spk_synth {
 	int *default_vol;
 	int (*probe)(struct spk_synth *synth);
 	void (*release)(void);
-	const char *(*synth_immediate)(struct spk_synth *synth, const char *buff);
+	const char *(*synth_immediate)(struct spk_synth *synth,
+					const char *buff);
 	void (*catch_up)(struct spk_synth *synth);
 	void (*flush)(struct spk_synth *synth);
 	int (*is_alive)(struct spk_synth *synth);
-- 
1.9.1

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Richard Weinberger]

On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
> This patch fixes the checkpatch.pl warning:
> WARNING: line over 80 characters
>
> All line over 80 characters in driver/staging/speakup/* are fixed.
>
> Signed-off-by: Shirish Gajera <gshirishfree@gmail.com>
> ---
>  drivers/staging/speakup/main.c           | 9 ++++++---
>  drivers/staging/speakup/serialio.h       | 3 ++-
>  drivers/staging/speakup/speakup.h        | 6 ++++--
>  drivers/staging/speakup/speakup_decext.c | 6 ++++--
>  drivers/staging/speakup/speakup_decpc.c  | 6 ++++--
>  drivers/staging/speakup/spk_types.h      | 3 ++-
>  6 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index 1249f91..c955976 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -423,7 +423,8 @@ static void announce_edge(struct vc_data *vc, int msg_id)
>         if (spk_bleeps & 1)
>                 bleep(spk_y);
>         if ((spk_bleeps & 2) && (msg_id < edge_quiet))
> -               synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> +               synth_printf("%s\n",
> +                       spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));

Instead of blindly adding newlines to silence checkpatch.pl, what
about reworking the code?
printf("%s\n", ..) cries for a puts().

>  }
>
>  static void speak_char(u_char ch)
> @@ -1131,7 +1132,8 @@ static void spkup_write(const char *in_buf, int count)
>         if (in_count > 2 && rep_count > 2) {
>                 if (last_type & CH_RPT) {
>                         synth_printf(" ");
> -                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
> +                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
> +                                       ++rep_count);
>                         synth_printf(" ");

This printf stuff looks odd. synth_printf() seems to take a format
string, in this case the format string
is returned by spk_msg_get(), smells like a format string bug.

>                 }
>                 rep_count = 0;
> @@ -1847,7 +1849,8 @@ static void speakup_win_set(struct vc_data *vc)
>                         win_right = spk_x;
>                 }
>                 snprintf(info, sizeof(info), spk_msg_get(MSG_WINDOW_BOUNDARY),
> -                        (win_start) ? spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
> +                        (win_start) ?
> +                               spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
>                          (int)spk_y + 1, (int)spk_x + 1);

Same here. Also please resolve the ?: mess.

>         }
>         synth_printf("%s\n", info);
> diff --git a/drivers/staging/speakup/serialio.h b/drivers/staging/speakup/serialio.h
> index 317bb84..1b39921 100644
> --- a/drivers/staging/speakup/serialio.h
> +++ b/drivers/staging/speakup/serialio.h
> @@ -34,6 +34,7 @@ struct old_serial_port {
>  #define SPK_TIMEOUT 100
>  #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
>
> -#define spk_serial_tx_busy() ((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
> +#define spk_serial_tx_busy() \
> +       ((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)

This makro is ugly in many ways.
Why can't this be an static inline function without a dependency on
global state?

>
>  #endif
> diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
> index 898dce5..a7f4962 100644
> --- a/drivers/staging/speakup/speakup.h
> +++ b/drivers/staging/speakup/speakup.h
> @@ -61,10 +61,12 @@ extern struct st_var_header *spk_get_var_header(enum var_id_t var_id);
>  extern struct st_var_header *spk_var_header_by_name(const char *name);
>  extern struct punc_var_t *spk_get_punc_var(enum var_id_t var_id);
>  extern int spk_set_num_var(int val, struct st_var_header *var, int how);
> -extern int spk_set_string_var(const char *page, struct st_var_header *var, int len);
> +extern int spk_set_string_var(const char *page, struct st_var_header *var,
> +                               int len);
>  extern int spk_set_mask_bits(const char *input, const int which, const int how);
>  extern special_func spk_special_handler;
> -extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key);
> +extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch,
> +                               u_short key);
>  extern int synth_init(char *name);
>  extern void synth_release(void);
>
> diff --git a/drivers/staging/speakup/speakup_decext.c b/drivers/staging/speakup/speakup_decext.c
> index 2b772f8..e0b5db9 100644
> --- a/drivers/staging/speakup/speakup_decext.c
> +++ b/drivers/staging/speakup/speakup_decext.c
> @@ -207,10 +207,12 @@ static void do_catch_up(struct spk_synth *synth)
>                         if (time_after_eq(jiffies, jiff_max)) {
>                                 if (!in_escape)
>                                         spk_serial_out(PROCSPEECH);
> -                               spin_lock_irqsave(&speakup_info.spinlock, flags);
> +                               spin_lock_irqsave(&speakup_info.spinlock,
> +                                                       flags);
>                                 jiffy_delta_val = jiffy_delta->u.n.value;
>                                 delay_time_val = delay_time->u.n.value;
> -                               spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +                               spin_unlock_irqrestore(&speakup_info.spinlock,
> +                                                       flags);
>                                 schedule_timeout(msecs_to_jiffies
>                                                  (delay_time_val));
>                                 jiff_max = jiffies + jiffy_delta_val;
> diff --git a/drivers/staging/speakup/speakup_decpc.c b/drivers/staging/speakup/speakup_decpc.c
> index f7b9c8a..437e13a 100644
> --- a/drivers/staging/speakup/speakup_decpc.c
> +++ b/drivers/staging/speakup/speakup_decpc.c
> @@ -423,10 +423,12 @@ static void do_catch_up(struct spk_synth *synth)
>                         if (time_after_eq(jiffies, jiff_max)) {
>                                 if (!in_escape)
>                                         dt_sendchar(PROCSPEECH);
> -                               spin_lock_irqsave(&speakup_info.spinlock, flags);
> +                               spin_lock_irqsave(&speakup_info.spinlock,
> +                                                       flags);
>                                 jiffy_delta_val = jiffy_delta->u.n.value;
>                                 delay_time_val = delay_time->u.n.value;
> -                               spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> +                               spin_unlock_irqrestore(&speakup_info.spinlock,
> +                                                       flags);
>                                 schedule_timeout(msecs_to_jiffies
>                                                  (delay_time_val));
>                                 jiff_max = jiffies + jiffy_delta_val;
> diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
> index 55d6c9b..e8ff5d7 100644
> --- a/drivers/staging/speakup/spk_types.h
> +++ b/drivers/staging/speakup/spk_types.h
> @@ -168,7 +168,8 @@ struct spk_synth {
>         int *default_vol;
>         int (*probe)(struct spk_synth *synth);
>         void (*release)(void);
> -       const char *(*synth_immediate)(struct spk_synth *synth, const char *buff);
> +       const char *(*synth_immediate)(struct spk_synth *synth,
> +                                       const char *buff);
>         void (*catch_up)(struct spk_synth *synth);
>         void (*flush)(struct spk_synth *synth);
>         int (*is_alive)(struct spk_synth *synth);
> --
> 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/



-- 
Thanks,
//richard

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Joe Perches]

On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
> > This patch fixes the checkpatch.pl warning:

[]

> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
[]
> > @@ -423,7 +423,8 @@ static void announce_edge(struct vc_data *vc, int msg_id)
> >         if (spk_bleeps & 1)
> >                 bleep(spk_y);
> >         if ((spk_bleeps & 2) && (msg_id < edge_quiet))
> > -               synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> > +               synth_printf("%s\n",
> > +                       spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> 
> Instead of blindly adding newlines to silence checkpatch.pl, what
> about reworking the code?
> printf("%s\n", ..) cries for a puts().

There is no synth_puts

> > @@ -1131,7 +1132,8 @@ static void spkup_write(const char *in_buf, int count)
> >         if (in_count > 2 && rep_count > 2) {
> >                 if (last_type & CH_RPT) {
> >                         synth_printf(" ");
> > -                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
> > +                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
> > +                                       ++rep_count);
> >                         synth_printf(" ");
> 
> This printf stuff looks odd. synth_printf() seems to take a format
> string, in this case the format string
> is returned by spk_msg_get(), smells like a format string bug.

Nope, but it would be nicer to avoid these spk_msg_get
functions for the indices that are used with printf style
formatting.

> >                 }
> >                 rep_count = 0;
> > @@ -1847,7 +1849,8 @@ static void speakup_win_set(struct vc_data *vc)
> >                         win_right = spk_x;
> >                 }
> >                 snprintf(info, sizeof(info), spk_msg_get(MSG_WINDOW_BOUNDARY),
> > -                        (win_start) ? spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
> > +                        (win_start) ?
> > +                               spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
> >                          (int)spk_y + 1, (int)spk_x + 1);
> 
> Same here. Also please resolve the ?: mess.

I don't think there's a ?: mess, but the code looks wrong.  

	win_start ? MSG_END : MSG_START

sure looks backwards.

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Richard Weinberger]

Am 28.03.2015 um 22:18 schrieb Joe Perches:
> On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
>> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
>>> This patch fixes the checkpatch.pl warning:
> 
> []
> 
>>> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> []
>>> @@ -423,7 +423,8 @@ static void announce_edge(struct vc_data *vc, int msg_id)
>>>         if (spk_bleeps & 1)
>>>                 bleep(spk_y);
>>>         if ((spk_bleeps & 2) && (msg_id < edge_quiet))
>>> -               synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
>>> +               synth_printf("%s\n",
>>> +                       spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
>>
>> Instead of blindly adding newlines to silence checkpatch.pl, what
>> about reworking the code?
>> printf("%s\n", ..) cries for a puts().
> 
> There is no synth_puts

So what?
Fix it! :-)

>>> @@ -1131,7 +1132,8 @@ static void spkup_write(const char *in_buf, int count)
>>>         if (in_count > 2 && rep_count > 2) {
>>>                 if (last_type & CH_RPT) {
>>>                         synth_printf(" ");
>>> -                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
>>> +                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
>>> +                                       ++rep_count);
>>>                         synth_printf(" ");
>>
>> This printf stuff looks odd. synth_printf() seems to take a format
>> string, in this case the format string
>> is returned by spk_msg_get(), smells like a format string bug.
> 
> Nope, but it would be nicer to avoid these spk_msg_get
> functions for the indices that are used with printf style
> formatting.
> 
>>>                 }
>>>                 rep_count = 0;
>>> @@ -1847,7 +1849,8 @@ static void speakup_win_set(struct vc_data *vc)
>>>                         win_right = spk_x;
>>>                 }
>>>                 snprintf(info, sizeof(info), spk_msg_get(MSG_WINDOW_BOUNDARY),
>>> -                        (win_start) ? spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
>>> +                        (win_start) ?
>>> +                               spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
>>>                          (int)spk_y + 1, (int)spk_x + 1);
>>
>> Same here. Also please resolve the ?: mess.
> 
> I don't think there's a ?: mess, but the code looks wrong.  
> 
> 	win_start ? MSG_END : MSG_START

Face it, the whole code is horrible and lines other 80 chars are the *least*
problem.
Submitting a patch just for the sake of silencing checkpatch.pl is a waste of time.
After applying this patch the driver 0 better than before.

Thanks,
//richard

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Joe Perches]

On Sat, 2015-03-28 at 22:22 +0100, Richard Weinberger wrote:
> Am 28.03.2015 um 22:18 schrieb Joe Perches:
> > On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
> >> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
> >>> This patch fixes the checkpatch.pl warning:
[]
> >> Instead of blindly adding newlines to silence checkpatch.pl, what
> >> about reworking the code?
> >> printf("%s\n", ..) cries for a puts().
> > 
> > There is no synth_puts
> 
> So what?
> Fix it! :-)

Not sure that'd make the code better... ;-p

> the whole code is horrible and lines other 80 chars are the *least*
> problem.

Dunno about how horrible it is, my guess is it works.

> Submitting a patch just for the sake of silencing checkpatch.pl is a waste of time.
> After applying this patch the driver 0 better than before.

Agree with that.

And truly, checkpatch is only a guide.

Making the code better instead of merely style conforming
should be the primary goal of patches.

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Shirish Gajera]

On Sat, Mar 28, 2015 at 02:35:19PM -0700, Joe Perches wrote:
> On Sat, 2015-03-28 at 22:22 +0100, Richard Weinberger wrote:
> > Am 28.03.2015 um 22:18 schrieb Joe Perches:
> > > On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
> > >> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
> > >>> This patch fixes the checkpatch.pl warning:
> []
> > >> Instead of blindly adding newlines to silence checkpatch.pl, what
> > >> about reworking the code?
> > >> printf("%s\n", ..) cries for a puts().
> > > 
> > > There is no synth_puts
> > 
> > So what?
> > Fix it! :-)
> 
> Not sure that'd make the code better... ;-p
> 
> > the whole code is horrible and lines other 80 chars are the *least*
> > problem.
> 
> Dunno about how horrible it is, my guess is it works.
> 
> > Submitting a patch just for the sake of silencing checkpatch.pl is a waste of time.
> > After applying this patch the driver 0 better than before.
> 
> Agree with that.
> 
> And truly, checkpatch is only a guide.
> 
> Making the code better instead of merely style conforming
> should be the primary goal of patches.

This is my first patch.

Actually on the website it's return that 
"Pick a warning, and try to fix it. For your first patch, only pick one
warning. In the future you can group multiple changes into one patch,
but only if you follow the PatchPhilosophy of breaking each patch into
logical changes."

My main aim is to get the patch in and get familier with the full system
(code checking,flow etc.). So, I am fixing simple warning.

If this code require changes then I can do as part of future changes.

Thanks,
Shirish

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Richard Weinberger]

Am 29.03.2015 um 00:44 schrieb Shirish Gajera:
> On Sat, Mar 28, 2015 at 02:35:19PM -0700, Joe Perches wrote:
>> On Sat, 2015-03-28 at 22:22 +0100, Richard Weinberger wrote:
>>> Am 28.03.2015 um 22:18 schrieb Joe Perches:
>>>> On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
>>>>> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
>>>>>> This patch fixes the checkpatch.pl warning:
>> []
>>>>> Instead of blindly adding newlines to silence checkpatch.pl, what
>>>>> about reworking the code?
>>>>> printf("%s\n", ..) cries for a puts().
>>>>
>>>> There is no synth_puts
>>>
>>> So what?
>>> Fix it! :-)
>>
>> Not sure that'd make the code better... ;-p
>>
>>> the whole code is horrible and lines other 80 chars are the *least*
>>> problem.
>>
>> Dunno about how horrible it is, my guess is it works.
>>
>>> Submitting a patch just for the sake of silencing checkpatch.pl is a waste of time.
>>> After applying this patch the driver 0 better than before.
>>
>> Agree with that.
>>
>> And truly, checkpatch is only a guide.
>>
>> Making the code better instead of merely style conforming
>> should be the primary goal of patches.
> 
> This is my first patch.

Are you sure?
http://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-January/013187.html

> Actually on the website it's return that 
> "Pick a warning, and try to fix it. For your first patch, only pick one
> warning. In the future you can group multiple changes into one patch,
> but only if you follow the PatchPhilosophy of breaking each patch into
> logical changes."
> 
> My main aim is to get the patch in and get familier with the full system
> (code checking,flow etc.). So, I am fixing simple warning.
> 
> If this code require changes then I can do as part of future changes.

The future is now, please address these issues now. :-)
Again, blindly fixing checkpatch.pl warnings is worthless.

Thanks,
//richard

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Shirish Gajera]

On Sun, Mar 29, 2015 at 12:54:45AM +0100, Richard Weinberger wrote:
> Am 29.03.2015 um 00:44 schrieb Shirish Gajera:
> > On Sat, Mar 28, 2015 at 02:35:19PM -0700, Joe Perches wrote:
> >> On Sat, 2015-03-28 at 22:22 +0100, Richard Weinberger wrote:
> >>> Am 28.03.2015 um 22:18 schrieb Joe Perches:
> >>>> On Sat, 2015-03-28 at 21:40 +0100, Richard Weinberger wrote:
> >>>>> On Sat, Mar 28, 2015 at 9:21 PM, Shirish Gajera <gshirishfree@gmail.com> wrote:
> >>>>>> This patch fixes the checkpatch.pl warning:
> >> []
> >>>>> Instead of blindly adding newlines to silence checkpatch.pl, what
> >>>>> about reworking the code?
> >>>>> printf("%s\n", ..) cries for a puts().
> >>>>
> >>>> There is no synth_puts
> >>>
> >>> So what?
> >>> Fix it! :-)
> >>
> >> Not sure that'd make the code better... ;-p
> >>
> >>> the whole code is horrible and lines other 80 chars are the *least*
> >>> problem.
> >>
> >> Dunno about how horrible it is, my guess is it works.
> >>
> >>> Submitting a patch just for the sake of silencing checkpatch.pl is a waste of time.
> >>> After applying this patch the driver 0 better than before.
> >>
> >> Agree with that.
> >>
> >> And truly, checkpatch is only a guide.
> >>
> >> Making the code better instead of merely style conforming
> >> should be the primary goal of patches.
> > 
> > This is my first patch.
> 
> Are you sure?
> http://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-January/013187.html
Yup, this patch never got merge because I was doing changes against
wrong repo.
> 
> > Actually on the website it's return that 
> > "Pick a warning, and try to fix it. For your first patch, only pick one
> > warning. In the future you can group multiple changes into one patch,
> > but only if you follow the PatchPhilosophy of breaking each patch into
> > logical changes."
> > 
> > My main aim is to get the patch in and get familier with the full system
> > (code checking,flow etc.). So, I am fixing simple warning.
> > 
> > If this code require changes then I can do as part of future changes.
> 
> The future is now, please address these issues now. :-)
> Again, blindly fixing checkpatch.pl warnings is worthless.
> 
> Thanks,
> //richard

Are you sure you want me to do this changes. Because it will conflict
the things written on http://kernelnewbies.org/

Thanks,
Shirish

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Richard Weinberger]

Am 29.03.2015 um 01:26 schrieb Shirish Gajera:
> Are you sure you want me to do this changes. Because it will conflict
> the things written on http://kernelnewbies.org/

Conflict with what?

Thanks,
//richard

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Sudip Mukherjee]

On Sat, Mar 28, 2015 at 05:26:34PM -0700, Shirish Gajera wrote:
> On Sun, Mar 29, 2015 at 12:54:45AM +0100, Richard Weinberger wrote:
> > //richard
> 
> Are you sure you want me to do this changes. Because it will conflict
> the things written on http://kernelnewbies.org/

where is the conflict? you already know how to create patches, how to
send them. so make changes where you feel you can make the code better,
and send it. if your suggested improvement is ok, then it will be
accepted, else you will be told what is wrong in that. make it a point
to learn from those mistakes and you will be fine.

regards
sudip

> 
> Thanks,
> Shirish
> --
> 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] staging: speakup: Fix warning of line over 80 characters.

[Dan Carpenter]

On Sat, Mar 28, 2015 at 09:40:05PM +0100, Richard Weinberger wrote:
> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > index 1249f91..c955976 100644
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -423,7 +423,8 @@ static void announce_edge(struct vc_data *vc, int msg_id)
> >         if (spk_bleeps & 1)
> >                 bleep(spk_y);
> >         if ((spk_bleeps & 2) && (msg_id < edge_quiet))
> > -               synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> > +               synth_printf("%s\n",
> > +                       spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> 
> Instead of blindly adding newlines to silence checkpatch.pl, what
> about reworking the code?
> printf("%s\n", ..) cries for a puts().


printf is fine.  Not everything has to be a massive engineering project
which supports every method in the world.

> 
> >  }
> >
> >  static void speak_char(u_char ch)
> > @@ -1131,7 +1132,8 @@ static void spkup_write(const char *in_buf, int count)
> >         if (in_count > 2 && rep_count > 2) {
> >                 if (last_type & CH_RPT) {
> >                         synth_printf(" ");
> > -                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
> > +                       synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
> > +                                       ++rep_count);
> >                         synth_printf(" ");
> 
> This printf stuff looks odd. synth_printf() seems to take a format
> string, in this case the format string
> is returned by spk_msg_get(), smells like a format string bug.

It's not a bug, but it's definitely odd.  I think the reason they did it
that way is so they can translate the output to other languages.

Anyway, this patch is basically fine.

regards,
dan carpenter

[PATCH] staging: speakup: Fix warning of line over 80 characters.

[Shirish Gajera]

This patch fixes the checkpatch.pl warning:

WARNING: line over 80 characters

All line over 80 characters in driver/staging/speakup/* are fixed

Signed-off-by: Shirish Gajera <gajerashirish@gmail.com>
---
 drivers/staging/speakup/main.c           | 12 ++++++++----
 drivers/staging/speakup/serialio.h       |  3 ++-
 drivers/staging/speakup/speakup.h        |  6 ++++--
 drivers/staging/speakup/speakup_decext.c |  6 ++++--
 drivers/staging/speakup/speakup_decpc.c  |  6 ++++--
 drivers/staging/speakup/spk_priv.h       |  3 ++-
 drivers/staging/speakup/spk_types.h      |  3 ++-
 drivers/staging/speakup/synth.c          | 10 +++++-----
 8 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index e9f0c15..141abb7 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -422,8 +422,10 @@ static void announce_edge(struct vc_data *vc, int msg_id)
 {
 	if (spk_bleeps & 1)
 		bleep(spk_y);
-	if ((spk_bleeps & 2) && (msg_id < edge_quiet))
-		synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
+	if ((spk_bleeps & 2) && (msg_id < edge_quiet)) {
+		synth_printf("%s\n",
+		spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
+	}
 }
 
 static void speak_char(u_char ch)
@@ -1131,7 +1133,8 @@ static void spkup_write(const char *in_buf, int count)
 	if (in_count > 2 && rep_count > 2) {
 		if (last_type & CH_RPT) {
 			synth_printf(" ");
-			synth_printf(spk_msg_get(MSG_REPEAT_DESC2), ++rep_count);
+			synth_printf(spk_msg_get(MSG_REPEAT_DESC2),
+				     ++rep_count);
 			synth_printf(" ");
 		}
 		rep_count = 0;
@@ -1847,7 +1850,8 @@ static void speakup_win_set(struct vc_data *vc)
 			win_right = spk_x;
 		}
 		snprintf(info, sizeof(info), spk_msg_get(MSG_WINDOW_BOUNDARY),
-			 (win_start) ? spk_msg_get(MSG_END) : spk_msg_get(MSG_START),
+			 (win_start) ? spk_msg_get(MSG_END) :
+				       spk_msg_get(MSG_START),
 			 (int)spk_y + 1, (int)spk_x + 1);
 	}
 	synth_printf("%s\n", info);
diff --git a/drivers/staging/speakup/serialio.h b/drivers/staging/speakup/serialio.h
index 317bb84..6e54c3e 100644
--- a/drivers/staging/speakup/serialio.h
+++ b/drivers/staging/speakup/serialio.h
@@ -34,6 +34,7 @@ struct old_serial_port {
 #define SPK_TIMEOUT 100
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
-#define spk_serial_tx_busy() ((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
+#define spk_serial_tx_busy() \
+((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
 
 #endif
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index 898dce5..d194ebb 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -61,10 +61,12 @@ extern struct st_var_header *spk_get_var_header(enum var_id_t var_id);
 extern struct st_var_header *spk_var_header_by_name(const char *name);
 extern struct punc_var_t *spk_get_punc_var(enum var_id_t var_id);
 extern int spk_set_num_var(int val, struct st_var_header *var, int how);
-extern int spk_set_string_var(const char *page, struct st_var_header *var, int len);
+extern int spk_set_string_var(const char *page, struct st_var_header *var,
+			      int len);
 extern int spk_set_mask_bits(const char *input, const int which, const int how);
 extern special_func spk_special_handler;
-extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key);
+extern int spk_handle_help(struct vc_data *vc, u_char type, u_char ch,
+			   u_short key);
 extern int synth_init(char *name);
 extern void synth_release(void);
 
diff --git a/drivers/staging/speakup/speakup_decext.c b/drivers/staging/speakup/speakup_decext.c
index 5550290..d86a579 100644
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/drivers/staging/speakup/speakup_decext.c
@@ -207,10 +207,12 @@ static void do_catch_up(struct spk_synth *synth)
 			if (time_after_eq(jiffies, jiff_max)) {
 				if (!in_escape)
 					spk_serial_out(PROCSPEECH);
-				spin_lock_irqsave(&speakup_info.spinlock, flags);
+				spin_lock_irqsave(&speakup_info.spinlock,
+						  flags);
 				jiffy_delta_val = jiffy_delta->u.n.value;
 				delay_time_val = delay_time->u.n.value;
-				spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+				spin_unlock_irqrestore(&speakup_info.spinlock,
+						       flags);
 				schedule_timeout(msecs_to_jiffies
 						 (delay_time_val));
 				jiff_max = jiffies + jiffy_delta_val;
diff --git a/drivers/staging/speakup/speakup_decpc.c b/drivers/staging/speakup/speakup_decpc.c
index 7c9c432..b520a76 100644
--- a/drivers/staging/speakup/speakup_decpc.c
+++ b/drivers/staging/speakup/speakup_decpc.c
@@ -423,10 +423,12 @@ static void do_catch_up(struct spk_synth *synth)
 			if (time_after_eq(jiffies, jiff_max)) {
 				if (!in_escape)
 					dt_sendchar(PROCSPEECH);
-				spin_lock_irqsave(&speakup_info.spinlock, flags);
+				spin_lock_irqsave(&speakup_info.spinlock,
+						  flags);
 				jiffy_delta_val = jiffy_delta->u.n.value;
 				delay_time_val = delay_time->u.n.value;
-				spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+				spin_unlock_irqrestore(&speakup_info.spinlock,
+						       flags);
 				schedule_timeout(msecs_to_jiffies
 						 (delay_time_val));
 				jiff_max = jiffies + jiffy_delta_val;
diff --git a/drivers/staging/speakup/spk_priv.h b/drivers/staging/speakup/spk_priv.h
index 637ba67..fb5742e 100644
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -62,7 +62,8 @@ extern ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
 	const char *buf, size_t count);
 
 extern int spk_serial_synth_probe(struct spk_synth *synth);
-extern const char *spk_synth_immediate(struct spk_synth *synth, const char *buff);
+extern const char *spk_synth_immediate(struct spk_synth *synth,
+				       const char *buff);
 extern void spk_do_catch_up(struct spk_synth *synth);
 extern void spk_synth_flush(struct spk_synth *synth);
 extern int spk_synth_is_alive_nop(struct spk_synth *synth);
diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 8c565c9..d6e30ba 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -167,7 +167,8 @@ struct spk_synth {
 	int *default_vol;
 	int (*probe)(struct spk_synth *synth);
 	void (*release)(void);
-	const char *(*synth_immediate)(struct spk_synth *synth, const char *buff);
+	const char *(*synth_immediate)(struct spk_synth *synth,
+				       const char *buff);
 	void (*catch_up)(struct spk_synth *synth);
 	void (*flush)(struct spk_synth *synth);
 	int (*is_alive)(struct spk_synth *synth);
diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
index f3aa423..8d28cc9 100644
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -30,12 +30,12 @@ struct speakup_info_t speakup_info = {
 	 * must be taken at each kernel->speakup transition and released at
 	 * each corresponding speakup->kernel transition.
 	 *
-	 * The progression thread only interferes with the speakup machinery through
-	 * the synth buffer, so only needs to take the lock while tinkering with
-	 * the buffer.
+	 * The progression thread only interferes with the speakup machinery
+	 * through the synth buffer, so only needs to take the lock while
+	 * tinkering with the buffer.
 	 *
-	 * We use spin_lock/trylock_irqsave and spin_unlock_irqrestore with this
-	 * spinlock because speakup needs to disable the keyboard IRQ.
+	 * We use spin_lock/trylock_irqsave and spin_unlock_irqrestore with
+	 * this spinlock because speakup needs to disable the keyboard IRQ.
 	 */
 	.spinlock = __SPIN_LOCK_UNLOCKED(speakup_info.spinlock),
 	.flushing = 0,
-- 
1.9.1

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Robin Schroer]

On Sat, Jan 17, 2015 at 11:57:53PM -0800, Shirish Gajera wrote:
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index e9f0c15..141abb7 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -422,8 +422,10 @@ static void announce_edge(struct vc_data *vc, int msg_id)
>  {
>  	if (spk_bleeps & 1)
>  		bleep(spk_y);
> -	if ((spk_bleeps & 2) && (msg_id < edge_quiet))
> -		synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> +	if ((spk_bleeps & 2) && (msg_id < edge_quiet)) {
> +		synth_printf("%s\n",
> +		spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> +	}
>  }
>  
You do not actually need to add the braces as this remains a single
statement (mnemonic: without braces the if ends at the first semicolon).

-- 

Robin Schroer

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Dan Carpenter]

On Sun, Jan 18, 2015 at 09:29:21AM +0100, Robin Schroer wrote:
> On Sat, Jan 17, 2015 at 11:57:53PM -0800, Shirish Gajera wrote:
> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > index e9f0c15..141abb7 100644
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -422,8 +422,10 @@ static void announce_edge(struct vc_data *vc, int msg_id)
> >  {
> >  	if (spk_bleeps & 1)
> >  		bleep(spk_y);
> > -	if ((spk_bleeps & 2) && (msg_id < edge_quiet))
> > -		synth_printf("%s\n", spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> > +	if ((spk_bleeps & 2) && (msg_id < edge_quiet)) {
> > +		synth_printf("%s\n",
> > +		spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> > +	}
> >  }
> >  
> You do not actually need to add the braces as this remains a single
> statement (mnemonic: without braces the if ends at the first semicolon).
> 

In staging (and most other trees), we prefer that the curly braces be
there for multi-line indents.  It makes it more readable.

regards,
dan carpenter

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On Sat, 2015-01-17 at 23:57 -0800, Shirish Gajera wrote:
> This patch fixes the checkpatch.pl warning:
> 
> WARNING: line over 80 characters
> 
> All line over 80 characters in driver/staging/speakup/* are fixed
[...]

It is not important to fix all such warnings.  The code seems perfectly
readable as it is.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Dan Carpenter]

On Sun, Jan 18, 2015 at 09:55:50AM +0000, Ben Hutchings wrote:
> On Sat, 2015-01-17 at 23:57 -0800, Shirish Gajera wrote:
> > This patch fixes the checkpatch.pl warning:
> > 
> > WARNING: line over 80 characters
> > 
> > All line over 80 characters in driver/staging/speakup/* are fixed
> [...]
> 
> It is not important to fix all such warnings.  The code seems perfectly
> readable as it is.

You can't fight checkpatch (this is why I hate when checkpatch suggests
you introduce bugs).  In the end, people will keep sending these patches
until we merge one and this patch is actually pretty good.

regards,
dan carpenter

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Dan Carpenter]

On Mon, Jan 19, 2015 at 12:00:34PM -0800, shirish gajera wrote:
> I am confuse whether to wait till the patch got accepted or submit new
> patch set.

Just wait for Greg to get to the patch.  We pretty much always apply
long line fixes so just assume it's going to merged.

> 
> Also, how to identify whether the corresponding warning is real or not ?

It's all a matter of personal taste.  The end code should look better
than the original code.  I thought your changes looked pretty good...

regards,
dan carpenter

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Dan Carpenter]

Generally this patch is fine.  There were a couple places which weren't
perfect.

On Sat, Jan 17, 2015 at 11:57:53PM -0800, Shirish Gajera wrote:
> +	if ((spk_bleeps & 2) && (msg_id < edge_quiet)) {
> +		synth_printf("%s\n",
> +		spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
> +	}

This should be:

	if ((spk_bleeps & 2) && (msg_id < edge_quiet)) {
		synth_printf("%s\n",
			     spk_msg_get(MSG_EDGE_MSGS_START + msg_id - 1));
	}

> +#define spk_serial_tx_busy() \
> +((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)

This should be:

#define spk_serial_tx_busy() \
	((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)

Otherwise this patch is nice.  I'm fine with merging it as-is if people
want.

regards,
dan carpenter

Re: [PATCH] staging: speakup: Fix warning of line over 80 characters.

[Greg KH]

On Sat, Jan 17, 2015 at 11:57:53PM -0800, Shirish Gajera wrote:
> This patch fixes the checkpatch.pl warning:
> 
> WARNING: line over 80 characters
> 
> All line over 80 characters in driver/staging/speakup/* are fixed
> 
> Signed-off-by: Shirish Gajera <gajerashirish@gmail.com>
> ---
>  drivers/staging/speakup/main.c           | 12 ++++++++----
>  drivers/staging/speakup/serialio.h       |  3 ++-
>  drivers/staging/speakup/speakup.h        |  6 ++++--
>  drivers/staging/speakup/speakup_decext.c |  6 ++++--
>  drivers/staging/speakup/speakup_decpc.c  |  6 ++++--
>  drivers/staging/speakup/spk_priv.h       |  3 ++-
>  drivers/staging/speakup/spk_types.h      |  3 ++-
>  drivers/staging/speakup/synth.c          | 10 +++++-----
>  8 files changed, 31 insertions(+), 18 deletions(-)

This patch fails to apply to my staging-testing branch of my staging.git
tree, can you refresh it and resend, hopefully taking Dan's advice and
fixing up those two specific issues he pointed out.

thanks,

greg k-h