Re: [PATCH 02/14] ARM : SAMSUNG : Add RS485 support.
From: Paul Schilling
Date: Sun Oct 23 2011 - 13:12:20 EST
See Comments inline.
On Sat, Oct 22, 2011 at 8:47 AM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> On Fri, Oct 21, 2011 at 10:46:34PM -0500, Paul Schilling wrote:
>> +config SAMSUNG_HAS_RS485
>> + bool "Enable RS485 support for Samsung"
>> + depends on SERIAL_SAMSUNG && (MACH_CONDOR2440 || MACH_CONDOR2416 || MACH_MINI2440)
>> + default y if (MACH_CONDOR2440 || MACH_CONDOR2416)
>> + default n if (MACH_MINI2440)
>> +
>> +config SAMSUNG_485_LOW_RES_TIMER
>> + bool "Samsung RS-485 use low res timer during transmit"
>> + depends on SERIAL_SAMSUNG && SAMSUNG_HAS_RS485
>> + default n
>
> n is the default, so this doesn't need to be specified.
Sorry I can fix that.
>
>> +static void s3c24xx_serial_rx_fifo_enable(
>> + struct uart_port *port,
>> + unsigned int en)
>> +{
>> + unsigned long flags;
>> + unsigned int ucon;
>> + static unsigned int last_state = 1;
>> +/* FIXME */
>> + #if 0
>> + if (last_state != en) {
>> +
>> + struct s3c2410_uartcfg *cfg = s3c24xx_port_to_cfg(port);
>> +
>> + spin_lock_irqsave(&port->lock, flags);
>> +
>> + ucon = rd_regl(port, S3C2410_UCON);
>> +
>> + ucon &= ~(S3C2440_UFCON_RXTRIG32 | S3C2410_UCON_RXILEVEL);
>> +
>> + if (en) {
>> + ucon |= cfg->ucon;
>> + }
>> +
>> + wr_regl(port, S3C2410_UCON, ucon);
>> +
>> + spin_unlock_irqrestore(&port->lock, flags);
>> + }
>> +#endif
>
> This looks like dead code.
It is broken dead code. Performance of the RS485 code could be
increased if this code could be made to work.
right now I am forced to leave it in one character per interrupt so
that each character that's received is checked
against the token. This code was supposed leave it in multi-byte FIFO
mode until the token byte is sent then switch to receiving
one byte at a time until the token is received.
>
>> + } else {
>> + /* Set a short timer to toggle RTS */
>> + mod_timer(
>> + &(ourport->rs485_tx_timer),
>> + jiffies + usecs_to_jiffies(
>> + ourport->char_time_usec
>> + / 10));
>
> This could do with being better formatted. Also, & doesn't need following
> parens.
when I ran checkpatch it complained that it exceeded 80 characters. I
had trouble keeping this line
under 80 characters.
>
>> + /* Read UART transmit status register */
>> + utrstat = rd_regl(&(ourport->port), S3C2410_UTRSTAT);
>
> Doesn't need the parens.
I can remove the extra parentheses.
>
>> +/* Callback array*/
>> +enum hrtimer_restart (*callback_list[CONFIG_SERIAL_SAMSUNG_UARTS])(struct hrtimer *) = {
>> + &rs485_hr_timer_callback_uart0,
>> + &rs485_hr_timer_callback_uart1,
>> +
>> +#if CONFIG_SERIAL_SAMSUNG_UARTS > 2
>> + &rs485_hr_timer_callback_uart2,
>> +#endif
>> +
>> +#if CONFIG_SERIAL_SAMSUNG_UARTS > 3
>> + &rs485_hr_timer_callback_uart3,
>> +#endif
>
> Silly indentation - this doesn't need two tabs.
>
>> +};
>> +
>> +#endif
>> +#endif /* CONFIG_SAMSUNG_HAS_RS485 */
>> +
>> +
>> static void s3c24xx_serial_stop_tx(struct uart_port *port)
>> {
>> struct s3c24xx_uart_port *ourport = to_ourport(port);
>>
>> if (tx_enabled(port)) {
>> +#ifdef CONFIG_SAMSUNG_HAS_RS485
>> + if (ourport->rs485.flags & SER_RS485_ENABLED) {
>> +#ifdef CONFIG_SAMSUNG_485_LOW_RES_TIMER
>> + /* Set a short timer to toggle RTS */
>> + mod_timer(&(ourport->rs485_tx_timer),
>
> Doesn't need parens.
>
>> + jiffies + usecs_to_jiffies(ourport->char_time_usec * s3c24xx_serial_tx_getfifocnt(ourport)));
>> +#else
>> + ktime_t kt;
>> +
>> + /* Set time struct to one char time. */
>> + kt = ktime_set(0, ourport->char_time_nanosec);
>> +
>> + /* Start the high res timer. */
>> + hrtimer_start(&(ourport->hr_rs485_tx_timer), kt, HRTIMER_MODE_REL);
>
> Doesn't need parens.
>
>> +#endif /* CONFIG_SAMSUNG_485_LOW_RES_TIMER */
>> +
>> + s3c24xx_serial_rx_fifo_enable(port, 0);
>> +
>> + }
>> +#endif /* CONFIG_SAMSUNG_HAS_RS485 */
>> +
>> disable_irq_nosync(ourport->tx_irq);
>> tx_enabled(port) = 0;
>> - if (port->flags & UPF_CONS_FLOW)
>> + if (port->flags & UPF_CONS_FLOW) {
>> s3c24xx_serial_rx_enable(port);
>> + }
>
> Why are you reformatting code?
I will remove the reformatting of the code.
>
>> @@ -785,7 +1177,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
>> port->ignore_status_mask = 0;
>> if (termios->c_iflag & IGNPAR)
>> port->ignore_status_mask |= S3C2410_UERSTAT_OVERRUN;
>> - if (termios->c_iflag & IGNBRK && termios->c_iflag & IGNPAR)
>> + if ((termios->c_iflag & IGNBRK) && (termios->c_iflag & IGNPAR))
>
> More code reformatting.
>
>> @@ -830,7 +1228,7 @@ static void s3c24xx_serial_config_port(struct uart_port *port, int flags)
>> {
>> struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
>>
>> - if (flags & UART_CONFIG_TYPE &&
>> + if ((flags & UART_CONFIG_TYPE) &&
>
> And some more.
>
>> +#if 0
>> + dev_info(port., "rts: on send = %i, after = %i, enabled = %i",
>
> That can't be correct - and as its #if 0'd out, either remove this or
> fix it to be correct (and use dev_dbg if you want it to be debugging.)
>
>> +static ssize_t s3c24xx_serial_set_485_mode(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +
>> +{
>> + struct uart_port *port = s3c24xx_dev_to_port(dev);
>> + struct s3c24xx_uart_port *ourport = to_ourport(port);
>> +
>> + if (!strncmp(buf, "Enabled", 7)) {
>> + ourport->rs485.flags |= SER_RS485_ENABLED;
>> + } else if (!strncmp(buf, "Disabled", 8)) {
>
> Do you really require the first character to be capitalized?
>
>> +#ifdef CONFIG_SAMSUNG_HAS_RS485
>> +
>> + ret = device_create_file(&dev->dev, &dev_attr_485_status);
>> + if (ret < 0)
>> + printk(KERN_ERR "%s: failed to add 485 status attr.\n", __func__);
>
> pr_err() ? dev_err() ?
>
--
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/