[PATCH 1/3] staging: fwserial: (coding style) Turning every "unsigned" into "unsigned int"

[PATCH 1/3] staging: fwserial: (coding style) Turning every "unsigned" into "unsigned int"

[Dominique van den Broeck]

Coding-style-only modifications to remove every warning saying:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

Compiled against revision "next-20160327".

(checkpatch.pl was updated to treat "UNSPECIFIED_INT" warnings
 as of commit a1ce18e4f941d20 )

Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
 drivers/staging/fwserial/dma_fifo.c |  8 +++----
 drivers/staging/fwserial/dma_fifo.h | 16 +++++++-------
 drivers/staging/fwserial/fwserial.c | 38 +++++++++++++++++----------------
 drivers/staging/fwserial/fwserial.h | 42 ++++++++++++++++++-------------------
 4 files changed, 53 insertions(+), 51 deletions(-)

diff --git a/drivers/staging/fwserial/dma_fifo.c b/drivers/staging/fwserial/dma_fifo.c
index 4cd3ed3..8b23a55 100644
--- a/drivers/staging/fwserial/dma_fifo.c
+++ b/drivers/staging/fwserial/dma_fifo.c
@@ -35,7 +35,7 @@
 /*
  * private helper fn to determine if check is in open interval (lo,hi)
  */
-static bool addr_check(unsigned check, unsigned lo, unsigned hi)
+static bool addr_check(unsigned int check, unsigned int lo, unsigned int hi)
 {
 	return check - (lo + 1) < (hi - 1) - lo;
 }
@@ -64,7 +64,7 @@ void dma_fifo_init(struct dma_fifo *fifo)
  * The 'apparent' size will be rounded up to next greater aligned size.
  * Returns 0 if no error, otherwise an error code
  */
-int dma_fifo_alloc(struct dma_fifo *fifo, int size, unsigned align,
+int dma_fifo_alloc(struct dma_fifo *fifo, int size, unsigned int align,
 		   int tx_limit, int open_limit, gfp_t gfp_mask)
 {
 	int capacity;
@@ -190,7 +190,7 @@ int dma_fifo_in(struct dma_fifo *fifo, const void *src, int n)
  */
 int dma_fifo_out_pend(struct dma_fifo *fifo, struct dma_pending *pended)
 {
-	unsigned len, n, ofs, l, limit;
+	unsigned int len, n, ofs, l, limit;
 
 	if (!fifo->data)
 		return -ENOENT;
@@ -210,7 +210,7 @@ int dma_fifo_out_pend(struct dma_fifo *fifo, struct dma_pending *pended)
 	n = len;
 	ofs = fifo->out % fifo->capacity;
 	l = fifo->capacity - ofs;
-	limit = min_t(unsigned, l, fifo->tx_limit);
+	limit = min_t(unsigned int, l, fifo->tx_limit);
 	if (n > limit) {
 		n = limit;
 		fifo->out += limit;
diff --git a/drivers/staging/fwserial/dma_fifo.h b/drivers/staging/fwserial/dma_fifo.h
index 4109882..37a91c6 100644
--- a/drivers/staging/fwserial/dma_fifo.h
+++ b/drivers/staging/fwserial/dma_fifo.h
@@ -45,9 +45,9 @@
 #define DMA_FIFO_GUARD 3   /* # of cache lines to reserve for the guard area */
 
 struct dma_fifo {
-	unsigned	 in;
-	unsigned	 out;		/* updated when dma is pended         */
-	unsigned	 done;		/* updated upon dma completion        */
+	unsigned int	 in;
+	unsigned int	 out;		/* updated when dma is pended         */
+	unsigned int	 done;		/* updated upon dma completion        */
 	struct {
 		unsigned corrupt:1;
 	};
@@ -55,7 +55,7 @@ struct dma_fifo {
 	int		 guard;		/* ofs of guard area		      */
 	int		 capacity;	/* size + reserved                    */
 	int		 avail;		/* # of unused bytes in fifo          */
-	unsigned	 align;		/* must be power of 2                 */
+	unsigned int	 align;		/* must be power of 2                 */
 	int		 tx_limit;	/* max # of bytes per dma transaction */
 	int		 open_limit;	/* max # of outstanding allowed       */
 	int		 open;		/* # of outstanding dma transactions  */
@@ -66,9 +66,9 @@ struct dma_fifo {
 struct dma_pending {
 	struct list_head link;
 	void		 *data;
-	unsigned	 len;
-	unsigned         next;
-	unsigned         out;
+	unsigned int	 len;
+	unsigned int	 next;
+	unsigned int	 out;
 };
 
 static inline void dp_mark_completed(struct dma_pending *dp)
@@ -82,7 +82,7 @@ static inline bool dp_is_completed(struct dma_pending *dp)
 }
 
 void dma_fifo_init(struct dma_fifo *fifo);
-int dma_fifo_alloc(struct dma_fifo *fifo, int size, unsigned align,
+int dma_fifo_alloc(struct dma_fifo *fifo, int size, unsigned int align,
 		   int tx_limit, int open_limit, gfp_t gfp_mask);
 void dma_fifo_free(struct dma_fifo *fifo);
 void dma_fifo_reset(struct dma_fifo *fifo);
diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 9b23b5c..66e54e7 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -132,7 +132,7 @@ static struct fwtty_peer *__fwserial_peer_by_node_id(struct fw_card *card,
 
 #ifdef FWTTY_PROFILING
 
-static void fwtty_profile_fifo(struct fwtty_port *port, unsigned *stat)
+static void fwtty_profile_fifo(struct fwtty_port *port, unsigned int *stat)
 {
 	spin_lock_bh(&port->lock);
 	fwtty_profile_data(stat, dma_fifo_avail(&port->tx_fifo));
@@ -143,7 +143,7 @@ static void fwtty_dump_profile(struct seq_file *m, struct stats *stats)
 {
 	/* for each stat, print sum of 0 to 2^k, then individually */
 	int k = 4;
-	unsigned sum;
+	unsigned int sum;
 	int j;
 	char t[10];
 
@@ -303,9 +303,10 @@ static void fwtty_restart_tx(struct fwtty_port *port)
  * Note: in loopback, the port->lock is being held. Only use functions that
  * don't attempt to reclaim the port->lock.
  */
-static void fwtty_update_port_status(struct fwtty_port *port, unsigned status)
+static void fwtty_update_port_status(struct fwtty_port *port,
+				     unsigned int status)
 {
-	unsigned delta;
+	unsigned int delta;
 	struct tty_struct *tty;
 
 	/* simulated LSR/MSR status from remote */
@@ -396,9 +397,9 @@ static void fwtty_update_port_status(struct fwtty_port *port, unsigned status)
  *
  * Note: caller must be holding port lock
  */
-static unsigned __fwtty_port_line_status(struct fwtty_port *port)
+static unsigned int __fwtty_port_line_status(struct fwtty_port *port)
 {
-	unsigned status = 0;
+	unsigned int status = 0;
 
 	/* TODO: add module param to tie RNG to DTR as well */
 
@@ -424,7 +425,7 @@ static int __fwtty_write_port_status(struct fwtty_port *port)
 {
 	struct fwtty_peer *peer;
 	int err = -ENOENT;
-	unsigned status = __fwtty_port_line_status(port);
+	unsigned int status = __fwtty_port_line_status(port);
 
 	rcu_read_lock();
 	peer = rcu_dereference(port->peer);
@@ -454,7 +455,7 @@ static int fwtty_write_port_status(struct fwtty_port *port)
 static void fwtty_throttle_port(struct fwtty_port *port)
 {
 	struct tty_struct *tty;
-	unsigned old;
+	unsigned int old;
 
 	tty = tty_port_tty_get(&port->port);
 	if (!tty)
@@ -540,7 +541,7 @@ static void fwtty_emit_breaks(struct work_struct *work)
 static int fwtty_rx(struct fwtty_port *port, unsigned char *data, size_t len)
 {
 	int c, n = len;
-	unsigned lsr;
+	unsigned int lsr;
 	int err = 0;
 
 	fwtty_dbg(port, "%d\n", n);
@@ -635,7 +636,7 @@ static void fwtty_port_handler(struct fw_card *card,
 		if (addr != port->rx_handler.offset || len != 4) {
 			rcode = RCODE_ADDRESS_ERROR;
 		} else {
-			fwtty_update_port_status(port, *(unsigned *)data);
+			fwtty_update_port_status(port, *(unsigned int *)data);
 			rcode = RCODE_COMPLETE;
 		}
 		break;
@@ -828,7 +829,7 @@ static void fwtty_write_xchar(struct fwtty_port *port, char ch)
 	rcu_read_unlock();
 }
 
-static struct fwtty_port *fwtty_port_get(unsigned index)
+static struct fwtty_port *fwtty_port_get(unsigned int index)
 {
 	struct fwtty_port *port;
 
@@ -934,9 +935,9 @@ static int fwtty_port_carrier_raised(struct tty_port *tty_port)
 	return rc;
 }
 
-static unsigned set_termios(struct fwtty_port *port, struct tty_struct *tty)
+static unsigned int set_termios(struct fwtty_port *port, struct tty_struct *tty)
 {
-	unsigned baud, frame;
+	unsigned int baud, frame;
 
 	baud = tty_termios_baud_rate(&tty->termios);
 	tty_termios_encode_baud_rate(&tty->termios, baud, baud);
@@ -988,7 +989,7 @@ static int fwtty_port_activate(struct tty_port *tty_port,
 			       struct tty_struct *tty)
 {
 	struct fwtty_port *port = to_port(tty_port, port);
-	unsigned baud;
+	unsigned int baud;
 	int err;
 
 	set_bit(TTY_IO_ERROR, &tty->flags);
@@ -1264,7 +1265,7 @@ static int set_serial_info(struct fwtty_port *port,
 	return 0;
 }
 
-static int fwtty_ioctl(struct tty_struct *tty, unsigned cmd,
+static int fwtty_ioctl(struct tty_struct *tty, unsigned int cmd,
 		       unsigned long arg)
 {
 	struct fwtty_port *port = tty->driver_data;
@@ -1297,7 +1298,7 @@ static int fwtty_ioctl(struct tty_struct *tty, unsigned cmd,
 static void fwtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
 	struct fwtty_port *port = tty->driver_data;
-	unsigned baud;
+	unsigned int baud;
 
 	spin_lock_bh(&port->lock);
 	baud = set_termios(port, tty);
@@ -1369,7 +1370,7 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
 static int fwtty_tiocmget(struct tty_struct *tty)
 {
 	struct fwtty_port *port = tty->driver_data;
-	unsigned tiocm;
+	unsigned int tiocm;
 
 	spin_lock_bh(&port->lock);
 	tiocm = (port->mctrl & MCTRL_MASK) | (port->mstatus & ~MCTRL_MASK);
@@ -1380,7 +1381,8 @@ static int fwtty_tiocmget(struct tty_struct *tty)
 	return tiocm;
 }
 
-static int fwtty_tiocmset(struct tty_struct *tty, unsigned set, unsigned clear)
+static int fwtty_tiocmset(struct tty_struct *tty,
+			  unsigned int set, unsigned int clear)
 {
 	struct fwtty_port *port = tty->driver_data;
 
diff --git a/drivers/staging/fwserial/fwserial.h b/drivers/staging/fwserial/fwserial.h
index 6fa9365..30b2481 100644
--- a/drivers/staging/fwserial/fwserial.h
+++ b/drivers/staging/fwserial/fwserial.h
@@ -22,7 +22,7 @@
 #ifdef FWTTY_PROFILING
 #define DISTRIBUTION_MAX_SIZE     8192
 #define DISTRIBUTION_MAX_INDEX    (ilog2(DISTRIBUTION_MAX_SIZE) + 1)
-static inline void fwtty_profile_data(unsigned stat[], unsigned val)
+static inline void fwtty_profile_data(unsigned int stat[], unsigned int val)
 {
 	int n = (val) ? min(ilog2(val) + 1, DISTRIBUTION_MAX_INDEX) : 0;
 	++stat[n];
@@ -78,7 +78,7 @@ struct fwtty_peer {
 	u64			guid;
 	int			generation;
 	int			node_id;
-	unsigned		speed;
+	unsigned int		speed;
 	int			max_payload;
 	u64			mgmt_addr;
 
@@ -160,17 +160,17 @@ struct fwserial_mgmt_pkt {
 #define VIRT_CABLE_PLUG_TIMEOUT		(60 * HZ)
 
 struct stats {
-	unsigned	xchars;
-	unsigned	dropped;
-	unsigned	tx_stall;
-	unsigned	fifo_errs;
-	unsigned	sent;
-	unsigned	lost;
-	unsigned	throttled;
-	unsigned	reads[DISTRIBUTION_MAX_INDEX + 1];
-	unsigned	writes[DISTRIBUTION_MAX_INDEX + 1];
-	unsigned	txns[DISTRIBUTION_MAX_INDEX + 1];
-	unsigned	unthrottle[DISTRIBUTION_MAX_INDEX + 1];
+	unsigned int	xchars;
+	unsigned int	dropped;
+	unsigned int	tx_stall;
+	unsigned int	fifo_errs;
+	unsigned int	sent;
+	unsigned int	lost;
+	unsigned int	throttled;
+	unsigned int	reads[DISTRIBUTION_MAX_INDEX + 1];
+	unsigned int	writes[DISTRIBUTION_MAX_INDEX + 1];
+	unsigned int	txns[DISTRIBUTION_MAX_INDEX + 1];
+	unsigned int	unthrottle[DISTRIBUTION_MAX_INDEX + 1];
 };
 
 struct fwconsole_ops {
@@ -237,7 +237,7 @@ struct fwconsole_ops {
 struct fwtty_port {
 	struct tty_port		   port;
 	struct device		   *device;
-	unsigned		   index;
+	unsigned int		   index;
 	struct fw_serial	   *serial;
 	struct fw_address_handler  rx_handler;
 
@@ -246,21 +246,21 @@ struct fwtty_port {
 
 	wait_queue_head_t	   wait_tx;
 	struct delayed_work	   emit_breaks;
-	unsigned		   cps;
+	unsigned int		   cps;
 	unsigned long		   break_last;
 
 	struct work_struct	   hangup;
 
-	unsigned		   mstatus;
+	unsigned int		   mstatus;
 
 	spinlock_t		   lock;
-	unsigned		   mctrl;
+	unsigned int		   mctrl;
 	struct delayed_work	   drain;
 	struct dma_fifo		   tx_fifo;
 	int			   max_payload;
-	unsigned		   status_mask;
-	unsigned		   ignore_mask;
-	unsigned		   break_ctl:1,
+	unsigned int		   status_mask;
+	unsigned int		   ignore_mask;
+	unsigned int		   break_ctl:1,
 				   write_only:1,
 				   overrun:1,
 				   loopback:1;
@@ -349,7 +349,7 @@ extern struct tty_driver *fwtty_driver;
  *	being used for isochronous traffic)
  *   2) isochronous arbitration always wins.
  */
-static inline int link_speed_to_max_payload(unsigned speed)
+static inline int link_speed_to_max_payload(unsigned int speed)
 {
 	/* Max async payload is 4096 - see IEEE 1394-2008 tables 6-4, 16-18 */
 	return min(512 << speed, 4096);
-- 
2.4.3

[PATCH 2/3] staging: fwserial: (coding style) removing "!= NULL" to comply with checkpatch.pl

[Dominique van den Broeck]

Removing two "!= NULL" from fwserial.c as suggested by checkpatch.pl.
Note that the associated expression "port->port.console" is a 1-bit-field
that is already assumed as an implicit boolean (that is: without comparison)

Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
 drivers/staging/fwserial/fwserial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 66e54e7..4dd5304 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1701,7 +1701,7 @@ static void fwserial_virt_plug_complete(struct fwtty_peer *peer,
 	dma_fifo_change_tx_limit(&port->tx_fifo, port->max_payload);
 	spin_unlock_bh(&peer->port->lock);
 
-	if (port->port.console && port->fwcon_ops->notify != NULL)
+	if (port->port.console && port->fwcon_ops->notify)
 		(*port->fwcon_ops->notify)(FWCON_NOTIFY_ATTACH, port->con_data);
 
 	fwtty_info(&peer->unit, "peer (guid:%016llx) connected on %s\n",
@@ -1808,7 +1808,7 @@ static void fwserial_release_port(struct fwtty_port *port, bool reset)
 	RCU_INIT_POINTER(port->peer, NULL);
 	spin_unlock_bh(&port->lock);
 
-	if (port->port.console && port->fwcon_ops->notify != NULL)
+	if (port->port.console && port->fwcon_ops->notify)
 		(*port->fwcon_ops->notify)(FWCON_NOTIFY_DETACH, port->con_data);
 }
 
-- 
2.4.3

[PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Dominique van den Broeck]

Fixing a lone row exceeding 80 columns so the only remaining warnings
emitted by checkpatch.pl are missing comments on spinlocks and memory
barriers.

Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
 drivers/staging/fwserial/fwserial.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 4dd5304..c5f73ef 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
 
 	if (state == -1) {
 		set_bit(STOP_TX, &port->flags);
-		ret = wait_event_interruptible_timeout(port->wait_tx,
-					       !test_bit(IN_TX, &port->flags),
-					       10);
+		ret =
+		wait_event_interruptible_timeout(port->wait_tx,
+						 !test_bit(IN_TX, &port->flags),
+						 10);
+
 		if (ret == 0 || ret == -ERESTARTSYS) {
 			clear_bit(STOP_TX, &port->flags);
 			fwtty_restart_tx(port);
-- 
2.4.3

Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Joe Perches]

On Tue, 2016-03-29 at 19:14 +0200, Dominique van den Broeck wrote:
> Fixing a lone row exceeding 80 columns so the only remaining warnings
> emitted by checkpatch.pl are missing comments on spinlocks and memory
> barriers.
[]
> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
[]
> @@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
>  
>  	if (state == -1) {
>  		set_bit(STOP_TX, &port->flags);
> -		ret = wait_event_interruptible_timeout(port->wait_tx,
> -					       !test_bit(IN_TX, &port->flags),
> -					       10);
> +		ret =
> +		wait_event_interruptible_timeout(port->wait_tx,
> +						 !test_bit(IN_TX, &port->flags),
> +						 10);

Does this really look better to you?

Long identifiers like "wait_event_interruptible_timeout"
(32 chars) make
using 80 columns a bit silly.

Please remember checkpatch is a stupid script and that
not every warning it emits is dicta.

Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Dominique van den Broeck]

> Does this really look better to you?
>
> Long identifiers like "wait_event_interruptible_timeout"
> (32 chars) make
> using 80 columns a bit silly.
> 
> Please remember checkpatch is a stupid script and that
> not every warning it emits is dicta.

Actually, not much and as a matter of fact, I hesitated before sending
this particular patch. That's also why I submitted it as the last one
of them all and why it fixes only a single row : so we can eventually
reject it easily.

I'm not a checkpatch.pl nazi neither and anyway, even the "Development
Process" document specify that it's not to be considered as a strict
rule if it makes the things worse.

But I found it useful anyway because this set brings back all the
warnings to only two categories (comments over spinlocks and memory
barriers) and also because this kind of minor corrections could still
be appreciated by people that have better things to do... :-)

Cheers.

Re: [PATCH 1/3] staging: fwserial: (coding style) Turning every "unsigned" into "unsigned int"

[Peter Hurley]

On 03/29/2016 10:14 AM, Dominique van den Broeck wrote:
> Coding-style-only modifications to remove every warning saying:
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> 
> Compiled against revision "next-20160327".
> 
> (checkpatch.pl was updated to treat "UNSPECIFIED_INT" warnings
>  as of commit a1ce18e4f941d20 )

Please change $subject to "staging: fwserial: Convert unsigned to unsigned int"
With that,

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>


PS - For v2 of this patch, be sure to change the subject prefix to [PATCH v2 ...],
and note any changes in the diffstat area of the patch itself; eg.,


Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---

v2: change subject per Peter Hurley's comments


 drivers/staging/fwserial/dma_fifo.c |  8 +++----
 drivers/staging/fwserial/dma_fifo.h | 16 +++++++-------
 drivers/staging/fwserial/fwserial.c | 38 +++++++++++++++++----------------
 drivers/staging/fwserial/fwserial.h | 42 ++++++++++++++++++-------------------
 4 files changed, 53 insertions(+), 51 deletions(-)

Re: [PATCH 2/3] staging: fwserial: (coding style) removing "!= NULL" to comply with checkpatch.pl

[Peter Hurley]

On 03/29/2016 10:14 AM, Dominique van den Broeck wrote:
> Removing two "!= NULL" from fwserial.c as suggested by checkpatch.pl.
> Note that the associated expression "port->port.console" is a 1-bit-field
> that is already assumed as an implicit boolean (that is: without comparison)

Similarly, please change $subject to "staging: fwserial: Simplify NULL ptr check"
With that,

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Peter Hurley]

Hi Dominique,

On 03/29/2016 10:14 AM, Dominique van den Broeck wrote:
> Fixing a lone row exceeding 80 columns so the only remaining warnings
> emitted by checkpatch.pl are missing comments on spinlocks and memory
> barriers.
> 
> Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
> ---
>  drivers/staging/fwserial/fwserial.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
> index 4dd5304..c5f73ef 100644
> --- a/drivers/staging/fwserial/fwserial.c
> +++ b/drivers/staging/fwserial/fwserial.c
> @@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
>  
>  	if (state == -1) {
>  		set_bit(STOP_TX, &port->flags);
> -		ret = wait_event_interruptible_timeout(port->wait_tx,
> -					       !test_bit(IN_TX, &port->flags),
> -					       10);
> +		ret =
> +		wait_event_interruptible_timeout(port->wait_tx,
> +						 !test_bit(IN_TX, &port->flags),
> +						 10);
> +

I don't see a > 80-col line here?

And even if I did, this change would be super-ugly.
The preferred way to reduce this is to fold it into a helper function, like

	if (state == -1 && fwtty_wait_tx_complete(port))
		return -EINTR;


Regards,
Peter Hurley

>  		if (ret == 0 || ret == -ERESTARTSYS) {
>  			clear_bit(STOP_TX, &port->flags);
>  			fwtty_restart_tx(port);
> 

Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Dominique van den Broeck]

Hello Peter,
Thanks a lot for your review and kind advice !

> I don't see a > 80-col line here?

In fact, it was not even a 80-col issue but a mis-aligned parenthesis
one. Realign the rows in this state would make them exceed the 80th
column.

I tend to agree with the fact that the way it currently is remains the
best one.

> And even if I did, this change would be super-ugly.
> The preferred way to reduce this is to fold it into a helper
> function

Actually, before I resend my patches, I have two or three small
questions:

1) My v1 patches already made it to staging and linux-next trees.
   Should I resend them anyway ?
2) Would it be helpful to people if I write a function the way you 
   specified it or would it be better to let it as is ?
3) If we don't, and then discard the last patch, shall I number « n/2 »
   or « n/3 » anyway ?

Forgive me if these questions are lame, I still have only a few
experience of the kernel tree. Documentation/SubmittingPatches states
that no one should be expected to refer to a previous set of patches,
so I suppose this would be « 1/2 » and « 2/2 » but I prefer being OK
about this from the beginning.

Thanks for caring.

Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

[Peter Hurley]

On 04/01/2016 04:20 PM, Dominique van den Broeck wrote:
> Hello Peter,
> Thanks a lot for your review and kind advice !
> 
>> I don't see a > 80-col line here?
> 
> In fact, it was not even a 80-col issue but a mis-aligned parenthesis
> one. Realign the rows in this state would make them exceed the 80th
> column.

Ah, ok. Wasn't clear from the commit message.

> I tend to agree with the fact that the way it currently is remains the
> best one.

Ok.

>> And even if I did, this change would be super-ugly.
>> The preferred way to reduce this is to fold it into a helper
>> function
> 
> Actually, before I resend my patches, I have two or three small
> questions:
> 
> 1) My v1 patches already made it to staging and linux-next trees.
>    Should I resend them anyway ?

No, I didn't know they were already in staging-next.
Nevermind then :)

> 2) Would it be helpful to people if I write a function the way you 
>    specified it or would it be better to let it as is ?

As is, please.

> 3) If we don't, and then discard the last patch, shall I number « n/2 »
>    or « n/3 » anyway ?

n/a now.


> Forgive me if these questions are lame, I still have only a few
> experience of the kernel tree.

Your questions are not lame; no need to apologize.

> Documentation/SubmittingPatches states
> that no one should be expected to refer to a previous set of patches,
> so I suppose this would be « 1/2 » and « 2/2 » but I prefer being OK
> about this from the beginning.

If you would have sent the patches, yes, they would have been 1/2 and 2/2.
What I do there is send the v2 series in-reply-to the original 1/2 patch.


> Thanks for caring.

Regards,
Peter Hurley