Re: Inconsistent error state transition error frames

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 10.09.2014 13:53, Wolfgang Grandegger wrote:

>>>> +/* controller error counter values / data[6..7] */
>>>> +#define CAN_ERR_TXERR_BYTE 6
>>>> +#define CAN_ERR_RXERR_BYTE 7
>>>>     #endif /* _UAPI_CAN_ERROR_H */
>>>>
>>> I'm not sure what's considered good practice in kernel code but
> wouldn't
>>> it be better to use a macro like this instead?
>>>     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
>>> which would be used like this:
>>>     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...
>>
>> I'm not sure....but...
>>
>> Make it a static inline function and give it a proper name, something
>> like can_err_frame_get_ctrl(). Maybe without err_....

Maybe without _frame ? :-)

Think of 

	frame->data[CAN_ERR_CTRL_BYTE] |= CAN_ERR_CRTL_RX_OVERFLOW;

How would this look like?

	can_err_frame_set_ctrl(frame) = can_err_frame_get_ctrl(frame) | CAN_ERR_CRTL_RX_OVERFLOW;

Hm. Not nice.

	CAN_ERR_CRTL_DATA(frame) |= CAN_ERR_CRTL_RX_OVERFLOW;

Is hard to understand at first sight.

> 
> I'm not sure... we also need to set the value. Therefore I would
> vote for:
> 
>  frame->data[CAN_ERR_TXERR_BYTE];
> 
> It does make the code more readable.

Unfortunately CAN_ERR_TXERR_BYTE is pretty long and I have no good idea
how to make it shorter. CAN_ERR_TXERR_IDX is probably more handy.

E.g.

	frame->data[CAN_ERR_CTRL_IDX] |= CAN_ERR_CRTL_RX_OVERFLOW;

I would vote for something like this too.

Should I send a patch for this approach?

Regards,
Oliver