Re: [PATCH 8/8] imx: serial: add IrDA support to serial driver

[Oskar Schirmer]

On Tue, Jun 09, 2009 at 16:01:32 +0100, Alan Cox wrote:
> > +#ifdef CONFIG_IRDA
> > +#define USE_IRDA(sport)	((sport)->use_irda)
> > +#else
> > +#define USE_IRDA(sport)	(0)
> > +#endif
> > +
> 
> You define this but only use it in about half the cases ?

Yes, with CONFIG_IRDA undefined several locations look like e.g.:

if (0) {
	... (sport)->irda_inv_rx ...
}

Sure, the path will be optimized away, but still
the conditional statement is subjected to syntax
and scope analysis and gcc will correctly complain
that irda_inv_rx is not declared.

On the other hand, we could omit the USE_IRDA macro
altogether, but then we would have even more ifdefs
around.

> 
> > +#ifdef CONFIG_IRDA
> > +	if (sport->use_irda) {
> > +		/* half duplex - wait for end of transmission */
> > +		int n = 256;
> 
> Otherwise this series looks sound.
> 
> While you are fiddling with the baud rate the driver really should work
> out the resulting actual baud rate and call
> 
> 	tty_encode_baud_rate(tty, in-rate, out-rate);
> 
> that will report back the resulting rate and knows how to encode all the
> termios fields as well as reporting close values to the requested speed
> as the speed requested (eg if you ask for B38400 and it tries to encode
> 38215 or similar it will encode back B38400)

Ok, the driver will need another fix here with respect to its
interface to the upper layers. But the changes in
baud rate calculation we made are with focus on correct
peripheral register settings, because the old baud rate
calculation was not accurate enough for IrDA use.
That's why we introduced the approximation function
(and finally generalised it into lib/).

  Oskar
--
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/