Hi!

More about CAN related projects used and developed at the Faculty
of the Electrical Engineering (http://www.fel.cvut.cz/en/)
of Czech Technical University (https://www.cvut.cz/en)
in at Prague http://canbus.pages.fel.cvut.cz/ .

Should this go to Documentation, not a changelog?

+	help
+	  This driver adds support for the CTU CAN FD open-source IP core.
+	  More documentation and core sources at project page
+	  (https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core).
+	  The core integration to Xilinx Zynq system as platform driver
+	  is available (https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top).
+	  Implementation on Intel FGA based PCI Express board is available
+	  from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
+	  More about CAN related projects used and developed at the Faculty
+	  of the Electrical Engineering (http://www.fel.cvut.cz/en/)
+	  of Czech Technical University in Prague (https://www.cvut.cz/en)
+	  at http://canbus.pages.fel.cvut.cz/ .

Again, links to university main mage here are little bit excessive.

+// SPDX-License-Identifier: GPL-2.0-or-later
+/*******************************************************************************
+ *
+ * CTU CAN FD IP Core
+ * Copyright (C) 2015-2018
+ *
+ * Authors:
+ *     Ondrej Ille [off-list ref]
+ *     Martin Jerabek [off-list ref]
+ *     Jaroslav Beran [off-list ref]
+ *
+ * Project advisors:
+ *     Jiri Novak [off-list ref]
+ *     Pavel Pisa [off-list ref]
+ *
+ * Department of Measurement         (http://meas.fel.cvut.cz/)
+ * Faculty of Electrical Engineering (http://www.fel.cvut.cz)
+ * Czech Technical University        (http://www.cvut.cz/)

Again, a bit too many links... and important information is missing:
who is the copyright holder?

+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.

With SPDX, this should be removed.

+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Martin Jerabek");
+MODULE_DESCRIPTION("CTU CAN FD interface");

This normally goes to end of the file, MODULE_AUTHOR usually has <>
section with email address.

+/* TX buffer rotation:
+ * - when a buffer transitions to empty state, rotate order and priorities
+ * - if more buffers seem to transition at the same time, rotate
+ *   by the number of buffers
+ * - it may be assumed that buffers transition to empty state in FIFO order
+ *   (because we manage priorities that way)
+ * - at frame filling, do not rotate anything, just increment buffer modulo
+ *   counter
+ */
+
+#define CTUCAN_FLAG_RX_FFW_BUFFERED	1
+
+static int ctucan_reset(struct net_device *ndev)
+{
+	int i;
+	struct ctucan_priv *priv = netdev_priv(ndev);
+
+	netdev_dbg(ndev, "ctucan_reset");
+
+	ctucan_hw_reset(&priv->p);
+	for (i = 0; i < 100; ++i) {

i++ would be usual kernel style.

+		if (ctucan_hw_check_access(&priv->p))
+			return 0;
+		usleep_range(100, 200);
+	}
+
+	netdev_warn(ndev, "device did not leave reset\n");
+	return -ETIMEDOUT;
+}

This does extra sleep after last check_access... but I doubt that
matters.

+static int ctucan_set_bittiming(struct net_device *ndev)
+{
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+
+	netdev_dbg(ndev, "ctucan_set_bittiming");
+
+	if (ctucan_hw_is_enabled(&priv->p)) {
+		netdev_alert(ndev,
+			     "BUG! Cannot set bittiming - CAN is enabled\n");
+		return -EPERM;
+	}

alert is normally reserved for .. way higher severity.

+	/* Note that bt may be modified here */
+	ctucan_hw_set_nom_bittiming(&priv->p, bt);
+
+	return 0;
+}
+
+/**
+ * ctucan_set_data_bittiming - CAN set data bit timing routine
+ * @ndev:	Pointer to net_device structure
+ *
+ * This is the driver set data bittiming routine.
+ * Return: 0 on success and failure value on error
+ */
+static int ctucan_set_data_bittiming(struct net_device *ndev)
+{
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *dbt = &priv->can.data_bittiming;
+
+	netdev_dbg(ndev, "ctucan_set_data_bittiming");
+
+	if (ctucan_hw_is_enabled(&priv->p)) {
+		netdev_alert(ndev,
+			     "BUG! Cannot set bittiming - CAN is enabled\n");
+		return -EPERM;
+	}
+
+	/* Note that dbt may be modified here */
+	ctucan_hw_set_data_bittiming(&priv->p, dbt);
+
+	return 0;
+}
+
+/**
+ * ctucan_set_secondary_sample_point - CAN set secondary sample point routine
+ * @ndev:	Pointer to net_device structure
+ *
+ * Return: 0 on success and failure value on error
+ */
+static int ctucan_set_secondary_sample_point(struct net_device *ndev)
+{
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	struct can_bittiming *dbt = &priv->can.data_bittiming;
+	int ssp_offset = 0;
+	bool ssp_ena;
+
+	netdev_dbg(ndev, "ctucan_set_secondary_sample_point");
+
+	if (ctucan_hw_is_enabled(&priv->p)) {
+		netdev_alert(ndev,
+			     "BUG! Cannot set SSP - CAN is enabled\n");
+		return -EPERM;
+	}
+
+	// Use for bit-rates above 1 Mbits/s

/* */ style comment would be more common here.

+	if (dbt->bitrate > 1000000) {
+		ssp_ena = true;
+
+		// Calculate SSP in minimal time quanta
+		ssp_offset = (priv->can.clock.freq / 1000) *
+			      dbt->sample_point / dbt->bitrate;
+
+		if (ssp_offset > 127) {
+			netdev_warn(ndev, "SSP offset saturated to 127\n");
+			ssp_offset = 127;
+		}
+	} else {
+		ssp_ena = false;
+	}

Init ssp_ena to false, and you can get rid of else branch?

+/**
+ * ctucan_chip_start - This the drivers start routine
+ * @ndev:	Pointer to net_device structure
+ *
+ * This is the drivers start routine.

driver's?

+/**
+ * ctucan_start_xmit - Starts the transmission

...

+ * Return: 0 on success and failure value on error

Umm, no, AFAICT.

+static int ctucan_start_xmit(struct sk_buff *skb, struct net_device *ndev)

Should it return netdev_tx_t ?

+/**
+ * xcan_rx -  Is called from CAN isr to complete the received
+ *		frame  processing

Double space.

+static const char *ctucan_state_to_str(enum can_state state)
+{
+	if (state >= CAN_STATE_MAX)
+		return "UNKNOWN";
+	return ctucan_state_strings[state];
+}

Is enum always unsigned?

+/**
+ * ctucan_err_interrupt - error frame Isr
+ * @ndev:	net_device pointer
+ * @isr:	interrupt status register value
+ *
+ * This is the CAN error interrupt and it will
+ * check the the type of error and forward the error
+ * frame to upper layers.
+ */

You are free to use 80 columns...

+	skb = alloc_can_err_skb(ndev, &cf);
+
+	/* EWLI:  error warning limit condition met
+	 * FCSI: Fault confinement State changed
+	 * ALI:  arbitration lost (just informative)
+	 * BEI:  bus error interrupt
+	 */

Extra space before "error"... and something is wrong with big letters there.

+		if (state == CAN_STATE_BUS_OFF) {
+			priv->can.can_stats.bus_off++;
+			can_bus_off(ndev);
+			if (skb)
+				cf->can_id |= CAN_ERR_BUSOFF;
+		} else if (state == CAN_STATE_ERROR_PASSIVE) {
+			priv->can.can_stats.error_passive++;
+			if (skb) {
+				cf->can_id |= CAN_ERR_CRTL;
+				cf->data[1] = (berr.rxerr > 127) ?
+						CAN_ERR_CRTL_RX_PASSIVE :
+						CAN_ERR_CRTL_TX_PASSIVE;
+				cf->data[6] = berr.txerr;
+				cf->data[7] = berr.rxerr;
+			}
+		} else if (state == CAN_STATE_ERROR_WARNING) {
+			priv->can.can_stats.error_warning++;
+			if (skb) {
+				cf->can_id |= CAN_ERR_CRTL;
+				cf->data[1] |= (berr.txerr > berr.rxerr) ?
+					CAN_ERR_CRTL_TX_WARNING :
+					CAN_ERR_CRTL_RX_WARNING;
+				cf->data[6] = berr.txerr;
+				cf->data[7] = berr.rxerr;
+			}
+		} else if (state == CAN_STATE_ERROR_ACTIVE) {
+			cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+			cf->data[6] = berr.txerr;
+			cf->data[7] = berr.rxerr;
+		} else {
+			netdev_warn(ndev, "    unhandled error state (%d:%s)!",
+				    state, ctucan_state_to_str(state));
+		}

This sounds like switch (state) to me?

+	/* Check for Bus Error interrupt */
+	if (isr.s.bei) {
+		netdev_info(ndev, "  bus error");
+		priv->can.can_stats.bus_error++;
+		stats->tx_errors++; // TODO: really?

really? :-).

+		some_buffers_processed = false;
+		while ((int)(priv->txb_head - priv->txb_tail) > 0) {
+			u32 txb_idx = priv->txb_tail & priv->txb_mask;
+			u32 status = ctucan_hw_get_tx_status(&priv->p, txb_idx);
+
+			netdev_dbg(ndev, "TXI: TXB#%u: status 0x%x",
+				   txb_idx, status);
+
+			switch (status) {
+			case TXT_TOK:
+				netdev_dbg(ndev, "TXT_OK");
+				can_get_echo_skb(ndev, txb_idx);
+				stats->tx_packets++;
+				break;
+			case TXT_ERR:
+				/* This indicated that retransmit limit has been
+				 * reached. Obviously we should not echo the
+				 * frame, but also not indicate any kind
+				 * of error. If desired, it was already reported
+				 * (possible multiple times) on each arbitration
+				 * lost.
+				 */
+				netdev_warn(ndev, "TXB in Error state");
+				can_free_echo_skb(ndev, txb_idx);
+				stats->tx_dropped++;
+				break;
+			case TXT_ABT:
+				/* Same as for TXT_ERR, only with different
+				 * cause. We *could* re-queue the frame, but
+				 * multiqueue/abort is not supported yet anyway.
+				 */
+				netdev_warn(ndev, "TXB in Aborted state");
+				can_free_echo_skb(ndev, txb_idx);
+				stats->tx_dropped++;
+				break;
+			default:
+				/* Bug only if the first buffer is not finished,
+				 * otherwise it is pretty much expected
+				 */
+				if (first) {
+					netdev_err(ndev, "BUG: TXB#%u not in a finished state (0x%x)!",
+						   txb_idx, status);
+					spin_unlock_irqrestore(&priv->tx_lock,
+							       flags);
+					/* do not clear nor wake */
+					return;
+				}
+				goto clear;
+			}
+			priv->txb_tail++;
+			first = false;
+			some_buffers_processed = true;
+			/* Adjust priorities *before* marking the buffer
+			 * as empty.
+			 */
+			ctucan_rotate_txb_prio(ndev);
+			ctucan_hw_txt_set_empty(&priv->p, txb_idx);
+		}
+clear:
+		spin_unlock_irqrestore(&priv->tx_lock, flags);

Could some part of this be split into separate function?

+		/* If no buffers were processed this time, wa cannot

we

+		 * clear - that would introduce a race condition.
+		 */
+		if (some_buffers_processed) {
+			/* Clear the interrupt again as not to receive it again
+			 * for a buffer we already handled (possibly causing
+			 * the bug log)
+			 */

Not sure this is correct english.

+static irqreturn_t ctucan_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = (struct net_device *)dev_id;
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	union ctu_can_fd_int_stat isr, icr;
+	int irq_loops = 0;
+
+	netdev_dbg(ndev, "ctucan_interrupt");
+
+	do {
+		/* Get the interrupt status */
+		isr = ctu_can_fd_int_sts(&priv->p);

+		}
+		/* Ignore RI, TI, LFI, RFI, BSI */
+	} while (irq_loops++ < 10000);

For loop would be more usual here.

+static void ctucan_chip_stop(struct net_device *ndev)
+{
+	struct ctucan_priv *priv = netdev_priv(ndev);
+	union ctu_can_fd_int_stat mask;
+
+	netdev_dbg(ndev, "ctucan_chip_stop");
+
+	mask.u32 = 0xffffffff;
+
+	/* Disable interrupts and disable can */

can->CAN?

+	netdev_dbg(ndev, "ctucan_open");
+
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+			   __func__, ret);
+		return ret;
+	}

IIRC pm_runtime_get... is special, and you need to put even in the
error case.

+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+			   __func__, ret);
+		return ret;

Same here.

+int ctucan_suspend(struct device *dev)
+EXPORT_SYMBOL(ctucan_suspend);
+int ctucan_resume(struct device *dev)
+EXPORT_SYMBOL(ctucan_resume);

Exporting these is quite unusual.

+int ctucan_probe_common(struct device *dev, void __iomem *addr,
+			int irq, unsigned int ntxbufs, unsigned long can_clk_rate,
+			int pm_enable_call, void (*set_drvdata_fnc)(struct device *dev,
+			struct net_device *ndev))

At least format it so that set_drvdata_fnc is on single line?

+{

...

+	if (set_drvdata_fnc != NULL)
+		set_drvdata_fnc(dev, ndev);

No need for != NULL.

+	SET_NETDEV_DEV(ndev, dev);
+	ndev->netdev_ops = &ctucan_netdev_ops;
+
+	/* Getting the CAN can_clk info */
+	if (can_clk_rate == 0) {

!can_clk_rate would work, too.

+		priv->can_clk = devm_clk_get(dev, NULL);
+		if (IS_ERR(priv->can_clk)) {
+			dev_err(dev, "Device clock not found.\n");
+			ret = PTR_ERR(priv->can_clk);
+			goto err_free;
+		}
+		can_clk_rate = clk_get_rate(priv->can_clk);
+	}
+
+	priv->p.write_reg = ctucan_hw_write32;
+	priv->p.read_reg = ctucan_hw_read32;
+
+	if (pm_enable_call)
+		pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {

put?

+	if ((priv->p.read_reg(&priv->p, CTU_CAN_FD_DEVICE_ID) &
+			    0xFFFF) != CTU_CAN_FD_ID) {
+		priv->p.write_reg = ctucan_hw_write32_be;
+		priv->p.read_reg = ctucan_hw_read32_be;
+		if ((priv->p.read_reg(&priv->p, CTU_CAN_FD_DEVICE_ID) &
+			      0xFFFF) != CTU_CAN_FD_ID) {
+			netdev_err(ndev, "CTU_CAN_FD signature not found\n");
+			ret = -ENODEV;
+			goto err_disableclks;
+		}
+	}
+
+	ret = ctucan_reset(ndev);
+	if (ret < 0)
+		goto err_pmdisable;

Should be goto err_disableclks, AFAICT. Plus that label is quite confusing.

+static __init int ctucan_init(void)
+{
+	pr_info("%s CAN netdevice driver\n", DRV_NAME);
+
+	return 0;
+}
+
+module_init(ctucan_init);

+static __exit void ctucan_exit(void)
+{
+	pr_info("%s: driver removed\n", DRV_NAME);
+}
+
+module_exit(ctucan_exit);

Remove?

+#ifndef __KERNEL__
+# include "ctu_can_fd_linux_defs.h"
+#else
+# include <linux/can/dev.h>
+#endif

Should always assume kernel?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html