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 <[email protected]> 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/[email protected]?part=13
