Re: [PATCH] option: added support for Thales Cinterion EXS82 option port.

[Johan Hovold <johan@kernel.org>]

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