Hi Vincent,
On 22.11.21 03:22, Vincent MAILHOL wrote:
Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> a écrit :
#include <linux/kernel.h>
+#include <linux/units.h>
#include <asm/unaligned.h>
#include "es58x_core.h"
@@ -469,8 +470,8 @@ const struct es58x_parameters es581_4_param = {
.bittiming_const = &es581_4_bittiming_const,
.data_bittiming_const = NULL,
.tdc_const = NULL,
- .bitrate_max = 1 * CAN_MBPS,
- .clock = {.freq = 50 * CAN_MHZ},
+ .bitrate_max = 1 * MEGA,
+ .clock = {.freq = 50 * MEGA},
IMO we are losing information here.
It feels you suggest to replace MHz with M.
When I introduced the CAN_{K,M}BPS and CAN_MHZ macros, my primary
intent was to avoid having to write more than five zeros in a
row (because the human brain is bad at counting those). And the
KILO/MEGA prefixes perfectly cover that intent.
You are correct to say that the information of the unit is
lost. But I assume this information to be implicit (frequencies
are in Hz, baudrate are in bits/second). So yes, I suggest
replacing MHz with M.
Do you really think that people will be confused by this change?
It is not about confusing people but about the quality of documentation
and readability.
I am not strongly opposed to keeping it either (hey, I was the
one who introduced it in the first place). I just think that
using linux/units.h is sufficient.
So where is the Hz information then?
It is in the comment of can_clock:freq :)
https://elixir.bootlin.com/linux/v5.15/source/include/uapi/linux/can/netlink.h#L63
Haha, you are funny ;-)
But the fact that you provide this URL shows that the information is not
found or easily accessible when someone reads the code here.
- .bitrate_max = 8 * CAN_MBPS,
- .clock = {.freq = 80 * CAN_MHZ},
+ .bitrate_max = 8 * MEGA,
+ .clock = {.freq = 80 * MEGA},
What about
+ .bitrate_max = 8 * MEGA, /* bits per second */
+ .clock = {.freq = 80 * MEGA}, /* Hz */
which uses the SI constants but maintains the unit?
Best regards,
Oliver
