Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Greg KH <greg@kroah.com>]

On Wed, Dec 01, 2010 at 08:10:44AM +0000, Jamie Iles wrote:
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -454,21 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
>  		writeb(value, p->membase + offset);
>  }
>  
> +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> +					int value)
> +{
> +	struct uart_8250_port *up =
> +		container_of(p, struct uart_8250_port, port);

container_of, when the original code did a simple cast:

>  static void dwapb_serial_out(struct uart_port *p, int offset, int value)
>  {
>  	int save_offset = offset;
>  	offset = map_8250_out_reg(p, offset) << p->regshift;
> -	/* Save the LCR value so it can be re-written when a
> -	 * Busy Detect interrupt occurs. */
> -	if (save_offset == UART_LCR) {
> -		struct uart_8250_port *up = (struct uart_8250_port *)p;
> -		up->lcr = value;
> -	}
> +	dwapb_save_out_value(p, save_offset, value);

Because of that, are you sure this is correct now?  You might just be
getting lucky due to the location of the pointer within the structure,
but then the old code was wrong.

Either way, I don't feel comfortable with this, do you?

thanks,

greg k-h