Re: [PATCH v8 5/7] can: canxl: update CAN infrastructure for CAN XL frames

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun. 11 Sept. 2022 at 21:35, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 11.09.22 09:53, Vincent Mailhol wrote:
> > On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
>
> (..)
>
> >> +/* get length element value from can[|fd|xl]_frame structure */
> >>   static inline unsigned int can_skb_get_len_val(struct sk_buff *skb)
> >>   {
> >> +       const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >>          const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >
> > Nitpick: what would be the acronyms for cfx and cfd? I thought that
> > cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of
> > cfx for *C*AN-*XL* frame.
> > On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for
> > CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).
>
> You need to start from the original
>
> struct can_frame cf; *C*AN *F*RAME
>
> Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN
> *FD* FRAME where is was not forseable that there ever would be CAN XL.
>
> For me it is more intuitive to generally name CAN frames 'cf<whatever>'.
>
> cf -> cfd -> cfx
>
> So it is about 'cf' with an extra attribute and not an abbreviation of
> CAN variants.

I still disagree on that one. For me:
  * Classical CAN frames: ccf (or the legacy cf before introduction of CAN-FD)
  * CAN FD frames: cfd
  * CAN XL frames: cxl
is the most consistent.

cfx is not consistent with the cfd acronym and it is out of order: the
structure name is canxl_frame, not can_frame_xl.
At least, that should be cxf for struct *c*an*x*l_*f*rame. CAN frame
XL just sounds odd to me.

Regardless, this remains a nitpick and I will not NACK the series for
that. So do as you prefer in the v9, my Acked-by remains valid.


Yours sincerely,
Vincent Mailhol



[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux