Re: [PATCH 2/4] cp210x: Unify code for set/get config control messages

[Johan Hovold <johan@kernel.org>]

On Fri, Jul 24, 2015 at 08:48:09AM +0200, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.cz>
> 
> There is a lot of overlap between the two functions (e.g. calculation
> of the buffer size), so this removes a bit of code duplication, but
> most importantly, a more generic function can be easily reused for
> other message types.

I'm not sure I consider this is an improvement yet.

> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  drivers/usb/serial/cp210x.c | 109 ++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 1bae015..69f03b6 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -307,14 +307,17 @@ enum cp210x_request_type {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> - * cp210x_get_config
> - * Reads from the CP210x configuration registers
> + * cp210x_control_msg
> + * Sends a generic control message, taking care of endianness
> + * and error messages.
>   * 'size' is specified in bytes.
> - * 'data' is a pointer to a pre-allocated array of integers large
> - * enough to hold 'size' bytes (with 4 bytes to each integer)
> + * 'data' is a pointer to the input/output buffer. For output, it holds
> + * the data (in host order) to be sent. For input, it receives data from
> + * the device and must be big enough to hold 'size' bytes.
>   */
> -static int cp210x_get_config(struct usb_serial_port *port, u8 request,
> -		unsigned int *data, int size)
> +static int cp210x_control_msg(struct usb_serial_port *port, u8 request,
> +			      u8 requesttype, u16 value, u32 *data, int size,
> +			      int timeout)

Should you not use your new request type enum here?

>  {
>  	struct usb_serial *serial = port->serial;
>  	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> @@ -328,20 +331,22 @@ static int cp210x_get_config(struct usb_serial_port *port, u8 request,
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	/* Issue the request, attempting to read 'size' bytes */
> -	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> -				request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
> -				spriv->bInterfaceNumber, buf, size,
> -				USB_CTRL_GET_TIMEOUT);
> +	if (!(requesttype & USB_DIR_IN)) {
> +		for (i = 0; i < length; i++)
> +			buf[i] = cpu_to_le32(data[i]);
> +	}
>  
> -	/* Convert data into an array of integers */
> -	for (i = 0; i < length; i++)
> -		data[i] = le32_to_cpu(buf[i]);
> +	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),

And this should be usb_sndctrlpipe for outgoing messages.

> +				 request, requesttype, value,
> +				 spriv->bInterfaceNumber, buf, size, timeout);

Please resend this when you start using your generalised function (for
the gpio work?).

I'll drop all four for now.

Thanks,
Johan