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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Claudiu,

On Wed, Nov 6, 2024 at 4:17 PM Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> On 06.11.2024 16:56, Geert Uytterhoeven wrote:
> > On Wed, Nov 6, 2024 at 9:19 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxx>
> >
> > 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds