[PATCH v2 0/3] can: gs_usb: cleanups and termination support

[PATCH v2 0/3] can: gs_usb: cleanups and termination support

[Marc Kleine-Budde]

Hello,

this series first cleans up the driver a bit and then adds switchable
termination support.

regards,
Marc

changes since v1 https://lore.kernel.org/all/20220918202348.675850-1-mkl@pengutronix.de:
- add 1/3: "can: gs_usb: gs_make_candev(): convert from usb_control_msg() to usb_control_msg_recv()"
  to make error handling easier
- add 2/3: "can: gs_usb: gs_make_candev(): clean up error handling"
  which introduces a cleanup label
- move 1/3 to 3/3:
  - fix gs_usb_get_termination(), set term value
  - remove check of termination value from gs_usb_set_termination()
  - check for initial termination in gs_make_candev()

[PATCH v2 1/3] can: gs_usb: gs_make_candev(): convert from usb_control_msg() to usb_control_msg_recv()

[Marc Kleine-Budde]

Convert the gs_make_candev() function to use usb_control_msg_recv()
instead of usb_control_msg(). Which allows the received data to be
placed on the stack.

This makes error handling a lot easier as we don't have to deal with
freeing the allocated memory.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 82 +++++++++++++++---------------------
 1 file changed, 33 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index cc363f1935ce..16e56394ef9b 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1124,26 +1124,21 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	struct gs_can *dev;
 	struct net_device *netdev;
 	int rc;
-	struct gs_device_bt_const *bt_const;
-	struct gs_device_bt_const_extended *bt_const_extended;
+	struct gs_device_bt_const_extended bt_const_extended;
+	struct gs_device_bt_const bt_const;
 	u32 feature;
 
-	bt_const = kmalloc(sizeof(*bt_const), GFP_KERNEL);
-	if (!bt_const)
-		return ERR_PTR(-ENOMEM);
-
 	/* fetch bit timing constants */
-	rc = usb_control_msg(interface_to_usbdev(intf),
-			     usb_rcvctrlpipe(interface_to_usbdev(intf), 0),
-			     GS_USB_BREQ_BT_CONST,
-			     USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
-			     channel, 0, bt_const, sizeof(*bt_const), 1000);
+	rc = usb_control_msg_recv(interface_to_usbdev(intf), 0,
+				  GS_USB_BREQ_BT_CONST,
+				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				  channel, 0, &bt_const, sizeof(bt_const), 1000,
+				  GFP_KERNEL);
 
-	if (rc < 0) {
+	if (rc) {
 		dev_err(&intf->dev,
 			"Couldn't get bit timing const for channel (err=%d)\n",
 			rc);
-		kfree(bt_const);
 		return ERR_PTR(rc);
 	}
 
@@ -1151,7 +1146,6 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 	netdev = alloc_candev(sizeof(struct gs_can), GS_MAX_TX_URBS);
 	if (!netdev) {
 		dev_err(&intf->dev, "Couldn't allocate candev\n");
-		kfree(bt_const);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -1164,14 +1158,14 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	/* dev setup */
 	strcpy(dev->bt_const.name, KBUILD_MODNAME);
-	dev->bt_const.tseg1_min = le32_to_cpu(bt_const->tseg1_min);
-	dev->bt_const.tseg1_max = le32_to_cpu(bt_const->tseg1_max);
-	dev->bt_const.tseg2_min = le32_to_cpu(bt_const->tseg2_min);
-	dev->bt_const.tseg2_max = le32_to_cpu(bt_const->tseg2_max);
-	dev->bt_const.sjw_max = le32_to_cpu(bt_const->sjw_max);
-	dev->bt_const.brp_min = le32_to_cpu(bt_const->brp_min);
-	dev->bt_const.brp_max = le32_to_cpu(bt_const->brp_max);
-	dev->bt_const.brp_inc = le32_to_cpu(bt_const->brp_inc);
+	dev->bt_const.tseg1_min = le32_to_cpu(bt_const.tseg1_min);
+	dev->bt_const.tseg1_max = le32_to_cpu(bt_const.tseg1_max);
+	dev->bt_const.tseg2_min = le32_to_cpu(bt_const.tseg2_min);
+	dev->bt_const.tseg2_max = le32_to_cpu(bt_const.tseg2_max);
+	dev->bt_const.sjw_max = le32_to_cpu(bt_const.sjw_max);
+	dev->bt_const.brp_min = le32_to_cpu(bt_const.brp_min);
+	dev->bt_const.brp_max = le32_to_cpu(bt_const.brp_max);
+	dev->bt_const.brp_inc = le32_to_cpu(bt_const.brp_inc);
 
 	dev->udev = interface_to_usbdev(intf);
 	dev->iface = intf;
@@ -1188,13 +1182,13 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	/* can setup */
 	dev->can.state = CAN_STATE_STOPPED;
-	dev->can.clock.freq = le32_to_cpu(bt_const->fclk_can);
+	dev->can.clock.freq = le32_to_cpu(bt_const.fclk_can);
 	dev->can.bittiming_const = &dev->bt_const;
 	dev->can.do_set_bittiming = gs_usb_set_bittiming;
 
 	dev->can.ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC;
 
-	feature = le32_to_cpu(bt_const->feature);
+	feature = le32_to_cpu(bt_const.feature);
 	dev->feature = FIELD_GET(GS_CAN_FEATURE_MASK, feature);
 	if (feature & GS_CAN_FEATURE_LISTEN_ONLY)
 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
@@ -1244,45 +1238,35 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		if (feature & GS_CAN_FEATURE_IDENTIFY)
 			netdev->ethtool_ops = &gs_usb_ethtool_ops;
 
-	kfree(bt_const);
-
 	/* fetch extended bit timing constants if device has feature
 	 * GS_CAN_FEATURE_FD and GS_CAN_FEATURE_BT_CONST_EXT
 	 */
 	if (feature & GS_CAN_FEATURE_FD &&
 	    feature & GS_CAN_FEATURE_BT_CONST_EXT) {
-		bt_const_extended = kmalloc(sizeof(*bt_const_extended), GFP_KERNEL);
-		if (!bt_const_extended)
-			return ERR_PTR(-ENOMEM);
-
-		rc = usb_control_msg(interface_to_usbdev(intf),
-				     usb_rcvctrlpipe(interface_to_usbdev(intf), 0),
-				     GS_USB_BREQ_BT_CONST_EXT,
-				     USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
-				     channel, 0, bt_const_extended,
-				     sizeof(*bt_const_extended),
-				     1000);
-		if (rc < 0) {
+		rc = usb_control_msg_recv(interface_to_usbdev(intf), 0,
+					  GS_USB_BREQ_BT_CONST_EXT,
+					  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+					  channel, 0, &bt_const_extended,
+					  sizeof(bt_const_extended),
+					  1000, GFP_KERNEL);
+		if (rc) {
 			dev_err(&intf->dev,
 				"Couldn't get extended bit timing const for channel (err=%d)\n",
 				rc);
-			kfree(bt_const_extended);
 			return ERR_PTR(rc);
 		}
 
 		strcpy(dev->data_bt_const.name, KBUILD_MODNAME);
-		dev->data_bt_const.tseg1_min = le32_to_cpu(bt_const_extended->dtseg1_min);
-		dev->data_bt_const.tseg1_max = le32_to_cpu(bt_const_extended->dtseg1_max);
-		dev->data_bt_const.tseg2_min = le32_to_cpu(bt_const_extended->dtseg2_min);
-		dev->data_bt_const.tseg2_max = le32_to_cpu(bt_const_extended->dtseg2_max);
-		dev->data_bt_const.sjw_max = le32_to_cpu(bt_const_extended->dsjw_max);
-		dev->data_bt_const.brp_min = le32_to_cpu(bt_const_extended->dbrp_min);
-		dev->data_bt_const.brp_max = le32_to_cpu(bt_const_extended->dbrp_max);
-		dev->data_bt_const.brp_inc = le32_to_cpu(bt_const_extended->dbrp_inc);
+		dev->data_bt_const.tseg1_min = le32_to_cpu(bt_const_extended.dtseg1_min);
+		dev->data_bt_const.tseg1_max = le32_to_cpu(bt_const_extended.dtseg1_max);
+		dev->data_bt_const.tseg2_min = le32_to_cpu(bt_const_extended.dtseg2_min);
+		dev->data_bt_const.tseg2_max = le32_to_cpu(bt_const_extended.dtseg2_max);
+		dev->data_bt_const.sjw_max = le32_to_cpu(bt_const_extended.dsjw_max);
+		dev->data_bt_const.brp_min = le32_to_cpu(bt_const_extended.dbrp_min);
+		dev->data_bt_const.brp_max = le32_to_cpu(bt_const_extended.dbrp_max);
+		dev->data_bt_const.brp_inc = le32_to_cpu(bt_const_extended.dbrp_inc);
 
 		dev->can.data_bittiming_const = &dev->data_bt_const;
-
-		kfree(bt_const_extended);
 	}
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
-- 
2.35.1

[PATCH v2 2/3] can: gs_usb: gs_make_candev(): clean up error handling

[Marc Kleine-Budde]

Introduce a label to free the allocated candev in case of a error and
make use of if. Fix a memory leak if the extended bit timing cannot be
read. Extend the error messages to print the number of the failing
channel and the symbolic error name.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 16e56394ef9b..bfa061687d13 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -1137,8 +1137,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	if (rc) {
 		dev_err(&intf->dev,
-			"Couldn't get bit timing const for channel (err=%d)\n",
-			rc);
+			"Couldn't get bit timing const for channel %d (%pe)\n",
+			channel, ERR_PTR(rc));
 		return ERR_PTR(rc);
 	}
 
@@ -1251,9 +1251,9 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 					  1000, GFP_KERNEL);
 		if (rc) {
 			dev_err(&intf->dev,
-				"Couldn't get extended bit timing const for channel (err=%d)\n",
-				rc);
-			return ERR_PTR(rc);
+				"Couldn't get extended bit timing const for channel %d (%pe)\n",
+				channel, ERR_PTR(rc));
+			goto out_free_candev;
 		}
 
 		strcpy(dev->data_bt_const.name, KBUILD_MODNAME);
@@ -1273,12 +1273,17 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 
 	rc = register_candev(dev->netdev);
 	if (rc) {
-		free_candev(dev->netdev);
-		dev_err(&intf->dev, "Couldn't register candev (err=%d)\n", rc);
-		return ERR_PTR(rc);
+		dev_err(&intf->dev,
+			"Couldn't register candev for channel %d (%pe)\n",
+			channel, ERR_PTR(rc));
+		goto out_free_candev;
 	}
 
 	return dev;
+
+ out_free_candev:
+	free_candev(dev->netdev);
+	return ERR_PTR(rc);
 }
 
 static void gs_destroy_candev(struct gs_can *dev)
-- 
2.35.1

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

[Marc Kleine-Budde]

The candleLight community is working on switchable termination support
for the candleLight firmware. As the the Linux CAN framework supports
switchable termination add this feature to the gs_usb driver.

Devices supporting the feature should set the
GS_CAN_FEATURE_TERMINATION and implement the
GS_USB_BREQ_SET_TERMINATION and GS_USB_BREQ_GET_TERMINATION control
messages.

For now the driver assumes for activated termination the standard
termination value of 120Ω.

Link: https://github.com/candle-usb/candleLight_fw/issues/92
Link: https://github.com/candle-usb/candleLight_fw/pull/108
Cc: Daniel Trevitz <daniel.trevitz@wika.com>
Cc: Ryan Edwards <ryan.edwards@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 78 +++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index bfa061687d13..d0c491d7319a 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -64,6 +64,8 @@ enum gs_usb_breq {
 	GS_USB_BREQ_SET_USER_ID,
 	GS_USB_BREQ_DATA_BITTIMING,
 	GS_USB_BREQ_BT_CONST_EXT,
+	GS_USB_BREQ_SET_TERMINATION,
+	GS_USB_BREQ_GET_TERMINATION,
 };
 
 enum gs_can_mode {
@@ -87,6 +89,14 @@ enum gs_can_identify_mode {
 	GS_CAN_IDENTIFY_ON
 };
 
+enum gs_can_termination_state {
+	GS_CAN_TERMINATION_STATE_OFF = 0,
+	GS_CAN_TERMINATION_STATE_ON
+};
+
+#define GS_USB_TERMINATION_DISABLED CAN_TERMINATION_DISABLED
+#define GS_USB_TERMINATION_ENABLED 120
+
 /* data types passed between host and device */
 
 /* The firmware on the original USB2CAN by Geschwister Schneider
@@ -123,6 +133,7 @@ struct gs_device_config {
 #define GS_CAN_MODE_FD BIT(8)
 /* GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9) */
 /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
+/* GS_CAN_FEATURE_TERMINATION BIT(11) */
 
 struct gs_device_mode {
 	__le32 mode;
@@ -147,6 +158,10 @@ struct gs_identify_mode {
 	__le32 mode;
 } __packed;
 
+struct gs_termination_state {
+	__le32 state;
+} __packed;
+
 #define GS_CAN_FEATURE_LISTEN_ONLY BIT(0)
 #define GS_CAN_FEATURE_LOOP_BACK BIT(1)
 #define GS_CAN_FEATURE_TRIPLE_SAMPLE BIT(2)
@@ -158,7 +173,8 @@ struct gs_identify_mode {
 #define GS_CAN_FEATURE_FD BIT(8)
 #define GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX BIT(9)
 #define GS_CAN_FEATURE_BT_CONST_EXT BIT(10)
-#define GS_CAN_FEATURE_MASK GENMASK(10, 0)
+#define GS_CAN_FEATURE_TERMINATION BIT(11)
+#define GS_CAN_FEATURE_MASK GENMASK(11, 0)
 
 /* internal quirks - keep in GS_CAN_FEATURE space for now */
 
@@ -1117,6 +1133,52 @@ static const struct ethtool_ops gs_usb_ethtool_ops = {
 	.get_ts_info = gs_usb_get_ts_info,
 };
 
+static int gs_usb_get_termination(struct net_device *netdev, u16 *term)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+	struct gs_termination_state term_state;
+	int rc;
+
+	rc = usb_control_msg_recv(interface_to_usbdev(dev->iface), 0,
+				  GS_USB_BREQ_GET_TERMINATION,
+				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				  dev->channel, 0,
+				  &term_state, sizeof(term_state), 1000,
+				  GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	if (term_state.state == cpu_to_le32(GS_CAN_TERMINATION_STATE_ON))
+		*term = GS_USB_TERMINATION_ENABLED;
+	else
+		*term = GS_USB_TERMINATION_DISABLED;
+
+	return 0;
+}
+
+static int gs_usb_set_termination(struct net_device *netdev, u16 term)
+{
+	struct gs_can *dev = netdev_priv(netdev);
+	struct gs_termination_state term_state;
+
+	if (term == GS_USB_TERMINATION_ENABLED)
+		term_state.state = cpu_to_le32(GS_CAN_TERMINATION_STATE_ON);
+	else
+		term_state.state = cpu_to_le32(GS_CAN_TERMINATION_STATE_OFF);
+
+	return usb_control_msg_send(interface_to_usbdev(dev->iface), 0,
+				    GS_USB_BREQ_SET_TERMINATION,
+				    USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
+				    dev->channel, 0,
+				    &term_state, sizeof(term_state), 1000,
+				    GFP_KERNEL);
+}
+
+static const u16 gs_usb_termination_const[] = {
+	GS_USB_TERMINATION_DISABLED,
+	GS_USB_TERMINATION_ENABLED
+};
+
 static struct gs_can *gs_make_candev(unsigned int channel,
 				     struct usb_interface *intf,
 				     struct gs_device_config *dconf)
@@ -1211,6 +1273,20 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->can.do_set_data_bittiming = gs_usb_set_data_bittiming;
 	}
 
+	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;
+		}
+	}
+
 	/* The CANtact Pro from LinkLayer Labs is based on the
 	 * LPC54616 µC, which is affected by the NXP LPC USB transfer
 	 * erratum. However, the current firmware (version 2) doesn't
-- 
2.35.1

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

[Trevitz, Daniel]

Marc,

+       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.
This way you could have a multi-can-channel USB device but with only specific channels supporting configurable termination resistors.

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;
}

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. 

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

[Marc Kleine-Budde]

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

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

[Trevitz, Daniel]

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 |

RE: [PATCH v2 0/3] can: gs_usb: cleanups and termination support

[Trevitz, Daniel]

Is there a place to document this?

Example setting termination state:
	on: ip link set dev can0 type can termination 120
	off: ip link set dev can0 type can termination 0

Read the state:
	ip -details link show can0 

58: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0 
    can state STOPPED restart-ms 0 
          gs_usb: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp_inc 1
          termination 120 [ 0, 120
          clock 48000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 gro_max_size 65536 parentbus usb parentdev 3-12.2:1.0





**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: Sunday, September 18, 2022 5:18 PM
To: linux-can@vger.kernel.org
Cc: Trevitz, Daniel <Daniel.Trevitz@wika.com>; Ryan Edwards <ryan.edwards@gmail.com>
Subject: [PATCH v2 0/3] can: gs_usb: cleanups and termination support


Hello,

this series first cleans up the driver a bit and then adds switchable termination support.

regards,
Marc

changes since v1 https://lore.kernel.org/all/20220918202348.675850-1-mkl@pengutronix.de:
- add 1/3: "can: gs_usb: gs_make_candev(): convert from usb_control_msg() to usb_control_msg_recv()"
  to make error handling easier
- add 2/3: "can: gs_usb: gs_make_candev(): clean up error handling"
  which introduces a cleanup label
- move 1/3 to 3/3:
  - fix gs_usb_get_termination(), set term value
  - remove check of termination value from gs_usb_set_termination()
  - check for initial termination in gs_make_candev()

Re: [PATCH v2 0/3] can: gs_usb: cleanups and termination support

[Marc Kleine-Budde]

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

On 18.09.2022 22:00:35, Trevitz, Daniel wrote:
> Is there a place to document this?

Please add it to the SocketCAN documentation:

| https://github.com/torvalds/linux/blob/master/Documentation/networking/can.rst

> Example setting termination state:
> 	on: ip link set dev can0 type can termination 120
> 	off: ip link set dev can0 type can termination 0
> 
> Read the state:
> 	ip -details link show can0 
> 
> 58: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0 
>     can state STOPPED restart-ms 0 
>           gs_usb: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp_inc 1
>           termination 120 [ 0, 120
>           clock 48000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 gro_max_size 65536 parentbus usb parentdev 3-12.2:1.0

A patch against this file on this mailing list is preferred, please
include your Signed-off-by. For details see:

| https://github.com/marckleinebudde/linux/blob/master/Documentation/process/submitting-patches.rst#sign-your-work---the-developers-certificate-of-origin

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