[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(-)
  

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.

> ---
>  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'.

...

> @@ -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.

> +	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(spi, tcan4x5x_id_table);
>  
> -- 
> 2.39.2
>
  

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
>
  

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