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 16:07:17 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Yan Xinyu <sdlyyxy@bupt.edu.cn>, 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: <Yt1SFXlwbW1JCohE@hovoldconsulting.com>
References: <20220722085040.704885-1-sdlyyxy@bupt.edu.cn>
 <Yt0MfqQQTwe4ztuN@hovoldconsulting.com>
 <Yt1OPKNlWZuMrZv4@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yt1OPKNlWZuMrZv4@kroah.com>
Precedence: bulk

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