Re: [PATCH 13/16] arm: dts: omap3pandora: create new DT node for the sound card

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double decrement of sample_rate_users on hw_params error path.
- [High] Using `IS_BUILTIN()` to change CPU and Platform component names breaks DAI linking for built-in driver initialization.
- [Medium] Incorrect `$id` and `$schema` fields in the newly added audio DT bindings.
- [Medium] The `ti,pcm1773` binding schema does not document necessary properties and sets `additionalProperties: false`, causing DT validation failures.
--

--- Patch [13]: [PATCH 13/16] arm: dts: omap3pandora: create new DT node for the sound card ---
commit 9dab89ea3f463775080f6924b99e5cea84e88df3
Author: Stefan Leichter <sle85276@gmx.de>

arm: dts: omap3pandora: create new DT node for the sound card

This means that the sound card driver (snd-soc-omap3pandora) will get
loaded by default. To make this work, we also add dac pcm1773-codec and
widgets and routing as needed.

> diff --git a/Documentation/devicetree/bindings/sound/openpandora,omap3pandora-sound.yaml b/Documentation/devicetree/bindings/sound/openpandora,omap3pandora-sound.yaml
> new file mode 100644
> index 0000000000000..d3c747e5c58d6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/openpandora,omap3pandora-sound.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org
> +$schema: http://devicetree.org

[Severity: Medium]
Are these $id and $schema paths correct? The $id field breaks the schema
cross-reference system if it doesn't include the full path, and could cause
dt_binding_check to report misleading errors or skip validation.

[ ... ]

> diff --git a/Documentation/devicetree/bindings/sound/pcm1773.yaml b/Documentation/devicetree/bindings/sound/pcm1773.yaml
> new file mode 100644
> index 0000000000000..f3e640705bf70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/pcm1773.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/sound/pcm1773.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[Severity: Medium]
Should this $id field omit the extraneous bindings/ directory to match the
standard path format?

[ ... ]

> +properties:
> +  compatible:
> +    const: ti,pcm1773
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false

[Severity: Medium]
Since additionalProperties is set to false but only the compatible string is
documented, won't valid device tree nodes that use properties like
vcc-supply, enable-gpio, status, and #sound-dai-cells fail validation?

[ ... ]

> diff --git a/sound/soc/ti/omap3pandora.c b/sound/soc/ti/omap3pandora.c
> index 6c9c184cd9d6f..d3509473f5df0 100644
> --- a/sound/soc/ti/omap3pandora.c
> +++ b/sound/soc/ti/omap3pandora.c
[ ... ]
> +static int omap3pandora_playback_hw_params(struct snd_pcm_substream *substream,
> +					   struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	if (!ctx->playback_stream) {
> +		ctx->playback_stream = substream;
> +		ret = grab_sample_rate(ctx, substream, params);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
If grab_sample_rate() returns an error, it drops sample_rate_users back
down, but ctx->playback_stream remains set to this substream.

When this stream is subsequently closed, omap3pandora_playback_hw_free()
will see ctx->playback_stream is set and call release_sample_rate() again.

Will this result in a double decrement of sample_rate_users that could
prematurely unlock constraints for an active stream?

[ ... ]

> +static int omap3pandora_capture_hw_params(struct snd_pcm_substream *substream,
> +					  struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	if (!ctx->capture_stream) {
> +		ctx->capture_stream = substream;
> +		ret = grab_sample_rate(ctx, substream, params);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Similarly, does this have the same double decrement issue if
grab_sample_rate() fails, leaving ctx->capture_stream set for
omap3pandora_capture_hw_free() to clean up later?

[ ... ]

> +#if IS_BUILTIN(CONFIG_SND_SOC_OMAP3_PANDORA)
>  SND_SOC_DAILINK_DEFS(out,
>  	DAILINK_COMP_ARRAY(COMP_CPU("omap-mcbsp.2")),
> -	DAILINK_COMP_ARRAY(COMP_CODEC("twl4030-codec", "twl4030-hifi")),
> +	DAILINK_COMP_ARRAY(COMP_CODEC("pcm1773-codec", "pcm1773-hifi")),
>  	DAILINK_COMP_ARRAY(COMP_PLATFORM("omap-mcbsp.2")));
>  
>  SND_SOC_DAILINK_DEFS(in,
>  	DAILINK_COMP_ARRAY(COMP_CPU("omap-mcbsp.4")),
>  	DAILINK_COMP_ARRAY(COMP_CODEC("twl4030-codec", "twl4030-hifi")),
>  	DAILINK_COMP_ARRAY(COMP_PLATFORM("omap-mcbsp.4")));
> +#else /* IS_BUILTIN(CONFIG_SND_SOC_OMAP3_PANDORA) */
> +SND_SOC_DAILINK_DEFS(out,
> +	DAILINK_COMP_ARRAY(COMP_CPU("49022000.mcbsp")),
> +	DAILINK_COMP_ARRAY(COMP_CODEC("pcm1773-codec", "pcm1773-hifi")),
> +	DAILINK_COMP_ARRAY(COMP_PLATFORM("49022000.mcbsp")));
> +
> +SND_SOC_DAILINK_DEFS(in,
> +	DAILINK_COMP_ARRAY(COMP_CPU("49026000.mcbsp")),
> +	DAILINK_COMP_ARRAY(COMP_CODEC("twl4030-codec", "twl4030-hifi")),
> +	DAILINK_COMP_ARRAY(COMP_PLATFORM("49026000.mcbsp")));
> +#endif /* IS_BUILTIN(CONFIG_SND_SOC_OMAP3_PANDORA) */

[Severity: High]
Does this IS_BUILTIN() check break DAI linking when the driver is built into
the kernel?

Since the McBSP devices are populated via the Device Tree and use unit
addresses (like 49022000.mcbsp) regardless of the driver's compilation
state, could ALSA fail to find omap-mcbsp.2 when probing the built-in
driver?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783749722.git.hns@goldelico.com?part=13