Re: [PATCH can-next v2 01/15] can: kvaser_usb: Add helper functions to convert device timestamp into ktime

[Jimmy Assarsson <jimmyassarsson@gmail.com>]

On 7/2/24 06:47, Vincent MAILHOL wrote:
> On Tue. 2 juil. 2024 at 00:50, Jimmy Assarsson <extja@kvaser.com> wrote:
>> Add helper function kvaser_usb_ticks_to_ktime() that converts from
>> device ticks to ktime.
>> And kvaser_usb_timestamp{48,64}_to_ktime() that converts from device
>> 48-bit or 64-bit timestamp, to ktime.
>>
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
>> ---
>> Changes in v2:
>>    - New in v2. Replaces
>>      can: kvaser_usb: Add function kvaser_usb_ticks_to_ktime()
>>    - Add two more helper functions, kvaser_usb_timestamp{48,64}_to_ktime()
>>      for converting timestamps, suggested by Vincent MAILHOL [2][3]
>> [2] https://lore.kernel.org/linux-can/CAMZ6RqKSa-6KjvgfmN9eL7A=A65gMkYsRrnaF41Azhsc45FA2Q@mail.gmail.com/
>> [3] https://lore.kernel.org/linux-can/CAMZ6Rq+Xd7+th=dKV+vrqzRtS+GY-xq2UziH1CURcQ3HxEXMqQ@mail.gmail.com/
>>
>>   drivers/net/can/usb/kvaser_usb/kvaser_usb.h   | 24 +++++++++++++++++++
>>   .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 10 ++++----
>>   2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> index ff10b3790d84..4256a0caae20 100644
>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> @@ -22,6 +22,8 @@
>>    */
>>
>>   #include <linux/completion.h>
>> +#include <linux/ktime.h>
>> +#include <linux/math64.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/types.h>
>>   #include <linux/usb.h>
>> @@ -216,4 +218,26 @@ int kvaser_usb_can_rx_over_error(struct net_device *netdev);
>>
>>   extern const struct can_bittiming_const kvaser_usb_flexc_bittiming_const;
>>
>> +static inline ktime_t kvaser_usb_ticks_to_ktime(const struct kvaser_usb_dev_cfg *cfg,
>> +                                               u64 ticks)
>> +{
>> +       return ns_to_ktime(div_u64(ticks * 1000, cfg->timestamp_freq));
>> +}
>> +
>> +static inline ktime_t kvaser_usb_timestamp48_to_ktime(const struct kvaser_usb_dev_cfg *cfg,
>> +                                                     const __le16 *timestamp)
>> +{
>> +       u64 ticks = le16_to_cpu(timestamp[0]) |
>> +                   (u64)(le16_to_cpu(timestamp[1])) << 16 |
>> +                   (u64)(le16_to_cpu(timestamp[2])) << 32;
>> +
>> +       return kvaser_usb_ticks_to_ktime(cfg, ticks);
>> +}
>> +
>> +static inline ktime_t kvaser_usb_timestamp64_to_ktime(const struct kvaser_usb_dev_cfg *cfg,
>> +                                                     __le64 timestamp)
>> +{
>> +       return kvaser_usb_ticks_to_ktime(cfg, le64_to_cpu(timestamp));
>> +}
>> +
>>   #endif /* KVASER_USB_H */
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
>> index c7ba768dfe17..ad1c6101a0cd 100644
>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
>> @@ -526,19 +526,17 @@ static ktime_t
>>   kvaser_usb_hydra_ktime_from_rx_cmd(const struct kvaser_usb_dev_cfg *cfg,
>>                                     const struct kvaser_cmd *cmd)
>>   {
>> -       u64 ticks;
>> +       ktime_t hwtstamp = 0;
>>
>>          if (cmd->header.cmd_no == CMD_EXTENDED) {
>>                  struct kvaser_cmd_ext *cmd_ext = (struct kvaser_cmd_ext *)cmd;
>>
>> -               ticks = le64_to_cpu(cmd_ext->rx_can.timestamp);
>> +               hwtstamp = kvaser_usb_timestamp64_to_ktime(cfg, cmd_ext->rx_can.timestamp);
>>          } else {
>> -               ticks = le16_to_cpu(cmd->rx_can.timestamp[0]);
>> -               ticks += (u64)(le16_to_cpu(cmd->rx_can.timestamp[1])) << 16;
>> -               ticks += (u64)(le16_to_cpu(cmd->rx_can.timestamp[2])) << 32;
>> +               hwtstamp = kvaser_usb_timestamp48_to_ktime(cfg, cmd->rx_can.timestamp);
>>          }
>>
>> -       return ns_to_ktime(div_u64(ticks * 1000, cfg->timestamp_freq));
>> +       return hwtstamp;
>>   }
> 
> Nitpick: this can slightly be simplified by dropping the hwtstamp
> local variable:
> 
>    kvaser_usb_hydra_ktime_from_rx_cmd(const struct kvaser_usb_dev_cfg *cfg,
>                                       const struct kvaser_cmd *cmd)
>    {
>           if (cmd->header.cmd_no == CMD_EXTENDED) {
>                   struct kvaser_cmd_ext *cmd_ext = (struct kvaser_cmd_ext *)cmd;
> 
>                   return kvaser_usb_timestamp64_to_ktime(cfg,
> cmd_ext->rx_can.timestamp);
>           } else {
>                   return kvaser_usb_timestamp48_to_ktime(cfg,
> cmd->rx_can.timestamp);
>           }
>    }

I'll keep it as is. I prefer to have a single return at the end of the function.

Regards,
/jimmy