* [PATCH] can: bittiming: replace CAN units with the SI metric
@ 2021-11-19 16:18 Vincent Mailhol
2021-11-21 18:27 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Vincent Mailhol @ 2021-11-19 16:18 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, Vincent Mailhol, Jimmy Assarsson
In [1], we introduced a set of units in linux/can/bittiming.h. Since
then, generic SI prefix were added to linux/units.h in [2]. Those new
prefix can perfectly replace the CAN specific units.
This patch replaces all occurrences of the CAN units with their
corresponding prefix according to below table.
CAN units SI metric prefix
-------------------------------
CAN_KBPS KILO
CAN_MBPS MEGA
CAM_MHZ MEGA
The macro declarations are then removed from linux/can/bittiming.h
[1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and
CAN_MHZ macros")
[2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions")
Suggested-by: Jimmy Assarsson <extja@kvaser.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/bittiming.c | 5 +++--
drivers/net/can/usb/etas_es58x/es581_4.c | 5 +++--
drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +++--
include/linux/can/bittiming.h | 7 -------
4 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 0509625c3082..a5c9f973802a 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -4,6 +4,7 @@
* Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
*/
+#include <linux/units.h>
#include <linux/can/dev.h>
#ifdef CONFIG_CAN_CALC_BITTIMING
@@ -81,9 +82,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
if (bt->sample_point) {
sample_point_nominal = bt->sample_point;
} else {
- if (bt->bitrate > 800 * CAN_KBPS)
+ if (bt->bitrate > 800 * KILO)
sample_point_nominal = 750;
- else if (bt->bitrate > 500 * CAN_KBPS)
+ else if (bt->bitrate > 500 * KILO)
sample_point_nominal = 800;
else
sample_point_nominal = 875;
diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
index 14e360c9f2c9..ed340141c712 100644
--- a/drivers/net/can/usb/etas_es58x/es581_4.c
+++ b/drivers/net/can/usb/etas_es58x/es581_4.c
@@ -10,6 +10,7 @@
*/
#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},
.ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC,
.tx_start_of_frame = 0xAFAF,
.rx_start_of_frame = 0xFAFA,
diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
index 4f0cae29f4d8..aec299bed6dc 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
@@ -12,6 +12,7 @@
*/
#include <linux/kernel.h>
+#include <linux/units.h>
#include <asm/unaligned.h>
#include "es58x_core.h"
@@ -522,8 +523,8 @@ const struct es58x_parameters es58x_fd_param = {
* Mbps work in an optimal environment but are not recommended
* for production environment.
*/
- .bitrate_max = 8 * CAN_MBPS,
- .clock = {.freq = 80 * CAN_MHZ},
+ .bitrate_max = 8 * MEGA,
+ .clock = {.freq = 80 * MEGA},
.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 20b50baf3a02..a81652d1c6f3 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -12,13 +12,6 @@
#define CAN_SYNC_SEG 1
-/* Kilobits and Megabits per second */
-#define CAN_KBPS 1000UL
-#define CAN_MBPS 1000000UL
-
-/* Megahertz */
-#define CAN_MHZ 1000000UL
-
#define CAN_CTRLMODE_TDC_MASK \
(CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] can: bittiming: replace CAN units with the SI metric
2021-11-19 16:18 [PATCH] can: bittiming: replace CAN units with the SI metric Vincent Mailhol
@ 2021-11-21 18:27 ` Oliver Hartkopp
2021-11-22 2:22 ` Vincent MAILHOL
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2021-11-21 18:27 UTC (permalink / raw)
To: Vincent Mailhol, Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, Jimmy Assarsson
On 19.11.21 17:18, Vincent Mailhol wrote:
> In [1], we introduced a set of units in linux/can/bittiming.h. Since
> then, generic SI prefix were added to linux/units.h in [2]. Those new
> prefix can perfectly replace the CAN specific units.
>
> This patch replaces all occurrences of the CAN units with their
> corresponding prefix according to below table.
>
> CAN units SI metric prefix
> -------------------------------
> CAN_KBPS KILO
> CAN_MBPS MEGA
> CAM_MHZ MEGA
>
> The macro declarations are then removed from linux/can/bittiming.h
>
> [1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and
> CAN_MHZ macros")
>
> [2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions")
>
> Suggested-by: Jimmy Assarsson <extja@kvaser.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> drivers/net/can/dev/bittiming.c | 5 +++--
> drivers/net/can/usb/etas_es58x/es581_4.c | 5 +++--
> drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +++--
> include/linux/can/bittiming.h | 7 -------
> 4 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> index 0509625c3082..a5c9f973802a 100644
> --- a/drivers/net/can/dev/bittiming.c
> +++ b/drivers/net/can/dev/bittiming.c
> @@ -4,6 +4,7 @@
> * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
> */
>
> +#include <linux/units.h>
> #include <linux/can/dev.h>
>
> #ifdef CONFIG_CAN_CALC_BITTIMING
> @@ -81,9 +82,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> if (bt->sample_point) {
> sample_point_nominal = bt->sample_point;
> } else {
> - if (bt->bitrate > 800 * CAN_KBPS)
> + if (bt->bitrate > 800 * KILO)
> sample_point_nominal = 750;
> - else if (bt->bitrate > 500 * CAN_KBPS)
> + else if (bt->bitrate > 500 * KILO)
> sample_point_nominal = 800;
> else
> sample_point_nominal = 875;
> diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
> index 14e360c9f2c9..ed340141c712 100644
> --- a/drivers/net/can/usb/etas_es58x/es581_4.c
> +++ b/drivers/net/can/usb/etas_es58x/es581_4.c
> @@ -10,6 +10,7 @@
> */
>
> #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.
So where is the Hz information then?
> .ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC,
> .tx_start_of_frame = 0xAFAF,
> .rx_start_of_frame = 0xFAFA,
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> index 4f0cae29f4d8..aec299bed6dc 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> @@ -12,6 +12,7 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/units.h>
> #include <asm/unaligned.h>
>
> #include "es58x_core.h"
> @@ -522,8 +523,8 @@ const struct es58x_parameters es58x_fd_param = {
> * Mbps work in an optimal environment but are not recommended
> * for production environment.
> */
> - .bitrate_max = 8 * CAN_MBPS,
> - .clock = {.freq = 80 * CAN_MHZ},
> + .bitrate_max = 8 * MEGA,
> + .clock = {.freq = 80 * MEGA},
> .ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
> CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
> CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
> diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> index 20b50baf3a02..a81652d1c6f3 100644
> --- a/include/linux/can/bittiming.h
> +++ b/include/linux/can/bittiming.h
> @@ -12,13 +12,6 @@
> #define CAN_SYNC_SEG 1
>
>
> -/* Kilobits and Megabits per second */
> -#define CAN_KBPS 1000UL
> -#define CAN_MBPS 1000000UL
> -
> -/* Megahertz */
> -#define CAN_MHZ 1000000UL
So what about
#define CAN_KBPS KILO /* kilo bits per second */
#define CAN_MBPS MEGA /* mega bits per second */
#define CAN_MHZ MEGA /* mega hertz */
??
Regards,
Oliver
> -
> #define CAN_CTRLMODE_TDC_MASK \
> (CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] can: bittiming: replace CAN units with the SI metric
2021-11-21 18:27 ` Oliver Hartkopp
@ 2021-11-22 2:22 ` Vincent MAILHOL
2021-11-23 7:44 ` Jimmy Assarsson
2021-11-23 20:53 ` Oliver Hartkopp
0 siblings, 2 replies; 6+ messages in thread
From: Vincent MAILHOL @ 2021-11-22 2:22 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel,
Jimmy Assarsson
Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@hartkopp.net> a écrit :
>
>
>
> On 19.11.21 17:18, Vincent Mailhol wrote:
> > In [1], we introduced a set of units in linux/can/bittiming.h. Since
> > then, generic SI prefix were added to linux/units.h in [2]. Those new
> > prefix can perfectly replace the CAN specific units.
> >
> > This patch replaces all occurrences of the CAN units with their
> > corresponding prefix according to below table.
> >
> > CAN units SI metric prefix
> > -------------------------------
> > CAN_KBPS KILO
> > CAN_MBPS MEGA
> > CAM_MHZ MEGA
> >
> > The macro declarations are then removed from linux/can/bittiming.h
> >
> > [1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and
> > CAN_MHZ macros")
> >
> > [2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions")
> >
> > Suggested-by: Jimmy Assarsson <extja@kvaser.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > drivers/net/can/dev/bittiming.c | 5 +++--
> > drivers/net/can/usb/etas_es58x/es581_4.c | 5 +++--
> > drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +++--
> > include/linux/can/bittiming.h | 7 -------
> > 4 files changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> > index 0509625c3082..a5c9f973802a 100644
> > --- a/drivers/net/can/dev/bittiming.c
> > +++ b/drivers/net/can/dev/bittiming.c
> > @@ -4,6 +4,7 @@
> > * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
> > */
> >
> > +#include <linux/units.h>
> > #include <linux/can/dev.h>
> >
> > #ifdef CONFIG_CAN_CALC_BITTIMING
> > @@ -81,9 +82,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> > if (bt->sample_point) {
> > sample_point_nominal = bt->sample_point;
> > } else {
> > - if (bt->bitrate > 800 * CAN_KBPS)
> > + if (bt->bitrate > 800 * KILO)
> > sample_point_nominal = 750;
> > - else if (bt->bitrate > 500 * CAN_KBPS)
> > + else if (bt->bitrate > 500 * KILO)
> > sample_point_nominal = 800;
> > else
> > sample_point_nominal = 875;
> > diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
> > index 14e360c9f2c9..ed340141c712 100644
> > --- a/drivers/net/can/usb/etas_es58x/es581_4.c
> > +++ b/drivers/net/can/usb/etas_es58x/es581_4.c
> > @@ -10,6 +10,7 @@
> > */
> >
> > #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?
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
> > .ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC,
> > .tx_start_of_frame = 0xAFAF,
> > .rx_start_of_frame = 0xFAFA,
> > diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > index 4f0cae29f4d8..aec299bed6dc 100644
> > --- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > @@ -12,6 +12,7 @@
> > */
> >
> > #include <linux/kernel.h>
> > +#include <linux/units.h>
> > #include <asm/unaligned.h>
> >
> > #include "es58x_core.h"
> > @@ -522,8 +523,8 @@ const struct es58x_parameters es58x_fd_param = {
> > * Mbps work in an optimal environment but are not recommended
> > * for production environment.
> > */
> > - .bitrate_max = 8 * CAN_MBPS,
> > - .clock = {.freq = 80 * CAN_MHZ},
> > + .bitrate_max = 8 * MEGA,
> > + .clock = {.freq = 80 * MEGA},
> > .ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
> > CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
> > CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
> > diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > index 20b50baf3a02..a81652d1c6f3 100644
> > --- a/include/linux/can/bittiming.h
> > +++ b/include/linux/can/bittiming.h
> > @@ -12,13 +12,6 @@
> > #define CAN_SYNC_SEG 1
> >
> >
> > -/* Kilobits and Megabits per second */
> > -#define CAN_KBPS 1000UL
> > -#define CAN_MBPS 1000000UL
> > -
> > -/* Megahertz */
> > -#define CAN_MHZ 1000000UL
>
> So what about
>
> #define CAN_KBPS KILO /* kilo bits per second */
> #define CAN_MBPS MEGA /* mega bits per second */
>
> #define CAN_MHZ MEGA /* mega hertz */
>
>
> ??
>
> Regards,
> Oliver
>
>
> > -
> > #define CAN_CTRLMODE_TDC_MASK \
> > (CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] can: bittiming: replace CAN units with the SI metric
2021-11-22 2:22 ` Vincent MAILHOL
@ 2021-11-23 7:44 ` Jimmy Assarsson
2021-11-23 20:53 ` Oliver Hartkopp
1 sibling, 0 replies; 6+ messages in thread
From: Jimmy Assarsson @ 2021-11-23 7:44 UTC (permalink / raw)
To: Vincent MAILHOL, Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel
On 2021-11-22 03:22, Vincent MAILHOL wrote:
> Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@hartkopp.net> a écrit :
>> On 19.11.21 17:18, Vincent Mailhol wrote:
>>> In [1], we introduced a set of units in linux/can/bittiming.h. Since
>>> then, generic SI prefix were added to linux/units.h in [2]. Those new
>>> prefix can perfectly replace the CAN specific units.
>>>
>>> This patch replaces all occurrences of the CAN units with their
>>> corresponding prefix according to below table.
>>>
>>> CAN units SI metric prefix
>>> -------------------------------
>>> CAN_KBPS KILO
>>> CAN_MBPS MEGA
>>> CAM_MHZ MEGA
>>>
>>> The macro declarations are then removed from linux/can/bittiming.h
>>>
>>> [1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and
>>> CAN_MHZ macros")
>>>
>>> [2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions")
>>>
>>> Suggested-by: Jimmy Assarsson <extja@kvaser.com>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> ---
>>> drivers/net/can/dev/bittiming.c | 5 +++--
>>> drivers/net/can/usb/etas_es58x/es581_4.c | 5 +++--
>>> drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +++--
>>> include/linux/can/bittiming.h | 7 -------
>>> 4 files changed, 9 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
>>> index 0509625c3082..a5c9f973802a 100644
>>> --- a/drivers/net/can/dev/bittiming.c
>>> +++ b/drivers/net/can/dev/bittiming.c
>>> @@ -4,6 +4,7 @@
>>> * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
>>> */
>>>
>>> +#include <linux/units.h>
>>> #include <linux/can/dev.h>
>>>
>>> #ifdef CONFIG_CAN_CALC_BITTIMING
>>> @@ -81,9 +82,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
>>> if (bt->sample_point) {
>>> sample_point_nominal = bt->sample_point;
>>> } else {
>>> - if (bt->bitrate > 800 * CAN_KBPS)
>>> + if (bt->bitrate > 800 * KILO)
>>> sample_point_nominal = 750;
>>> - else if (bt->bitrate > 500 * CAN_KBPS)
>>> + else if (bt->bitrate > 500 * KILO)
>>> sample_point_nominal = 800;
>>> else
>>> sample_point_nominal = 875;
>>> diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
>>> index 14e360c9f2c9..ed340141c712 100644
>>> --- a/drivers/net/can/usb/etas_es58x/es581_4.c
>>> +++ b/drivers/net/can/usb/etas_es58x/es581_4.c
>>> @@ -10,6 +10,7 @@
>>> */
>>>
>>> #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?
>
> 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.
Came across linux/units.h when looking at a different driver, and thought
that it was also possible to utilize them in the CAN drivers.
I've no strong opinion about any of the suggested solutions.
I'm fine with keeping it as it is, just wanted to raise the question :)
Best regards,
jimmy
>> 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
>
>>> .ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC,
>>> .tx_start_of_frame = 0xAFAF,
>>> .rx_start_of_frame = 0xFAFA,
>>> diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
>>> index 4f0cae29f4d8..aec299bed6dc 100644
>>> --- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
>>> +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
>>> @@ -12,6 +12,7 @@
>>> */
>>>
>>> #include <linux/kernel.h>
>>> +#include <linux/units.h>
>>> #include <asm/unaligned.h>
>>>
>>> #include "es58x_core.h"
>>> @@ -522,8 +523,8 @@ const struct es58x_parameters es58x_fd_param = {
>>> * Mbps work in an optimal environment but are not recommended
>>> * for production environment.
>>> */
>>> - .bitrate_max = 8 * CAN_MBPS,
>>> - .clock = {.freq = 80 * CAN_MHZ},
>>> + .bitrate_max = 8 * MEGA,
>>> + .clock = {.freq = 80 * MEGA},
>>> .ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
>>> CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
>>> CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
>>> diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
>>> index 20b50baf3a02..a81652d1c6f3 100644
>>> --- a/include/linux/can/bittiming.h
>>> +++ b/include/linux/can/bittiming.h
>>> @@ -12,13 +12,6 @@
>>> #define CAN_SYNC_SEG 1
>>>
>>>
>>> -/* Kilobits and Megabits per second */
>>> -#define CAN_KBPS 1000UL
>>> -#define CAN_MBPS 1000000UL
>>> -
>>> -/* Megahertz */
>>> -#define CAN_MHZ 1000000UL
>>
>> So what about
>>
>> #define CAN_KBPS KILO /* kilo bits per second */
>> #define CAN_MBPS MEGA /* mega bits per second */
>>
>> #define CAN_MHZ MEGA /* mega hertz */
>>
>>
>> ??
>>
>> Regards,
>> Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] can: bittiming: replace CAN units with the SI metric
2021-11-22 2:22 ` Vincent MAILHOL
2021-11-23 7:44 ` Jimmy Assarsson
@ 2021-11-23 20:53 ` Oliver Hartkopp
2021-11-23 23:26 ` Vincent MAILHOL
1 sibling, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2021-11-23 20:53 UTC (permalink / raw)
To: Vincent MAILHOL
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel,
Jimmy Assarsson
Hi Vincent,
On 22.11.21 03:22, Vincent MAILHOL wrote:
> Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@hartkopp.net> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] can: bittiming: replace CAN units with the SI metric
2021-11-23 20:53 ` Oliver Hartkopp
@ 2021-11-23 23:26 ` Vincent MAILHOL
0 siblings, 0 replies; 6+ messages in thread
From: Vincent MAILHOL @ 2021-11-23 23:26 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel,
Jimmy Assarsson
On Wed. 24 Nov. 2021 à 05:53, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
> On 22.11.21 03:22, Vincent MAILHOL wrote:
> > Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@hartkopp.net> 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?
This works with. Actually, I also hesitated to add such comments
when writing this patch. For the sake of the quality of the
documentation, I will prepare a v2.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-23 23:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-19 16:18 [PATCH] can: bittiming: replace CAN units with the SI metric Vincent Mailhol
2021-11-21 18:27 ` Oliver Hartkopp
2021-11-22 2:22 ` Vincent MAILHOL
2021-11-23 7:44 ` Jimmy Assarsson
2021-11-23 20:53 ` Oliver Hartkopp
2021-11-23 23:26 ` Vincent MAILHOL
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox