1. Patchew
  2. linux
  3. View series

[PATCH] can: tcan4x5x: fix reset gpio usage during probe

Brett Werling posted 1 patch 3 months ago
There is a newer version of this series
drivers/net/can/m_can/tcan4x5x-core.c | 56 +++++++++++++++------------
1 file changed, 32 insertions(+), 24 deletions(-)
[PATCH] can: tcan4x5x: fix reset gpio usage during probe
Posted by Brett Werling 3 months ago 
Fixes reset GPIO usage during probe by ensuring we retrieve the GPIO and
take the device out of reset (if it defaults to being in reset) before
we attempt to communicate with the device. This is achieved by moving
the call to tcan4x5x_get_gpios() before tcan4x5x_find_version() and
avoiding any device communication while getting the GPIOs. Once we
determine the version, we can then take the knowledge of which GPIOs we
obtained and use it to decide whether we need to disable the wake or
state pin functions within the device.

This change is necessary in a situation where the reset GPIO is pulled
high externally before the CPU takes control of it, meaning we need to
explicitly bring the device out of reset before we can start
communicating with it at all.

This also has the effect of fixing an issue where a reset of the device
would occur after having called tcan4x5x_disable_wake(), making the
original behavior not actually disable the wake. This patch should now
disable wake or state pin functions well after the reset occurs.

Signed-off-by: Brett Werling <brett.werling@garmin.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 56 +++++++++++++++------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 8edaa339d590..c37733bf124e 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -343,21 +343,19 @@ static void tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
 		of_property_read_bool(cdev->dev->of_node, "ti,nwkrq-voltage-vio");
 }
 
-static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
-			      const struct tcan4x5x_version_info *version_info)
+static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
 {
 	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
 	int ret;
 
-	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->device_wake_gpio = devm_gpiod_get_optional(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->device_wake_gpio = NULL;
 	}
 
 	tcan4x5x->reset_gpio = devm_gpiod_get_optional(cdev->dev, "reset",
@@ -369,19 +367,27 @@ static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
 	if (ret)
 		return ret;
 
-	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);
-		}
-	}
+	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;
 
 	return 0;
 }
 
+static void tcan4x5x_check_gpios(struct m_can_classdev *cdev,
+				 const struct tcan4x5x_version_info *version_info)
+{
+	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
+
+	if (version_info->has_wake_pin && !tcan4x5x->device_wake_gpio)
+		tcan4x5x_disable_wake(cdev);
+
+	if (version_info->has_state_pin && !tcan4x5x->device_state_gpio)
+		tcan4x5x_disable_state(cdev);
+}
+
 static const struct m_can_ops tcan4x5x_ops = {
 	.init = tcan4x5x_init,
 	.deinit = tcan4x5x_deinit,
@@ -468,17 +474,19 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 		goto out_m_can_class_free_dev;
 	}
 
+	ret = tcan4x5x_get_gpios(mcan_class);
+	if (ret) {
+		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
+		goto out_power;
+	}
+
 	version_info = tcan4x5x_find_version(priv);
 	if (IS_ERR(version_info)) {
 		ret = PTR_ERR(version_info);
 		goto out_power;
 	}
 
-	ret = tcan4x5x_get_gpios(mcan_class, version_info);
-	if (ret) {
-		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
-		goto out_power;
-	}
+	tcan4x5x_check_gpios(mcan_class, version_info);
 
 	tcan4x5x_get_dt_data(mcan_class);
 
-- 
2.49.0
Re: [PATCH] can: tcan4x5x: fix reset gpio usage during probe
Posted by Marc Kleine-Budde 2 months, 4 weeks ago 
On 08.07.2025 13:11:53, Brett Werling wrote:
> Fixes reset GPIO usage during probe by ensuring we retrieve the GPIO and
> take the device out of reset (if it defaults to being in reset) before
> we attempt to communicate with the device. This is achieved by moving
> the call to tcan4x5x_get_gpios() before tcan4x5x_find_version() and
> avoiding any device communication while getting the GPIOs. Once we
> determine the version, we can then take the knowledge of which GPIOs we
> obtained and use it to decide whether we need to disable the wake or
> state pin functions within the device.
> 
> This change is necessary in a situation where the reset GPIO is pulled
> high externally before the CPU takes control of it, meaning we need to
> explicitly bring the device out of reset before we can start
> communicating with it at all.
> 
> This also has the effect of fixing an issue where a reset of the device
> would occur after having called tcan4x5x_disable_wake(), making the
> original behavior not actually disable the wake. This patch should now
> disable wake or state pin functions well after the reset occurs.
> 
> Signed-off-by: Brett Werling <brett.werling@garmin.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 56 +++++++++++++++------------
>  1 file changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 8edaa339d590..c37733bf124e 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -343,21 +343,19 @@ static void tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
>  		of_property_read_bool(cdev->dev->of_node, "ti,nwkrq-voltage-vio");
>  }
>  
> -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
> -			      const struct tcan4x5x_version_info *version_info)
> +static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
>  {
>  	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
>  	int ret;
>  
> -	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->device_wake_gpio = devm_gpiod_get_optional(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);

Although the original code doesn't propagate any errors....

> -		}
> +		tcan4x5x->device_wake_gpio = NULL;
>  	}
>  
>  	tcan4x5x->reset_gpio = devm_gpiod_get_optional(cdev->dev, "reset",
> @@ -369,19 +367,27 @@ static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
>  	if (ret)
>  		return ret;
>  
> -	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);
> -		}
> -	}
> +	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;
>  
>  	return 0;
>  }
>  
> +static void tcan4x5x_check_gpios(struct m_can_classdev *cdev,
> +				 const struct tcan4x5x_version_info *version_info)
> +{
> +	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> +
> +	if (version_info->has_wake_pin && !tcan4x5x->device_wake_gpio)
> +		tcan4x5x_disable_wake(cdev);

... please add error checking and propagate errors.

> +
> +	if (version_info->has_state_pin && !tcan4x5x->device_state_gpio)
> +		tcan4x5x_disable_state(cdev);
> +}
> +
>  static const struct m_can_ops tcan4x5x_ops = {
>  	.init = tcan4x5x_init,
>  	.deinit = tcan4x5x_deinit,
> @@ -468,17 +474,19 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  		goto out_m_can_class_free_dev;
>  	}
>  
> +	ret = tcan4x5x_get_gpios(mcan_class);
> +	if (ret) {
> +		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
> +		goto out_power;
> +	}
> +
>  	version_info = tcan4x5x_find_version(priv);
>  	if (IS_ERR(version_info)) {
>  		ret = PTR_ERR(version_info);
>  		goto out_power;
>  	}
>  
> -	ret = tcan4x5x_get_gpios(mcan_class, version_info);
> -	if (ret) {
> -		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
> -		goto out_power;
> -	}
> +	tcan4x5x_check_gpios(mcan_class, version_info);

same here

>  
>  	tcan4x5x_get_dt_data(mcan_class);

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   |
[PATCH v2] can: tcan4x5x: fix reset gpio usage during probe
Posted by Brett Werling 2 months, 3 weeks ago 
Fixes reset GPIO usage during probe by ensuring we retrieve the GPIO and
take the device out of reset (if it defaults to being in reset) before
we attempt to communicate with the device. This is achieved by moving
the call to tcan4x5x_get_gpios() before tcan4x5x_find_version() and
avoiding any device communication while getting the GPIOs. Once we
determine the version, we can then take the knowledge of which GPIOs we
obtained and use it to decide whether we need to disable the wake or
state pin functions within the device.

This change is necessary in a situation where the reset GPIO is pulled
high externally before the CPU takes control of it, meaning we need to
explicitly bring the device out of reset before we can start
communicating with it at all.

This also has the effect of fixing an issue where a reset of the device
would occur after having called tcan4x5x_disable_wake(), making the
original behavior not actually disable the wake. This patch should now
disable wake or state pin functions well after the reset occurs.

Signed-off-by: Brett Werling <brett.werling@garmin.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 61 ++++++++++++++++++---------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 8edaa339d590..39b0b5277b11 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -343,21 +343,19 @@ static void tcan4x5x_get_dt_data(struct m_can_classdev *cdev)
 		of_property_read_bool(cdev->dev->of_node, "ti,nwkrq-voltage-vio");
 }
 
-static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
-			      const struct tcan4x5x_version_info *version_info)
+static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
 {
 	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
 	int ret;
 
-	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->device_wake_gpio = devm_gpiod_get_optional(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->device_wake_gpio = NULL;
 	}
 
 	tcan4x5x->reset_gpio = devm_gpiod_get_optional(cdev->dev, "reset",
@@ -369,14 +367,31 @@ static int tcan4x5x_get_gpios(struct m_can_classdev *cdev,
 	if (ret)
 		return ret;
 
-	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);
-		}
+	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;
+
+	return 0;
+}
+
+static int tcan4x5x_check_gpios(struct m_can_classdev *cdev,
+				const struct tcan4x5x_version_info *version_info)
+{
+	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
+	int ret;
+
+	if (version_info->has_wake_pin && !tcan4x5x->device_wake_gpio) {
+		ret = tcan4x5x_disable_wake(cdev);
+		if (ret)
+			return ret;
+	}
+
+	if (version_info->has_state_pin && !tcan4x5x->device_state_gpio) {
+		ret = tcan4x5x_disable_state(cdev);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -468,15 +483,21 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
 		goto out_m_can_class_free_dev;
 	}
 
+	ret = tcan4x5x_get_gpios(mcan_class);
+	if (ret) {
+		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
+		goto out_power;
+	}
+
 	version_info = tcan4x5x_find_version(priv);
 	if (IS_ERR(version_info)) {
 		ret = PTR_ERR(version_info);
 		goto out_power;
 	}
 
-	ret = tcan4x5x_get_gpios(mcan_class, version_info);
+	ret = tcan4x5x_check_gpios(mcan_class, version_info);
 	if (ret) {
-		dev_err(&spi->dev, "Getting gpios failed %pe\n", ERR_PTR(ret));
+		dev_err(&spi->dev, "Checking gpios failed %pe\n", ERR_PTR(ret));
 		goto out_power;
 	}
 
-- 
2.50.1
Re: [PATCH v2] can: tcan4x5x: fix reset gpio usage during probe
Posted by Marc Kleine-Budde 2 months, 3 weeks ago 
On 11.07.2025 09:17:28, Brett Werling wrote:
> Fixes reset GPIO usage during probe by ensuring we retrieve the GPIO and
> take the device out of reset (if it defaults to being in reset) before
> we attempt to communicate with the device. This is achieved by moving
> the call to tcan4x5x_get_gpios() before tcan4x5x_find_version() and
> avoiding any device communication while getting the GPIOs. Once we
> determine the version, we can then take the knowledge of which GPIOs we
> obtained and use it to decide whether we need to disable the wake or
> state pin functions within the device.
> 
> This change is necessary in a situation where the reset GPIO is pulled
> high externally before the CPU takes control of it, meaning we need to
> explicitly bring the device out of reset before we can start
> communicating with it at all.
> 
> This also has the effect of fixing an issue where a reset of the device
> would occur after having called tcan4x5x_disable_wake(), making the
> original behavior not actually disable the wake. This patch should now
> disable wake or state pin functions well after the reset occurs.
> 
> Signed-off-by: Brett Werling <brett.werling@garmin.com>

Thanks for the patch, applied to linux-can, added stable on Cc.

regards,
Marc

PS: start a new thread for a new version of a patch, don't reply to the
old version.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.