Hi Oliver,

Thanks for the review comments.

On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:

quoted

Changes since v1:
	* Removed testmodes & debugfs code (suggested by Oliver H)
	* Fixed tx path race issue by introducing lock (suggested by Marc K)
	* Removed __maybe_unused attribute of rcar_canfd_of_table

quoted

 obj-$(CONFIG_CAN_M_CAN)		+= m_can/
 obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
+obj-$(CONFIG_CANFD_RCAR)	+= rcar_canfd.o

Just for the sake of an ordered directory structure:

Would it make sense to create a rcar directory where rcar_can.c and
rcar_canfd.c are placed?

Yes. I'll place them in rcar dir.

Additionally (besides of one accident with the obsolete PCH_CAN) all CAN
drivers start with CONFIG_CAN_...

Following that scheme the config option should be named

	CONFIG_CAN_RCAR_CANFD

as we had in CONFIG_CAN_IFI_CANFD.

Agreed.

quoted

 obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
 obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
 obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o

diff --git a/drivers/net/can/rcar_canfd.c

b/drivers/net/can/rcar_canfd.c

(..)

quoted

+/* Channel priv data */
+struct rcar_canfd_channel {
+	struct can_priv can;			/* Must be the first member */
+	struct net_device *ndev;
+	struct rcar_canfd_global *gpriv;	/* Controller reference */
+	void __iomem *base;			/* Register base address */
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *dev_root;
+#endif /* CONFIG_DEBUG_FS */

Some missed stuff from debugfs removal?

:-(. Sorry. Will clean up.

quoted

+	struct napi_struct napi;
+	u8  tx_len[RCANFD_FIFO_DEPTH];		/* For net stats */
+	u32 tx_head;				/* Incremented on xmit */
+	u32 tx_tail;				/* Incremented on xmit done */
+	u32 channel;				/* Channel number */
+	spinlock_t tx_lock;			/* To protect tx path */
+};

(..)

quoted

+static int rcar_canfd_start(struct net_device *ndev) {
+	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	int err = -EOPNOTSUPP;
+	u32 sts, ch = priv->channel;
+	u32 ridx = ch + RCANFD_RFFIFO_IDX;
+
+	/* Ensure channel starts in FD mode */
+	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
+		netdev_err(ndev, "enable can fd mode for channel %d\n", ch);
+		goto fail_mode;
+	}

What's the reason behind this check?

A CAN FD capable CAN controller can be either configured to run as CAN 2.0
(Classic CAN) or as CAN FD controller.

So why are to throwing an error here and produce an initialization
failure?

When this controller is configured in FD mode and used only with CAN 2.0 nodes, it still expects a DTSEG (data bitrate) configuration same as NTSEG (nominal bitrate). This check, specifically in ndo_open, ensures both are configured and will work fine with CAN 2.0 nodes (e.g.)

"ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on"

If I don't have this check, a configuration like this

"ip link set can0 up type can bitrate 1000000"

will bring up the controller without DTSEG configured. 

Thanks,
Ramesh