On Sun. 16 Jan 2021 at 04:00, Marc Kleine-Budde [off-list ref] wrote:

On 1/15/21 7:27 PM, Marc Kleine-Budde wrote:

quoted

quoted

quoted

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.

quoted

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....

quoted

+};
+
 /*
  * 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....

quoted

--- 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 |