[PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

[PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

[Xichao Zhao]

Replace the if-else statement with a ternary operator to
set cdev->irq_timer_wait. Use us_to_ktime() instead of
ns_to_ktime() with NSEC_PER_USEC multiplication. Simplify
the assignment of cdev->irq_timer_wait by combining
conditional checks into a single line.

Signed-off-by: Xichao Zhao <zhao.xichao@vivo.com>
---
v2: Simplify code. Modify title and content.
---
 drivers/net/can/m_can/m_can.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index fe74dbd2c966..d4621346e76a 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2211,13 +2211,9 @@ static int m_can_set_coalesce(struct net_device *dev,
 	cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
 	cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
 	cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
-
-	if (cdev->rx_coalesce_usecs_irq)
-		cdev->irq_timer_wait =
-			ns_to_ktime(cdev->rx_coalesce_usecs_irq * NSEC_PER_USEC);
-	else
-		cdev->irq_timer_wait =
-			ns_to_ktime(cdev->tx_coalesce_usecs_irq * NSEC_PER_USEC);
+	cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq ?
+					   cdev->rx_coalesce_usecs_irq :
+					   cdev->tx_coalesce_usecs_irq);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

[Markus Elfring]

> Replace the if-else statement with a ternary operator to
> set cdev->irq_timer_wait. Use us_to_ktime() instead of
> ns_to_ktime() with NSEC_PER_USEC multiplication. Simplify
…

You should occasionally use more than 57 characters in text lines
of such a change description.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc3#n638


Will an enumeration become helpful here?


…> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2211,13 +2211,9 @@ static int m_can_set_coalesce(struct net_device *dev,
…> +	cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq ?
> +					   cdev->rx_coalesce_usecs_irq :
> +					   cdev->tx_coalesce_usecs_irq);
…

I am curious how coding style preferences will evolve further also for
the usage of the conditional operator at such a place.

Regards,
Markus

Re: [PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

[Vincent Mailhol]

On 26/08/2025 at 14:43, Markus Elfring wrote:
>> Replace the if-else statement with a ternary operator to
>> set cdev->irq_timer_wait. Use us_to_ktime() instead of
>> ns_to_ktime() with NSEC_PER_USEC multiplication. Simplify
> …
> 
> You should occasionally use more than 57 characters in text lines
> of such a change description.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc3#n638
> 
> 
> Will an enumeration become helpful here?
> 
> 
> …> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -2211,13 +2211,9 @@ static int m_can_set_coalesce(struct net_device *dev,
> …> +	cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq ?
>> +					   cdev->rx_coalesce_usecs_irq :
>> +					   cdev->tx_coalesce_usecs_irq);
> …
> 
> I am curious how coding style preferences will evolve further also for
> the usage of the conditional operator at such a place.

The preferred style in the kernel is actually to *not* use the conditional
operator in such case and to use an explicit if/else. I appreciate that the
conditional operator is more succinct, but squeezing the code is not a goal. The
priority is readability, and the if/else does a better job at this.

And I this is not my personnal opinion. For example, see this message from Greg:

  https://lore.kernel.org/all/20250311150130.7a875e63@bahia/

TLDR; the v1 was better than the v2. Speaking of the format, the only nitpick I
might have is that after your change, the code fits in one line without
exceeding the 80th column:

	if (cdev->rx_coalesce_usecs_irq)
		cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq);
 	else
		cdev->irq_timer_wait = us_to_ktime(cdev->tx_coalesce_usecs_irq);


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On 26.08.2025 17:38:17, Vincent Mailhol wrote:
[...]
> TLDR; the v1 was better than the v2. Speaking of the format, the only nitpick I
> might have is that after your change, the code fits in one line without
> exceeding the 80th column:
> 
> 	if (cdev->rx_coalesce_usecs_irq)
> 		cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq);
>  	else
> 		cdev->irq_timer_wait = us_to_ktime(cdev->tx_coalesce_usecs_irq);

Good idea! Fixed why applying v1.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]