[PATCH v5 0/6] Add CAN FD infrastructure for CAN driver

[PATCH v5 0/6] Add CAN FD infrastructure for CAN driver

[Oliver Hartkopp]

This is the patch series to extend the CAN driver infrastructure with CAN FD.

Changes v4 -> v5:

Introduced a new helper function candev_has_bitrate() in patch 2 which leads
to simple changes in the following patches using this helper now.

Regards,
Oliver 

*** BLURB HERE ***

Oliver Hartkopp (6):
  can: preserve skbuff protocol in can_put_echo_skb
  can: only send bitrate data via netlink when available
  can: provide a separate bittiming_const parameter to bittiming
    functions
  can: introduce the data bitrate configuration for CAN FD
  can: allow to change the device mtu for CAN FD capable devices
  can: add bittiming check at interface open for CAN FD

 drivers/net/can/dev.c            | 146 +++++++++++++++++++++++++++++++++------
 include/linux/can/dev.h          |   7 +-
 include/uapi/linux/can/netlink.h |   3 +
 3 files changed, 134 insertions(+), 22 deletions(-)

-- 
1.9.0

[PATCH v5 1/6] can: preserve skbuff protocol in can_put_echo_skb

[Oliver Hartkopp]

The skbuff protocol value was formerly fixed/sanitized to ETH_P_CAN in
can_put_echo_skb(). With CAN FD this value has to be preserved.
This patch changes the hard assignment of the protocol value to a check of
valid protocol values for CAN and CAN FD.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c0563f1..e1a3741 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -317,7 +317,9 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	BUG_ON(idx >= priv->echo_skb_max);
 
 	/* check flag whether this packet has to be looped back */
-	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK ||
+	    (skb->protocol != htons(ETH_P_CAN) &&
+	     skb->protocol != htons(ETH_P_CANFD))) {
 		kfree_skb(skb);
 		return;
 	}
@@ -329,7 +331,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 			return;
 
 		/* make settings for echo to reduce code in irq context */
-		skb->protocol = htons(ETH_P_CAN);
 		skb->pkt_type = PACKET_BROADCAST;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 		skb->dev = dev;
-- 
1.9.0

[PATCH v5 2/6] can: only send bitrate data via netlink when available

[Oliver Hartkopp]

When setting the bitrate both can_calc_bittiming() and can_fixup_bittiming()
lead to the bitrate and the tq variable to be set, when a proper bit timing
is available.

This patch introduces a helper function and only provides the bit timing via
netlink when it's set properly.

This is also a preparation for the CAN FD specific bittiming configurations
which are not existing in non-FD drivers.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e1a3741..e88934c 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -597,6 +597,16 @@ void free_candev(struct net_device *dev)
 EXPORT_SYMBOL_GPL(free_candev);
 
 /*
+ * When setting the bitrate both can_calc_bittiming() and can_fixup_bittiming()
+ * lead to the bitrate and the tq variable to be set, when a proper bit timing
+ * is available.
+ */
+static inline int candev_has_bitrate(struct can_bittiming *bt)
+{
+	return (bt->bitrate && bt->tq);
+}
+
+/*
  * Common open function when the device gets opened.
  *
  * This function should be called in the open function of the device
@@ -606,7 +616,7 @@ int open_candev(struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
-	if (!priv->bittiming.tq && !priv->bittiming.bitrate) {
+	if (!candev_has_bitrate(&priv->bittiming)) {
 		netdev_err(dev, "bit-timing not yet defined\n");
 		return -EINVAL;
 	}
@@ -719,7 +729,8 @@ static size_t can_get_size(const struct net_device *dev)
 	struct can_priv *priv = netdev_priv(dev);
 	size_t size = 0;
 
-	size += nla_total_size(sizeof(struct can_bittiming));	/* IFLA_CAN_BITTIMING */
+	if (candev_has_bitrate(&priv->bittiming))		/* IFLA_CAN_BITTIMING */
+		size += nla_total_size(sizeof(struct can_bittiming));
 	if (priv->bittiming_const)				/* IFLA_CAN_BITTIMING_CONST */
 		size += nla_total_size(sizeof(struct can_bittiming_const));
 	size += nla_total_size(sizeof(struct can_clock));	/* IFLA_CAN_CLOCK */
@@ -741,15 +752,20 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	if (priv->do_get_state)
 		priv->do_get_state(dev, &state);
-	if (nla_put(skb, IFLA_CAN_BITTIMING,
-		    sizeof(priv->bittiming), &priv->bittiming) ||
+
+	if ((candev_has_bitrate(&priv->bittiming) &&
+	     nla_put(skb, IFLA_CAN_BITTIMING,
+		     sizeof(priv->bittiming), &priv->bittiming)) ||
+
 	    (priv->bittiming_const &&
 	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
 		     sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
+
 	    nla_put(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock) ||
 	    nla_put_u32(skb, IFLA_CAN_STATE, state) ||
 	    nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
 	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
+
 	    (priv->do_get_berr_counter &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
 	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
-- 
1.9.0

[PATCH v5 3/6] can: provide a separate bittiming_const parameter to bittiming functions

[Oliver Hartkopp]

As the bittiming calculation functions are to be used with different
bittiming_const structures for CAN and CAN FD the direct reference to
priv->bittiming_const inside these functions has to be removed.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e88934c..368d92d 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -99,10 +99,10 @@ static int can_update_spt(const struct can_bittiming_const *btc,
 	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
 }
 
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			      const struct can_bittiming_const *btc)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	const struct can_bittiming_const *btc = priv->bittiming_const;
 	long rate, best_rate = 0;
 	long best_error = 1000000000, error = 0;
 	int best_tseg = 0, best_brp = 0, brp = 0;
@@ -110,7 +110,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	int spt_error = 1000, spt = 0, sampl_pt;
 	u64 v64;
 
-	if (!priv->bittiming_const)
+	if (!btc)
 		return -ENOTSUPP;
 
 	/* Use CIA recommended sample points */
@@ -204,7 +204,8 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	return 0;
 }
 #else /* !CONFIG_CAN_CALC_BITTIMING */
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			      const struct can_bittiming_const *btc)
 {
 	netdev_err(dev, "bit-timing calculation not available\n");
 	return -EINVAL;
@@ -217,14 +218,14 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
  * prescaler value brp. You can find more information in the header
  * file linux/can/netlink.h.
  */
-static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			       const struct can_bittiming_const *btc)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	const struct can_bittiming_const *btc = priv->bittiming_const;
 	int tseg1, alltseg;
 	u64 brp64;
 
-	if (!priv->bittiming_const)
+	if (!btc)
 		return -ENOTSUPP;
 
 	tseg1 = bt->prop_seg + bt->phase_seg1;
@@ -254,21 +255,21 @@ static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	return 0;
 }
 
-static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			     const struct can_bittiming_const *btc)
 {
-	struct can_priv *priv = netdev_priv(dev);
 	int err;
 
 	/* Check if the CAN device has bit-timing parameters */
-	if (priv->bittiming_const) {
+	if (btc) {
 
 		/* Non-expert mode? Check if the bitrate has been pre-defined */
 		if (!bt->tq)
 			/* Determine bit-timing parameters */
-			err = can_calc_bittiming(dev, bt);
+			err = can_calc_bittiming(dev, bt, btc);
 		else
 			/* Check bit-timing params and calculate proper brp */
-			err = can_fixup_bittiming(dev, bt);
+			err = can_fixup_bittiming(dev, bt, btc);
 		if (err)
 			return err;
 	}
@@ -679,7 +680,7 @@ static int can_changelink(struct net_device *dev,
 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
 		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
 			return -EINVAL;
-		err = can_get_bittiming(dev, &bt);
+		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
 		if (err)
 			return err;
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
-- 
1.9.0

[PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

As CAN FD offers a second bitrate for the data section of the CAN frame the
infrastructure for storing and configuring this second bitrate is introduced.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c            | 42 +++++++++++++++++++++++++++++++++++++++-
 include/linux/can/dev.h          |  6 ++++--
 include/uapi/linux/can/netlink.h |  2 ++
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 368d92d..7887691 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -660,6 +660,10 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 				= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
 	[IFLA_CAN_BERR_COUNTER]	= { .len = sizeof(struct can_berr_counter) },
+	[IFLA_CAN_DATA_BITTIMING]
+				= { .len = sizeof(struct can_bittiming) },
+	[IFLA_CAN_DATA_BITTIMING_CONST]
+				= { .len = sizeof(struct can_bittiming_const) },
 };
 
 static int can_changelink(struct net_device *dev,
@@ -693,6 +697,29 @@ static int can_changelink(struct net_device *dev,
 		}
 	}
 
+	if (data[IFLA_CAN_DATA_BITTIMING]) {
+		struct can_bittiming bt;
+
+		/* Do not allow changing bittiming while running */
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
+		       sizeof(bt));
+		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
+			return -EINVAL;
+		err = can_get_bittiming(dev, &bt, priv->data_bittiming_const);
+		if (err)
+			return err;
+		memcpy(&priv->data_bittiming, &bt, sizeof(bt));
+
+		if (priv->do_set_data_bittiming) {
+			/* Finally, set the bit-timing registers */
+			err = priv->do_set_data_bittiming(dev);
+			if (err)
+				return err;
+		}
+	}
+
 	if (data[IFLA_CAN_CTRLMODE]) {
 		struct can_ctrlmode *cm;
 
@@ -740,6 +767,10 @@ static size_t can_get_size(const struct net_device *dev)
 	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_RESTART_MS */
 	if (priv->do_get_berr_counter)				/* IFLA_CAN_BERR_COUNTER */
 		size += nla_total_size(sizeof(struct can_berr_counter));
+	if (candev_has_bitrate(&priv->data_bittiming))		/* IFLA_CAN_DATA_BITTIMING */
+		size += nla_total_size(sizeof(struct can_bittiming));
+	if (priv->data_bittiming_const)				/* IFLA_CAN_DATA_BITTIMING_CONST */
+		size += nla_total_size(sizeof(struct can_bittiming_const));
 
 	return size;
 }
@@ -769,8 +800,17 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (priv->do_get_berr_counter &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
-	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
+	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
+
+	    (candev_has_bitrate(&priv->data_bittiming) &&
+	     nla_put(skb, IFLA_CAN_DATA_BITTIMING,
+		     sizeof(priv->data_bittiming), &priv->data_bittiming)) ||
+
+	    (priv->data_bittiming_const &&
+	     nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
+		     sizeof(*priv->data_bittiming_const), priv->data_bittiming_const)))
 		return -EMSGSIZE;
+
 	return 0;
 }
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index dc5f902..8adaee9 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -33,8 +33,9 @@ enum can_mode {
 struct can_priv {
 	struct can_device_stats can_stats;
 
-	struct can_bittiming bittiming;
-	const struct can_bittiming_const *bittiming_const;
+	struct can_bittiming bittiming, data_bittiming;
+	const struct can_bittiming_const *bittiming_const,
+		*data_bittiming_const;
 	struct can_clock clock;
 
 	enum can_state state;
@@ -45,6 +46,7 @@ struct can_priv {
 	struct timer_list restart_timer;
 
 	int (*do_set_bittiming)(struct net_device *dev);
+	int (*do_set_data_bittiming)(struct net_device *dev);
 	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
 	int (*do_get_state)(const struct net_device *dev,
 			    enum can_state *state);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index df944ed..b41933d 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -122,6 +122,8 @@ enum {
 	IFLA_CAN_RESTART_MS,
 	IFLA_CAN_RESTART,
 	IFLA_CAN_BERR_COUNTER,
+	IFLA_CAN_DATA_BITTIMING,
+	IFLA_CAN_DATA_BITTIMING_CONST,
 	__IFLA_CAN_MAX
 };
 
-- 
1.9.0

[PATCH v5 5/6] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp]

The configuration for CAN FD depends on CAN_CTRLMODE_FD enabled in the driver
specific ctrlmode_supported capabilities.

The configuration can be done either with the 'fd { on | off }' option in the
'ip' tool from iproute2 or by setting the CAN netdevice MTU to CAN_MTU (16) or
to CANFD_MTU (72).

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c            | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h          |  1 +
 include/uapi/linux/can/netlink.h |  1 +
 3 files changed, 42 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 7887691..3154fe0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -598,6 +598,40 @@ void free_candev(struct net_device *dev)
 EXPORT_SYMBOL_GPL(free_candev);
 
 /*
+ * changing MTU and control mode for CAN/CANFD devices
+ */
+int can_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	/* Do not allow changing the MTU while running */
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	/* allow change of MTU according to the CANFD ability of the device */
+	switch (new_mtu) {
+
+	case CAN_MTU:
+		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
+		break;
+
+	case CANFD_MTU:
+		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
+			return -EINVAL;
+
+		priv->ctrlmode |= CAN_CTRLMODE_FD;
+		break;
+
+	default:
+			return -EINVAL;
+	}
+
+	dev->mtu = new_mtu;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(can_change_mtu);
+
+/*
  * When setting the bitrate both can_calc_bittiming() and can_fixup_bittiming()
  * lead to the bitrate and the tq variable to be set, when a proper bit timing
  * is available.
@@ -731,6 +765,12 @@ static int can_changelink(struct net_device *dev,
 			return -EOPNOTSUPP;
 		priv->ctrlmode &= ~cm->mask;
 		priv->ctrlmode |= cm->flags;
+
+		/* CAN_CTRLMODE_FD can only be set when driver supports FD */
+		if (priv->ctrlmode & CAN_CTRLMODE_FD)
+			dev->mtu = CANFD_MTU;
+		else
+			dev->mtu = CAN_MTU;
 	}
 
 	if (data[IFLA_CAN_RESTART_MS]) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 8adaee9..3ce5e52 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -113,6 +113,7 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
 
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
+int can_change_mtu(struct net_device *dev, int new_mtu);
 
 int register_candev(struct net_device *dev);
 void unregister_candev(struct net_device *dev);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index b41933d..7e2e186 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -96,6 +96,7 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
 #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
 #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
+#define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */
 
 /*
  * CAN device statistics
-- 
1.9.0

[PATCH v5 6/6] can: add bittiming check at interface open for CAN FD

[Oliver Hartkopp]

Additionally to have the second (data) bitrate available the data bitrate
has to be greater or equal to the arbitration bitrate in CAN FD.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3154fe0..561ba8c 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -656,6 +656,14 @@ int open_candev(struct net_device *dev)
 		return -EINVAL;
 	}
 
+	/* For CAN FD the data bitrate has to be >= the arbitration bitrate */
+	if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
+	    (!candev_has_bitrate(&priv->data_bittiming) ||
+	     (priv->data_bittiming.bitrate < priv->bittiming.bitrate))) {
+		netdev_err(dev, "incorrect/missing data bit-timing\n");
+		return -EINVAL;
+	}
+
 	/* Switch carrier on if device was stopped while in bus-off state */
 	if (!netif_carrier_ok(dev))
 		netif_carrier_on(dev);
-- 
1.9.0

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/26/2014 11:11 PM, Oliver Hartkopp wrote:
> As CAN FD offers a second bitrate for the data section of the CAN frame the
> infrastructure for storing and configuring this second bitrate is introduced.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  drivers/net/can/dev.c            | 42 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/can/dev.h          |  6 ++++--
>  include/uapi/linux/can/netlink.h |  2 ++
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 368d92d..7887691 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -660,6 +660,10 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>  				= { .len = sizeof(struct can_bittiming_const) },
>  	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
>  	[IFLA_CAN_BERR_COUNTER]	= { .len = sizeof(struct can_berr_counter) },
> +	[IFLA_CAN_DATA_BITTIMING]
> +				= { .len = sizeof(struct can_bittiming) },
> +	[IFLA_CAN_DATA_BITTIMING_CONST]
> +				= { .len = sizeof(struct can_bittiming_const) },
>  };
>  
>  static int can_changelink(struct net_device *dev,
> @@ -693,6 +697,29 @@ static int can_changelink(struct net_device *dev,
>  		}
>  	}
>  
> +	if (data[IFLA_CAN_DATA_BITTIMING]) {
> +		struct can_bittiming bt;
> +
> +		/* Do not allow changing bittiming while running */
> +		if (dev->flags & IFF_UP)
> +			return -EBUSY;
> +		memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
> +		       sizeof(bt));
> +		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
> +			return -EINVAL;
> +		err = can_get_bittiming(dev, &bt, priv->data_bittiming_const);
> +		if (err)
> +			return err;
> +		memcpy(&priv->data_bittiming, &bt, sizeof(bt));
> +
> +		if (priv->do_set_data_bittiming) {
> +			/* Finally, set the bit-timing registers */
> +			err = priv->do_set_data_bittiming(dev);
> +			if (err)
> +				return err;
> +		}

Do we really need the do_set_data_bittiming() callback? Allmost all
drivers set the bitrate during open and not asynchronously (when the
interface is down) via the set_bittiming() callback. If there is a
driver which really needs this callback, we can introduce it.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

On 27.02.2014 09:43, Marc Kleine-Budde wrote:

>> +		if (priv->do_set_data_bittiming) {
>> +			/* Finally, set the bit-timing registers */
>> +			err = priv->do_set_data_bittiming(dev);
>> +			if (err)
>> +				return err;
>> +		}
> 
> Do we really need the do_set_data_bittiming() callback? Allmost all
> drivers set the bitrate during open and not asynchronously (when the
> interface is down) via the set_bittiming() callback. If there is a
> driver which really needs this callback, we can introduce it.

No. I assume you did not check the current implementation %-)

The bitrate is always set via do_set[_data]_bittiming() when the interface is
down - and *not* during the open process.

In the open process only the availability of the bitrate(s) is checked before
the controller is set into operational mode.

It is pretty straight forward to introduce do_set_data_bittiming() as a
different register set is addressed for CAN FD.

Regards,
Oliver

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/27/2014 07:05 PM, Oliver Hartkopp wrote:
> On 27.02.2014 09:43, Marc Kleine-Budde wrote:
> 
>>> +		if (priv->do_set_data_bittiming) {
>>> +			/* Finally, set the bit-timing registers */
>>> +			err = priv->do_set_data_bittiming(dev);
>>> +			if (err)
>>> +				return err;
>>> +		}
>>
>> Do we really need the do_set_data_bittiming() callback? Allmost all
>> drivers set the bitrate during open and not asynchronously (when the
>> interface is down) via the set_bittiming() callback. If there is a
>> driver which really needs this callback, we can introduce it.
> 
> No. I assume you did not check the current implementation %-)

Yes, but...no :)

> The bitrate is always set via do_set[_data]_bittiming() when the interface is
> down - and *not* during the open process.

No, not _always_. The flexcan and at91 drivers (maybe others, too) write
the values during open().

> In the open process only the availability of the bitrate(s) is checked before
> the controller is set into operational mode.

Yes, by the framework. This is good.

> It is pretty straight forward to introduce do_set_data_bittiming() as a
> different register set is addressed for CAN FD.

The reason why I don't want to introduce a do_set_data_bittiming() is
that it makes the driver more complicated. If you really want to write
it into the registers, you have to power up you CAN core, as on modern
controllers you cannot write to regs if the clocks are turned off. So
you have to turn on the clock, power up your chip, get into the correct
mode, write the bittiming and power down everything again. During open()
you do the same thing again but don't write the bit timing registers. So
better keep things simple.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

On 28.02.2014 11:56, Marc Kleine-Budde wrote:
> On 02/27/2014 07:05 PM, Oliver Hartkopp wrote:
>> On 27.02.2014 09:43, Marc Kleine-Budde wrote:
>>
>>>> +		if (priv->do_set_data_bittiming) {


> 
>> The bitrate is always set via do_set[_data]_bittiming() when the interface is
>> down - and *not* during the open process.
> 
> No, not _always_. The flexcan and at91 drivers (maybe others, too) write
> the values during open().

flexcan does not assign priv->do_set_bittiming() and does the bitrate setting
at open time.

Thats ok too.

This is up to the driver, whether do_set[_data]_bittiming() is assigned and
when the stuff in the bitrate structures is set into the registers.

I don't understand your problem.

Regards,
Oliver

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/28/2014 01:20 PM, Oliver Hartkopp wrote:
> 
> 
> On 28.02.2014 11:56, Marc Kleine-Budde wrote:
>> On 02/27/2014 07:05 PM, Oliver Hartkopp wrote:
>>> On 27.02.2014 09:43, Marc Kleine-Budde wrote:
>>>
>>>>> +		if (priv->do_set_data_bittiming) {
> 
> 
>>
>>> The bitrate is always set via do_set[_data]_bittiming() when the interface is
>>> down - and *not* during the open process.
>>
>> No, not _always_. The flexcan and at91 drivers (maybe others, too) write
>> the values during open().
> 
> flexcan does not assign priv->do_set_bittiming() and does the bitrate setting
> at open time.
> 
> Thats ok too.
> 
> This is up to the driver, whether do_set[_data]_bittiming() is assigned and
> when the stuff in the bitrate structures is set into the registers.
> 
> I don't understand your problem.

A new do_set_data_bittiming() callback is not needed, as it's better do
set the data and "normal" bit timing registers during open().

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

On 28.02.2014 13:30, Marc Kleine-Budde wrote:
> On 02/28/2014 01:20 PM, Oliver Hartkopp wrote:
>>
>>
>> On 28.02.2014 11:56, Marc Kleine-Budde wrote:
>>> On 02/27/2014 07:05 PM, Oliver Hartkopp wrote:
>>>> On 27.02.2014 09:43, Marc Kleine-Budde wrote:
>>>>
>>>>>> +		if (priv->do_set_data_bittiming) {
>>
>>
>>>
>>>> The bitrate is always set via do_set[_data]_bittiming() when the interface is
>>>> down - and *not* during the open process.
>>>
>>> No, not _always_. The flexcan and at91 drivers (maybe others, too) write
>>> the values during open().
>>
>> flexcan does not assign priv->do_set_bittiming() and does the bitrate setting
>> at open time.
>>
>> Thats ok too.
>>
>> This is up to the driver, whether do_set[_data]_bittiming() is assigned and
>> when the stuff in the bitrate structures is set into the registers.
>>
>> I don't understand your problem.
> 
> A new do_set_data_bittiming() callback is not needed, as it's better do
> set the data and "normal" bit timing registers during open().

With this argumentation do_set_bittiming() has to be removed too.

Is this your intention?

Regards,
Oliver

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/28/2014 01:33 PM, Oliver Hartkopp wrote:
> 
> 
> On 28.02.2014 13:30, Marc Kleine-Budde wrote:
>> On 02/28/2014 01:20 PM, Oliver Hartkopp wrote:
>>>
>>>
>>> On 28.02.2014 11:56, Marc Kleine-Budde wrote:
>>>> On 02/27/2014 07:05 PM, Oliver Hartkopp wrote:
>>>>> On 27.02.2014 09:43, Marc Kleine-Budde wrote:
>>>>>
>>>>>>> +		if (priv->do_set_data_bittiming) {
>>>
>>>
>>>>
>>>>> The bitrate is always set via do_set[_data]_bittiming() when the interface is
>>>>> down - and *not* during the open process.
>>>>
>>>> No, not _always_. The flexcan and at91 drivers (maybe others, too) write
>>>> the values during open().
>>>
>>> flexcan does not assign priv->do_set_bittiming() and does the bitrate setting
>>> at open time.
>>>
>>> Thats ok too.
>>>
>>> This is up to the driver, whether do_set[_data]_bittiming() is assigned and
>>> when the stuff in the bitrate structures is set into the registers.
>>>
>>> I don't understand your problem.
>>
>> A new do_set_data_bittiming() callback is not needed, as it's better do
>> set the data and "normal" bit timing registers during open().
> 
> With this argumentation do_set_bittiming() has to be removed too.
> 
> Is this your intention?

Yes. But it's in many drivers so let's keep it, for now.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

On 28.02.2014 13:52, Marc Kleine-Budde wrote:
> On 02/28/2014 01:33 PM, Oliver Hartkopp wrote:


>>> A new do_set_data_bittiming() callback is not needed, as it's better do
>>> set the data and "normal" bit timing registers during open().
>>
>> With this argumentation do_set_bittiming() has to be removed too.
>>
>> Is this your intention?
> 
> Yes. But it's in many drivers so let's keep it, for now.
> 

Good.

Btw. this could be a general simplification topic for the future.

I do not have any objections to remove the callbacks in can_priv and call the
device specific functions for the (data) bittiming setting in the open
function then.

Regards,
Oliver

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/28/2014 01:58 PM, Oliver Hartkopp wrote:
>>>> A new do_set_data_bittiming() callback is not needed, as it's better do
>>>> set the data and "normal" bit timing registers during open().
>>>
>>> With this argumentation do_set_bittiming() has to be removed too.
>>>
>>> Is this your intention?
>>
>> Yes. But it's in many drivers so let's keep it, for now.
>>
> 
> Good.
> 
> Btw. this could be a general simplification topic for the future.

Yes, removing do_set_bittiming() is a different topic.

> I do not have any objections to remove the callbacks in can_priv and call the
> device specific functions for the (data) bittiming setting in the open
> function then.

\o/

Can you can an Acked-by from Kurt for the whole series?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

On 28.02.2014 14:05, Marc Kleine-Budde wrote:
> On 02/28/2014 01:58 PM, Oliver Hartkopp wrote:


>> Btw. this could be a general simplification topic for the future.
> 
> Yes, removing do_set_bittiming() is a different topic.
> 
>> I do not have any objections to remove the callbacks in can_priv and call the
>> device specific functions for the (data) bittiming setting in the open
>> function then.
> 
> \o/
> 
> Can you can an Acked-by from Kurt for the whole series?

I assume you talk about Stephane ?!?

I'll send out a v8 for him for a final review.

Tnx!

Oliver

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/28/2014 02:09 PM, Oliver Hartkopp wrote:
> On 28.02.2014 14:05, Marc Kleine-Budde wrote:
>> On 02/28/2014 01:58 PM, Oliver Hartkopp wrote:
> 
> 
>>> Btw. this could be a general simplification topic for the future.
>>
>> Yes, removing do_set_bittiming() is a different topic.
>>
>>> I do not have any objections to remove the callbacks in can_priv and call the
>>> device specific functions for the (data) bittiming setting in the open
>>> function then.
>>
>> \o/
>>
>> Can you can an Acked-by from Kurt for the whole series?
> 
> I assume you talk about Stephane ?!?

Yes, of course. Thanks for following my thoughts:)

> I'll send out a v8 for him for a final review.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v5 4/6] can: introduce the data bitrate configuration for CAN FD

[Marc Kleine-Budde]

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

On 02/28/2014 02:05 PM, Marc Kleine-Budde wrote:
> On 02/28/2014 01:58 PM, Oliver Hartkopp wrote:
>>>>> A new do_set_data_bittiming() callback is not needed, as it's better do
>>>>> set the data and "normal" bit timing registers during open().
>>>>
>>>> With this argumentation do_set_bittiming() has to be removed too.
>>>>
>>>> Is this your intention?
>>>
>>> Yes. But it's in many drivers so let's keep it, for now.
>>>
>>
>> Good.
>>
>> Btw. this could be a general simplification topic for the future.
> 
> Yes, removing do_set_bittiming() is a different topic.
> 
>> I do not have any objections to remove the callbacks in can_priv and call the
>> device specific functions for the (data) bittiming setting in the open
>> function then.

It's still present in v9, I'll send a v10 which removes the
"priv->do_set_data_bittiming()" callback.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]