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 <[email protected]> > --- > 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-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
