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 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/