On 02.06.2022 08:30:30, Jimmy Assarsson wrote:
> The firmware of M32C based Leaf devices expects bittiming parameters
> calculated for 16MHz clock.
> Since we use the actual clock frequency of the device, the device may end
> up with wrong bittiming parameters, depending on user requested parameters.
> 
> This regression affects M32C based Leaf devices with non-16MHz clock.

Oh. Thanks for the patch!

> Fixes: fb12797ab1fe ("can: kvaser_usb: get CAN clock frequency from device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  4 +++
>  .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 20 +++++++++++----
>  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 25 ++++++++++++-------
>  3 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> index 3a49257f9fa6..cb588228d7a1 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> @@ -44,6 +44,9 @@
>  #define KVASER_USB_CAP_EXT_CAP			0x02
>  #define KVASER_USB_HYDRA_CAP_EXT_CMD		0x04
>  
> +/* Quriks */
      ^^^^^^
Typo

> +#define KVASER_USB_QUIRK_IGNORE_CLK_FREQ BIT(0)
> +
>  struct kvaser_usb_dev_cfg;
>  
>  enum kvaser_usb_leaf_family {
> @@ -65,6 +68,7 @@ struct kvaser_usb_dev_card_data_hydra {
>  struct kvaser_usb_dev_card_data {
>  	u32 ctrlmode_supported;
>  	u32 capabilities;
> +	u32 quirks;
>  	union {
>  		struct {
>  			enum kvaser_usb_leaf_family family;
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index e67658b53d02..5880e9719c9d 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -94,10 +94,14 @@
>  
>  static inline bool kvaser_is_leaf(const struct usb_device_id *id)
>  {
> -	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> -		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
> -		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> -		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
> +	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> +	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
> +}
> +
> +static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
> +{
> +	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> +	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
>  }

Is this getting a bit complicated now?
In this driver we have:

1) struct usb_device_id::driver_info
2) kvaser_is_*()

which is used to set

3) dev->card_data.leaf.family
4) dev->ops

and now you're adding:

5) dev->card_data.quirks

which then affects

6) dev->cfg

The straight forward way would be to define a struct that describes the
a device completely:

struct kvaser_driver_info {
       u32 quirks;        /* KVASER_USB_HAS_ */
       enum kvaser_usb_leaf_family;
       const struct kvaser_usb_dev_*ops;
       const struct kvaser_usb_dev_*cfg;
};

and then assign that to every device listed in the kvaser_usb_table.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |