On Mon, 2015-10-19 at 17:01 -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.
Hi,
I am afraid there are some issues.
>
> 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;
1. If you already get passed a pointer to a buffer, why use another
buffer?
2. You are doing DMA on the stack. That is forbidden.
> + 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);
> + 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);
It would make more sense inverting the debug statement and the error
conversion, as you wouldn't lose information.
> + return result;
> + }
> + *pval = le16_to_cpu(le16_value);
> + return 0;
> +}
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
