Re: [Xenomai-core] new features for 16550A driver

[Jan Kiszka <jan.kiszka@domain.hid>]

Guillaume Gaudonville wrote:
> I've attached the patch drafts. It works fine for me. it prepare the work to
> implement the other iflag values as describe in man tcsetattr.
> 

...

> 
> Index: ksrc/drivers/serial/16550A.c
> ===================================================================
> --- ksrc/drivers/serial/16550A.c	(révision 2765)
> +++ ksrc/drivers/serial/16550A.c	(copie de travail)
> @@ -153,7 +153,10 @@ static inline int rt_16550_rx_interrupt(
>  	do {
>  		c = rt_16550_reg_in(mode, base, RHR);	/* read input char */
>  
> -		ctx->in_buf[ctx->in_tail] = c;
> +		if (testbits(ctx->config.iflags, RTSER_IFLAG_ISTRIP))
> +			ctx->in_buf[ctx->in_tail] = c & 0x7f;
> +		else
> +			ctx->in_buf[ctx->in_tail] = c;

OK, things become clearer.

Question: Switching parity checking on in the hardware does not work/is 
no option for you? Shouldn't that clear the parity information as well?

>  		if (ctx->in_history)
>  			ctx->in_history[ctx->in_tail] = *timestamp;
>  		ctx->in_tail = (ctx->in_tail + 1) & (IN_BUFFER_SIZE - 1);
> @@ -426,6 +429,10 @@ static int rt_16550_set_config(struct rt
>  		rtdm_lock_put_irqrestore(&ctx->lock, lock_ctx);
>  	}
>  
> +	if (testbits(config->config_mask, RTSER_SET_IFLAGS)) {
> +		ctx->config.iflags = config->iflags;
> +	}
> +
>  	return err;
>  }
>  
> Index: include/rtdm/rtserial.h
> ===================================================================
> --- include/rtdm/rtserial.h	(révision 2765)
> +++ include/rtdm/rtserial.h	(copie de travail)
> @@ -184,6 +184,7 @@
>  #define RTSER_SET_TIMEOUT_EVENT		0x0400
>  #define RTSER_SET_TIMESTAMP_HISTORY	0x0800
>  #define RTSER_SET_EVENT_MASK		0x1000
> +#define RTSER_SET_IFLAGS		0x2000
>  /** @} */
>  
>  
> @@ -238,6 +239,15 @@
>  #define RTSER_BREAK_SET			0x01
>  
>  
> +/*!
> + * @anchor RTSER_IFLAG_xxx   @name RTSER_IFLAG_xxx
> + * Input behaviour (when used every bits will be set by the ioctl command)
> + * @{ */
> +#define RTSER_IFLAG_NONE		0x00
> +#define RTSER_IFLAG_ISTRIP		0x01
> +#define RTSER_IFLAG_DEFAULT		0x00
> +
> +
>  /**
>   * Serial device configuration
>   */
> @@ -280,6 +290,11 @@ typedef struct rtser_config {
>  	/** event mask to be used with @ref RTSER_RTIOC_WAIT_EVENT, see
>  	 *  @ref RTSER_EVENT_xxx */
>  	int		event_mask;
> +
> +	/** input flags, see @ref RTSER_IFLAG_xxx, RTSER_IFLAG_ISTRIP is
> +	 *  the only one supported for the moment (when used every bits
> +	 *  must be set according to the desired configuration)*/
> +	int		iflags;
>  } rtser_config_t;
>  
>  /**

The patch looks very clean, no question! Assuming my attempt above to 
use the hardware for this is nonsense, I then wonder if the feature is 
worth the additional conditional branch + another potential cache miss 
(for ctx->config.iflags) inside the critical reception path.

This is precisely that question of balance: Is the extension of some 
broader interest? Do we need it inside the driver (I see no reason yet 
why the application cannot do the masking, and we are not forced into 
compatibility with termios)? And does the extension impact critical paths?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux