| Message ID | 20220722085040.704885-1-sdlyyxy@bupt.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] USB: serial: usb_wwan: replace DTR/RTS magic numbers with macros | expand |
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
On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote: > 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? I think Yan should write a patch series to unify these and make it right, instead of just papering over it all. Also this "../" stuff in a #include directive is not ok, I wouldn't recommend this change be taken as-is. thanks, greg k-h
On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote: > On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote: > > 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? > > I think Yan should write a patch series to unify these and make it > right, instead of just papering over it all. Ok, I just fear it will be more work for us since that involves decisions like whether it should be added to the uapi header, and then we get into naming, etc. But we're in no rush. > Also this "../" stuff in a > #include directive is not ok, I wouldn't recommend this change be taken > as-is. That was never an option, but I'd be ok with taking the v2 which added defines for the constants directly in the driver. Johan
On Sun, Jul 24, 2022 at 04:07:17PM +0200, Johan Hovold wrote: > On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote: > > On Sun, Jul 24, 2022 at 11:10:22AM +0200, Johan Hovold wrote: > > > 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? > > > > I think Yan should write a patch series to unify these and make it > > right, instead of just papering over it all. > > Ok, I just fear it will be more work for us since that involves > decisions like whether it should be added to the uapi header, and then > we get into naming, etc. But we're in no rush. > > > Also this "../" stuff in a > > #include directive is not ok, I wouldn't recommend this change be taken > > as-is. > > That was never an option, but I'd be ok with taking the v2 which added > defines for the constants directly in the driver. These are global defines, in a public spec, and they should all just be in one place in the kernel. What's wrong with include/uapi/linux/cdc.h which is where the other ACM defined values are at? thanks, greg k-h
On Sun, Jul 24, 2022 at 04:15:55PM +0200, Greg Kroah-Hartman wrote: > On Sun, Jul 24, 2022 at 04:07:17PM +0200, Johan Hovold wrote: > > On Sun, Jul 24, 2022 at 03:50:52PM +0200, Greg Kroah-Hartman wrote: > > > I think Yan should write a patch series to unify these and make it > > > right, instead of just papering over it all. > > > > Ok, I just fear it will be more work for us since that involves > > decisions like whether it should be added to the uapi header, and then > > we get into naming, etc. But we're in no rush. > > > > > Also this "../" stuff in a > > > #include directive is not ok, I wouldn't recommend this change be taken > > > as-is. > > > > That was never an option, but I'd be ok with taking the v2 which added > > defines for the constants directly in the driver. > > These are global defines, in a public spec, and they should all just be > in one place in the kernel. What's wrong with include/uapi/linux/cdc.h > which is where the other ACM defined values are at? Nothing. We'd just need to figure out how best to name them if they're going to become UAPI, that's all (e.g. at least use a USB_CDC_ prefix to match the other defines). And the in-tree users would need to be updated to match. And it's not just the control lines. We have the state notification bits as well. Someone would need to dig out the spec. So we go from accepting a small clean-up patch to some non-trivial tree-wide work and review. But sure, bring it on. Johan
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" #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);
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(-)