[PATCH v2 0/6] can: tcan4x5x: Introduce tcan4552/4553

[PATCH v2 0/6] can: tcan4x5x: Introduce tcan4552/4553

[Markus Schneider-Pargmann]

Hi everyone,

This series introduces two new chips tcan-4552 and tcan-4553. The
generic driver works in general but needs a few small changes. These are
caused by the removal of wake and state pins.

In v2 I updated the bindings to use tcan4x5x always as a fallback. The
driver now uses the first more specific binding if available. If the
given binding does not match the chip that is present, a warning is
printed and the correct driver data is loaded instead.

Based on v6.4-rc1.

Best,
Markus

Changes in v2:
- Update the binding documentation to specify tcan4552 and tcan4553 with
  the tcan4x5x as fallback
- Update the driver to use auto detection as well. If compatible differs
  from the ID2 register, use the ID2 register and print a warning.
- Small style changes

Previous versions:
v1 - https://lore.kernel.org/lkml/20230314151201.2317134-1-msp@baylibre.com

Markus Schneider-Pargmann (6):
  dt-bindings: can: tcan4x5x: Add tcan4552 and tcan4553 variants
  can: tcan4x5x: Remove reserved register 0x814 from writable table
  can: tcan4x5x: Check size of mram configuration
  can: tcan4x5x: Rename ID registers to match datasheet
  can: tcan4x5x: Add support for tcan4552/4553
  can: tcan4x5x: Add error messages in probe

 .../devicetree/bindings/net/can/tcan4x5x.txt  |  11 +-
 drivers/net/can/m_can/m_can.c                 |  16 ++
 drivers/net/can/m_can/m_can.h                 |   1 +
 drivers/net/can/m_can/tcan4x5x-core.c         | 161 ++++++++++++++----
 drivers/net/can/m_can/tcan4x5x-regmap.c       |   1 -
 5 files changed, 155 insertions(+), 35 deletions(-)


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.40.1

[PATCH v2 1/6] dt-bindings: can: tcan4x5x: Add tcan4552 and tcan4553 variants

[Markus Schneider-Pargmann]

These two new chips do not have state or wake pins.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 .../devicetree/bindings/net/can/tcan4x5x.txt          | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
index e3501bfa22e9..170e23f0610d 100644
--- a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
+++ b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
@@ -4,7 +4,10 @@ Texas Instruments TCAN4x5x CAN Controller
 This file provides device node information for the TCAN4x5x interface contains.
 
 Required properties:
-	- compatible: "ti,tcan4x5x"
+	- compatible:
+		"ti,tcan4552", "ti,tcan4x5x"
+		"ti,tcan4553", "ti,tcan4x5x" or
+		"ti,tcan4x5x"
 	- reg: 0
 	- #address-cells: 1
 	- #size-cells: 0
@@ -21,8 +24,10 @@ Optional properties:
 	- reset-gpios: Hardwired output GPIO. If not defined then software
 		       reset.
 	- device-state-gpios: Input GPIO that indicates if the device is in
-			      a sleep state or if the device is active.
-	- device-wake-gpios: Wake up GPIO to wake up the TCAN device.
+			      a sleep state or if the device is active. Not
+			      available with tcan4552/4553.
+	- device-wake-gpios: Wake up GPIO to wake up the TCAN device. Not
+			     available with tcan4552/4553.
 
 Example:
 tcan4x5x: tcan4x5x@0 {
-- 
2.40.1

Re: [PATCH v2 1/6] dt-bindings: can: tcan4x5x: Add tcan4552 and tcan4553 variants

[Krzysztof Kozlowski]

On 21/06/2023 11:30, Markus Schneider-Pargmann wrote:
> These two new chips do not have state or wake pins.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  .../devicetree/bindings/net/can/tcan4x5x.txt          | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> index e3501bfa22e9..170e23f0610d 100644
> --- a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> +++ b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> @@ -4,7 +4,10 @@ Texas Instruments TCAN4x5x CAN Controller
>  This file provides device node information for the TCAN4x5x interface contains.
>  
>  Required properties:
> -	- compatible: "ti,tcan4x5x"
> +	- compatible:
> +		"ti,tcan4552", "ti,tcan4x5x"
> +		"ti,tcan4553", "ti,tcan4x5x" or
> +		"ti,tcan4x5x"

One more example why wildcards are not allowed. Ehh,


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: can: tcan4x5x: Add tcan4552 and tcan4553 variants

[Krzysztof Kozlowski]

On 21/06/2023 11:30, Markus Schneider-Pargmann wrote:
> These two new chips do not have state or wake pins.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

BTW, why did you ignore the tag?

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions. However, there's no need to repost patches *only* to add the
tags. The upstream maintainer will do that for acks received on the
version they apply.

https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540

If a tag was not added on purpose, please state why and what changed.

Best regards,
Krzysztof

Re: [PATCH v2 1/6] dt-bindings: can: tcan4x5x: Add tcan4552 and tcan4553 variants

[Markus Schneider-Pargmann]

Hi Krzysztof,

On Wed, Jun 21, 2023 at 12:29:40PM +0200, Krzysztof Kozlowski wrote:
> On 21/06/2023 11:30, Markus Schneider-Pargmann wrote:
> > These two new chips do not have state or wake pins.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> 
> BTW, why did you ignore the tag?

Thanks for your Acked-by.

I did not add it from v1 because:
- We had a long discussion after you sent your tag
- I changed the binding documentation according to the discussion on v1
  as stated in the cover letter:
    "- Update the binding documentation to specify tcan4552 and tcan4553 with
       the tcan4x5x as fallback"

Thanks,
Markus

[PATCH v2 2/6] can: tcan4x5x: Remove reserved register 0x814 from writable table

[Markus Schneider-Pargmann]

The mentioned register is not writable. It is reserved and should not be
written.

Fixes: 39dbb21b6a29 ("can: tcan4x5x: Specify separate read/write ranges")
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
---
 drivers/net/can/m_can/tcan4x5x-regmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-regmap.c b/drivers/net/can/m_can/tcan4x5x-regmap.c
index 2b218ce04e9f..fafa6daa67e6 100644
--- a/drivers/net/can/m_can/tcan4x5x-regmap.c
+++ b/drivers/net/can/m_can/tcan4x5x-regmap.c
@@ -95,7 +95,6 @@ static const struct regmap_range tcan4x5x_reg_table_wr_range[] = {
 	regmap_reg_range(0x000c, 0x0010),
 	/* Device configuration registers and Interrupt Flags*/
 	regmap_reg_range(0x0800, 0x080c),
-	regmap_reg_range(0x0814, 0x0814),
 	regmap_reg_range(0x0820, 0x0820),
 	regmap_reg_range(0x0830, 0x0830),
 	/* M_CAN */
-- 
2.40.1

[PATCH v2 3/6] can: tcan4x5x: Check size of mram configuration

[Markus Schneider-Pargmann]

To reduce debugging effort in case the mram is misconfigured, add this
size check of the DT configuration. Currently if the mram configuration
doesn't fit into the available MRAM it just overwrites other areas of
the MRAM.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
---
 drivers/net/can/m_can/m_can.c         | 16 ++++++++++++++++
 drivers/net/can/m_can/m_can.h         |  1 +
 drivers/net/can/m_can/tcan4x5x-core.c |  5 +++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a5003435802b..1cdd2675f022 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1887,6 +1887,22 @@ static int register_m_can_dev(struct net_device *dev)
 	return register_candev(dev);
 }
 
+int m_can_check_mram_cfg(struct m_can_classdev *cdev, u32 mram_max_size)
+{
+	u32 total_size;
+
+	total_size = cdev->mcfg[MRAM_TXB].off - cdev->mcfg[MRAM_SIDF].off +
+			cdev->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
+	if (total_size > mram_max_size) {
+		dev_err(cdev->dev, "Total size of mram config(%u) exceeds mram(%u)\n",
+			total_size, mram_max_size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(m_can_check_mram_cfg);
+
 static void m_can_of_parse_mram(struct m_can_classdev *cdev,
 				const u32 *mram_config_vals)
 {
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index a839dc71dc9b..d8150d8128e7 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -101,6 +101,7 @@ int m_can_class_register(struct m_can_classdev *cdev);
 void m_can_class_unregister(struct m_can_classdev *cdev);
 int m_can_class_get_clocks(struct m_can_classdev *cdev);
 int m_can_init_ram(struct m_can_classdev *priv);
+int m_can_check_mram_cfg(struct m_can_classdev *cdev, u32 mram_max_size);
 
 int m_can_class_suspend(struct device *dev);
 int m_can_class_resume(struct device *dev);
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 2342aa011647..e706518176e4 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -80,6 +80,7 @@
 	 TCAN4X5X_MCAN_IR_RF1F)
 
 #define TCAN4X5X_MRAM_START 0x8000
+#define TCAN4X5X_MRAM_SIZE 0x800
 #define TCAN4X5X_MCAN_OFFSET 0x1000
 
 #define TCAN4X5X_CLEAR_ALL_INT 0xffffffff
@@ -307,6 +308,10 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 	if (!mcan_class)
 		return -ENOMEM;
 
+	ret = m_can_check_mram_cfg(mcan_class, TCAN4X5X_MRAM_SIZE);
+	if (ret)
+		goto out_m_can_class_free_dev;
+
 	priv = cdev_to_priv(mcan_class);
 
 	priv->power = devm_regulator_get_optional(&spi->dev, "vsup");
-- 
2.40.1

[PATCH v2 4/6] can: tcan4x5x: Rename ID registers to match datasheet

[Markus Schneider-Pargmann]

The datasheet calls these registers ID1 and ID2. Rename these to avoid
confusion.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index e706518176e4..fb9375fa20ec 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -6,8 +6,8 @@
 
 #define TCAN4X5X_EXT_CLK_DEF 40000000
 
-#define TCAN4X5X_DEV_ID0 0x00
-#define TCAN4X5X_DEV_ID1 0x04
+#define TCAN4X5X_DEV_ID1 0x00
+#define TCAN4X5X_DEV_ID2 0x04
 #define TCAN4X5X_REV 0x08
 #define TCAN4X5X_STATUS 0x0C
 #define TCAN4X5X_ERROR_STATUS_MASK 0x10
-- 
2.40.1

[PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Markus Schneider-Pargmann]

tcan4552 and tcan4553 do not have wake or state pins, so they are
currently not compatible with the generic driver. The generic driver
uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
are not defined. These functions use register bits that are not
available in tcan4552/4553.

This patch adds support by introducing version information to reflect if
the chip has wake and state pins. Also the version is now checked.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 128 +++++++++++++++++++++-----
 1 file changed, 104 insertions(+), 24 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index fb9375fa20ec..756acd122075 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -7,6 +7,7 @@
 #define TCAN4X5X_EXT_CLK_DEF 40000000
 
 #define TCAN4X5X_DEV_ID1 0x00
+#define TCAN4X5X_DEV_ID1_TCAN 0x4e414354 /* ASCII TCAN */
 #define TCAN4X5X_DEV_ID2 0x04
 #define TCAN4X5X_REV 0x08
 #define TCAN4X5X_STATUS 0x0C
@@ -103,6 +104,13 @@
 #define TCAN4X5X_WD_3_S_TIMER BIT(29)
 #define TCAN4X5X_WD_6_S_TIMER (BIT(28) | BIT(29))
 
+struct tcan4x5x_version_info {
+	u32 id2_register;
+
+	bool has_wake_pin;
+	bool has_state_pin;
+};
+
 static inline struct tcan4x5x_priv *cdev_to_priv(struct m_can_classdev *cdev)
 {
 	return container_of(cdev, struct tcan4x5x_priv, cdev);
@@ -254,18 +262,68 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
 				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
 }
 
-static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
+static const struct tcan4x5x_version_info tcan4x5x_generic;
+static const struct of_device_id tcan4x5x_of_match[];
+
+static const struct tcan4x5x_version_info
+*tcan4x5x_find_version_info(struct tcan4x5x_priv *priv, u32 id2_value)
+{
+	for (int i = 0; tcan4x5x_of_match[i].data; ++i) {
+		const struct tcan4x5x_version_info *vinfo =
+			tcan4x5x_of_match[i].data;
+		if (!vinfo->id2_register || id2_value == vinfo->id2_register) {
+			dev_warn(&priv->spi->dev, "TCAN device is %s, please use it in DT\n",
+				 tcan4x5x_of_match[i].compatible);
+			return vinfo;
+		}
+	}
+
+	return &tcan4x5x_generic;
+}
+
+static int tcan4x5x_verify_version(struct tcan4x5x_priv *priv,
+				   const struct tcan4x5x_version_info **info)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read(priv->regmap, TCAN4X5X_DEV_ID1, &val);
+	if (ret)
+		return ret;
+
+	if (val != TCAN4X5X_DEV_ID1_TCAN) {
+		dev_err(&priv->spi->dev, "Not a tcan device %x\n", val);
+		return -ENODEV;
+	}
+
+	if (!(*info)->id2_register)
+		return 0;
+
+	ret = regmap_read(priv->regmap, TCAN4X5X_DEV_ID2, &val);
+	if (ret)
+		return ret;
+
+	if ((*info)->id2_register != val)
+		*info = tcan4x5x_find_version_info(priv, val);
+
+	return 0;
+}
+
+static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
+			      const struct tcan4x5x_version_info *version_info)
 {
 	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
 	int ret;
 
-	tcan4x5x->device_wake_gpio = devm_gpiod_get(cdev->dev, "device-wake",
-						    GPIOD_OUT_HIGH);
-	if (IS_ERR(tcan4x5x->device_wake_gpio)) {
-		if (PTR_ERR(tcan4x5x->device_wake_gpio) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+	if (version_info->has_wake_pin) {
+		tcan4x5x->device_wake_gpio = devm_gpiod_get(cdev->dev, "device-wake",
+							    GPIOD_OUT_HIGH);
+		if (IS_ERR(tcan4x5x->device_wake_gpio)) {
+			if (PTR_ERR(tcan4x5x->device_wake_gpio) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 
-		tcan4x5x_disable_wake(cdev);
+			tcan4x5x_disable_wake(cdev);
+		}
 	}
 
 	tcan4x5x->reset_gpio = devm_gpiod_get_optional(cdev->dev, "reset",
@@ -277,12 +335,14 @@ static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
-	tcan4x5x->device_state_gpio = devm_gpiod_get_optional(cdev->dev,
-							      "device-state",
-							      GPIOD_IN);
-	if (IS_ERR(tcan4x5x->device_state_gpio)) {
-		tcan4x5x->device_state_gpio = NULL;
-		tcan4x5x_disable_state(cdev);
+	if (version_info->has_state_pin) {
+		tcan4x5x->device_state_gpio = devm_gpiod_get_optional(cdev->dev,
+								      "device-state",
+								      GPIOD_IN);
+		if (IS_ERR(tcan4x5x->device_state_gpio)) {
+			tcan4x5x->device_state_gpio = NULL;
+			tcan4x5x_disable_state(cdev);
+		}
 	}
 
 	return 0;
@@ -299,10 +359,15 @@ static struct m_can_ops tcan4x5x_ops = {
 
 static int tcan4x5x_can_probe(struct spi_device *spi)
 {
+	const struct tcan4x5x_version_info *version_info;
 	struct tcan4x5x_priv *priv;
 	struct m_can_classdev *mcan_class;
 	int freq, ret;
 
+	version_info = of_device_get_match_data(&spi->dev);
+	if (!version_info)
+		version_info = (void *)spi_get_device_id(spi)->driver_data;
+
 	mcan_class = m_can_class_allocate_dev(&spi->dev,
 					      sizeof(struct tcan4x5x_priv));
 	if (!mcan_class)
@@ -361,7 +426,11 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 	if (ret)
 		goto out_m_can_class_free_dev;
 
-	ret = tcan4x5x_get_gpios(mcan_class);
+	ret = tcan4x5x_verify_version(priv, &version_info);
+	if (ret)
+		goto out_power;
+
+	ret = tcan4x5x_get_gpios(mcan_class, version_info);
 	if (ret)
 		goto out_power;
 
@@ -394,21 +463,32 @@ static void tcan4x5x_can_remove(struct spi_device *spi)
 	m_can_class_free_dev(priv->cdev.net);
 }
 
+static const struct tcan4x5x_version_info tcan4x5x_generic = {
+	.has_state_pin = true,
+	.has_wake_pin = true,
+};
+
+static const struct tcan4x5x_version_info tcan4x5x_tcan4552 = {
+	.id2_register = 0x32353534, /* ASCII = 4552 */
+};
+
+static const struct tcan4x5x_version_info tcan4x5x_tcan4553 = {
+	.id2_register = 0x33353534, /* ASCII = 4553 */
+};
+
 static const struct of_device_id tcan4x5x_of_match[] = {
-	{
-		.compatible = "ti,tcan4x5x",
-	}, {
-		/* sentinel */
-	},
+	{ .compatible = "ti,tcan4552", .data = &tcan4x5x_tcan4552 },
+	{ .compatible = "ti,tcan4553", .data = &tcan4x5x_tcan4553 },
+	{ .compatible = "ti,tcan4x5x", .data = &tcan4x5x_generic },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, tcan4x5x_of_match);
 
 static const struct spi_device_id tcan4x5x_id_table[] = {
-	{
-		.name = "tcan4x5x",
-	}, {
-		/* sentinel */
-	},
+	{ .name = "tcan4x5x", .driver_data = (unsigned long)&tcan4x5x_generic, },
+	{ .name = "tcan4552", .driver_data = (unsigned long)&tcan4x5x_tcan4552, },
+	{ .name = "tcan4553", .driver_data = (unsigned long)&tcan4x5x_tcan4553, },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, tcan4x5x_id_table);
 
-- 
2.40.1

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Krzysztof Kozlowski]

On 21/06/2023 11:31, Markus Schneider-Pargmann wrote:
> tcan4552 and tcan4553 do not have wake or state pins, so they are
> currently not compatible with the generic driver. The generic driver
> uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
> are not defined. These functions use register bits that are not
> available in tcan4552/4553.
> 
> This patch adds support by introducing version information to reflect if
> the chip has wake and state pins. Also the version is now checked.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 128 +++++++++++++++++++++-----
>  1 file changed, 104 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index fb9375fa20ec..756acd122075 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -7,6 +7,7 @@
>  #define TCAN4X5X_EXT_CLK_DEF 40000000
>  
>  #define TCAN4X5X_DEV_ID1 0x00
> +#define TCAN4X5X_DEV_ID1_TCAN 0x4e414354 /* ASCII TCAN */
>  #define TCAN4X5X_DEV_ID2 0x04
>  #define TCAN4X5X_REV 0x08
>  #define TCAN4X5X_STATUS 0x0C
> @@ -103,6 +104,13 @@
>  #define TCAN4X5X_WD_3_S_TIMER BIT(29)
>  #define TCAN4X5X_WD_6_S_TIMER (BIT(28) | BIT(29))
>  
> +struct tcan4x5x_version_info {
> +	u32 id2_register;
> +
> +	bool has_wake_pin;
> +	bool has_state_pin;
> +};
> +
>  static inline struct tcan4x5x_priv *cdev_to_priv(struct m_can_classdev *cdev)
>  {
>  	return container_of(cdev, struct tcan4x5x_priv, cdev);
> @@ -254,18 +262,68 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
>  				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
>  }
>  
> -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
> +static const struct tcan4x5x_version_info tcan4x5x_generic;
> +static const struct of_device_id tcan4x5x_of_match[];
> +
> +static const struct tcan4x5x_version_info
> +*tcan4x5x_find_version_info(struct tcan4x5x_priv *priv, u32 id2_value)
> +{
> +	for (int i = 0; tcan4x5x_of_match[i].data; ++i) {
> +		const struct tcan4x5x_version_info *vinfo =
> +			tcan4x5x_of_match[i].data;
> +		if (!vinfo->id2_register || id2_value == vinfo->id2_register) {
> +			dev_warn(&priv->spi->dev, "TCAN device is %s, please use it in DT\n",
> +				 tcan4x5x_of_match[i].compatible);
> +			return vinfo;
> +		}
> +	}
> +
> +	return &tcan4x5x_generic;

I don't understand what do you want to achieve here. Kernel job is not
to validate DTB, so if DTB says you have 4552, there is no need to
double check. On the other hand, you have Id register so entire idea of
custom compatibles can be dropped and instead you should detect the
variant based on the ID.

Best regards,
Krzysztof

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Markus Schneider-Pargmann]

Hi Krzysztof,

On Wed, Jun 21, 2023 at 12:28:34PM +0200, Krzysztof Kozlowski wrote:
> On 21/06/2023 11:31, Markus Schneider-Pargmann wrote:
> > tcan4552 and tcan4553 do not have wake or state pins, so they are
> > currently not compatible with the generic driver. The generic driver
> > uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
> > are not defined. These functions use register bits that are not
> > available in tcan4552/4553.
> > 
> > This patch adds support by introducing version information to reflect if
> > the chip has wake and state pins. Also the version is now checked.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >  drivers/net/can/m_can/tcan4x5x-core.c | 128 +++++++++++++++++++++-----
> >  1 file changed, 104 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> > index fb9375fa20ec..756acd122075 100644
> > --- a/drivers/net/can/m_can/tcan4x5x-core.c
> > +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> > @@ -7,6 +7,7 @@
> >  #define TCAN4X5X_EXT_CLK_DEF 40000000
> >  
> >  #define TCAN4X5X_DEV_ID1 0x00
> > +#define TCAN4X5X_DEV_ID1_TCAN 0x4e414354 /* ASCII TCAN */
> >  #define TCAN4X5X_DEV_ID2 0x04
> >  #define TCAN4X5X_REV 0x08
> >  #define TCAN4X5X_STATUS 0x0C
> > @@ -103,6 +104,13 @@
> >  #define TCAN4X5X_WD_3_S_TIMER BIT(29)
> >  #define TCAN4X5X_WD_6_S_TIMER (BIT(28) | BIT(29))
> >  
> > +struct tcan4x5x_version_info {
> > +	u32 id2_register;
> > +
> > +	bool has_wake_pin;
> > +	bool has_state_pin;
> > +};
> > +
> >  static inline struct tcan4x5x_priv *cdev_to_priv(struct m_can_classdev *cdev)
> >  {
> >  	return container_of(cdev, struct tcan4x5x_priv, cdev);
> > @@ -254,18 +262,68 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
> >  				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
> >  }
> >  
> > -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
> > +static const struct tcan4x5x_version_info tcan4x5x_generic;
> > +static const struct of_device_id tcan4x5x_of_match[];
> > +
> > +static const struct tcan4x5x_version_info
> > +*tcan4x5x_find_version_info(struct tcan4x5x_priv *priv, u32 id2_value)
> > +{
> > +	for (int i = 0; tcan4x5x_of_match[i].data; ++i) {
> > +		const struct tcan4x5x_version_info *vinfo =
> > +			tcan4x5x_of_match[i].data;
> > +		if (!vinfo->id2_register || id2_value == vinfo->id2_register) {
> > +			dev_warn(&priv->spi->dev, "TCAN device is %s, please use it in DT\n",
> > +				 tcan4x5x_of_match[i].compatible);
> > +			return vinfo;
> > +		}
> > +	}
> > +
> > +	return &tcan4x5x_generic;
> 
> I don't understand what do you want to achieve here. Kernel job is not
> to validate DTB, so if DTB says you have 4552, there is no need to
> double check. On the other hand, you have Id register so entire idea of
> custom compatibles can be dropped and instead you should detect the
> variant based on the ID.

I can read the ID register but tcan4552 and 4553 do not have two
devicetree properties that tcan4550 has, namely state and wake gpios.
See v1 discussion about that [1].

In v1 Marc pointed out that mcp251xfd is using an autodetection and warn
mechanism which I implemented here as well. [2]

Best,
Markus


[1] https://lore.kernel.org/lkml/5f9fe7fb-9483-7dee-82c8-bd6564abcaab@linaro.org/
[2] https://lore.kernel.org/lkml/20230315112905.qutggrdnpsttbase@pengutronix.de/

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Krzysztof Kozlowski]

On 21/06/2023 14:31, Markus Schneider-Pargmann wrote:
> Hi Krzysztof,
> 
> On Wed, Jun 21, 2023 at 12:28:34PM +0200, Krzysztof Kozlowski wrote:
>> On 21/06/2023 11:31, Markus Schneider-Pargmann wrote:
>>> tcan4552 and tcan4553 do not have wake or state pins, so they are
>>> currently not compatible with the generic driver. The generic driver
>>> uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
>>> are not defined. These functions use register bits that are not
>>> available in tcan4552/4553.
>>>
>>> This patch adds support by introducing version information to reflect if
>>> the chip has wake and state pins. Also the version is now checked.
>>>
>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>> ---
>>>  drivers/net/can/m_can/tcan4x5x-core.c | 128 +++++++++++++++++++++-----
>>>  1 file changed, 104 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
>>> index fb9375fa20ec..756acd122075 100644
>>> --- a/drivers/net/can/m_can/tcan4x5x-core.c
>>> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
>>> @@ -7,6 +7,7 @@
>>>  #define TCAN4X5X_EXT_CLK_DEF 40000000
>>>  
>>>  #define TCAN4X5X_DEV_ID1 0x00
>>> +#define TCAN4X5X_DEV_ID1_TCAN 0x4e414354 /* ASCII TCAN */
>>>  #define TCAN4X5X_DEV_ID2 0x04
>>>  #define TCAN4X5X_REV 0x08
>>>  #define TCAN4X5X_STATUS 0x0C
>>> @@ -103,6 +104,13 @@
>>>  #define TCAN4X5X_WD_3_S_TIMER BIT(29)
>>>  #define TCAN4X5X_WD_6_S_TIMER (BIT(28) | BIT(29))
>>>  
>>> +struct tcan4x5x_version_info {
>>> +	u32 id2_register;
>>> +
>>> +	bool has_wake_pin;
>>> +	bool has_state_pin;
>>> +};
>>> +
>>>  static inline struct tcan4x5x_priv *cdev_to_priv(struct m_can_classdev *cdev)
>>>  {
>>>  	return container_of(cdev, struct tcan4x5x_priv, cdev);
>>> @@ -254,18 +262,68 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
>>>  				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
>>>  }
>>>  
>>> -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
>>> +static const struct tcan4x5x_version_info tcan4x5x_generic;
>>> +static const struct of_device_id tcan4x5x_of_match[];
>>> +
>>> +static const struct tcan4x5x_version_info
>>> +*tcan4x5x_find_version_info(struct tcan4x5x_priv *priv, u32 id2_value)
>>> +{
>>> +	for (int i = 0; tcan4x5x_of_match[i].data; ++i) {
>>> +		const struct tcan4x5x_version_info *vinfo =
>>> +			tcan4x5x_of_match[i].data;
>>> +		if (!vinfo->id2_register || id2_value == vinfo->id2_register) {
>>> +			dev_warn(&priv->spi->dev, "TCAN device is %s, please use it in DT\n",
>>> +				 tcan4x5x_of_match[i].compatible);
>>> +			return vinfo;
>>> +		}
>>> +	}
>>> +
>>> +	return &tcan4x5x_generic;
>>
>> I don't understand what do you want to achieve here. Kernel job is not
>> to validate DTB, so if DTB says you have 4552, there is no need to
>> double check. On the other hand, you have Id register so entire idea of
>> custom compatibles can be dropped and instead you should detect the
>> variant based on the ID.
> 
> I can read the ID register but tcan4552 and 4553 do not have two
> devicetree properties that tcan4550 has, namely state and wake gpios.

Does not matter, you don't use OF matching to then differentiate
handling of GPIOs to then read the register. You first read registers,
so everything is auto-detectable.

> See v1 discussion about that [1].

Yeah, but your code is different, although maybe we just misunderstood
each other. You wrote that you cannot use the GPIOs, so I assumed you
need to know the variant before using the GPIOs. Then you need
compatibles. It's not the case here. You can read the variant and based
on this skip entirely GPIOs as they are entirely missing.

> 
> In v1 Marc pointed out that mcp251xfd is using an autodetection and warn
> mechanism which I implemented here as well. [2]

But why? Just read the ID and detect the variant based on this. Your DT
still can have separate compatibles followed by fallback, that's not a
problem.


Best regards,
Krzysztof

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Markus Schneider-Pargmann]

Hi Krzysztof,

On Wed, Jun 21, 2023 at 03:00:39PM +0200, Krzysztof Kozlowski wrote:
> On 21/06/2023 14:31, Markus Schneider-Pargmann wrote:
> > Hi Krzysztof,
> > 
> > On Wed, Jun 21, 2023 at 12:28:34PM +0200, Krzysztof Kozlowski wrote:
> >> On 21/06/2023 11:31, Markus Schneider-Pargmann wrote:
> >>> tcan4552 and tcan4553 do not have wake or state pins, so they are
> >>> currently not compatible with the generic driver. The generic driver
> >>> uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
> >>> are not defined. These functions use register bits that are not
> >>> available in tcan4552/4553.
> >>>
> >>> This patch adds support by introducing version information to reflect if
> >>> the chip has wake and state pins. Also the version is now checked.
> >>>
> >>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> >>> ---
> >>>  drivers/net/can/m_can/tcan4x5x-core.c | 128 +++++++++++++++++++++-----
> >>>  1 file changed, 104 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> >>> index fb9375fa20ec..756acd122075 100644
> >>> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> >>> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> >>> @@ -7,6 +7,7 @@
> >>>  #define TCAN4X5X_EXT_CLK_DEF 40000000
> >>>  
> >>>  #define TCAN4X5X_DEV_ID1 0x00
> >>> +#define TCAN4X5X_DEV_ID1_TCAN 0x4e414354 /* ASCII TCAN */
> >>>  #define TCAN4X5X_DEV_ID2 0x04
> >>>  #define TCAN4X5X_REV 0x08
> >>>  #define TCAN4X5X_STATUS 0x0C
> >>> @@ -103,6 +104,13 @@
> >>>  #define TCAN4X5X_WD_3_S_TIMER BIT(29)
> >>>  #define TCAN4X5X_WD_6_S_TIMER (BIT(28) | BIT(29))
> >>>  
> >>> +struct tcan4x5x_version_info {
> >>> +	u32 id2_register;
> >>> +
> >>> +	bool has_wake_pin;
> >>> +	bool has_state_pin;
> >>> +};
> >>> +
> >>>  static inline struct tcan4x5x_priv *cdev_to_priv(struct m_can_classdev *cdev)
> >>>  {
> >>>  	return container_of(cdev, struct tcan4x5x_priv, cdev);
> >>> @@ -254,18 +262,68 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
> >>>  				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
> >>>  }
> >>>  
> >>> -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
> >>> +static const struct tcan4x5x_version_info tcan4x5x_generic;
> >>> +static const struct of_device_id tcan4x5x_of_match[];
> >>> +
> >>> +static const struct tcan4x5x_version_info
> >>> +*tcan4x5x_find_version_info(struct tcan4x5x_priv *priv, u32 id2_value)
> >>> +{
> >>> +	for (int i = 0; tcan4x5x_of_match[i].data; ++i) {
> >>> +		const struct tcan4x5x_version_info *vinfo =
> >>> +			tcan4x5x_of_match[i].data;
> >>> +		if (!vinfo->id2_register || id2_value == vinfo->id2_register) {
> >>> +			dev_warn(&priv->spi->dev, "TCAN device is %s, please use it in DT\n",
> >>> +				 tcan4x5x_of_match[i].compatible);
> >>> +			return vinfo;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return &tcan4x5x_generic;
> >>
> >> I don't understand what do you want to achieve here. Kernel job is not
> >> to validate DTB, so if DTB says you have 4552, there is no need to
> >> double check. On the other hand, you have Id register so entire idea of
> >> custom compatibles can be dropped and instead you should detect the
> >> variant based on the ID.
> > 
> > I can read the ID register but tcan4552 and 4553 do not have two
> > devicetree properties that tcan4550 has, namely state and wake gpios.
> 
> Does not matter, you don't use OF matching to then differentiate
> handling of GPIOs to then read the register. You first read registers,
> so everything is auto-detectable.
> 
> > See v1 discussion about that [1].
> 
> Yeah, but your code is different, although maybe we just misunderstood
> each other. You wrote that you cannot use the GPIOs, so I assumed you
> need to know the variant before using the GPIOs. Then you need
> compatibles. It's not the case here. You can read the variant and based
> on this skip entirely GPIOs as they are entirely missing.

The version information is always readable for that chip, regardless of
state and wake GPIOs as far as I know. So yes it is possible to setup
the GPIOs based on the content of the ID register.

I personally would prefer separate compatibles. The binding
documentation needs to address that wake and state GPIOs are not
available for tcan4552/4553. I think having compatibles that are for
these chips would make sense then. However this is my opinion, you are
the maintainer.

Best,
Markus

> 
> > 
> > In v1 Marc pointed out that mcp251xfd is using an autodetection and warn
> > mechanism which I implemented here as well. [2]
> 
> But why? Just read the ID and detect the variant based on this. Your DT
> still can have separate compatibles followed by fallback, that's not a
> problem.
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Krzysztof Kozlowski]

On 22/06/2023 14:23, Markus Schneider-Pargmann wrote:
>>
>> Yeah, but your code is different, although maybe we just misunderstood
>> each other. You wrote that you cannot use the GPIOs, so I assumed you
>> need to know the variant before using the GPIOs. Then you need
>> compatibles. It's not the case here. You can read the variant and based
>> on this skip entirely GPIOs as they are entirely missing.
> 
> The version information is always readable for that chip, regardless of
> state and wake GPIOs as far as I know. So yes it is possible to setup
> the GPIOs based on the content of the ID register.
> 
> I personally would prefer separate compatibles. The binding
> documentation needs to address that wake and state GPIOs are not
> available for tcan4552/4553. I think having compatibles that are for
> these chips would make sense then. However this is my opinion, you are
> the maintainer.

We do not talk about compatibles in the bindings here. This is
discussion about your driver. The entire logic of validating DTB is
flawed and not needed. Detect the variant and act based on this.

Best regards,
Krzysztof

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Markus Schneider-Pargmann]

Hi Krzysztof,

On Thu, Jun 22, 2023 at 02:52:48PM +0200, Krzysztof Kozlowski wrote:
> On 22/06/2023 14:23, Markus Schneider-Pargmann wrote:
> >>
> >> Yeah, but your code is different, although maybe we just misunderstood
> >> each other. You wrote that you cannot use the GPIOs, so I assumed you
> >> need to know the variant before using the GPIOs. Then you need
> >> compatibles. It's not the case here. You can read the variant and based
> >> on this skip entirely GPIOs as they are entirely missing.
> > 
> > The version information is always readable for that chip, regardless of
> > state and wake GPIOs as far as I know. So yes it is possible to setup
> > the GPIOs based on the content of the ID register.
> > 
> > I personally would prefer separate compatibles. The binding
> > documentation needs to address that wake and state GPIOs are not
> > available for tcan4552/4553. I think having compatibles that are for
> > these chips would make sense then. However this is my opinion, you are
> > the maintainer.
> 
> We do not talk about compatibles in the bindings here. This is
> discussion about your driver. The entire logic of validating DTB is
> flawed and not needed. Detect the variant and act based on this.

I thought it was about the bindings, sorry.

So to summarize the compatibles ti,tcan4552 and ti,tcan4553 are fine.
But the driver should use the ID register for detection and not compare
the detected variant with the given compatible?

In my opinion it is useful to have an error messages that says there is
something wrong with the devicetree as this can be very helpful for the
developers who bringup new devices. This helps to quickly find issues
with the devicetree.

@Marc: What do you think? Maybe I misinterpreted your mail from last
version?

Best,
Markus

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Krzysztof Kozlowski]

On 27/06/2023 16:23, Markus Schneider-Pargmann wrote:

>>> The version information is always readable for that chip, regardless of
>>> state and wake GPIOs as far as I know. So yes it is possible to setup
>>> the GPIOs based on the content of the ID register.
>>>
>>> I personally would prefer separate compatibles. The binding
>>> documentation needs to address that wake and state GPIOs are not
>>> available for tcan4552/4553. I think having compatibles that are for
>>> these chips would make sense then. However this is my opinion, you are
>>> the maintainer.
>>
>> We do not talk about compatibles in the bindings here. This is
>> discussion about your driver. The entire logic of validating DTB is
>> flawed and not needed. Detect the variant and act based on this.
> 
> I thought it was about the bindings, sorry.
> 
> So to summarize the compatibles ti,tcan4552 and ti,tcan4553 are fine.
> But the driver should use the ID register for detection and not compare
> the detected variant with the given compatible?
> 
> In my opinion it is useful to have an error messages that says there is
> something wrong with the devicetree as this can be very helpful for the
> developers who bringup new devices. This helps to quickly find issues
> with the devicetree.

That's not a current policy for other drivers, so this shouldn't be
really special. Kernel is poor in validating DTS. It's not its job. It's
the job of the DT schema.

Best regards,
Krzysztof

Re: [PATCH v2 5/6] can: tcan4x5x: Add support for tcan4552/4553

[Marc Kleine-Budde]

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

On 01.07.2023 10:34:00, Krzysztof Kozlowski wrote:
> On 27/06/2023 16:23, Markus Schneider-Pargmann wrote:
> 
> >>> The version information is always readable for that chip, regardless of
> >>> state and wake GPIOs as far as I know. So yes it is possible to setup
> >>> the GPIOs based on the content of the ID register.
> >>>
> >>> I personally would prefer separate compatibles. The binding
> >>> documentation needs to address that wake and state GPIOs are not
> >>> available for tcan4552/4553. I think having compatibles that are for
> >>> these chips would make sense then. However this is my opinion, you are
> >>> the maintainer.
> >>
> >> We do not talk about compatibles in the bindings here. This is
> >> discussion about your driver. The entire logic of validating DTB is
> >> flawed and not needed. Detect the variant and act based on this.
> > 
> > I thought it was about the bindings, sorry.
> > 
> > So to summarize the compatibles ti,tcan4552 and ti,tcan4553 are fine.
> > But the driver should use the ID register for detection and not compare
> > the detected variant with the given compatible?
> > 
> > In my opinion it is useful to have an error messages that says there is
> > something wrong with the devicetree as this can be very helpful for the
> > developers who bringup new devices. This helps to quickly find issues
> > with the devicetree.
> 
> That's not a current policy for other drivers, so this shouldn't be
> really special. Kernel is poor in validating DTS. It's not its job. It's
> the job of the DT schema.

Fine with me.

I decided to have a check of the auto-detected chip variant against the
specified one in the mcp251xfd driver, as it widely used with raspi
boards, where commonly DT overlays are used. It also helps remote
diagnostics of people, who don't focus on kernel development.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 6/6] can: tcan4x5x: Add error messages in probe

[Markus Schneider-Pargmann]

To be able to understand issues during probe easier, add error messages
if something fails.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 756acd122075..e30faa1cf893 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -397,6 +397,8 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 
 	/* Sanity check */
 	if (freq < 20000000 || freq > TCAN4X5X_EXT_CLK_DEF) {
+		dev_err(&spi->dev, "Clock frequency is out of supported range %d\n",
+			freq);
 		ret = -ERANGE;
 		goto out_m_can_class_free_dev;
 	}
@@ -415,32 +417,44 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 	/* Configure the SPI bus */
 	spi->bits_per_word = 8;
 	ret = spi_setup(spi);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "SPI setup failed %d\n", ret);
 		goto out_m_can_class_free_dev;
+	}
 
 	ret = tcan4x5x_regmap_init(priv);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "regmap init failed %d\n", ret);
 		goto out_m_can_class_free_dev;
+	}
 
 	ret = tcan4x5x_power_enable(priv->power, 1);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "Enabling regulator failed %d\n", ret);
 		goto out_m_can_class_free_dev;
+	}
 
 	ret = tcan4x5x_verify_version(priv, &version_info);
 	if (ret)
 		goto out_power;
 
 	ret = tcan4x5x_get_gpios(mcan_class, version_info);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "Getting gpios failed %d\n", ret);
 		goto out_power;
+	}
 
 	ret = tcan4x5x_init(mcan_class);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "tcan initialization failed %d\n", ret);
 		goto out_power;
+	}
 
 	ret = m_can_class_register(mcan_class);
-	if (ret)
+	if (ret) {
+		dev_err(&spi->dev, "Failed registering m_can device %d\n", ret);
 		goto out_power;
+	}
 
 	netdev_info(mcan_class->net, "TCAN4X5X successfully initialized.\n");
 	return 0;
-- 
2.40.1