| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAC=G6syuf-J4am1mHRg-5gzqy661yOT9pUK2vQyToLPtv4tvEg@mail.gmail.com>
Date: Sun, 23 Oct 2011 12:12:13 -0500
From: Paul Schilling <paul.s.schilling@...il.com>
To: Russell King - ARM Linux <linux@....linux.org.uk>
Cc: Ben Dooks <ben-linux@...ff.org>,
Kukjin Kim <kgene.kim@...sung.com>,
Alan Cox <alan@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...e.de>,
Boojin Kim <boojin.kim@...sung.com>,
Nicolas Pitre <nicolas.pitre@...aro.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH 02/14] ARM : SAMSUNG : Add RS485 support.
See Comments inline.
On Sat, Oct 22, 2011 at 8:47 AM, Russell King - ARM Linux
<linux@....linux.org.uk> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists