[v2] can: tcan4x5x/m_can: use standby mode when down and in suspend

[PATCH can-next v2 1/3] can: m_can: add deinit callback

Posted by Sean Nyekjaer 1 year, 2 months ago

This is added in preparation for calling standby mode in the tcan4x5x
driver or other users of m_can.
For the tcan4x5x; If Vsup 12V, standby mode will save 7-8mA, when the
interface is down.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/net/can/m_can/m_can.c | 10 +++++++---
 drivers/net/can/m_can/m_can.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a7b3bc439ae596527493a73d62b4b7a120ae4e49..667c70f8dc5e7e8b15b667119b63dea1fe667e8a 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1750,12 +1750,16 @@ static void m_can_stop(struct net_device *dev)
 
 	/* Set init mode to disengage from the network */
 	ret = m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
-	if (ret)
-		netdev_err(dev, "failed to enter standby mode: %pe\n",
-			   ERR_PTR(ret));
 
 	/* set the state as STOPPED */
 	cdev->can.state = CAN_STATE_STOPPED;
+
+	if (!ret && cdev->ops->deinit)
+		ret = cdev->ops->deinit(cdev);
+
+	if (ret)
+		netdev_err(dev, "failed to enter standby mode: %pe\n",
+			   ERR_PTR(ret));
 }
 
 static int m_can_close(struct net_device *dev)
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..6206535341a22a68d7c5570f619e6c4d05e6fcf4 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -68,6 +68,7 @@ struct m_can_ops {
 	int (*write_fifo)(struct m_can_classdev *cdev, int addr_offset,
 			  const void *val, size_t val_count);
 	int (*init)(struct m_can_classdev *cdev);
+	int (*deinit)(struct m_can_classdev *cdev);
 };
 
 struct m_can_tx_op {

-- 
2.46.2

Re: [PATCH can-next v2 1/3] can: m_can: add deinit callback

Posted by Vincent Mailhol 1 year, 2 months ago

On 15/11/2024 at 17:15, Sean Nyekjaer wrote:
> This is added in preparation for calling standby mode in the tcan4x5x
> driver or other users of m_can.
> For the tcan4x5x; If Vsup 12V, standby mode will save 7-8mA, when the
> interface is down.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/net/can/m_can/m_can.c | 10 +++++++---
>  drivers/net/can/m_can/m_can.h |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a7b3bc439ae596527493a73d62b4b7a120ae4e49..667c70f8dc5e7e8b15b667119b63dea1fe667e8a 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1750,12 +1750,16 @@ static void m_can_stop(struct net_device *dev)
>  
>  	/* Set init mode to disengage from the network */
>  	ret = m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
> -	if (ret)
> -		netdev_err(dev, "failed to enter standby mode: %pe\n",
> -			   ERR_PTR(ret));
>  
>  	/* set the state as STOPPED */
>  	cdev->can.state = CAN_STATE_STOPPED;
> +
> +	if (!ret && cdev->ops->deinit)
> +		ret = cdev->ops->deinit(cdev);

Question: is there a reason not to try to deinit() even if the
m_can_cccr_update_bits() failed?

> +	if (ret)
> +		netdev_err(dev, "failed to enter standby mode: %pe\n",
> +			   ERR_PTR(ret));>  }
>  
>  static int m_can_close(struct net_device *dev)
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..6206535341a22a68d7c5570f619e6c4d05e6fcf4 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -68,6 +68,7 @@ struct m_can_ops {
>  	int (*write_fifo)(struct m_can_classdev *cdev, int addr_offset,
>  			  const void *val, size_t val_count);
>  	int (*init)(struct m_can_classdev *cdev);
> +	int (*deinit)(struct m_can_classdev *cdev);
>  };
>  
>  struct m_can_tx_op {
> 

Yours sincerely,
Vincent Mailhol

Re: [PATCH can-next v2 1/3] can: m_can: add deinit callback

Posted by Sean Nyekjaer 1 year, 2 months ago

On Fri, Nov 15, 2024 at 11:23:49PM +0100, Vincent Mailhol wrote:
> On 15/11/2024 at 17:15, Sean Nyekjaer wrote:
> > This is added in preparation for calling standby mode in the tcan4x5x
> > driver or other users of m_can.
> > For the tcan4x5x; If Vsup 12V, standby mode will save 7-8mA, when the
> > interface is down.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 10 +++++++---
> >  drivers/net/can/m_can/m_can.h |  1 +
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index a7b3bc439ae596527493a73d62b4b7a120ae4e49..667c70f8dc5e7e8b15b667119b63dea1fe667e8a 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1750,12 +1750,16 @@ static void m_can_stop(struct net_device *dev)
> >  
> >  	/* Set init mode to disengage from the network */
> >  	ret = m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
> > -	if (ret)
> > -		netdev_err(dev, "failed to enter standby mode: %pe\n",
> > -			   ERR_PTR(ret));
> >  
> >  	/* set the state as STOPPED */
> >  	cdev->can.state = CAN_STATE_STOPPED;
> > +
> > +	if (!ret && cdev->ops->deinit)
> > +		ret = cdev->ops->deinit(cdev);
> 
> Question: is there a reason not to try to deinit() even if the
> m_can_cccr_update_bits() failed?

Good question.
If the call the m_can core fails, then there bigger problems than setting
the (in nmy case) tcan4x5x in standby mode.

> 
> > +	if (ret)
> > +		netdev_err(dev, "failed to enter standby mode: %pe\n",
> > +			   ERR_PTR(ret));>  }
> >  
> >  static int m_can_close(struct net_device *dev)
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index 92b2bd8628e6b31370f4accbc2e28f3b2257a71d..6206535341a22a68d7c5570f619e6c4d05e6fcf4 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -68,6 +68,7 @@ struct m_can_ops {
> >  	int (*write_fifo)(struct m_can_classdev *cdev, int addr_offset,
> >  			  const void *val, size_t val_count);
> >  	int (*init)(struct m_can_classdev *cdev);
> > +	int (*deinit)(struct m_can_classdev *cdev);
> >  };
> >  
> >  struct m_can_tx_op {
> > 
> 
> Yours sincerely,
> Vincent Mailhol
> 

/Sean