Re: [PATCH v3 2/3] can: tcan4x5x: support resuming from rx interrupt signal

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

Hi Martin,

On Mon, Nov 13, 2023 at 02:14:51PM +0100, Martin Hundebøll wrote:
> Implement the "wakeup-source" device tree property, so the chip is left
> running when suspending, and its rx interrupt is used as a wakeup source
> to resume operation.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
> 
> Change in v3:
>  * Updated to use the property in struct m_can_classdev instead of
>    passing parameters to suspend/resume functions.
> 
> Change in v2:
>  * Added `static` keyword to dev_pm_ops sturcture
> 
>  drivers/net/can/m_can/tcan4x5x-core.c | 34 +++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 870ab4aef610..0f4c2ac7e4f6 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -411,7 +411,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  	priv->spi = spi;
>  
>  	mcan_class->pm_clock_support = 0;
> -	mcan_class->pm_wake_source = 0;
> +	mcan_class->pm_wake_source = device_property_read_bool(&spi->dev, "wakeup-source");
>  	mcan_class->can.clock.freq = freq;
>  	mcan_class->dev = &spi->dev;
>  	mcan_class->ops = &tcan4x5x_ops;
> @@ -460,6 +460,9 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  		goto out_power;
>  	}
>  
> +	if (mcan_class->pm_wake_source)
> +		device_init_wakeup(&spi->dev, true);
> +

You are automatically enabling the device for wakeup here.

What do you think about using ethtool's wake-on-lan settings to actually
enable tcan as a wakeup source? So the devicetree would describe if the
hardware is capable of wakeups and the software (ethtool) can be used by
the user to decide if CAN wakeups should be enabled.

I am currently working on something similar for m_can, where m_can can
be the wakeup source from very deep sleep states. However I can't keep
the wakeup source always enabled. So this is a kind of a conflict for me
in this patch. Also I would need to use the wakeup-source flag in m_can
device nodes as well.

I can share my work later as well, so we can find a good solution that
works in both cases.

Best,
Markus

>  	ret = m_can_class_register(mcan_class);
>  	if (ret) {
>  		dev_err(&spi->dev, "Failed registering m_can device %pe\n",
> @@ -488,6 +491,29 @@ static void tcan4x5x_can_remove(struct spi_device *spi)
>  	m_can_class_free_dev(priv->cdev.net);
>  }
>  
> +static int __maybe_unused tcan4x5x_suspend(struct device *dev)
> +{
> +	struct m_can_classdev *cdev = dev_get_drvdata(dev);
> +	struct spi_device *spi = to_spi_device(dev);
> +
> +	if (cdev->pm_wake_source)
> +		enable_irq_wake(spi->irq);
> +
> +	return m_can_class_suspend(dev);
> +}
> +
> +static int __maybe_unused tcan4x5x_resume(struct device *dev)
> +{
> +	struct m_can_classdev *cdev = dev_get_drvdata(dev);
> +	struct spi_device *spi = to_spi_device(dev);
> +	int ret = m_can_class_resume(dev);
> +
> +	if (cdev->pm_wake_source)
> +		disable_irq_wake(spi->irq);
> +
> +	return ret;
> +}
> +
>  static const struct of_device_id tcan4x5x_of_match[] = {
>  	{
>  		.compatible = "ti,tcan4x5x",
> @@ -506,11 +532,15 @@ static const struct spi_device_id tcan4x5x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, tcan4x5x_id_table);
>  
> +static const struct dev_pm_ops tcan4x5x_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tcan4x5x_suspend, tcan4x5x_resume)
> +};
> +
>  static struct spi_driver tcan4x5x_can_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
>  		.of_match_table = tcan4x5x_of_match,
> -		.pm = NULL,
> +		.pm = &tcan4x5x_pm_ops,
>  	},
>  	.id_table = tcan4x5x_id_table,
>  	.probe = tcan4x5x_can_probe,
> -- 
> 2.42.0
>