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

[Markus Schneider-Pargmann <msp@baylibre.com>]

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
>