Re: [PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

On 02.06.2022 11:22:31, Jimmy Assarsson wrote:
> > > 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.
> 
> Thanks for the feedback!
> I agree, but I prefer if we can keep assigning dev->cfg based on the
> information that we get from the device.

Ok, if you cannot tell from the USB product ID.

> So we get:
> struct kvaser_driver_info {
>         u32 quirks;        /* KVASER_USB_HAS_ */

That holds the existing quirks and the new one.

>         enum kvaser_usb_leaf_family;
>         const struct kvaser_usb_dev_*ops;
> };
> And quirks and family still affect dev->cfg.

...as is depends on sw_options read from the device?

regards,
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]