On Sun. 16 Jan 2021 at 04:00, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 1/15/21 7:27 PM, Marc Kleine-Budde wrote:
> >>> Option 4:
> >>> We can introduce a struct can_bitiming_fd with the first member being the struct
> >>> can_bitiming and add tdc related variables after that. This way we can use the
> >>> same function to calculate the bit timing on both CAN and CAN-FD.
> >>
> >> While option 3 is slightly easier, my preference will go to option 4.
> >
> > We still need the netlink enhancement from option 3.
>
> Just tried option 4.
>
> > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> > index 7faf6a37d5b2..bf2326fe22a1 100644
> > --- a/include/linux/can/dev.h
> > +++ b/include/linux/can/dev.h
> > @@ -32,6 +32,10 @@ enum can_mode {
> > CAN_MODE_SLEEP
> > };
> >
> > +struct canfd_bittiming {
> > + struct can_bittiming dbt;
>
> tdc to be added here....
>
> > +};
> > +
> > /*
> > * CAN common private data
> > */
> > @@ -39,7 +43,8 @@ struct can_priv {
> > struct net_device *dev;
> > struct can_device_stats can_stats;
> >
> > - struct can_bittiming bittiming, data_bittiming;
> > + struct can_bittiming bittiming;
> > + struct canfd_bittiming data_bittiming;
> > const struct can_bittiming_const *bittiming_const,
> > *data_bittiming_const;
> > const u16 *termination_const;
>
> But I had to add that ".dbt" everywhere....
>
> > --- a/drivers/net/can/dev/dev.c
> > +++ b/drivers/net/can/dev/dev.c
> > @@ -345,8 +345,8 @@ int open_candev(struct net_device *dev)
> >
> > /* For CAN FD the data bitrate has to be >= the arbitration bitrate */
> > if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
> > - (!priv->data_bittiming.bitrate ||
> > - priv->data_bittiming.bitrate < priv->bittiming.bitrate)) {
> > + (!priv->data_bittiming.dbt.bitrate ||
> > + priv->data_bittiming.dbt.bitrate < priv->bittiming.bitrate)) {
> > netdev_err(dev, "incorrect/missing data bit-timing\n");
> > return -EINVAL;
> > }
> > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > index 3ae884cdf677..c8341cbd8a66 100644
> > --- a/drivers/net/can/dev/netlink.c
> > +++ b/drivers/net/can/dev/netlink.c
> > @@ -239,7 +239,7 @@ static size_t can_get_size(const struct net_device *dev)
> > size += nla_total_size(sizeof(u32)); /* IFLA_CAN_RESTART_MS */
> > if (priv->do_get_berr_counter) /* IFLA_CAN_BERR_COUNTER */
> > size += nla_total_size(sizeof(struct can_berr_counter));
> > - if (priv->data_bittiming.bitrate) /* IFLA_CAN_DATA_BITTIMING */
> > + if (priv->data_bittiming.dbt.bitrate) /* IFLA_CAN_DATA_BITTIMING */
> > size += nla_total_size(sizeof(struct can_bittiming));
> > if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
> > size += nla_total_size(sizeof(struct can_bittiming_const));
> > @@ -286,7 +286,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
> > !priv->do_get_berr_counter(dev, &bec) &&
> > nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
> >
> > - (priv->data_bittiming.bitrate &&
> > + (priv->data_bittiming.dbt.bitrate &&
> > nla_put(skb, IFLA_CAN_DATA_BITTIMING,
> > sizeof(priv->data_bittiming), &priv->data_bittiming)) ||
> >
>
> And that's not even everything.
I also expected that many lines would have to be changed:
$ grep -R data_bittiming drivers/net/can/dev | wc -l
15
And (full kernel tree):
$ grep -R data_bittiming | wc -l
126
But those changes are all trivial, that's why I liked the idea.
> I think best it to add a "struct can_tdc" directly to the struct can_priv. Then
> add a netlink interface that returns the existing can_bittiming and the can_tdc,
> not as a struct, but each member with a separate tag (or whatever it's called).
Not a bad idea, still need to think about the pros and cons.
For now, I will use the "struct can_tdc" in my RFC. Right now, I
will not be working on the netlink interface immediately. I will
first focus on modifying the ES58X driver's FIFO and go back to
the netlink interface later.
Yours sincerely,
Vincent
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
