Hi Marc and André,
thanks for your Review.
On Mon, Aug 13, 2018 at 1:28 PM Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> On 08/09/2018 09:04 AM, "André Hartmann" wrote:
> >>> +
> >>> + 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.
>
> You're right. The kernel expects you to set bitrate and data_bitrate at
> the same time to keep the interface in a consistent state.
>
> > 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)
I did not think about that, good point.
> Or we need a function to set the bitrate and data_bitrate at the same
> time, just like the kernel expects it.
But how should this function(s) look like?
There is currently 3 ways to set the bitrate:
* can_set_bittiming
* can_set_bitrate
* can_set_bitrate_samplepoint
can_set_bitrate and can_set_bitrate_samplepoint calculates bittiming that
it internaly uses to call can_set_bittiming.
Should we make functions that returns the bittiming struct
so one later can use to set bitrate and databitrate,
I guess that will be a little confusing for current users.
One could make 3 functions that uses the same way to set bitrate and
databitrate,
or just make 9 functions to cover every combination.
Lars
