Re: [PATCH v6 4/8 rebased] can: replace can_dlc as variable/element for payload length

From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can@vger.kernel.org
Subject: Re: [PATCH v6 4/8 rebased] can: replace can_dlc as variable/element for payload length
Date: Fri, 20 Nov 2020 12:18:13 +0100	[thread overview]
Message-ID: <4f32923c-4fad-4b63-4d9a-562e7879c153@hartkopp.net> (raw)
In-Reply-To: <1003aa33-7717-c7a9-dbe2-0b4c6561bf85@pengutronix.de>

On 20.11.20 11:49, Marc Kleine-Budde wrote:
> On 11/20/20 11:04 AM, Oliver Hartkopp wrote:
>> The naming of can_dlc as element of struct can_frame and also as variable
>> name is misleading as it claims to be a 'data length CODE' but in reality
>> it always was a plain data length.
>>
>> With the indroduction of a new 'len' element in struct can_frame we can now
>> remove can_dlc as name and make clear which of the former uses was a plain
>> length (-> 'len') or a data length code (-> 'dlc') value.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> [...]
> 
>> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
>> index b1729b208788..940589667a7f 100644
>> --- a/drivers/net/can/usb/gs_usb.c
>> +++ b/drivers/net/can/usb/gs_usb.c
>> @@ -133,11 +133,11 @@ struct gs_device_bt_const {
>>   
>>   struct gs_host_frame {
>>   	u32 echo_id;
>>   	u32 can_id;
>>   
>> -	u8 can_dlc;
>> +	u8 len;
> 
> At least in the candleLight firmware, this is the dlc value from the CAN
> controller, not a sanitized len value.

Oh, yes! You are right.

In fact it looks like that all USB adapters I've checked so far provide 
the raw DLC value on the USB communication layer - which is really fine!

As the gs_host_frame struct defines the USB content, can_dlc is the 
better naming here.

Thanks for the review.

Best,
Oliver

> 
>>   	u8 channel;
>>   	u8 flags;
>>   	u8 reserved;
> 
> https://github.com/candle-usb/candleLight_fw/blob/master/src/can.c#L152
> 
>>>          if (can_is_rx_pending(hcan)) {
>>>                  CAN_FIFOMailBox_TypeDef *fifo = &can->sFIFOMailBox[0];
>>>
>>>                  if (fifo->RIR &  CAN_RI0R_IDE) {
>>>                          rx_frame->can_id = CAN_EFF_FLAG | ((fifo->RIR >> 3) & 0x1FFFFFFF);
>>>                  } else {
>>>                          rx_frame->can_id = (fifo->RIR >> 21) & 0x7FF;
>>>                  }
>>>
>>>                  if (fifo->RIR & CAN_RI0R_RTR)  {
>>>                          rx_frame->can_id |= CAN_RTR_FLAG;
>>>                  }
>>>
>>>                  rx_frame->can_dlc = fifo->RDTR & CAN_RDT0R_DLC;
>>>
>>>                  rx_frame->data[0] = (fifo->RDLR >>  0) & 0xFF;
>>>                  rx_frame->data[1] = (fifo->RDLR >>  8) & 0xFF;
>>>                  rx_frame->data[2] = (fifo->RDLR >> 16) & 0xFF;
>>>                  rx_frame->data[3] = (fifo->RDLR >> 24) & 0xFF;
>>>                  rx_frame->data[4] = (fifo->RDHR >>  0) & 0xFF;
>>>                  rx_frame->data[5] = (fifo->RDHR >>  8) & 0xFF;
>>>                  rx_frame->data[6] = (fifo->RDHR >> 16) & 0xFF;
>>>                  rx_frame->data[7] = (fifo->RDHR >> 24) & 0xFF;
>>>
>>>                  can->RF0R |= CAN_RF0R_RFOM0; // release FIFO
> 
> I think we should keep the variable name can_dlc.
> 
> regards,
> Marc
> 