Re: [PATCH 4/4] n_tracerouter and n_tracesink ldisc additions.

[Jesper Juhl]

On Fri, 22 Apr 2011, james_p_freyensee@xxxxxxxxxxxxxxx wrote:

> From: J Freyensee <james_p_freyensee@xxxxxxxxxxxxxxx>
> 
> The n_tracerouter and n_tracesink line discpline drivers use the
> Linux tty line discpline framework to route trace data coming
> from a tty port (say UART for example) to the trace sink line
> discipline driver and to another tty port(say USB).  Those
> these two line discipline drivers can be used together,
> independently from pti.c, they are part of the original
> implementation solution of the MIPI P1149.7, compact JTAG, PTI
> solution for Intel mobile platforms starting with the
> Medfield platform.
> 
> Signed-off-by: J Freyensee <james_p_freyensee@xxxxxxxxxxxxxxx>

A few minor comments can be found below.


...
> +static int n_tracesink_open(struct tty_struct *tty)
> +{
> +	int retval = -EEXIST;
> +
> +	mutex_lock(&writelock);
> +	if (this_tty == NULL) {
> +
> +		this_tty = tty_kref_get(tty);
> +

pointless blank line. I'd suggest removing it.


...
> +		if (this_tty == NULL)
> +			retval = -EFAULT;
> +		else {

If one branch requires braces, both should have them

        } else {


...
> +static void n_tracesink_close(struct tty_struct *tty)
> +{
> +
> +	mutex_lock(&writelock);
> +	tty_driver_flush_buffer(tty);
> +	tty_kref_put(this_tty);
> +	this_tty = NULL;
> +	tty->disc_data = NULL;
> +	mutex_unlock(&writelock);
> +
> +}

pointless blank line at end of function. Remove it.


> +static int __init n_tracesink_init(void)
> +{
> +	int retval;
> +
> +	/* Note N_TRACESINK is defined in linux/tty.h */
> +	retval = tty_register_ldisc(N_TRACESINK, &tty_n_tracesink);
> +
> +	if (retval < 0)
> +		pr_err("%s: Registration failed: %d\n", __func__, retval);
> +
> +	return retval;
> +}

How about shortening is a bit - like this?

static int __init n_tracesink_init(void)
{
     /* Note N_TRACESINK is defined in linux/tty.h */
     int retval = tty_register_ldisc(N_TRACESINK, &tty_n_tracesink);
     
     if (retval < 0)
             pr_err("%s: Registration failed: %d\n", __func__, retval);
             
     return retval;
}


...
> +static void __exit n_tracesink_exit(void)
> +{
> +	int retval;
> +
> +	retval = tty_unregister_ldisc(N_TRACESINK);
> +
> +	if (retval < 0)
> +		pr_err("%s: Unregistration failed: %d\n", __func__,  retval);
> +}

How about:

static void __exit n_tracesink_exit(void)
{
     int retval = tty_unregister_ldisc(N_TRACESINK);
     
     if (retval < 0)
             pr_err("%s: Unregistration failed: %d\n", __func__,  retval);
}


...
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -50,6 +50,8 @@
>  #define N_CAIF		20      /* CAIF protocol for talking to modems */
>  #define N_GSM0710	21	/* GSM 0710 Mux */
>  #define N_TI_WL		22	/* for TI's WL BT, FM, GPS combo chips */
> +#define N_TRACESINK	23	/* Trace data routing for MIPI P1149.7 */
> +#define N_TRACEROUTER	24	/* Trace data routing for MIPI P1149.7 */
>  

Lining up those defines would be nice :)


-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/