[PATCH 0/3] can/peak_usb: Added improvement to the peak_usb driver

[PATCH 0/3] can/peak_usb: Added improvement to the peak_usb driver

[Stephane Grosjean]

This series of patches adds the following improvements:

- allows to identify CAN ports by their LEDs
- completes the list of devices supported by the module
- adds ONE_SHOT mode to some devices

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
Stephane Grosjean (3):
  can/peak_usb: add support of ethtool set_phys_id() callback
  can/peak_usb: add forgotten supported devices
  can/peak_usb: add support of ONE_SHOT mode

 drivers/net/can/usb/peak_usb/pcan_usb.c      | 47 ++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_core.c |  4 ++
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  2 +
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 47 ++++++++++++++++++--
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 46 +++++++++++++++++--
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  6 +++
 6 files changed, 144 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/3] can/peak_usb: add support of ethtool set_phys_id()

[Stephane Grosjean]

This patch makes it possible to specifically flash the LED of a CAN port
of the CAN-USB interfaces of PEAK-System.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb.c      | 47 ++++++++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_core.c |  4 ++
 drivers/net/can/usb/peak_usb/pcan_usb_core.h |  2 +
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 33 ++++++++++++++
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 34 +++++++++++++-
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  6 +++
 6 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index e6c1e5d33924..aa89c1119e27 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -11,6 +11,7 @@
 #include <linux/netdevice.h>
 #include <linux/usb.h>
 #include <linux/module.h>
+#include <linux/ethtool.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -42,6 +43,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB adapter");
 #define PCAN_USB_CMD_REGISTER	9
 #define PCAN_USB_CMD_EXT_VCC	10
 #define PCAN_USB_CMD_ERR_FR	11
+#define PCAN_USB_CMD_LED	12
 
 /* PCAN_USB_CMD_SET_BUS number arg */
 #define PCAN_USB_BUS_XCVER		2
@@ -250,6 +252,15 @@ static int pcan_usb_set_ext_vcc(struct peak_usb_device *dev, u8 onoff)
 	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_EXT_VCC, PCAN_USB_SET, args);
 }
 
+static int pcan_usb_set_led(struct peak_usb_device *dev, u8 onoff)
+{
+	u8 args[PCAN_USB_CMD_ARGS_LEN] = {
+		[0] = !!onoff,
+	};
+
+	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_LED, PCAN_USB_SET, args);
+}
+
 /*
  * set bittiming value to can
  */
@@ -973,6 +984,40 @@ static int pcan_usb_probe(struct usb_interface *intf)
 	return 0;
 }
 
+static int pcan_usb_set_phys_id(struct net_device *netdev,
+				enum ethtool_phys_id_state state)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	int err = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* call ON/OFF twice a second */
+		return 2;
+
+	case ETHTOOL_ID_OFF:
+		err = pcan_usb_set_led(dev, 0);
+		break;
+
+	case ETHTOOL_ID_ON:
+		fallthrough;
+
+	case ETHTOOL_ID_INACTIVE:
+		/* restore LED default */
+		err = pcan_usb_set_led(dev, 1);
+		break;
+
+	default:
+		break;
+	}
+
+	return err;
+}
+
+static const struct ethtool_ops pcan_usb_ethtool_ops = {
+	.set_phys_id = pcan_usb_set_phys_id,
+};
+
 /*
  * describe the PCAN-USB adapter
  */
@@ -1003,6 +1048,8 @@ const struct peak_usb_adapter pcan_usb = {
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb),
 
+	.ethtool_ops = &pcan_usb_ethtool_ops,
+
 	/* timestamps usage */
 	.ts_used_bits = 16,
 	.ts_period = 24575, /* calibration period in ts. */
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 573b11559d73..685c59e2cfd3 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/usb.h>
+#include <linux/ethtool.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -820,6 +821,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
 
 	netdev->flags |= IFF_ECHO; /* we support local echo */
 
+	/* add ethtool support */
+	netdev->ethtool_ops = peak_usb_adapter->ethtool_ops;
+
 	init_usb_anchor(&dev->rx_submitted);
 
 	init_usb_anchor(&dev->tx_submitted);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index 4b1528a42a7b..e15b4c78f309 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -46,6 +46,8 @@ struct peak_usb_adapter {
 	const struct can_bittiming_const * const data_bittiming_const;
 	unsigned int ctrl_count;
 
+	const struct ethtool_ops *ethtool_ops;
+
 	int (*intf_probe)(struct usb_interface *intf);
 
 	int (*dev_init)(struct peak_usb_device *dev);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index f347ecc79aef..6183a42f6491 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -7,6 +7,7 @@
 #include <linux/netdevice.h>
 #include <linux/usb.h>
 #include <linux/module.h>
+#include <linux/ethtool.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -1009,6 +1010,31 @@ static void pcan_usb_fd_free(struct peak_usb_device *dev)
 	}
 }
 
+/* blink LED's */
+static int pcan_usb_fd_set_phys_id(struct net_device *netdev,
+				   enum ethtool_phys_id_state state)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	int err = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		err = pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_FAST);
+		break;
+	case ETHTOOL_ID_INACTIVE:
+		err = pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_DEF);
+		break;
+	default:
+		break;
+	}
+
+	return err;
+}
+
+static const struct ethtool_ops pcan_usb_fd_ethtool_ops = {
+	.set_phys_id = pcan_usb_fd_set_phys_id,
+};
+
 /* describes the PCAN-USB FD adapter */
 static const struct can_bittiming_const pcan_usb_fd_const = {
 	.name = "pcan_usb_fd",
@@ -1050,6 +1076,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
 
+	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
+
 	/* timestamps usage */
 	.ts_used_bits = 32,
 	.ts_period = 1000000, /* calibration period in ts. */
@@ -1122,6 +1150,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
 
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
+	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
 
 	/* timestamps usage */
 	.ts_used_bits = 32,
@@ -1196,6 +1225,8 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
 
+	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
+
 	/* timestamps usage */
 	.ts_used_bits = 32,
 	.ts_period = 1000000, /* calibration period in ts. */
@@ -1269,6 +1300,8 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
 
+	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
+
 	/* timestamps usage */
 	.ts_used_bits = 32,
 	.ts_period = 1000000, /* calibration period in ts. */
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 275087c39602..ff740b4203fa 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -9,6 +9,7 @@
 #include <linux/netdevice.h>
 #include <linux/usb.h>
 #include <linux/module.h>
+#include <linux/ethtool.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -908,7 +909,7 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
 	usb_if->dev[dev->ctrl_idx] = dev;
 
 	/* set LED in default state (end of init phase) */
-	pcan_usb_pro_set_led(dev, 0, 1);
+	pcan_usb_pro_set_led(dev, FW_USBPRO_LED_DEVICE, 1);
 
 	kfree(bi);
 	kfree(fi);
@@ -992,6 +993,35 @@ int pcan_usb_pro_probe(struct usb_interface *intf)
 	return 0;
 }
 
+static int pcan_usb_pro_set_phys_id(struct net_device *netdev,
+				    enum ethtool_phys_id_state state)
+{
+	struct peak_usb_device *dev = netdev_priv(netdev);
+	int err = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* fast blinking forever */
+		err = pcan_usb_pro_set_led(dev, FW_USBPRO_LED_BLINK_FAST,
+					   0xffffffff);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		/* restore LED default */
+		err = pcan_usb_pro_set_led(dev, FW_USBPRO_LED_DEVICE, 1);
+		break;
+
+	default:
+		break;
+	}
+
+	return err;
+}
+
+static const struct ethtool_ops pcan_usb_pro_ethtool_ops = {
+	.set_phys_id = pcan_usb_pro_set_phys_id,
+};
+
 /*
  * describe the PCAN-USB Pro adapter
  */
@@ -1020,6 +1050,8 @@ const struct peak_usb_adapter pcan_usb_pro = {
 	/* size of device private data */
 	.sizeof_dev_private = sizeof(struct pcan_usb_pro_device),
 
+	.ethtool_ops = &pcan_usb_pro_ethtool_ops,
+
 	/* timestamps usage */
 	.ts_used_bits = 32,
 	.ts_period = 1000000, /* calibration period in ts. */
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
index 6bb12357d078..421104ee29f4 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
@@ -115,6 +115,12 @@ struct __packed pcan_usb_pro_devid {
 	__le32 serial_num;
 };
 
+#define FW_USBPRO_LED_DEVICE		0x00
+#define FW_USBPRO_LED_BLINK_FAST	0x01
+#define FW_USBPRO_LED_BLINK_SLOW	0x02
+#define FW_USBPRO_LED_ON		0x03
+#define FW_USBPRO_LED_OFF		0x04
+
 struct __packed pcan_usb_pro_setled {
 	u8  data_type;
 	u8  channel;
-- 
2.25.1

Re: [PATCH 1/3] can/peak_usb: add support of ethtool set_phys_id()

[Marc Kleine-Budde]

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

On 09.03.2021 09:21:26, Stephane Grosjean wrote:
> This patch makes it possible to specifically flash the LED of a CAN port
> of the CAN-USB interfaces of PEAK-System.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>

Added this one to linux-can-next/testing.

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 1/3] can/peak_usb: add support of ethtool set_phys_id()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 10172 bytes --]

On 3/9/21 9:21 AM, Stephane Grosjean wrote:
> This patch makes it possible to specifically flash the LED of a CAN port
> of the CAN-USB interfaces of PEAK-System.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb.c      | 47 ++++++++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_core.c |  4 ++
>  drivers/net/can/usb/peak_usb/pcan_usb_core.h |  2 +
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c   | 33 ++++++++++++++
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c  | 34 +++++++++++++-
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.h  |  6 +++
>  6 files changed, 125 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index e6c1e5d33924..aa89c1119e27 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/usb.h>
>  #include <linux/module.h>
> +#include <linux/ethtool.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -42,6 +43,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB adapter");
>  #define PCAN_USB_CMD_REGISTER	9
>  #define PCAN_USB_CMD_EXT_VCC	10
>  #define PCAN_USB_CMD_ERR_FR	11
> +#define PCAN_USB_CMD_LED	12
>  
>  /* PCAN_USB_CMD_SET_BUS number arg */
>  #define PCAN_USB_BUS_XCVER		2
> @@ -250,6 +252,15 @@ static int pcan_usb_set_ext_vcc(struct peak_usb_device *dev, u8 onoff)
>  	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_EXT_VCC, PCAN_USB_SET, args);
>  }
>  
> +static int pcan_usb_set_led(struct peak_usb_device *dev, u8 onoff)
> +{
> +	u8 args[PCAN_USB_CMD_ARGS_LEN] = {
> +		[0] = !!onoff,
> +	};
> +
> +	return pcan_usb_send_cmd(dev, PCAN_USB_CMD_LED, PCAN_USB_SET, args);
> +}
> +
>  /*
>   * set bittiming value to can
>   */
> @@ -973,6 +984,40 @@ static int pcan_usb_probe(struct usb_interface *intf)
>  	return 0;
>  }
>  
> +static int pcan_usb_set_phys_id(struct net_device *netdev,
> +				enum ethtool_phys_id_state state)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	int err = 0;
> +
> +	switch (state) {
> +	case ETHTOOL_ID_ACTIVE:
> +		/* call ON/OFF twice a second */
> +		return 2;
> +
> +	case ETHTOOL_ID_OFF:
> +		err = pcan_usb_set_led(dev, 0);
> +		break;
> +
> +	case ETHTOOL_ID_ON:
> +		fallthrough;
> +
> +	case ETHTOOL_ID_INACTIVE:
> +		/* restore LED default */
> +		err = pcan_usb_set_led(dev, 1);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static const struct ethtool_ops pcan_usb_ethtool_ops = {
> +	.set_phys_id = pcan_usb_set_phys_id,
> +};
> +
>  /*
>   * describe the PCAN-USB adapter
>   */
> @@ -1003,6 +1048,8 @@ const struct peak_usb_adapter pcan_usb = {
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb),
>  
> +	.ethtool_ops = &pcan_usb_ethtool_ops,
> +
>  	/* timestamps usage */
>  	.ts_used_bits = 16,
>  	.ts_period = 24575, /* calibration period in ts. */
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index 573b11559d73..685c59e2cfd3 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
>  #include <linux/usb.h>
> +#include <linux/ethtool.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -820,6 +821,9 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  
>  	netdev->flags |= IFF_ECHO; /* we support local echo */
>  
> +	/* add ethtool support */
> +	netdev->ethtool_ops = peak_usb_adapter->ethtool_ops;
> +
>  	init_usb_anchor(&dev->rx_submitted);
>  
>  	init_usb_anchor(&dev->tx_submitted);
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> index 4b1528a42a7b..e15b4c78f309 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> @@ -46,6 +46,8 @@ struct peak_usb_adapter {
>  	const struct can_bittiming_const * const data_bittiming_const;
>  	unsigned int ctrl_count;
>  
> +	const struct ethtool_ops *ethtool_ops;
> +
>  	int (*intf_probe)(struct usb_interface *intf);
>  
>  	int (*dev_init)(struct peak_usb_device *dev);
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index f347ecc79aef..6183a42f6491 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -7,6 +7,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/usb.h>
>  #include <linux/module.h>
> +#include <linux/ethtool.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -1009,6 +1010,31 @@ static void pcan_usb_fd_free(struct peak_usb_device *dev)
>  	}
>  }
>  
> +/* blink LED's */
> +static int pcan_usb_fd_set_phys_id(struct net_device *netdev,
> +				   enum ethtool_phys_id_state state)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	int err = 0;
> +
> +	switch (state) {
> +	case ETHTOOL_ID_ACTIVE:
> +		err = pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_FAST);
> +		break;
> +	case ETHTOOL_ID_INACTIVE:
> +		err = pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_DEF);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static const struct ethtool_ops pcan_usb_fd_ethtool_ops = {
> +	.set_phys_id = pcan_usb_fd_set_phys_id,
> +};
> +
>  /* describes the PCAN-USB FD adapter */
>  static const struct can_bittiming_const pcan_usb_fd_const = {
>  	.name = "pcan_usb_fd",
> @@ -1050,6 +1076,8 @@ const struct peak_usb_adapter pcan_usb_fd = {
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
>  
> +	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
> +
>  	/* timestamps usage */
>  	.ts_used_bits = 32,
>  	.ts_period = 1000000, /* calibration period in ts. */
> @@ -1122,6 +1150,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
>  
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
> +	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
>  
>  	/* timestamps usage */
>  	.ts_used_bits = 32,
> @@ -1196,6 +1225,8 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
>  
> +	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
> +
>  	/* timestamps usage */
>  	.ts_used_bits = 32,
>  	.ts_period = 1000000, /* calibration period in ts. */
> @@ -1269,6 +1300,8 @@ const struct peak_usb_adapter pcan_usb_x6 = {
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb_fd_device),
>  
> +	.ethtool_ops = &pcan_usb_fd_ethtool_ops,
> +
>  	/* timestamps usage */
>  	.ts_used_bits = 32,
>  	.ts_period = 1000000, /* calibration period in ts. */
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index 275087c39602..ff740b4203fa 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -9,6 +9,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/usb.h>
>  #include <linux/module.h>
> +#include <linux/ethtool.h>
>  
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
> @@ -908,7 +909,7 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>  	usb_if->dev[dev->ctrl_idx] = dev;
>  
>  	/* set LED in default state (end of init phase) */
> -	pcan_usb_pro_set_led(dev, 0, 1);
> +	pcan_usb_pro_set_led(dev, FW_USBPRO_LED_DEVICE, 1);
>  
>  	kfree(bi);
>  	kfree(fi);
> @@ -992,6 +993,35 @@ int pcan_usb_pro_probe(struct usb_interface *intf)
>  	return 0;
>  }
>  
> +static int pcan_usb_pro_set_phys_id(struct net_device *netdev,
> +				    enum ethtool_phys_id_state state)
> +{
> +	struct peak_usb_device *dev = netdev_priv(netdev);
> +	int err = 0;
> +
> +	switch (state) {
> +	case ETHTOOL_ID_ACTIVE:
> +		/* fast blinking forever */
> +		err = pcan_usb_pro_set_led(dev, FW_USBPRO_LED_BLINK_FAST,
> +					   0xffffffff);
> +		break;
> +
> +	case ETHTOOL_ID_INACTIVE:
> +		/* restore LED default */
> +		err = pcan_usb_pro_set_led(dev, FW_USBPRO_LED_DEVICE, 1);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static const struct ethtool_ops pcan_usb_pro_ethtool_ops = {
> +	.set_phys_id = pcan_usb_pro_set_phys_id,
> +};
> +
>  /*
>   * describe the PCAN-USB Pro adapter
>   */
> @@ -1020,6 +1050,8 @@ const struct peak_usb_adapter pcan_usb_pro = {
>  	/* size of device private data */
>  	.sizeof_dev_private = sizeof(struct pcan_usb_pro_device),
>  
> +	.ethtool_ops = &pcan_usb_pro_ethtool_ops,
> +
>  	/* timestamps usage */
>  	.ts_used_bits = 32,
>  	.ts_period = 1000000, /* calibration period in ts. */
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> index 6bb12357d078..421104ee29f4 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> @@ -115,6 +115,12 @@ struct __packed pcan_usb_pro_devid {
>  	__le32 serial_num;
>  };
>  
> +#define FW_USBPRO_LED_DEVICE		0x00
> +#define FW_USBPRO_LED_BLINK_FAST	0x01
> +#define FW_USBPRO_LED_BLINK_SLOW	0x02
> +#define FW_USBPRO_LED_ON		0x03
> +#define FW_USBPRO_LED_OFF		0x04

What about replacing the FW_ with PCAN_, so that we're using the same prefix
within the driver? I can do this while applying.

> +
>  struct __packed pcan_usb_pro_setled {
>  	u8  data_type;
>  	u8  channel;
> 

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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] can/peak_usb: add support of ethtool set_phys_id()

[Marc Kleine-Budde]

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

On 09.03.2021 13:15:27, Marc Kleine-Budde wrote:
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> > index 6bb12357d078..421104ee29f4 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> > @@ -115,6 +115,12 @@ struct __packed pcan_usb_pro_devid {
> >  	__le32 serial_num;
> >  };
> >  
> > +#define FW_USBPRO_LED_DEVICE		0x00
> > +#define FW_USBPRO_LED_BLINK_FAST	0x01
> > +#define FW_USBPRO_LED_BLINK_SLOW	0x02
> > +#define FW_USBPRO_LED_ON		0x03
> > +#define FW_USBPRO_LED_OFF		0x04
> 
> What about replacing the FW_ with PCAN_, so that we're using the same prefix
> within the driver? I can do this while applying.

I've replaced the FW_ with PCAN_ while applying the patch.

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

[PATCH 2/3] can/peak_usb: add forgotten supported devices

[Stephane Grosjean]

Since the peak_usb driver also supports the CAN-USB interfaces
"PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
their names to the list of explicitly supported devices.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 6183a42f6491..8e6250c4c417 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -19,6 +19,8 @@
 
 MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
 MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
+MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
+MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
 
 #define PCAN_USBPROFD_CHANNEL_COUNT	2
 #define PCAN_USBFD_CHANNEL_COUNT	1
-- 
2.25.1

Re: [PATCH 2/3] can/peak_usb: add forgotten supported devices

[Marc Kleine-Budde]

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

On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> Since the peak_usb driver also supports the CAN-USB interfaces
> "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> their names to the list of explicitly supported devices.

Can you look up which patch added support for these interfaces? Then we
can add a Fixes tag.

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 2/3] can/peak_usb: add forgotten supported devices

[Stéphane Grosjean]

Hi Marc,

PCAN-USB X6 support has been added by
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f00b534ded60bd0a23c2fa8dec4ece52aa7d235f

PCAN-Chip USB support has been added by
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea8b65b596d78969629562f9728f76cbf565fbec

Should I update the patch description with these commit Id and push a v2?

--- Stéphane





            From: Marc Kleine-Budde
Sent: Tuesday, March 9, 2021 12:07
To: Stéphane Grosjean
Cc: linux-can Mailing List
Subject: Re: [PATCH 2/3] can/peak_usb: add forgotten supported devices




On 09.03.2021 09:21:27, Stephane Grosjean wrote:

> Since the peak_usb driver also supports the CAN-USB interfaces

> "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds

> their names to the list of explicitly supported devices.



Can you look up which patch added support for these interfaces? Then we

can add a Fixes tag.



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 |


--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

Re: [PATCH 2/3] can/peak_usb: add forgotten supported devices

[Marc Kleine-Budde]

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

On 09.03.2021 14:22:38, Stéphane Grosjean wrote:
> Hi Marc,
> 
> PCAN-USB X6 support has been added by
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f00b534ded60bd0a23c2fa8dec4ece52aa7d235f
> 
> PCAN-Chip USB support has been added by
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea8b65b596d78969629562f9728f76cbf565fbec
> 
> Should I update the patch description with these commit Id and push a
> v2?

No need, I've updated the patch.

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 2/3] can/peak_usb: add forgotten supported devices

[Marc Kleine-Budde]

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

On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> Since the peak_usb driver also supports the CAN-USB interfaces
> "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> their names to the list of explicitly supported devices.
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index 6183a42f6491..8e6250c4c417 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -19,6 +19,8 @@
>  
>  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
>  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");

I've exchanged these, to correspond the order of the device ids.

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 2/3] can/peak_usb: add forgotten supported devices

[Marc Kleine-Budde]

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

On 09.03.2021 16:28:37, Marc Kleine-Budde wrote:
> On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> > Since the peak_usb driver also supports the CAN-USB interfaces
> > "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> > their names to the list of explicitly supported devices.
> > 
> > Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> > ---
> >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > index 6183a42f6491..8e6250c4c417 100644
> > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > @@ -19,6 +19,8 @@
> >  
> >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
> 
> I've exchanged these, to correspond the order of the device ids.

Funny side note:
MODULE_SUPPORTED_DEVICE was a noop define. All uses have been globally
removed from Linus' tree after this patch hit linux-net/master, but
before it landed in Linus' tree.

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 2/3] can/peak_usb: add forgotten supported devices

[Vincent MAILHOL]

On Fri. 19 Mar 2021 at 17:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09.03.2021 16:28:37, Marc Kleine-Budde wrote:
> > On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> > > Since the peak_usb driver also supports the CAN-USB interfaces
> > > "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> > > their names to the list of explicitly supported devices.
> > >
> > > Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> > > ---
> > >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > index 6183a42f6491..8e6250c4c417 100644
> > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > @@ -19,6 +19,8 @@
> > >
> > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
> >
> > I've exchanged these, to correspond the order of the device ids.
>
> Funny side note:
> MODULE_SUPPORTED_DEVICE was a noop define. All uses have been globally
> removed from Linus' tree after this patch hit linux-net/master, but
> before it landed in Linus' tree.

Silly question but does it mean that we should not use
MODULE_SUPPORTED_DEVICE in newly submitted patches? After seeing
Stéphane's patch, I added it to my driver. Even if it is a noop
define, it adds meta information in the source code so I was
inclined to keep it.


Yours sincerely,
Vincent

Re: [PATCH 2/3] can/peak_usb: add forgotten supported devices

[Marc Kleine-Budde]

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

On 19.03.2021 18:47:06, Vincent MAILHOL wrote:
> On Fri. 19 Mar 2021 at 17:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 09.03.2021 16:28:37, Marc Kleine-Budde wrote:
> > > On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> > > > Since the peak_usb driver also supports the CAN-USB interfaces
> > > > "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> > > > their names to the list of explicitly supported devices.
> > > >
> > > > Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> > > > ---
> > > >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > index 6183a42f6491..8e6250c4c417 100644
> > > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > @@ -19,6 +19,8 @@
> > > >
> > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
> > >
> > > I've exchanged these, to correspond the order of the device ids.
> >
> > Funny side note:
> > MODULE_SUPPORTED_DEVICE was a noop define. All uses have been globally
> > removed from Linus' tree after this patch hit linux-net/master, but
> > before it landed in Linus' tree.
> 
> Silly question but does it mean that we should not use
> MODULE_SUPPORTED_DEVICE in newly submitted patches?

ACK - It's been removed from Linus' tree, see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6417f03132a6952cd17ddd8eaddbac92b61b17e0

> After seeing Stéphane's patch, I added it to my driver. Even if it is
> a noop define, it adds meta information in the source code so I was
> inclined to keep it.

As the noop define has been removed, the driver will no longer compile.

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 2/3] can/peak_usb: add forgotten supported devices

[Vincent MAILHOL]

On Fri. 19 mars 2021 at 18:56, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 19.03.2021 18:47:06, Vincent MAILHOL wrote:
> > On Fri. 19 Mar 2021 at 17:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 09.03.2021 16:28:37, Marc Kleine-Budde wrote:
> > > > On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> > > > > Since the peak_usb driver also supports the CAN-USB interfaces
> > > > > "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> > > > > their names to the list of explicitly supported devices.
> > > > >
> > > > > Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> > > > > ---
> > > > >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > index 6183a42f6491..8e6250c4c417 100644
> > > > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > @@ -19,6 +19,8 @@
> > > > >
> > > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> > > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> > > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> > > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
> > > >
> > > > I've exchanged these, to correspond the order of the device ids.
> > >
> > > Funny side note:
> > > MODULE_SUPPORTED_DEVICE was a noop define. All uses have been globally
> > > removed from Linus' tree after this patch hit linux-net/master, but
> > > before it landed in Linus' tree.
> >
> > Silly question but does it mean that we should not use
> > MODULE_SUPPORTED_DEVICE in newly submitted patches?
>
> ACK - It's been removed from Linus' tree, see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6417f03132a6952cd17ddd8eaddbac92b61b17e0
>
> > After seeing Stéphane's patch, I added it to my driver. Even if it is
> > a noop define, it adds meta information in the source code so I was
> > inclined to keep it.
>
> As the noop define has been removed, the driver will no longer compile.

Got it, thanks for the link to the commit!


Yours sincerely,
Vincent

RE: [PATCH 2/3] can/peak_usb: add forgotten supported devices

[Stéphane Grosjean]

Hi,

Sorry guys for all that noise... :-/

-- Stéphane


De : Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Envoyé : vendredi 19 mars 2021 11:07
À : Marc Kleine-Budde <mkl@pengutronix.de>
Cc : Stéphane Grosjean <s.grosjean@peak-system.com>; linux-can Mailing List <linux-can@vger.kernel.org>
Objet : Re: [PATCH 2/3] can/peak_usb: add forgotten supported devices

On Fri. 19 mars 2021 at 18:56, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 19.03.2021 18:47:06, Vincent MAILHOL wrote:
> > On Fri. 19 Mar 2021 at 17:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 09.03.2021 16:28:37, Marc Kleine-Budde wrote:
> > > > On 09.03.2021 09:21:27, Stephane Grosjean wrote:
> > > > > Since the peak_usb driver also supports the CAN-USB interfaces
> > > > > "PCAN-USB X6" and "PCAN-Chip USB" from PEAK-System GmbH, this patch adds
> > > > > their names to the list of explicitly supported devices.
> > > > >
> > > > > Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> > > > > ---
> > > > >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > index 6183a42f6491..8e6250c4c417 100644
> > > > > --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> > > > > @@ -19,6 +19,8 @@
> > > > >
> > > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB FD adapter");
> > > > >  MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro FD adapter");
> > > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB X6 adapter");
> > > > > +MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-Chip USB");
> > > >
> > > > I've exchanged these, to correspond the order of the device ids.
> > >
> > > Funny side note:
> > > MODULE_SUPPORTED_DEVICE was a noop define. All uses have been globally
> > > removed from Linus' tree after this patch hit linux-net/master, but
> > > before it landed in Linus' tree.
> >
> > Silly question but does it mean that we should not use
> > MODULE_SUPPORTED_DEVICE in newly submitted patches?
>
> ACK - It's been removed from Linus' tree, see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6417f03132a6952cd17ddd8eaddbac92b61b17e0
>
> > After seeing Stéphane's patch, I added it to my driver. Even if it is
> > a noop define, it adds meta information in the source code so I was
> > inclined to keep it.
>
> As the noop define has been removed, the driver will no longer compile.

Got it, thanks for the link to the commit!


Yours sincerely,
Vincent

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html

[PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode

[Stephane Grosjean]

This patch adds "ONE-SHOT" mode support to the following CAN-USB
PEAK-System GmbH interfaces:
- PCAN-USB X6
- PCAN-USB FD
- PCAN-USB Pro FD
- PCAN-Chip USB
- PCAN-USB Pro

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c  | 12 ++++++++----
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 12 +++++++++---
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 8e6250c4c417..0e3bb3a4945d 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -779,6 +779,10 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
 			tx_msg_flags |= PUCAN_MSG_RTR;
 	}
 
+	/* Single-Shot frame */
+	if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		tx_msg_flags |= PUCAN_MSG_SINGLE_SHOT;
+
 	tx_msg->flags = cpu_to_le16(tx_msg_flags);
 	tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, dlc);
 	memcpy(tx_msg->d, cfd->data, cfd->len);
@@ -1068,7 +1072,7 @@ const struct peak_usb_adapter pcan_usb_fd = {
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1143,7 +1147,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
 	.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 		CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-		CAN_CTRLMODE_CC_LEN8_DLC,
+		CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1217,7 +1221,7 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
@@ -1292,7 +1296,7 @@ const struct peak_usb_adapter pcan_usb_x6 = {
 	.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
 	.ctrlmode_supported = CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
-			CAN_CTRLMODE_CC_LEN8_DLC,
+			CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
 	.clock = {
 		.freq = PCAN_UFD_CRYSTAL_HZ,
 	},
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index ff740b4203fa..5b098d1e3746 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -39,6 +39,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
 
 #define PCAN_USBPRO_RTR			0x01
 #define PCAN_USBPRO_EXT			0x02
+#define PCAN_USBPRO_SS			0x08
 
 #define PCAN_USBPRO_CMD_BUFFER_SIZE	512
 
@@ -779,9 +780,13 @@ static int pcan_usb_pro_encode_msg(struct peak_usb_device *dev,
 
 	flags = 0;
 	if (cf->can_id & CAN_EFF_FLAG)
-		flags |= 0x02;
+		flags |= PCAN_USBPRO_EXT;
 	if (cf->can_id & CAN_RTR_FLAG)
-		flags |= 0x01;
+		flags |= PCAN_USBPRO_RTR;
+
+	/* Single-Shot frame */
+	if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		flags |= PCAN_USBPRO_SS;
 
 	pcan_msg_add_rec(&usb_msg, data_type, 0, flags, len, cf->can_id,
 			 cf->data);
@@ -1041,7 +1046,8 @@ const struct peak_usb_adapter pcan_usb_pro = {
 	.name = "PCAN-USB Pro",
 	.device_id = PCAN_USBPRO_PRODUCT_ID,
 	.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
-	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
+	.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
+			      CAN_CTRLMODE_ONE_SHOT,
 	.clock = {
 		.freq = PCAN_USBPRO_CRYSTAL_HZ,
 	},
-- 
2.25.1

Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 693 bytes --]

On 3/9/21 9:21 AM, Stephane Grosjean wrote:
> This patch adds "ONE-SHOT" mode support to the following CAN-USB
> PEAK-System GmbH interfaces:
> - PCAN-USB X6
> - PCAN-USB FD
> - PCAN-USB Pro FD
> - PCAN-Chip USB
> - PCAN-USB Pro
> 
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>

What happens if in one shot mode and the frame is not send? Who takes care of
the echo skb?

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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode

[Oliver Hartkopp]

On 09.03.21 11:36, Marc Kleine-Budde wrote:
> On 3/9/21 9:21 AM, Stephane Grosjean wrote:
>> This patch adds "ONE-SHOT" mode support to the following CAN-USB
>> PEAK-System GmbH interfaces:
>> - PCAN-USB X6
>> - PCAN-USB FD
>> - PCAN-USB Pro FD
>> - PCAN-Chip USB
>> - PCAN-USB Pro
>>
>> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> 
> What happens if in one shot mode and the frame is not send? Who takes care of
> the echo skb?

ONE-SHOT only means that the CAN controller would not retry the 
transmission when e.g. loosing the arbitration or getting an error flag.

The sja1000 does it this way, when the TX interrupt flag is set in the 
interrupt register:

                 if (isrc & IRQ_TI) {
                         /* transmission buffer released */
                         if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
                             !(status & SR_TCS)) {
                                 stats->tx_errors++;
                                 can_free_echo_skb(dev, 0);
                         } else {
                                 /* transmission complete */
                                 stats->tx_bytes +=
                                         priv->read_reg(priv, 
SJA1000_FI) & 0xf;
                                 stats->tx_packets++;
                                 can_get_echo_skb(dev, 0, NULL);
                         }
                         netif_wake_queue(dev);
                         can_led_event(dev, CAN_LED_EVENT_TX);
                 }


Do we need to check this for the other drivers?

Regards,
Oliver

Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1639 bytes --]

On 3/9/21 11:53 AM, Oliver Hartkopp wrote:
>> What happens if in one shot mode and the frame is not send? Who takes care of
>> the echo skb?
> 
> ONE-SHOT only means that the CAN controller would not retry the 
> transmission when e.g. loosing the arbitration or getting an error flag.
> 
> The sja1000 does it this way, when the TX interrupt flag is set in the 
> interrupt register:
> 
>                  if (isrc & IRQ_TI) {
>                          /* transmission buffer released */
>                          if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
>                              !(status & SR_TCS)) {
>                                  stats->tx_errors++;
>                                  can_free_echo_skb(dev, 0);
>                          } else {
>                                  /* transmission complete */
>                                  stats->tx_bytes +=
>                                          priv->read_reg(priv, 
> SJA1000_FI) & 0xf;
>                                  stats->tx_packets++;
>                                  can_get_echo_skb(dev, 0, NULL);
>                          }
>                          netif_wake_queue(dev);
>                          can_led_event(dev, CAN_LED_EVENT_TX);
>                  }
> 
> 
> Do we need to check this for the other drivers?

Yes.

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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode

[Marc Kleine-Budde]

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

On 09.03.2021 09:21:28, Stephane Grosjean wrote:
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -39,6 +39,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
>  
>  #define PCAN_USBPRO_RTR			0x01
>  #define PCAN_USBPRO_EXT			0x02
> +#define PCAN_USBPRO_SS			0x08
>  
>  #define PCAN_USBPRO_CMD_BUFFER_SIZE	512
>  
> @@ -779,9 +780,13 @@ static int pcan_usb_pro_encode_msg(struct peak_usb_device *dev,
>  
>  	flags = 0;
>  	if (cf->can_id & CAN_EFF_FLAG)
> -		flags |= 0x02;
> +		flags |= PCAN_USBPRO_EXT;
>  	if (cf->can_id & CAN_RTR_FLAG)
> -		flags |= 0x01;
> +		flags |= PCAN_USBPRO_RTR;

I've put this change in a separate patch and applied both to linux-can-next/testing.

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