| Message ID | 20201124094155.10510-1-gciofono@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | option: added support for Thales Cinterion EXS82 option port. | expand |
On Tue, Nov 24, 2020 at 10:41:55AM +0100, Giacinto Cifelli wrote: > There is a single option port in this modem, and it is used as debug port > > Signed-off-by: Giacinto Cifelli <gciofono@gmail.com> Thanks for the update (and thanks for the review, Lars). Using the option driver for just a debug port seems like overkill, but ok. Some form issues: - When updating a patch always include a patch revision number in the Subject prefix (e.g. "[PATCH v2] USB: serial: option: add ..."). - Include a short changelog below the "---" line so we know what changed since the previous version. - Try to follow the convention used by the subsystem for the Subject prefix (i.e. "USB: serial: option: add ..."). > --- And please keep the lsusb -v info here below the cut-off line when resending. > drivers/usb/serial/option.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c > index 54ca85cc920d..fda291e68e4b 100644 > --- a/drivers/usb/serial/option.c > +++ b/drivers/usb/serial/option.c > @@ -424,6 +424,7 @@ static void option_instat_callback(struct urb *urb); > #define CINTERION_PRODUCT_AHXX_2RMNET 0x0084 > #define CINTERION_PRODUCT_AHXX_AUDIO 0x0085 > #define CINTERION_PRODUCT_CLS8 0x00b0 > +#define CINTERION_PRODUCT_EXS82 0x006c Please keep the entries sorted by PID. > > /* Olivetti products */ > #define OLIVETTI_VENDOR_ID 0x0b3c > @@ -1908,6 +1909,7 @@ static const struct usb_device_id option_ids[] = { > { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC25_MDMNET) }, > { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC28_MDM) }, /* HC28 enumerates with Siemens or Cinterion VID depending on FW revision */ > { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC28_MDMNET) }, > + { USB_DEVICE_INTERFACE_CLASS(CINTERION_VENDOR_ID, CINTERION_PRODUCT_EXS82, 0xff) }, And keep these sorted alphabetically if possible (at least keep the CINTERION_VENDOR_ID entries together). > { USB_DEVICE(OLIVETTI_VENDOR_ID, OLIVETTI_PRODUCT_OLICARD100), > .driver_info = RSVD(4) }, > { USB_DEVICE(OLIVETTI_VENDOR_ID, OLIVETTI_PRODUCT_OLICARD120), Care to fix that up in a v3? Johan
Hi Johan, On Wed, Nov 25, 2020 at 10:40 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Nov 24, 2020 at 10:41:55AM +0100, Giacinto Cifelli wrote: > > There is a single option port in this modem, and it is used as debug port > > > > Signed-off-by: Giacinto Cifelli <gciofono@gmail.com> > > Thanks for the update (and thanks for the review, Lars). > > Using the option driver for just a debug port seems like overkill, but > ok. > > Some form issues: > > - When updating a patch always include a patch revision number in the > Subject prefix (e.g. "[PATCH v2] USB: serial: option: add ..."). > technically this is a new patch, as I have changed the name, but I was reserving the v2 for the actual MV31 patch that I will send in the near future. > - Include a short changelog below the "---" line so we know what > changed since the previous version. ok > > - Try to follow the convention used by the subsystem for the Subject > prefix (i.e. "USB: serial: option: add ..."). ok. Should I re-submit this patch? Do you prefer v2 or v3 for a new submit? thank you, Giacinto
On Wed, Nov 25, 2020 at 11:00:26AM +0100, Giacinto Cifelli wrote: > Hi Johan, > > On Wed, Nov 25, 2020 at 10:40 AM Johan Hovold <johan@kernel.org> wrote: > > > > On Tue, Nov 24, 2020 at 10:41:55AM +0100, Giacinto Cifelli wrote: > > > There is a single option port in this modem, and it is used as debug port > > > > > > Signed-off-by: Giacinto Cifelli <gciofono@gmail.com> > > > > Thanks for the update (and thanks for the review, Lars). > > > > Using the option driver for just a debug port seems like overkill, but > > ok. > > > > Some form issues: > > > > - When updating a patch always include a patch revision number in the > > Subject prefix (e.g. "[PATCH v2] USB: serial: option: add ..."). > > > > technically this is a new patch, as I have changed the name, but I was > reserving the v2 for the actual MV31 patch that I will send in the > near future. Since it's the same PID you're adding I'd still consider this a v2 even if you changed the product name, but I see your point. > > - Try to follow the convention used by the subsystem for the Subject > > prefix (i.e. "USB: serial: option: add ..."). > > ok. Should I re-submit this patch? Do you prefer v2 or v3 for a new submit? Yes, please do send a v3. Johan
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c index 54ca85cc920d..fda291e68e4b 100644 --- a/drivers/usb/serial/option.c +++ b/drivers/usb/serial/option.c @@ -424,6 +424,7 @@ static void option_instat_callback(struct urb *urb); #define CINTERION_PRODUCT_AHXX_2RMNET 0x0084 #define CINTERION_PRODUCT_AHXX_AUDIO 0x0085 #define CINTERION_PRODUCT_CLS8 0x00b0 +#define CINTERION_PRODUCT_EXS82 0x006c /* Olivetti products */ #define OLIVETTI_VENDOR_ID 0x0b3c @@ -1908,6 +1909,7 @@ static const struct usb_device_id option_ids[] = { { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC25_MDMNET) }, { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC28_MDM) }, /* HC28 enumerates with Siemens or Cinterion VID depending on FW revision */ { USB_DEVICE(SIEMENS_VENDOR_ID, CINTERION_PRODUCT_HC28_MDMNET) }, + { USB_DEVICE_INTERFACE_CLASS(CINTERION_VENDOR_ID, CINTERION_PRODUCT_EXS82, 0xff) }, { USB_DEVICE(OLIVETTI_VENDOR_ID, OLIVETTI_PRODUCT_OLICARD100), .driver_info = RSVD(4) }, { USB_DEVICE(OLIVETTI_VENDOR_ID, OLIVETTI_PRODUCT_OLICARD120),
There is a single option port in this modem, and it is used as debug port Signed-off-by: Giacinto Cifelli <gciofono@gmail.com> --- drivers/usb/serial/option.c | 2 ++ 1 file changed, 2 insertions(+)