Re: [PATCH] ASoC: soc-core: Stop using of_property_read_bool() for non-boolean properties

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Morimoto-san,

On Thu, Jan 23, 2025 at 12:43 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> > On R-Car:
> >
> >     OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value.
> >     OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value.
> >
> > or:
> >
> >     OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'bitclock-master' with a value.
> >     OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'frame-master' with a value.
> >
> > The use of of_property_read_bool() for non-boolean properties is
> > deprecated in favor of of_property_present() when testing for property
> > presence.
> >
> > Replace testing for presence before calling of_property_read_u32() by
> > testing for an -EINVAL return value from the latter, to simplify the
> > code.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> (snip)
> > -     if (of_property_read_bool(np, "dai-tdm-slot-num")) {
> > -             ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> > -             if (ret)
> > -                     return ret;
> > -
> > -             if (slots)
> > -                     *slots = val;
> > -     }
> (snip)
> > +     ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> > +     if (ret && ret != -EINVAL)
> > +             return ret;
> > +     if (!ret && slots)
> > +             *slots = val;
>
> Looks good to me
>
> Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>

Thank you!

> If my understanding was correct, old/new code should have same behavior.

Indeed, that was my objective...

> But because of the original code, new code looks complex for me.
> The case which this function return error are
>
>         (A) if property does not have a value
>         (B) if the property data isn't large enough
>
> I think "DT checker" will indicates error for both case ?

Correct, of_property_read_u32_array() would return -ENODATA resp.
-EOVERFLOW.

> If so, we can simply ignore these 2 cases. Then, the code will be more
> simple
>
>         ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> -       if (ret && ret != -EINVAL)
> -               return ret;
>         if (!ret && slots)
>                 *slots = val;
>
> I think this should be extra new patch (if people can agree about it).

That would be a change in behavior. Probably it would be fine for
existing users, though, as no existing DTS should cause these errors,
else sound wouldn't work.  For a new DTS, it would silently ignore errors.
You are in a better position to make that decision, though.

BTW, is there any specific reason the code always checks for the
presence of "dai-tdm-slot-num", even if slots is NULL, and the result
sn't used? I.e. would

    if (slots) {
            ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
            if (!ret)
                    *slots = val;
            else if (ret != -EINVAL)
                    return ret;
    }

(perhaps dropping the else, as per above) be acceptable?
Thanks!

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