Re: [PATCH 3/7] can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active

[Marc Kleine-Budde]

On 09.09.2025 16:28:48, Marc Kleine-Budde wrote:
> On 20.08.2025 11:09:16, Markus Schneider-Pargmann wrote:
> > >> --- a/drivers/net/can/m_can/m_can.c
> > >> +++ b/drivers/net/can/m_can/m_can.c
> 
> [...]
> 
> > >> +static enum can_state
> > >> +m_can_can_state_get_by_psr(const u32 psr)
> > >> +{
> > >> +	if (psr & PSR_BO)
> > >> +		return CAN_STATE_BUS_OFF;
> > >> +	if (psr & PSR_EP)
> > >> +		return CAN_STATE_ERROR_PASSIVE;
> > >> +	if (psr & PSR_EW)
> > >> +		return CAN_STATE_ERROR_WARNING;
> > >
> > > Why should m_can_handle_state_errors() should be called if none of these
> > > flags are set?
> > >
> > > m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> > > which is defined as:
> > >   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
> > >
> > > This is the for the interrupt register but will the PSR register bits be
> > > set without the interrupt register being set?
> > 
> > After reading the other users of the above function, I do see why this
> > was added. I am still wondering if there is a way to return to
> > ERROR_ACTIVE once the errors are cleared from the error register.
> 
> Sorry, I don't get what you mean.
> 
> > Also looking at all the users added for the function above, could you
> > read the register inside the function? Currently you are adding a
> > reg variable and a read call for each call to this function.
> > m_can_handle_state_errors() also doesn't need the psr value with your
> > refactoring.
> 
> That makes sense.

I'm also preparing the driver to have error handling for all register
reads...so I'll have to return in case of an error and pass the state
via a pointer.

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   |

Attachment: signature.asc
Description: PGP signature