diff mbox series

[01/15] usb: serial: ark3116: use usb_control_msg_recv() and usb_control_msg_send()

Message ID 20201104064703.15123-2-himadrispandya@gmail.com
State New
Headers show
Series usb: serial: avoid using usb_control_msg() directly | expand

Commit Message

Himadri Pandya Nov. 4, 2020, 6:46 a.m. UTC
The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
usb_control_msg() with proper error check. Hence use the wrappers
instead of calling usb_control_msg() directly.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
---
 drivers/usb/serial/ark3116.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

Comments

Johan Hovold Dec. 4, 2020, 9:16 a.m. UTC | #1
On Wed, Nov 04, 2020 at 12:16:49PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps

> usb_control_msg() with proper error check. Hence use the wrappers

> instead of calling usb_control_msg() directly.

> 

> Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>

> ---

>  drivers/usb/serial/ark3116.c | 29 ++++-------------------------

>  1 file changed, 4 insertions(+), 25 deletions(-)

> 

> diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c

> index 71a9206ea1e2..51302892c779 100644

> --- a/drivers/usb/serial/ark3116.c

> +++ b/drivers/usb/serial/ark3116.c

> @@ -77,38 +77,17 @@ struct ark3116_private {

>  static int ark3116_write_reg(struct usb_serial *serial,

>  			     unsigned reg, __u8 val)

>  {

> -	int result;

>  	 /* 0xfe 0x40 are magic values taken from original driver */

> -	result = usb_control_msg(serial->dev,

> -				 usb_sndctrlpipe(serial->dev, 0),

> -				 0xfe, 0x40, val, reg,

> -				 NULL, 0, ARK_TIMEOUT);

> -	if (result)

> -		return result;

> -

> -	return 0;

> +	return usb_control_msg_send(serial->dev, 0, 0xfe, 0x40, val, reg, NULL, 0,

> +				    ARK_TIMEOUT, GFP_KERNEL);


For control transfers without a data stage there's no point in using
usb_control_msg_send() as it already returns a negative errno on error
or 0 on success.

>  }

>  

>  static int ark3116_read_reg(struct usb_serial *serial,

>  			    unsigned reg, unsigned char *buf)

>  {

> -	int result;

>  	/* 0xfe 0xc0 are magic values taken from original driver */

> -	result = usb_control_msg(serial->dev,

> -				 usb_rcvctrlpipe(serial->dev, 0),

> -				 0xfe, 0xc0, 0, reg,

> -				 buf, 1, ARK_TIMEOUT);

> -	if (result < 1) {

> -		dev_err(&serial->interface->dev,

> -				"failed to read register %u: %d\n",

> -				reg, result);

> -		if (result >= 0)

> -			result = -EIO;

> -

> -		return result;

> -	}

> -

> -	return 0;

> +	return usb_control_msg_recv(serial->dev, 0, 0xfe, 0xc0, 0, reg, buf, 1,

> +				    ARK_TIMEOUT, GFP_KERNEL);


This driver already use a DMA-able transfer buffer which is allocated
once and then passed to this helper repeatedly. This change would
introduce additional redandant memdup + memcpy for every call and for no
real gain.

You also have an unrelated change here as you simply remove an existing
error message.

Please drop this patch.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
index 71a9206ea1e2..51302892c779 100644
--- a/drivers/usb/serial/ark3116.c
+++ b/drivers/usb/serial/ark3116.c
@@ -77,38 +77,17 @@  struct ark3116_private {
 static int ark3116_write_reg(struct usb_serial *serial,
 			     unsigned reg, __u8 val)
 {
-	int result;
 	 /* 0xfe 0x40 are magic values taken from original driver */
-	result = usb_control_msg(serial->dev,
-				 usb_sndctrlpipe(serial->dev, 0),
-				 0xfe, 0x40, val, reg,
-				 NULL, 0, ARK_TIMEOUT);
-	if (result)
-		return result;
-
-	return 0;
+	return usb_control_msg_send(serial->dev, 0, 0xfe, 0x40, val, reg, NULL, 0,
+				    ARK_TIMEOUT, GFP_KERNEL);
 }
 
 static int ark3116_read_reg(struct usb_serial *serial,
 			    unsigned reg, unsigned char *buf)
 {
-	int result;
 	/* 0xfe 0xc0 are magic values taken from original driver */
-	result = usb_control_msg(serial->dev,
-				 usb_rcvctrlpipe(serial->dev, 0),
-				 0xfe, 0xc0, 0, reg,
-				 buf, 1, ARK_TIMEOUT);
-	if (result < 1) {
-		dev_err(&serial->interface->dev,
-				"failed to read register %u: %d\n",
-				reg, result);
-		if (result >= 0)
-			result = -EIO;
-
-		return result;
-	}
-
-	return 0;
+	return usb_control_msg_recv(serial->dev, 0, 0xfe, 0xc0, 0, reg, buf, 1,
+				    ARK_TIMEOUT, GFP_KERNEL);
 }
 
 static inline int calc_divisor(int bps)