Hi Marc,
thanks for your Review.
> Please send patches with git send-email, not as an attachment.
Oops, sorry. Will do better next time.
> > +
> > + memset(&cm, 0, sizeof(cm));
> > +
> > + // Need to enable can FD to be able to set data bit timing
> > + cm.mask = CAN_CTRLMODE_FD;
> > + cm.flags = CAN_CTRLMODE_FD;
>
> I'm not sure if you want to enable the FD implicitly here, use
> can_set_ctrlmode() from your application instead.
Agreed. Will just need to test that once again.
> > +
> > + result = can_get_bittiming(name,&bitTiming);
> > + if (result == -1)
> > + return result;
>
> Why do you get the bittiming from the kernel here?
>
> > +
> > + // Kernel wants to recalc, remove bitrate to avoid EINVAL in can_get_bittiming
> > + bitTiming.bitrate = 0;
> > +
> > + struct req_info req_info = {
> > + .bittiming = &bitTiming,
>
> and why do you set it?
Because we have two functions to set the (arbitration) bitrate and the data bitrate, but
set_link() will set both at once. So if you set the data bitrate, just must ensure that
the previous set arbitration bitrate stays.
The question arises, if a similar behaviour would be needed in can_set_bitrate also?
(to set a new arbitration bitrate, but keeping the existing data bitrate)
> > + .databittiming = databt,
> > + .ctrlmode = &cm
> > + };
> > +
> > + return set_link(name, 0, &req_info);
> > +}
André
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
