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
