mbox series

[v4,0/6] CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation

Message ID cover.1596408856.git.pisa@cmp.felk.cvut.cz
Headers show
Series CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation | expand

Message

Pavel Pisa Aug. 3, 2020, 6:34 p.m. UTC
From: Pavel Pisa <pisa@cmp.felk.cvut.cz>

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 FPGA based PCI Express board is available
from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
The CTU CAN FD core emulation send for review for QEMU mainline.
Development repository for QEMU emulation - ctu-canfd branch of
  https://gitlab.fel.cvut.cz/canbus/qemu-canbus

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 Prague at http://canbus.pages.fel.cvut.cz/ .

Martin Jerabek (1):
  can: ctucanfd: add support for CTU CAN FD open-source IP core - bus
    independent part.

Pavel Pisa (5):
  dt-bindings: vendor-prefix: add prefix for the Czech Technical
    University in Prague.
  dt-bindings: net: can: binding for CTU CAN FD open-source IP core.
  can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.
  can: ctucanfd: CTU CAN FD open-source IP core - platform/SoC support.
  docs: ctucanfd: CTU CAN FD open-source IP core documentation.

The version 4 changes:
  - changes summary, 169 non-merge commits, 6 driver,
    32 IP core sources enhancements and fixes, 58 tests
    in master and about additional 30 iso-testbench
    preparation branch.
  - convert device-tree binding documentation to YAML
  - QEMU model of CTU CAN FD IP core and generic extension
    of QEMU CAN bus emulation developed by Jan Charvat.
  - driver tested on QEMU emulated Malta big-endian MIPS
    platform and big endian-support fixed.
  - checkpatch from 5.4 kernel used to cleanup driver formatting
  - header files generated from IP core IP-Xact description
    updated to include protocol exception (pex) field.
    Mechanism to set it from the driver is not provided yet.

The version 3 changes:
  - sent at 2019-12-21
  - adapts device tree bindings documentation according to
    Rob Herring suggestions.
  - the driver has been separated to individual modules for core support,
    PCI bus integration and platform, SoC integration.
  - the FPGA design has been cleaned up and CAN protocol FSM redesigned
    by Ondrej Ille (the core redesign has been reason to pause attempts to driver
    submission)
  - the work from February 2019 on core, test framework and driver
    1601 commits in total, 436 commits in the core sources, 144 commits
    in the driver, 151 documentation, 502 in tests.
  - not all continuous integration tests updated for latest design version yet
    https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/pipelines
  - Zynq hardware in the loop test show no issues for after driver PCI and platform
    separation and latest VHDL sources updates.
  - driver code has been periodically tested on 4.18.5-rt3 and 4.19 long term
    stable kernels.
  - test of the patches before submission is run on 5.4 kernel
  - the core has been integrated by Jaroslav Beran <jara.beran@gmail.com>
    into Intel FPGA based SoC used in the tester developed for Skoda auto
    at Department of Measurement, Faculty of Electrical Engineering,
    Czech Technical University https://meas.fel.cvut.cz/ . He has contributed
    feedback and fixes to the project.

The version 2 sent at 2019-02-27

The version 1 sent at 2019-02-22

Ondrej Ille has prepared the CTU CAN IP Core sources for new release.
We are waiting with it for the driver review, our intention
is to release IP when driver is reviewed and mainlined.

DKMS CTU CAN FD driver build by OpenBuildService to ease integration
into Debian systems when driver is not provided by the distribution

https://build.opensuse.org/package/show/home:ppisa/ctu_can_fd

Jan Charvat <charvj10@fel.cvut.cz> finished work to extend already
mainlined QEMU SJA1000 and SocketCAN support to provide even CAN FD
support and CTU CAN FD core support.

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/-/tree/ctu-canfd

The patches has been sent for review to QEMU mainlining list.

Thanks in advance to all who help us to deliver the project into public.

Thanks to all colleagues, reviewers and other providing feedback,
infrastructure and enthusiasm and motivation for open-source work.

Build infrastructure and hardware is provided by
  Department of Control Engineering,
  Faculty of Electrical Engineering,
  Czech Technical University in Prague
  https://dce.fel.cvut.cz/en

 .../devicetree/bindings/net/can/ctu,ctucanfd.yaml  |   70 ++
 .../devicetree/bindings/vendor-prefixes.yaml       |    2 +
 .../device_drivers/ctu/FSM_TXT_Buffer_user.png     |  Bin 0 -> 174807 bytes
 .../device_drivers/ctu/ctucanfd-driver.rst         |  635 +++++++++++
 drivers/net/can/Kconfig                            |    1 +
 drivers/net/can/Makefile                           |    1 +
 drivers/net/can/ctucanfd/Kconfig                   |   38 +
 drivers/net/can/ctucanfd/Makefile                  |   13 +
 drivers/net/can/ctucanfd/ctu_can_fd.c              | 1114 ++++++++++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd.h              |   88 ++
 drivers/net/can/ctucanfd/ctu_can_fd_frame.h        |  190 ++++
 drivers/net/can/ctucanfd/ctu_can_fd_hw.c           |  781 ++++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd_hw.h           |  917 ++++++++++++++++
 drivers/net/can/ctucanfd/ctu_can_fd_pci.c          |  316 ++++++
 drivers/net/can/ctucanfd/ctu_can_fd_platform.c     |  145 +++
 drivers/net/can/ctucanfd/ctu_can_fd_regs.h         |  972 +++++++++++++++++
 16 files changed, 5283 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
 create mode 100644 Documentation/networking/device_drivers/ctu/FSM_TXT_Buffer_user.png
 create mode 100644 Documentation/networking/device_drivers/ctu/ctucanfd-driver.rst
 create mode 100644 drivers/net/can/ctucanfd/Kconfig
 create mode 100644 drivers/net/can/ctucanfd/Makefile
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_frame.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_hw.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_hw.h
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_pci.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_platform.c
 create mode 100644 drivers/net/can/ctucanfd/ctu_can_fd_regs.h

Comments

Randy Dunlap Aug. 3, 2020, 7:32 p.m. UTC | #1
On 8/3/20 11:34 AM, pisa@cmp.felk.cvut.cz wrote:
> diff --git a/drivers/net/can/ctucanfd/Kconfig b/drivers/net/can/ctucanfd/Kconfig
> index 0620111d57fd..8a5f5d05fa72 100644
> --- a/drivers/net/can/ctucanfd/Kconfig
> +++ b/drivers/net/can/ctucanfd/Kconfig
> @@ -15,4 +15,13 @@ config CAN_CTUCANFD
>  
>  if CAN_CTUCANFD
>  
> +config CAN_CTUCANFD_PCI
> +    tristate "CTU CAN-FD IP core PCI/PCIe driver"
> +    depends on PCI
> +	help

"help" should be indented with one tab only (no spaces).

> +	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
> +	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
> +	  PCIe board with PiKRON.com designed transceiver riser shield is available
> +	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .

help text should be indented with one tab + 2 spaces according to
Documentation/process/coding-style.rst.

> +
>  endif

thanks.
Jakub Kicinski Aug. 3, 2020, 8:29 p.m. UTC | #2
On Mon,  3 Aug 2020 20:34:48 +0200 pisa@cmp.felk.cvut.cz wrote:
> From: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> 
> 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 FPGA based PCI Express board is available
> from project (https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd).
> The CTU CAN FD core emulation send for review for QEMU mainline.
> Development repository for QEMU emulation - ctu-canfd branch of
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus
> 
> 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 Prague at http://canbus.pages.fel.cvut.cz/ .

Patches 3 and 4 have warnings when built with W=1 C=1 flags.

Please also remove the uses of static inline in C sources.
Those are rarely necessary and hide unused code warnings.
Pavel Machek Aug. 4, 2020, 9:57 a.m. UTC | #3
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 <ondrej.ille@gmail.com>
> + *     Martin Jerabek <martin.jerabek01@gmail.com>
> + *     Jaroslav Beran <jara.beran@gmail.com>
> + *
> + * Project advisors:
> + *     Jiri Novak <jnovak@fel.cvut.cz>
> + *     Pavel Pisa <pisa@cmp.felk.cvut.cz>
> + *
> + * 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