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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >>> ---
> >>> 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
>
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
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
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
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 |