Re: [PATCH V7 06/11] USB: f81232: implement MCR/MSR function

Johan Hovold
2015-02-25T20:45:16Z

On Tue, Feb 24, 2015 at 06:16:21PM +0800, Peter Hung wrote:
> This patch implement relative MCR/MSR function, such like
> tiocmget()/tiocmset()/dtr_rts()/carrier_raised()
>
> original f81232_carrier_raised() compared with wrong value UART_DCD.
> It's should compared with UART_MSR_DCD.
>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

This patch as well as a few of the other patches in v7 now have
checkpatch warnings and errors.

> ---
> drivers/usb/serial/f81232.c | 103 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 81 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 339be30..21f606f 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -37,6 +37,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define F81232_SET_REGISTER 0x40
>
> #define SERIAL_BASE_ADDRESS 0x0120
> +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
> #define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>
> #define CONTROL_DTR 0x01
> @@ -55,7 +56,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
>
> struct f81232_private {
> struct mutex lock;
> - u8 line_control;
> + u8 modem_control;
> u8 modem_status;
> struct work_struct interrupt_work;
> struct usb_serial_port *port;
> @@ -198,6 +199,52 @@ static void f81232_read_msr(struct usb_serial_port *port)
> wake_up_interruptible(&port->port.delta_msr_wait);
> }
>
> +static int f81232_set_mctrl(struct usb_serial_port *port,
> + unsigned int set, unsigned int clear)
> +{
> + u8 urb_value;
> + int status;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0)
> + return 0; /* no change */
> +
> + /* 'set' takes precedence over 'clear' */
> + clear &= ~set;
> +
> + /* force enable interrupt with OUT2 */
> + mutex_lock(&priv->lock);
> + urb_value = UART_MCR_OUT2 | priv->modem_control;
> + mutex_unlock(&priv->lock);

So this is one of the places where the port mute should protect the
whole operation.

> +
> + if (clear & TIOCM_DTR)
> + urb_value &= ~UART_MCR_DTR;
> +
> + if (clear & TIOCM_RTS)
> + urb_value &= ~UART_MCR_RTS;
> +
> + if (set & TIOCM_DTR)
> + urb_value |= UART_MCR_DTR;
> +
> + if (set & TIOCM_RTS)
> + urb_value |= UART_MCR_RTS;
> +
> + dev_dbg(&port->dev, "%s new:%02x old:%02x\n", __func__,
> + urb_value, priv->modem_control);
> +
> + status = f81232_set_register(port, MODEM_CONTROL_REGISTER, urb_value);
> + if (status) {
> + dev_err(&port->dev, "%s set MCR status < 0\n", __func__);
> + return status;
> + } else {
> + mutex_lock(&priv->lock);
> + priv->modem_control = urb_value;
> + mutex_unlock(&priv->lock);
> + }

No need for else-branch as you return on errors.

> +
> + return 0;
> +}

Johan