Re: [PATCH] USB: serial: cp210x: Cleaned up USB access functions.

[Johan Hovold]

On Mon, Nov 30, 2015 at 04:50:38PM -0600, Konstantin Shkolnyy wrote:
> cp210x_get_config and cp210x_set_config were hard to use. They required
> the buffer as an array of 32-bit values even for smaller values, and did
> endian conversions on per-32-bit value basis, which is wrong for some
> cp210x data structures (although not for any that were actually used.)
> This change introduces separate register accessor functions for single
> 8, 16 and 32-bit values, with endian conversion, as well as "block"
> access functions without conversion.
> 
> Signed-off-by: Konstantin Shkolnyy 
> ---
>  drivers/usb/serial/cp210x.c | 314 
> ++--
>  1 file changed, 186 insertions(+), 128 deletions(-)

There's a bit too much going on here at once. Please split this into
multiple patches to facilitate review (e.g. separate get and set, and
possibly also the three widths).

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] USB: serial: cp210x: Cleaned up USB access functions.

[Konstantin Shkolnyy]

cp210x_get_config and cp210x_set_config were hard to use. They required
the buffer as an array of 32-bit values even for smaller values, and did
endian conversions on per-32-bit value basis, which is wrong for some
cp210x data structures (although not for any that were actually used.)
This change introduces separate register accessor functions for single
8, 16 and 32-bit values, with endian conversion, as well as "block"
access functions without conversion.

Signed-off-by: Konstantin Shkolnyy 
---
 drivers/usb/serial/cp210x.c | 314 ++--
 1 file changed, 186 insertions(+), 128 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index fd67958..ce80d5f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -323,113 +323,169 @@ struct cp210x_comm_status {
 #define PURGE_ALL  0x000f
 
 /*
- * cp210x_get_config
- * Reads from the CP210x configuration registers
- * '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)
+ * Reads a variable-sized block of CP210X_ registers, identified by req.
+ * Returns data into buf in native USB byte order.
  */
-static int cp210x_get_config(struct usb_serial_port *port, u8 request,
-   unsigned int *data, int size)
+static int cp210x_read_reg_block(struct usb_serial_port *port, u8 req,
+   void *buf, int bufsize)
 {
struct usb_serial *serial = port->serial;
struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
-   __le32 *buf;
-   int result, i, length;
-
-   /* Number of integers required to contain the array */
-   length = (((size - 1) | 3) + 1) / 4;
+   void *dmabuf;
+   int result;
 
-   buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
-   if (!buf)
+   dmabuf = kmalloc(bufsize, GFP_KERNEL);
+   if (!dmabuf) {
+   /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+   memset(buf, 0, bufsize);
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, 0x,
-   port_priv->bInterfaceNumber, buf, size,
-   USB_CTRL_GET_TIMEOUT);
+   req, REQTYPE_INTERFACE_TO_HOST, 0,
+   port_priv->bInterfaceNumber, dmabuf, bufsize,
+   USB_CTRL_SET_TIMEOUT);
+   if (result == bufsize) {
+   memcpy(buf, dmabuf, bufsize);
+   result = 0;
+   } else {
+   dev_err(>dev, "failed get req 0x%x size %d status: %d\n",
+   req, bufsize, result);
+   if (result >= 0)
+   result = -EPROTO;
 
-   /* Convert data into an array of integers */
-   for (i = 0; i < length; i++)
-   data[i] = le32_to_cpu(buf[i]);
+   /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+   memset(buf, 0, bufsize);
+   }
 
-   kfree(buf);
+   kfree(dmabuf);
 
-   if (result != size) {
-   dev_dbg(>dev, "%s - Unable to send config request, 
request=0x%x size=%d result=%d\n",
-   __func__, request, size, result);
-   if (result > 0)
-   result = -EPROTO;
+   return result;
+}
+
+/*
+ * Reads any 32-bit CP210X_ register identified by req.
+ */
+static int cp210x_read_u32_reg(struct usb_serial_port *port, u8 req, u32 *val)
+{
+   __le32 le32_val;
+   int err;
 
-   return result;
+   err = cp210x_read_reg_block(port, req, _val, sizeof(le32_val));
+   if (err) {
+   /*
+* FIXME Some callers don't bother to check for error,
+* at least give them consistent junk until they are fixed
+*/
+   *val = 0;
+   return err;
}
 
+   *val = le32_to_cpu(le32_val);
+
return 0;
 }
 
 /*
- * cp210x_set_config
- * Writes to the CP210x configuration registers
- * Values less than 16 bits wide are sent directly
- * 'size' is specified in bytes.
+ * Reads any 16-bit CP210X_ register identified by req.
  */
-static int cp210x_set_config(struct usb_serial_port *port, u8 request,
-   unsigned int *data, int size)
+static int cp210x_read_u16_reg(struct usb_serial_port *port, u8 req, u16 *val)
+{
+   __le16 le16_val;
+   int err;
+
+   err = cp210x_read_reg_block(port, req,