LinuxLists.cc - [PATCH 0/5] can: tcan4x5x: Introduce tcan4552/4553

2023-03-14 15:12:28

Subject: [PATCH 0/5] can: tcan4x5x: Introduce tcan4552/4553

Hi Marc and 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.

I included two patches from the optimization series and will remove them
from the optimization series. Hopefully it avoids conflicts and not
polute the other series with tcan4552/4553 stuff.

Best,
Markus

optimization series:
https://lore.kernel.org/lkml/[email protected]

Markus Schneider-Pargmann (5):
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

.../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 | 122 ++++++++++++++----
drivers/net/can/m_can/tcan4x5x-regmap.c | 1 -
5 files changed, 121 insertions(+), 30 deletions(-)

--
2.39.2

2023-03-14 15:12:29

by Markus Schneider-Pargmann

[permalink] [raw]

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

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

Signed-off-by: Markus Schneider-Pargmann <[email protected]>
---
.../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..38a2b5369b44 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,tcan4x5x" or
+ "ti,tcan4552" or
+ "ti,tcan4553"
- 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.39.2

2023-03-14 15:12:33

by Markus Schneider-Pargmann

[permalink] [raw]

Subject: [PATCH 3/5] can: tcan4x5x: Check size of mram configuration

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 <[email protected]>
---
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 8e83d6963d85..8ccf20f093f8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1886,6 +1886,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.39.2

2023-03-14 15:12:35

by Markus Schneider-Pargmann

[permalink] [raw]

Subject: [PATCH 4/5] can: tcan4x5x: Rename ID registers to match datasheet

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

Signed-off-by: Markus Schneider-Pargmann <[email protected]>
---
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.39.2

2023-03-14 15:12:39

by Markus Schneider-Pargmann

[permalink] [raw]

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

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 <[email protected]>
---
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.39.2

2023-03-14 15:12:42

by Markus Schneider-Pargmann

[permalink] [raw]

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

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
---
drivers/net/can/m_can/tcan4x5x-core.c | 113 ++++++++++++++++++++------
1 file changed, 89 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..e7fa509dacc9 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,53 @@ 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 int tcan4x5x_verify_version(
+ struct tcan4x5x_priv *priv,
+ const struct tcan4x5x_version_info *version_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 (!version_info->id2_register)
+ return 0;
+
+ ret = regmap_read(priv->regmap, TCAN4X5X_DEV_ID2, &val);
+ if (ret)
+ return ret;
+
+ if (version_info->id2_register != val) {
+ dev_err(&priv->spi->dev, "Not the specified TCAN device, id2: %x != %x\n",
+ version_info->id2_register, val);
+ return -ENODEV;
+ }
+
+ 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 +320,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;
@@ -301,8 +346,13 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
{
struct tcan4x5x_priv *priv;
struct m_can_classdev *mcan_class;
+ const struct tcan4x5x_version_info *version_info;
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 +411,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 +448,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,tcan4x5x", .data = &tcan4x5x_generic },
+ { .compatible = "ti,tcan4552", .data = &tcan4x5x_tcan4552 },
+ { .compatible = "ti,tcan4553", .data = &tcan4x5x_tcan4553 },
+ { /* 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.39.2

2023-03-14 16:13:41

by Simon Horman

[permalink] [raw]

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

On Tue, Mar 14, 2023 at 04:11:57PM +0100, Markus Schneider-Pargmann wrote:
> These two new chips do not have state or wake pins.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>

Not a requirement from my side,
but perhaps it would be worth converting this binding to yaml
at some point.

> ---
> .../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..38a2b5369b44 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,tcan4x5x" or
> + "ti,tcan4552" or
> + "ti,tcan4553"
> - 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.39.2
>

2023-03-14 16:23:30

by Simon Horman

[permalink] [raw]

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

2023-03-14 18:20:05

by Michal Kubiak

[permalink] [raw]

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

On Tue, Mar 14, 2023 at 04:12:01PM +0100, 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
> ---
> drivers/net/can/m_can/tcan4x5x-core.c | 113 ++++++++++++++++++++------
> 1 file changed, 89 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..e7fa509dacc9 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,53 @@ 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 int tcan4x5x_verify_version(
> + struct tcan4x5x_priv *priv,
> + const struct tcan4x5x_version_info *version_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 (!version_info->id2_register)
> + return 0;
> +
> + ret = regmap_read(priv->regmap, TCAN4X5X_DEV_ID2, &val);
> + if (ret)
> + return ret;
> +
> + if (version_info->id2_register != val) {
> + dev_err(&priv->spi->dev, "Not the specified TCAN device, id2: %x != %x\n",
> + version_info->id2_register, val);
> + return -ENODEV;
> + }
> +
> + 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 +320,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;
> @@ -301,8 +346,13 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
> {
> struct tcan4x5x_priv *priv;
> struct m_can_classdev *mcan_class;
> + const struct tcan4x5x_version_info *version_info;
> int freq, ret;

Nitpick: RCT.

>
> + 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 +411,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 +448,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,tcan4x5x", .data = &tcan4x5x_generic },
> + { .compatible = "ti,tcan4552", .data = &tcan4x5x_tcan4552 },
> + { .compatible = "ti,tcan4553", .data = &tcan4x5x_tcan4553 },
> + { /* 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);

Thanks,
Michal

>
> --
> 2.39.2
>

2023-03-14 18:27:20

by Michal Kubiak

[permalink] [raw]

Subject: Re: [PATCH 0/5] can: tcan4x5x: Introduce tcan4552/4553

On Tue, Mar 14, 2023 at 04:11:56PM +0100, Markus Schneider-Pargmann wrote:
> Hi Marc and 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.
>
> I included two patches from the optimization series and will remove them
> from the optimization series. Hopefully it avoids conflicts and not
> polute the other series with tcan4552/4553 stuff.
>
> Best,
> Markus
>
> optimization series:
> https://lore.kernel.org/lkml/[email protected]
>
> Markus Schneider-Pargmann (5):
> 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
>
> .../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 | 122 ++++++++++++++----
> drivers/net/can/m_can/tcan4x5x-regmap.c | 1 -
> 5 files changed, 121 insertions(+), 30 deletions(-)
>

The logic and coding style looks OK to me, but CAN-specific stuff should
be reviewed by someone else.
Just one nitpick in the last patch.

Thanks,
Michal

For entire series:
Reviewed-by: Michal Kubiak <[email protected]>

> --
> 2.39.2
>

2023-03-14 20:01:28

by Krzysztof Kozlowski

[permalink] [raw]

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

On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> These two new chips do not have state or wake pins.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> ---
> .../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..38a2b5369b44 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,tcan4x5x" or
> + "ti,tcan4552" or
> + "ti,tcan4553"

Awesome, they nicely fit into wildcard... Would be useful to deprecate
the wildcard at some point and switch to proper compatibles in such
case, because now they became confusing.

Anyway:

Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2023-03-15 10:41:42

by Markus Schneider-Pargmann

[permalink] [raw]

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

Hi Simon,

On Tue, Mar 14, 2023 at 05:22:02PM +0100, Simon Horman wrote:
> On Tue, Mar 14, 2023 at 04:12:01PM +0100, 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
>
> Hi Markus,
>
> you forgot your email address in the signed-off-by line.

Thank you, I am wondering how I managed to do that :D.

>
> > ---
> > drivers/net/can/m_can/tcan4x5x-core.c | 113 ++++++++++++++++++++------
> > 1 file changed, 89 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..e7fa509dacc9 100644
> > --- a/drivers/net/can/m_can/tcan4x5x-core.c
> > +++ b/drivers/net/can/m_can/tcan4x5x-core.c
>
> ...
>
> > @@ -254,18 +262,53 @@ 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 int tcan4x5x_verify_version(
> > + struct tcan4x5x_priv *priv,
> > + const struct tcan4x5x_version_info *version_info)
>
> nit:
>
> static int
> tcan4x5x_verify_version(struct tcan4x5x_priv *priv, const struct tcan4x5x_version_info *version_info)
>
> or:
>
> static int tcan4x5x_verify_version(struct tcan4x5x_priv *priv, const struct tcan4x5x_version_info *version_info)
>
> Your could make the line shorter by renaming the 'version_info' parameter,
> say to 'info'.

Thanks, fixed. I would like to keep version_info as it is used like that
everywhere else. I think/hope breaking the 80c here is fine.

>
> ...
>
> > @@ -394,21 +448,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,tcan4x5x", .data = &tcan4x5x_generic },
> > + { .compatible = "ti,tcan4552", .data = &tcan4x5x_tcan4552 },
> > + { .compatible = "ti,tcan4553", .data = &tcan4x5x_tcan4553 },
> > + { /* 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, },
>
> nit: checkpatch tells me that no space is necessary after a cast.

Fixed as well.

Thanks for reviewing.

Best,
Markus

2023-03-15 10:45:29

by Markus Schneider-Pargmann

[permalink] [raw]

Subject: Re: [PATCH 0/5] can: tcan4x5x: Introduce tcan4552/4553

Hi Michal,

On Tue, Mar 14, 2023 at 07:26:59PM +0100, Michal Kubiak wrote:
> On Tue, Mar 14, 2023 at 04:11:56PM +0100, Markus Schneider-Pargmann wrote:
> > Hi Marc and 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.
> >
> > I included two patches from the optimization series and will remove them
> > from the optimization series. Hopefully it avoids conflicts and not
> > polute the other series with tcan4552/4553 stuff.
> >
> > Best,
> > Markus
> >
> > optimization series:
> > https://lore.kernel.org/lkml/[email protected]
> >
> > Markus Schneider-Pargmann (5):
> > 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
> >
> > .../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 | 122 ++++++++++++++----
> > drivers/net/can/m_can/tcan4x5x-regmap.c | 1 -
> > 5 files changed, 121 insertions(+), 30 deletions(-)
> >
>
> The logic and coding style looks OK to me, but CAN-specific stuff should
> be reviewed by someone else.
> Just one nitpick in the last patch.
>
> Thanks,
> Michal
>
> For entire series:
> Reviewed-by: Michal Kubiak <[email protected]>

Thanks for your review, I fixed the nitpick.

Best,
Markus

2023-03-15 10:49:39

by Markus Schneider-Pargmann

[permalink] [raw]

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

Hi Krzysztof,

On Tue, Mar 14, 2023 at 09:01:10PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> > These two new chips do not have state or wake pins.
> >
> > Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> > ---
> > .../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..38a2b5369b44 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,tcan4x5x" or
> > + "ti,tcan4552" or
> > + "ti,tcan4553"
>
> Awesome, they nicely fit into wildcard... Would be useful to deprecate
> the wildcard at some point and switch to proper compatibles in such
> case, because now they became confusing.
>
> Anyway:
>
> Acked-by: Krzysztof Kozlowski <[email protected]>

Thank you. Indeed the old generic name could be replaced, unfortunately
I don't have a list of devices that this generic wildcard matches.

Best,
Markus

2023-03-15 11:26:28

by Marc Kleine-Budde

[permalink] [raw]

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

On 14.03.2023 21:01:10, Krzysztof Kozlowski wrote:
> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> > These two new chips do not have state or wake pins.
> >
> > Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> > ---
> > .../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..38a2b5369b44 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,tcan4x5x" or
> > + "ti,tcan4552" or
> > + "ti,tcan4553"
>
> Awesome, they nicely fit into wildcard... Would be useful to deprecate
> the wildcard at some point and switch to proper compatibles in such
> case, because now they became confusing.

I plead for DT stability!

As I understand correctly, the exact version of the chip (4550, 4552, or
4553) can be detected via the ID2 register.

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (1.49 kB)
signature.asc (488.00 B)
Download all attachments

2023-03-15 11:29:26

by Marc Kleine-Budde

[permalink] [raw]

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

On 15.03.2023 11:49:14, Markus Schneider-Pargmann wrote:
> Hi Krzysztof,
>
> On Tue, Mar 14, 2023 at 09:01:10PM +0100, Krzysztof Kozlowski wrote:
> > On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> > > These two new chips do not have state or wake pins.
> > >
> > > Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> > > ---
> > > .../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..38a2b5369b44 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,tcan4x5x" or
> > > + "ti,tcan4552" or
> > > + "ti,tcan4553"
> >
> > Awesome, they nicely fit into wildcard... Would be useful to deprecate
> > the wildcard at some point and switch to proper compatibles in such
> > case, because now they became confusing.
> >
> > Anyway:
> >
> > Acked-by: Krzysztof Kozlowski <[email protected]>
>
> Thank you. Indeed the old generic name could be replaced, unfortunately
> I don't have a list of devices that this generic wildcard matches.

The mcp251xfd driver supports "microchip,mcp2517fd",
"microchip,mcp2518fd", "microchip,mcp251863", and "microchip,mcp251xfd".
It always does auto detection and throws a warning if the found chip is
not consistent with the firmware (DT, ACPI).

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (1.95 kB)
signature.asc (488.00 B)
Download all attachments

2023-03-15 13:15:23

by Krzysztof Kozlowski

[permalink] [raw]

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

On 15/03/2023 12:25, Marc Kleine-Budde wrote:
> On 14.03.2023 21:01:10, Krzysztof Kozlowski wrote:
>> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
>>> These two new chips do not have state or wake pins.
>>>
>>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>>> ---
>>> .../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..38a2b5369b44 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,tcan4x5x" or
>>> + "ti,tcan4552" or
>>> + "ti,tcan4553"
>>
>> Awesome, they nicely fit into wildcard... Would be useful to deprecate
>> the wildcard at some point and switch to proper compatibles in such
>> case, because now they became confusing.
>
> I plead for DT stability!
>
> As I understand correctly, the exact version of the chip (4550, 4552, or
> 4553) can be detected via the ID2 register.

So maybe there is no need for this patch at all? Or the new compatibles
should be made compatible with generic fallback?

Best regards,
Krzysztof

2023-03-15 15:59:14

by Markus Schneider-Pargmann

[permalink] [raw]

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

On Wed, Mar 15, 2023 at 02:14:27PM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2023 12:25, Marc Kleine-Budde wrote:
> > On 14.03.2023 21:01:10, Krzysztof Kozlowski wrote:
> >> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> >>> These two new chips do not have state or wake pins.
> >>>
> >>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> >>> ---
> >>> .../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..38a2b5369b44 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,tcan4x5x" or
> >>> + "ti,tcan4552" or
> >>> + "ti,tcan4553"
> >>
> >> Awesome, they nicely fit into wildcard... Would be useful to deprecate
> >> the wildcard at some point and switch to proper compatibles in such
> >> case, because now they became confusing.
> >
> > I plead for DT stability!
> >
> > As I understand correctly, the exact version of the chip (4550, 4552, or
> > 4553) can be detected via the ID2 register.
>
> So maybe there is no need for this patch at all? Or the new compatibles
> should be made compatible with generic fallback?

I can use the value being read from the ID2 register to get the version.
This at least holds the correct value for tcan4550, 4552 and 4553. But
the state and wake gpios can't be used in case of a 4552 or 4553.

So yes, it is possible to do it without the new compatibles but the
changes for state and wake gpios need to stay.

What do you two prefer?

Best,
Markus

2023-03-15 16:05:24

by Simon Horman

[permalink] [raw]

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

On Wed, Mar 15, 2023 at 04:58:33PM +0100, Markus Schneider-Pargmann wrote:
> On Wed, Mar 15, 2023 at 02:14:27PM +0100, Krzysztof Kozlowski wrote:
> > On 15/03/2023 12:25, Marc Kleine-Budde wrote:
> > > On 14.03.2023 21:01:10, Krzysztof Kozlowski wrote:
> > >> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
> > >>> These two new chips do not have state or wake pins.
> > >>>
> > >>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> > >>> ---
> > >>> .../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..38a2b5369b44 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,tcan4x5x" or
> > >>> + "ti,tcan4552" or
> > >>> + "ti,tcan4553"
> > >>
> > >> Awesome, they nicely fit into wildcard... Would be useful to deprecate
> > >> the wildcard at some point and switch to proper compatibles in such
> > >> case, because now they became confusing.
> > >
> > > I plead for DT stability!
> > >
> > > As I understand correctly, the exact version of the chip (4550, 4552, or
> > > 4553) can be detected via the ID2 register.
> >
> > So maybe there is no need for this patch at all? Or the new compatibles
> > should be made compatible with generic fallback?
>
> I can use the value being read from the ID2 register to get the version.
> This at least holds the correct value for tcan4550, 4552 and 4553. But
> the state and wake gpios can't be used in case of a 4552 or 4553.
>
> So yes, it is possible to do it without the new compatibles but the
> changes for state and wake gpios need to stay.
>
> What do you two prefer?

FWIIW, I think it is good to have the extra compat strings,
even if the driver only uses the fallback string for now.
This would allow the driver to take into account HW differences that come
to light later, without needing to update the bindings.

2023-03-15 16:08:40

by Krzysztof Kozlowski

[permalink] [raw]

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

On 15/03/2023 16:58, Markus Schneider-Pargmann wrote:
> On Wed, Mar 15, 2023 at 02:14:27PM +0100, Krzysztof Kozlowski wrote:
>> On 15/03/2023 12:25, Marc Kleine-Budde wrote:
>>> On 14.03.2023 21:01:10, Krzysztof Kozlowski wrote:
>>>> On 14/03/2023 16:11, Markus Schneider-Pargmann wrote:
>>>>> These two new chips do not have state or wake pins.
>>>>>
>>>>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>>>>> ---
>>>>> .../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..38a2b5369b44 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,tcan4x5x" or
>>>>> + "ti,tcan4552" or
>>>>> + "ti,tcan4553"
>>>>
>>>> Awesome, they nicely fit into wildcard... Would be useful to deprecate
>>>> the wildcard at some point and switch to proper compatibles in such
>>>> case, because now they became confusing.
>>>
>>> I plead for DT stability!
>>>
>>> As I understand correctly, the exact version of the chip (4550, 4552, or
>>> 4553) can be detected via the ID2 register.
>>
>> So maybe there is no need for this patch at all? Or the new compatibles
>> should be made compatible with generic fallback?
>
> I can use the value being read from the ID2 register to get the version.
> This at least holds the correct value for tcan4550, 4552 and 4553. But
> the state and wake gpios can't be used in case of a 4552 or 4553.
>
> So yes, it is possible to do it without the new compatibles but the
> changes for state and wake gpios need to stay.
>
> What do you two prefer?

Then specific with generic fallback compatibles, although for driver it
still won't matter as you need to customize driver_data anyway.

Best regards,
Krzysztof