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

[Johan Hovold <johan@kernel.org>]

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