Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 24 Jul 2022 11:10:22 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Yan Xinyu <sdlyyxy@bupt.edu.cn>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] USB: serial: usb_wwan: replace DTR/RTS magic numbers
 with macros
Message-ID: <Yt0MfqQQTwe4ztuN@hovoldconsulting.com>
References: <20220722085040.704885-1-sdlyyxy@bupt.edu.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220722085040.704885-1-sdlyyxy@bupt.edu.cn>
Precedence: bulk

On Fri, Jul 22, 2022 at 04:50:40PM +0800, Yan Xinyu wrote:
> The usb_wwan_send_setup function generates DTR/RTS signals in compliance
> with CDC ACM standard. This patch changes magic numbers in this function
> to equivalent macros.
> 
> Signed-off-by: Yan Xinyu <sdlyyxy@bupt.edu.cn>
> ---
> v1->v2:
>  * Fix Signed-off-by name.
> v2->v3:
>  * Use already defined ACM_CTRL_DTR and ACM_CTRL_RTS in drivers/usb/class/cdc-acm.h
> ---
>  drivers/usb/serial/usb_wwan.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index dab38b63eaf7..5c8303bd3676 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -29,8 +29,10 @@
>  #include <linux/bitops.h>
>  #include <linux/uaccess.h>
>  #include <linux/usb.h>
> +#include <linux/usb/cdc.h>
>  #include <linux/usb/serial.h>
>  #include <linux/serial.h>
> +#include "../class/cdc-acm.h"

If we are to use common defines, these would need to be added to
linux/usb/cdc.h first (parts of which are exposed to user space).

Note that we already have at least three copies of these defines in the
tree.

I'm fine with adding another copy for now and not have to deal with with
naming and cross driver updates. What do you think, Greg?

>  #include "usb-wwan.h"
>  
>  /*
> @@ -48,9 +50,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
>  	portdata = usb_get_serial_port_data(port);
>  
>  	if (portdata->dtr_state)
> -		val |= 0x01;
> +		val |= ACM_CTRL_DTR;
>  	if (portdata->rts_state)
> -		val |= 0x02;
> +		val |= ACM_CTRL_RTS;
>  
>  	ifnum = serial->interface->cur_altsetting->desc.bInterfaceNumber;
>  
> @@ -59,8 +61,9 @@ static int usb_wwan_send_setup(struct usb_serial_port *port)
>  		return res;
>  
>  	res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> -				0x22, 0x21, val, ifnum, NULL, 0,
> -				USB_CTRL_SET_TIMEOUT);
> +				USB_CDC_REQ_SET_CONTROL_LINE_STATE,
> +				USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +				val, ifnum, NULL, 0, USB_CTRL_SET_TIMEOUT);
>  
>  	usb_autopm_put_interface(port->serial->interface);

Looks good otherwise.

Johan