RE: [PATCH v2 3/3] can: gs_usb: add switchable termination support

["Trevitz, Daniel" <Daniel.Trevitz@wika.com>]

Looks good!

Once my hardware comes in with the termination resistor built in I'll run this latest version.
I've been doing all my tests on an earlier version without it, just monitoring the gpio state.
It probably won't be for a few weeks till they get in.

BR,
  Daniel





**CONFIDENTIALITY NOTICE**


This communication, including any attachments, is from WIKA Mobile Control, LP and contains confidential information intended only for the addressee(s). If you are not the intended recipient, any use, dissemination, distribution or copying of this document or its contents is strictly prohibited. If you have received this communication in error, please contact the sender by reply e-mail immediately and destroy all copies of the original message. 


-----Original Message-----
From: Marc Kleine-Budde <mkl@pengutronix.de> 
Sent: Monday, September 19, 2022 3:34 PM
To: Trevitz, Daniel <Daniel.Trevitz@wika.com>
Cc: linux-can@vger.kernel.org; Ryan Edwards <ryan.edwards@gmail.com>
Subject: Re: [PATCH v2 3/3] can: gs_usb: add switchable termination support

On 19.09.2022 17:14:39, Trevitz, Daniel wrote:
> +       if (feature & GS_CAN_FEATURE_TERMINATION) {
> +               dev->can.termination_const = gs_usb_termination_const;
> +               dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
> +               dev->can.do_set_termination = gs_usb_set_termination;
> +
> +               rc = gs_usb_get_termination(netdev, &dev->can.termination);
> +               if (rc) {
> +                       dev_err(&intf->dev,
> +                               "Couldn't get current termination state for channel %d (%pe)\n",
> +                               channel, ERR_PTR(rc));
> +                       goto out_free_candev;
> +               }
> +       }
> 
> Does it make sense to check if we have the termination support, then 
> set the values? My logic is that just because the termination is not 
> working correctly, it does not mean everything is broken.

Makes sense!

> This way you could have a multi-can-channel USB device but with only 
> specific channels supporting configurable termination resistors.

At least from the Linux driver's perspective the feature bits are per channel not per device.

> Something like:
> 
> rc = gs_usb_get_termination(netdev, &dev->can.termination);  if (rc) {
> 	dev_err(&intf->dev,
> 		"Couldn't get current termination state for channel %d (%pe). Not enabling termination support for this channel\n",
> 		channel, ERR_PTR(rc));
>  } else {
> 	dev->can.termination_const = gs_usb_termination_const;
> 	dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
> 	dev->can.do_set_termination = gs_usb_set_termination; }

I've reduced the dev_err() to dev_info() and tried to keep the error message short. Also I remove the termination feature flag. The incremental patch looks like this:

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index 5c988e528734..b0273fab1843 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1287,16 +1287,17 @@ static struct gs_can *gs_make_candev(unsigned int channel,
        }
 
        if (feature & GS_CAN_FEATURE_TERMINATION) {
-               dev->can.termination_const = gs_usb_termination_const;
-               dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
-               dev->can.do_set_termination = gs_usb_set_termination;
-
                rc = gs_usb_get_termination(netdev, &dev->can.termination);
                if (rc) {
-                       dev_err(&intf->dev,
-                               "Couldn't get current termination state for channel %d (%pe)\n",
-                               channel, ERR_PTR(rc));
-                       goto out_free_candev;
+                       dev->feature &= ~GS_CAN_FEATURE_TERMINATION;
+
+                       dev_info(&intf->dev,
+                                "Disabling termination support for channel %d (%pe)\n",
+                                channel, ERR_PTR(rc));
+               } else {
+                       dev->can.termination_const = gs_usb_termination_const;
+                       dev->can.termination_const_cnt = ARRAY_SIZE(gs_usb_termination_const);
+                       dev->can.do_set_termination = 
+ gs_usb_set_termination;
                }
        }

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | BLOCKEDpengutronix[.]deBLOCKED  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |