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

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

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

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                   | 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 --]