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
