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:
signature.asc
Description: PGP signature
