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

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

[Geert Uytterhoeven]

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@glider.be>
---
Seen since commit c141ecc3cecd7647 ("of: Warn when
of_property_read_bool() is used on non-boolean properties") in
dt-rh/for-next.

I could not exercise all code paths, so review/testing would be
appreciated. Thanks!
---
 sound/soc/soc-core.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 3c6d8aef4130901c..26b34b6885083908 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3046,7 +3046,7 @@ int snd_soc_of_parse_pin_switches(struct snd_soc_card *card, const char *prop)
 	unsigned int i, nb_controls;
 	int ret;
 
-	if (!of_property_read_bool(dev->of_node, prop))
+	if (!of_property_present(dev->of_node, prop))
 		return 0;
 
 	strings = devm_kcalloc(dev, nb_controls_max,
@@ -3120,23 +3120,17 @@ int snd_soc_of_parse_tdm_slot(struct device_node *np,
 	if (rx_mask)
 		snd_soc_of_get_slot_mask(np, "dai-tdm-slot-rx-mask", rx_mask);
 
-	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;
-	}
-
-	if (of_property_read_bool(np, "dai-tdm-slot-width")) {
-		ret = of_property_read_u32(np, "dai-tdm-slot-width", &val);
-		if (ret)
-			return ret;
+	ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
+	if (ret && ret != -EINVAL)
+		return ret;
+	if (!ret && slots)
+		*slots = val;
 
-		if (slot_width)
-			*slot_width = val;
-	}
+	ret = of_property_read_u32(np, "dai-tdm-slot-width", &val);
+	if (ret && ret != -EINVAL)
+		return ret;
+	if (!ret && slot_width)
+		*slot_width = val;
 
 	return 0;
 }
@@ -3403,12 +3397,12 @@ unsigned int snd_soc_daifmt_parse_clock_provider_raw(struct device_node *np,
 	 * check "[prefix]frame-master"
 	 */
 	snprintf(prop, sizeof(prop), "%sbitclock-master", prefix);
-	bit = of_property_read_bool(np, prop);
+	bit = of_property_present(np, prop);
 	if (bit && bitclkmaster)
 		*bitclkmaster = of_parse_phandle(np, prop, 0);
 
 	snprintf(prop, sizeof(prop), "%sframe-master", prefix);
-	frame = of_property_read_bool(np, prop);
+	frame = of_property_present(np, prop);
 	if (frame && framemaster)
 		*framemaster = of_parse_phandle(np, prop, 0);
 
-- 
2.43.0

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

[Kuninori Morimoto]

Hi Geert, Mark

Thank you for the patch

> 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@glider.be>
> ---
(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@renesas.com>


If my understanding was correct, old/new code should have same behavior.
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 ?
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).

Thank you for your help !!

Best regards
---
Kuninori Morimoto

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

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, Jan 23, 2025 at 12:43 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> 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@glider.be>
> > ---
> (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@renesas.com>

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@linux-m68k.org

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

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

[Kuninori Morimoto]

Hi Geert, Mark

> >         ret = of_property_read_u32(np, "dai-tdm-slot-num", &val);
> > -       if (ret && ret != -EINVAL)
> > -               return ret;
> >         if (!ret && slots)
> >                 *slots = val;
(snip)
> 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.

Thanks
I will post that patch if Gerrt's patch was applied.

> 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;
>     }

I'm not 100% sure, but -num is used by the driver which can select the
number of TDM. Some driver uses default / fixed number. And -width too.
So I think these can be independent.


Thank you for your help !!

Best regards
---
Kuninori Morimoto

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

[Rob Herring]

On Wed, Jan 22, 2025 at 2:21 AM Geert Uytterhoeven
<geert+renesas@glider.be> 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@glider.be>
> ---
> Seen since commit c141ecc3cecd7647 ("of: Warn when
> of_property_read_bool() is used on non-boolean properties") in
> dt-rh/for-next.
>
> I could not exercise all code paths, so review/testing would be
> appreciated. Thanks!
> ---
>  sound/soc/soc-core.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)

Acked-by: Rob Herring (Arm) <robh@kernel.org>

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

[Mark Brown]

On Wed, 22 Jan 2025 09:21:27 +0100, Geert Uytterhoeven 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:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: soc-core: Stop using of_property_read_bool() for non-boolean properties
      commit: 6eab7034579917f207ca6d8e3f4e11e85e0ab7d5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark