Assign 'codec->inc' first and then use it instead of the hardcoded
value 4 repeatedly.

Replace the if/else statement with a ternary operator and calculate
'codec->mod' directly. Remove the unnecessary local variable 'mod'.

Return the computed rate directly instead of updating the local variable
first.

No functional changes intended.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 sound/mips/hal2.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
index 991793e6bda9..dd74bab531b4 100644
--- a/sound/mips/hal2.c
+++ b/sound/mips/hal2.c
@@ -313,21 +313,11 @@ static irqreturn_t hal2_interrupt(int irq, void *dev_id)
 
 static int hal2_compute_rate(struct hal2_codec *codec, unsigned int rate)
 {
-	unsigned short mod;
-
-	if (44100 % rate < 48000 % rate) {
-		mod = 4 * 44100 / rate;
-		codec->master = 44100;
-	} else {
-		mod = 4 * 48000 / rate;
-		codec->master = 48000;
-	}
-
 	codec->inc = 4;
-	codec->mod = mod;
-	rate = 4 * codec->master / mod;
+	codec->master = (44100 % rate < 48000 % rate) ? 44100 : 48000;
+	codec->mod = codec->inc * codec->master / rate;
 
-	return rate;
+	return codec->inc * codec->master / codec->mod;
 }
 
 static void hal2_set_dac_rate(struct snd_hal2 *hal2)
-- 
2.50.0

On Mon, 30 Jun 2025 23:45:52 +0200,
Thorsten Blum wrote:
> 
> Assign 'codec->inc' first and then use it instead of the hardcoded
> value 4 repeatedly.
> 
> Replace the if/else statement with a ternary operator and calculate
> 'codec->mod' directly. Remove the unnecessary local variable 'mod'.
> 
> Return the computed rate directly instead of updating the local variable
> first.
> 
> No functional changes intended.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  sound/mips/hal2.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> index 991793e6bda9..dd74bab531b4 100644
> --- a/sound/mips/hal2.c
> +++ b/sound/mips/hal2.c
> @@ -313,21 +313,11 @@ static irqreturn_t hal2_interrupt(int irq, void *dev_id)
>  
>  static int hal2_compute_rate(struct hal2_codec *codec, unsigned int rate)
>  {
> -	unsigned short mod;
> -
> -	if (44100 % rate < 48000 % rate) {
> -		mod = 4 * 44100 / rate;
> -		codec->master = 44100;
> -	} else {
> -		mod = 4 * 48000 / rate;
> -		codec->master = 48000;
> -	}
> -
>  	codec->inc = 4;
> -	codec->mod = mod;
> -	rate = 4 * codec->master / mod;
> +	codec->master = (44100 % rate < 48000 % rate) ? 44100 : 48000;
> +	codec->mod = codec->inc * codec->master / rate;
>  
> -	return rate;
> +	return codec->inc * codec->master / codec->mod;

IMHO, this doesn't look improving the code readability than the
original code.  And the generated code doesn't seem significantly
better, either.


thanks,

Takashi

On 1. Jul 2025, at 11:25, Takashi Iwai wrote:
> IMHO, this doesn't look improving the code readability than the
> original code.  And the generated code doesn't seem significantly
> better, either.

I didn't look at the generated code, but I think the patch definitely
improves the function (not necessarily its runtime, but its readability
and maintainability).

I think the patch primarily improves maintainability by eliminating the
magic number '4' that was scattered throughout the function. Now the
scaling factor is assigned once to the semantically more meaningful
variable 'codec->inc' and used consistently.

It also improves consistency by using 'codec->master' when calculating
'codec->mod' instead of repeating the constants '44100' and '48000'.

Additionally, it removes the unnecessary local variable 'mod' and the
'rate' update, making the function more concise (4 vs 12 lines).

Thanks,
Thorsten

On Tue, 01 Jul 2025 12:18:44 +0200,
Thorsten Blum wrote:
> 
> On 1. Jul 2025, at 11:25, Takashi Iwai wrote:
> > IMHO, this doesn't look improving the code readability than the
> > original code.  And the generated code doesn't seem significantly
> > better, either.
> 
> I didn't look at the generated code, but I think the patch definitely
> improves the function (not necessarily its runtime, but its readability
> and maintainability).
> 
> I think the patch primarily improves maintainability by eliminating the
> magic number '4' that was scattered throughout the function. Now the
> scaling factor is assigned once to the semantically more meaningful
> variable 'codec->inc' and used consistently.
> 
> It also improves consistency by using 'codec->master' when calculating
> 'codec->mod' instead of repeating the constants '44100' and '48000'.
> 
> Additionally, it removes the unnecessary local variable 'mod' and the
> 'rate' update, making the function more concise (4 vs 12 lines).

The line length doesn't matter.  It's a small code.  And it's no
hot-path, and no common code used by many drivers, either.
That is, the only question is how better the code is readable.
And, often a simple if-block can be easier to follow the code flow --
that's your case, too.

In anyway, this is really really minor issue and we shouldn't waste
too much time just for this kind of optimization. 


thanks,

Takashi