Re: [PATCH 12/16] arm: dts: omap3pandora: Add device node for PCM1773 codec

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect lifecycle for `devm_*` resource allocations in the ASoC component probe causes memory leaks and rebinding failures.
- [High] The driver swallows `-EPROBE_DEFER` from `devm_regulator_get()`, breaking deferred probing for the VCC supply.
- [Low] The commit uses the deprecated property name `enable-gpio` instead of `enable-gpios`.
- [Low] Missing property definitions in the PCM1773 YAML binding schema will cause `dtbs_check` failures.
- [Low] Busy-waiting with `mdelay()` is used in a sleepable context.
--

--- Patch [12]: [PATCH 12/16] arm: dts: omap3pandora: Add device node for PCM1773 codec ---
commit 4dbdbb04f5f389f900287ce1a74c35a57890d4b3
Author: Grond <[email protected]>

arm: dts: omap3pandora: Add device node for PCM1773 codec

This commit adds a new device node for the PCM1773 codec to the OpenPandora
device tree. This ensures the new PCM1773 driver gets loaded automatically.

> diff --git a/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi b/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> --- a/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> @@ -42,6 +42,13 @@ tv_connector_in: endpoint {
>  		};
>  	};
>  
> +	dac: pcm1773-codec {
> +		compatible = "ti,pcm1773";
> +		vcc-supply = <&vsim>;
> +		enable-gpio = <&gpio4 22 GPIO_ACTIVE_HIGH>; /* GPIO 118 */

[Severity: Low]
Is it expected to use the deprecated property name enable-gpio instead of the
mandated enable-gpios suffix?

The standard requires the plural form even for single GPIOs.

> +		status = "okay";
> +	};
> +
>  	gpio-leds {

> diff --git a/Documentation/devicetree/bindings/sound/pcm1773.yaml b/Documentation/devicetree/bindings/sound/pcm1773.yaml
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/pcm1773.yaml
[ ... ]
> +properties:
> +  compatible:
> +    const: ti,pcm1773
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false

[Severity: Low]
Will this schema cause dtbs_check validation errors?

The schema includes additionalProperties: false but fails to list enable-gpios
and vcc-supply in the properties dictionary.

The provided device tree nodes specify these properties, which will lead to
schema validation failures.

> +
> +examples:

> diff --git a/sound/soc/codecs/pcm1773.c b/sound/soc/codecs/pcm1773.c
> --- /dev/null
> +++ b/sound/soc/codecs/pcm1773.c
[ ... ]
> +static int pcm1773_dac_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *k, int event)
> +{
[ ... ]
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		if (ctx->regulator) {
> +			ret = regulator_enable(ctx->regulator);
> +			if (ret) {
> +				dev_err(dev, "Failed to power DAC: %d\n", ret);
> +				return ret;
> +			}
> +			mdelay(1);

[Severity: Low]
Should this use usleep_range() or msleep() instead of busy-waiting?

pcm1773_dac_event() is executed during DAPM widget power up/down events, which
run in a sleepable context (as demonstrated by calls to
gpiod_set_value_cansleep()). Using mdelay() here wastes CPU cycles.

> +		}
[ ... ]
> +static int pcm1773_probe(struct snd_soc_component *component)
> +{
> +	struct pcm1773 *ctx = NULL;
> +	struct device *dev = component->dev;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	snd_soc_component_set_drvdata(component, ctx);
> +
> +	ctx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);

[Severity: High]
Can this incorrect lifecycle for devm_ allocations cause memory leaks and
rebinding failures?

pcm1773_probe() executes during sound card binding, using devm_kzalloc() and
devm_gpiod_get_optional() with the platform device (component->dev).

Since the platform device outlives the sound card lifecycle, these resources
are not freed upon unbind, and a subsequent bind will hit -EBUSY when
requesting the GPIO or regulator.

> +	if (IS_ERR(ctx->enable_gpio)) {
[ ... ]
> +	ctx->regulator = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(ctx->regulator)) {
> +		dev_warn(dev, "cannot get regulator 'vcc'");
> +		ctx->regulator = NULL;
> +	}

[Severity: High]
Does this break deferred probing for the VCC supply by swallowing
-EPROBE_DEFER?

When the VCC regulator driver is loaded after the sound driver,
devm_regulator_get() returns -EPROBE_DEFER.

This code intercepts the error, sets the regulator to NULL, and continues
instead of propagating the error. This will cause the DAC to silently fail to
power up later.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12