| 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: <20111022134726.GD21374@n2100.arm.linux.org.uk>
Date: Sat, 22 Oct 2011 14:47:26 +0100
From: Russell King - ARM Linux <linux@....linux.org.uk>
To: Paul Schilling <paul.s.schilling@...il.com>
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.
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.
> +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.
> + } 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.
> + /* Read UART transmit status register */
> + utrstat = rd_regl(&(ourport->port), S3C2410_UTRSTAT);
Doesn't need the parens.
> +/* 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?
> @@ -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