Re: Inconsistent error state transition error frames

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

On 09/10/2014 12:19 PM, Andri Yngvason wrote:
>> diff --git a/include/uapi/linux/can/error.h
>> b/include/uapi/linux/can/error.h
>> index c247446..c290f30 100644
>> --- a/include/uapi/linux/can/error.h
>> +++ b/include/uapi/linux/can/error.h
>> @@ -58,10 +58,12 @@
>>   #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
>>     /* arbitration lost in bit ... / data[0] */
>> +#define CAN_ERR_LOSTARB_BYTE 0
>>   #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
>>                         /* else bit number in bitstream */
>>     /* error status of CAN-controller / data[1] */
>> +#define CAN_ERR_CRTL_BYTE 1
>>   #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
>>   #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
>>   #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
>> @@ -73,6 +75,7 @@
>>                         /* the protocol-defined level of 127)  */
>>     /* error in CAN protocol (type) / data[2] */
>> +#define CAN_ERR_PROT_BYTE 2
>>   #define CAN_ERR_PROT_UNSPEC      0x00 /* unspecified */
>>   #define CAN_ERR_PROT_BIT         0x01 /* single bit error */
>>   #define CAN_ERR_PROT_FORM        0x02 /* frame format error */
>> @@ -84,6 +87,7 @@
>>   #define CAN_ERR_PROT_TX          0x80 /* error occurred on
>> transmission */
>>     /* error in CAN protocol (location) / data[3] */
>> +#define CAN_ERR_PROT_LOC_BYTE 3
>>   #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
>>   #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
>>   #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 -
>> 3) */
>> @@ -106,6 +110,7 @@
>>   #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
>>     /* error status of CAN-transceiver / data[4] */
>> +#define CAN_ERR_TRX_BYTE 4
>>   /*                                             CANH CANL */
>>   #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
>>   #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
>> @@ -118,6 +123,10 @@
>>   #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
>>   #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
>>   -/* controller specific additional information / data[5..7] */
>> +/* controller specific additional information / data[5] */
>> +
>> +/* 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_....

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]