Re: patch "U6715 8250 serial like driver" added to gregkh-2.6 tree

[Philippe Langlais]

Hi,

On 07/23/10 10:01, Alan Cox wrote:
> On Thu, 22 Jul 2010 16:57:57 -0700
> <gregkh@xxxxxxx>  wrote:
>
>    
> > This is a note to let you know that I've just added the patch titled
> >      
> NAK this
>
> I've nakked several similar patches already from people trying to
> convolute the 8250 driver even further, doubly so when the patch
> redefines properties of existing chips according to a compile time option
>
>
>    
> >   	[PORT_16550A] = {
> >   		.name		= "16550A",
> > +#if defined(CONFIG_SERIAL_8250_U6XXX)
> > +		.fifo_size	= 64,
> > +		.tx_loadsz	= 64,
> > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > +#else
> >   		.fifo_size	= 16,
> >   		.tx_loadsz	= 16,
> > -		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> >   		.flags		= UART_CAP_FIFO,
> > +#endif
> > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> >   	},
> >      
> Define yourself a new port type
>    
I agree, but the problem is to recognize our 16550A IP vs others.
Until now I can't find a differentiator with other 16550A and this IP
is only use in our U6XXX SoCs family.
To avoid mis recognition can I patch autoconfig_16550a() in 8250.c like 
this:

static void autoconfig_16550a(struct uart_8250_port *up)
{
     unsigned char status1, status2;
     unsigned int iersave;

+#if !defined(CONFIG_PLAT_U6XXX)
     up->port.type = PORT_16550A;
+#else
+    up->port.type = PORT_U6_16550A;
+#endif

>    
> > @@ -2268,6 +2274,13 @@ serial8250_set_termios(struct uart_port
> >   	/*
> >   	 * Ask the core to calculate the divisor for us.
> >   	 */
> > +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
> > +	baud = uart_get_baud_rate(port, termios, old, 0,
> > +			CONFIG_SERIAL_8250_CUSTOM_MAX_BAUDRATE);
> > +	/* Calculate the new uart clock frequency if it is tunable */
> > +	port->uartclk = serial8250_get_custom_clock(port, baud);
> > +#endif
> > +
> >   	baud = uart_get_baud_rate(port, termios, old,
> >   				  port->uartclk / 16 / 0xffff,
> >   				  port->uartclk / 16);
> >      
>
> I think what you need to keep this clean is to create a
> port->set_termios() for the 8250 port (akin to
> port->serial_in/serial_out) which does the specials you need each end and
> calls the standard serial8250_set_termios)
>    
I don't think so, call a function which compute clock each time
we access UART registers isn't optimal, another function to overload ?
>    
> > @@ -2298,6 +2311,13 @@ serial8250_set_termios(struct uart_port
> >   		up->mcr&= ~UART_MCR_AFE;
> >   		if (termios->c_cflag&  CRTSCTS)
> >   			up->mcr |= UART_MCR_AFE;
> > +#if defined(CONFIG_SERIAL_8250_U6XXX)
> > +		/**
> > +		 * When AFE is active, let the HW handle the stop/restart TX
> > +		 * upon CTS change. It reacts much quicker than the SW driver.
> > +		 */
> > +		port->flags&= ~ASYNC_CTS_FLOW;
> > +#endif
> >      
> Why do you need to clear this flag when doing so ?
>    
I don't know if it's useful, I'll withdraw this & make tests.
>    
> > +unsigned int serial8250_enable_clock(struct uart_port *port)
> > +{
> > +	struct u6_uart *uart_u6 = port->private_data;
> > +
> > +	if (!uart_u6)
> > +		return uart_enable_clock(port);
> > +
> > +	if (IS_ERR(uart_u6->uartClk)) {
> > +		printk(KERN_WARNING "%s - uart clock failed error:%ld\n",
> > +		       __func__, PTR_ERR(uart_u6->uartClk));
> >      
> pr_warn()
>
> Also as the functions are specific to the u6175 can you use u6715_ names
> so any printk, traceback and the like is obvious in where to look.
>    
OK
>    
> > +unsigned int serial8250_get_custom_clock(struct uart_port *port,
> > +					 unsigned int baud)
> > +{
> > +	switch (baud) {
> > +	case 3250000:
> > +		return 52000000;
> > +	case 2000000:
> > +		return 32000000;
> > +	case 1843200:
> > +		return 29491200;
> > +	case 921600:
> > +		return 14745600;
> > +	default:
> > +		return 7372800;
> > +	}
> > +}
> >      
> Baud rates are arbitary values, so surely this should doing range checks ?
>    
Yes
>    
> > +config SERIAL_8250_CUSTOM_CLOCK
> > +	bool "Support serial ports with tunable input clock frequency"
> > +	depends on SERIAL_8250_EXTENDED&&  SERIAL_8250_U6XXX
> > +	default y
> > +	help
> > +	  Say Y here if your platform has specific registers to change UART clock frequency.
> >      
> Except the patch you've posted is actually a patch for a U6715 not
> generic tunable input clock support, so this Kconfig option/help will
> confuse people. Better to describe it as U6715 support.
>    
Yes
>    
> > +#ifdef CONFIG_SERIAL_8250_CUSTOM_CLOCK
> > +unsigned int serial8250_enable_clock(struct uart_port *port);
> > +unsigned int serial8250_disable_clock(struct uart_port *port);
> > +unsigned int serial8250_get_custom_clock(struct uart_port *port,
> > +		unsigned int baud);
> > +void serial8250_set_custom_clock(struct uart_port *port);
> > +#endif
> >      
> This shouldn't need ifdefs in the header
>    
Yes

Regards
Philippe
--
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/