Re: [PATCH v4 4/5] can: bcm: add support for CAN FD frames

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

[-- Attachment #1.1: Type: text/plain, Size: 1420 bytes --]

On 06/17/2016 12:37 PM, Oliver Hartkopp wrote:
> On 06/17/2016 12:23 PM, Marc Kleine-Budde wrote:
>> On 06/04/2016 09:26 PM, Oliver Hartkopp wrote:
> 
>>> -/* easy access to CAN frame payload */
>>> -static inline u64 GET_U64(const struct can_frame *cp)
>>> +/* easy access to the first 64 bit of can(fd)_frame payload */
>>> +static inline u64 GET_U64(const struct canfd_frame *cp, int offset)
>>>   {
>>> -	return *(u64 *)cp->data;
>>> +	return *(u64 *)(cp->data + offset);
>>
>> Can you ensure that offset is always a multiple of 8?
> 
> I would like to put that into a comment, as a check for this helper at 
> runtime is too expensive - and the caller is only in this C-file at two 
> places.

No need to check during runtime, if you know your code and can ensure
that offset is always a multiple of 8 :D

>> Unaligned pointer
>> access will be quite slow on some platforms.
> 
> cp->data is well aligned.

"cp->data + offset" must be aligned properly, this is why offset must be
a multiple of 8.

>> Can you please make this
>> function lower-case (as it's a function not a macro).
> 
> will do.

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: 455 bytes --]