Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Johan Hovold]

On Mon, Oct 19, 2015 at 05:01:24PM -0500, Konstantin Shkolnyy wrote:
> Existing register access functions cp210x_get_config and cp210x_set_config
> are cumbersome to use. This change introduces new functions specifically
> for 16-bit registers that read and write simple u16 values.

There's a bit too much going on in both these patches. Remember, one
logical change per patch.

Also make sure to include the patch revision in the subject and bump it
(for the whole series) when resending.

> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@xxxxxxxxx>
> ---
>  drivers/usb/serial/cp210x.c | 119 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..5a7c15e 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -301,6 +301,77 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> + * Reads any 16-bit CP210X_ register (req).
> + */
> +static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	__le16 le16_value;
> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_rcvctrlpipe(serial->dev, 0),
> +				req, REQTYPE_INTERFACE_TO_HOST, 0,
> +				spriv->bInterfaceNumber, &le16_value, 2,
> +				USB_CTRL_SET_TIMEOUT);

As Oliver already pointed out, you cannot use an automatic variable for
the data stage here.

> +	if (result != 2) {
> +		if (result > 0)
> +			result = -EPROTO;
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);
> +		return result;
> +	}
> +	*pval = le16_to_cpu(le16_value);
> +	return 0;
> +}
> +
> +/*
> + * Writes any 16-bit CP210X_ register (req) whose value is passed
> + * entirely in the wValue field of the USB request.
> + */
> +static int cp210x_write_u16_reg(struct usb_serial *serial, u8 req, u16 val)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_sndctrlpipe(serial->dev, 0),
> +				req, REQTYPE_HOST_TO_INTERFACE, val,
> +				spriv->bInterfaceNumber, NULL, 0,
> +				USB_CTRL_SET_TIMEOUT);
> +	if (result < 0) {
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);
> +	}
> +	return result;
> +}
> +
> +/*
> + * Command-specific wrappers around USB access functions
> + */
> +static int cp210x_ifc_enable(struct usb_serial_port *port)
> +{
> +	return cp210x_write_u16_reg(port->serial,
> +			CP210X_IFC_ENABLE, UART_ENABLE);
> +}
> +
> +static int cp210x_ifc_disable(struct usb_serial_port *port)
> +{
> +	return cp210x_write_u16_reg(port->serial,
> +			CP210X_IFC_ENABLE, UART_DISABLE);
> +}

Why are these here? This is unrelated to adding the line_ctl helpers.

> +
> +static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *pctrl)
> +{
> +	return cp210x_read_u16_reg(port->serial, CP210X_GET_LINE_CTL, pctrl);
> +}
> +
> +static int cp210x_set_line_ctl(struct usb_serial_port *port, u16 ctrl)
> +{
> +	return cp210x_write_u16_reg(port->serial, CP210X_SET_LINE_CTL, ctrl);
> +}

Instead of adding the new helpers to read u16 as a prerequisite for
fixing the broken cp2108 support, just reuse the current config register
helpers for now (in order to keep the fixes minimal and potentially
backportable). Once the fixes are in place, feel free to clean up the
remaining register accesses.

> +
> +/*
>   * cp210x_get_config
>   * Reads from the CP210x configuration registers
>   * 'size' is specified in bytes.
> @@ -400,17 +471,6 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
>  }
>  
>  /*
> - * cp210x_set_config_single
> - * Convenience function for calling cp210x_set_config on single data values
> - * without requiring an integer pointer
> - */
> -static inline int cp210x_set_config_single(struct usb_serial_port *port,
> -		u8 request, unsigned int data)
> -{
> -	return cp210x_set_config(port, request, &data, 2);
> -}
> -
> -/*
>   * cp210x_quantise_baudrate
>   * Quantises the baud rate as per AN205 Table 1
>   */
> @@ -456,12 +516,9 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
>  	int result;
>  
> -	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> -								UART_ENABLE);
> -	if (result) {
> -		dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
> +	result = cp210x_ifc_enable(port);
> +	if (result)
>  		return result;
> -	}

So this is unrelated, and should be dropped from this patch.

Johan
--
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/