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

[Vincent Mailhol <mailhol@kernel.org>]

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