diff mbox series

[12/31] ASoC: sh: rz-ssi: Use a proper bitmask for clear bits

Message ID 20241106081826.1211088-13-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Headers show
Series Add audio support for the Renesas RZ/G3S SoC | expand

Commit Message

Claudiu Nov. 6, 2024, 8:18 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

While it is still correct to pass zero as the bit-clear mask it may be
confusing. For this, use a proper bitmask for clear bits.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 sound/soc/renesas/rz-ssi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven Nov. 6, 2024, 2:56 p.m. UTC | #1 
Hi Claudiu,

On Wed, Nov 6, 2024 at 9:19 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> While it is still correct to pass zero as the bit-clear mask it may be
> confusing. For this, use a proper bitmask for clear bits.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/sound/soc/renesas/rz-ssi.c
> +++ b/sound/soc/renesas/rz-ssi.c
> @@ -331,7 +331,7 @@ static void rz_ssi_set_idle(struct rz_ssi_priv *ssi)
>                 dev_info(ssi->dev, "timeout waiting for SSI idle\n");
>
>         /* Hold FIFOs in reset */
> -       rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_FIFO_RST);
> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_FIFO_RST, SSIFCR_FIFO_RST);

But you're not clearing SSIFCR_FIFO_RST, you're setting it?

>  }
>
>  static int rz_ssi_start(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)

Gr{oetje,eeting}s,

                        Geert
Claudiu Nov. 6, 2024, 3:17 p.m. UTC | #2 
Hi, Geert,

On 06.11.2024 16:56, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Nov 6, 2024 at 9:19 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> While it is still correct to pass zero as the bit-clear mask it may be
>> confusing. For this, use a proper bitmask for clear bits.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/sound/soc/renesas/rz-ssi.c
>> +++ b/sound/soc/renesas/rz-ssi.c
>> @@ -331,7 +331,7 @@ static void rz_ssi_set_idle(struct rz_ssi_priv *ssi)
>>                 dev_info(ssi->dev, "timeout waiting for SSI idle\n");
>>
>>         /* Hold FIFOs in reset */
>> -       rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_FIFO_RST);
>> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_FIFO_RST, SSIFCR_FIFO_RST);
> 
> But you're not clearing SSIFCR_FIFO_RST, you're setting it?

The bits should be set to reset the FIFOs.

By "Use a proper bitmask for clear bits" phrase in the patch title or
description I was referring at the 3rd argument of the
rz_ssi_reg_mask_setl() function, which has the following prototype:

static void rz_ssi_reg_mask_setl(struct rz_ssi_priv *priv, uint reg,

                                 u32 bclr, u32 bset)


Would you prefer to rephrase it in the next version?

Thank you,
Claudiu Beznea

> 
>>  }
>>
>>  static int rz_ssi_start(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Geert Uytterhoeven Nov. 6, 2024, 3:21 p.m. UTC | #3 
Hi Claudiu,

On Wed, Nov 6, 2024 at 4:17 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 06.11.2024 16:56, Geert Uytterhoeven wrote:
> > On Wed, Nov 6, 2024 at 9:19 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> While it is still correct to pass zero as the bit-clear mask it may be
> >> confusing. For this, use a proper bitmask for clear bits.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> >> --- a/sound/soc/renesas/rz-ssi.c
> >> +++ b/sound/soc/renesas/rz-ssi.c
> >> @@ -331,7 +331,7 @@ static void rz_ssi_set_idle(struct rz_ssi_priv *ssi)
> >>                 dev_info(ssi->dev, "timeout waiting for SSI idle\n");
> >>
> >>         /* Hold FIFOs in reset */
> >> -       rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_FIFO_RST);
> >> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_FIFO_RST, SSIFCR_FIFO_RST);
> >
> > But you're not clearing SSIFCR_FIFO_RST, you're setting it?
>
> The bits should be set to reset the FIFOs.
>
> By "Use a proper bitmask for clear bits" phrase in the patch title or
> description I was referring at the 3rd argument of the
> rz_ssi_reg_mask_setl() function, which has the following prototype:
>
> static void rz_ssi_reg_mask_setl(struct rz_ssi_priv *priv, uint reg,
>
>                                  u32 bclr, u32 bset)
>
>
> Would you prefer to rephrase it in the next version?

The idea behind such functions is to pass a bitmask representing the
bits to be cleared to "bclr", and a bitmask representing the bits
to be set to "bset".  In this case, you do not want to clear any bits,
so the "bclr" parameter should be zero, and the original code is fine.

Gr{oetje,eeting}s,

                        Geert
Claudiu Nov. 6, 2024, 3:25 p.m. UTC | #4 
On 06.11.2024 17:21, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Nov 6, 2024 at 4:17 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 06.11.2024 16:56, Geert Uytterhoeven wrote:
>>> On Wed, Nov 6, 2024 at 9:19 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> While it is still correct to pass zero as the bit-clear mask it may be
>>>> confusing. For this, use a proper bitmask for clear bits.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/sound/soc/renesas/rz-ssi.c
>>>> +++ b/sound/soc/renesas/rz-ssi.c
>>>> @@ -331,7 +331,7 @@ static void rz_ssi_set_idle(struct rz_ssi_priv *ssi)
>>>>                 dev_info(ssi->dev, "timeout waiting for SSI idle\n");
>>>>
>>>>         /* Hold FIFOs in reset */
>>>> -       rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_FIFO_RST);
>>>> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_FIFO_RST, SSIFCR_FIFO_RST);
>>>
>>> But you're not clearing SSIFCR_FIFO_RST, you're setting it?
>>
>> The bits should be set to reset the FIFOs.
>>
>> By "Use a proper bitmask for clear bits" phrase in the patch title or
>> description I was referring at the 3rd argument of the
>> rz_ssi_reg_mask_setl() function, which has the following prototype:
>>
>> static void rz_ssi_reg_mask_setl(struct rz_ssi_priv *priv, uint reg,
>>
>>                                  u32 bclr, u32 bset)
>>
>>
>> Would you prefer to rephrase it in the next version?
> 
> The idea behind such functions is to pass a bitmask representing the
> bits to be cleared to "bclr", and a bitmask representing the bits
> to be set to "bset".  In this case, you do not want to clear any bits,
> so the "bclr" parameter should be zero, and the original code is fine.

OK, I'll will drop this patch.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
diff mbox series

Patch

diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
index f230d63339e8..47b82fe549ac 100644
--- a/sound/soc/renesas/rz-ssi.c
+++ b/sound/soc/renesas/rz-ssi.c
@@ -331,7 +331,7 @@  static void rz_ssi_set_idle(struct rz_ssi_priv *ssi)
 		dev_info(ssi->dev, "timeout waiting for SSI idle\n");
 
 	/* Hold FIFOs in reset */
-	rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_FIFO_RST);
+	rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_FIFO_RST, SSIFCR_FIFO_RST);
 }
 
 static int rz_ssi_start(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)