Devicetree
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Herve Codina" <herve.codina@bootlin.com>
Cc: devicetree@vger.kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org
Subject: Re: [PATCH v3 09/17] ASoC: simple-amplifier: Introduce support for gpio-audio-amp
Date: Thu, 14 May 2026 03:51:57 +0000	[thread overview]
Message-ID: <20260514035157.81CEEC2BCB7@smtp.kernel.org> (raw)
In-Reply-To: <20260513081702.317117-10-herve.codina@bootlin.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Mixing of independent stereo audio paths in the DAPM graph due to the use of a single PGA widget.
- [Low] Preprocessor macro `#define SIMPLE_AUDIO_SUPPORT_PGA` is defined inside a struct definition.
--

commit d8f2282bd54ac87735c5b891e08ae94ea9bfa366
Author: Herve Codina <herve.codina@bootlin.com>

ASoC: simple-amplifier: Introduce support for gpio-audio-amp

This commit introduces preliminary support for GPIO-driven audio amplifiers.
It adds compatible strings and DAPM tables for both mono and stereo variants.

> diff --git a/sound/soc/codecs/simple-amplifier.c b/sound/soc/codecs/simple-amplifier.c
> index 3e644c1c26960..1704cdbb7de5d 100644
> --- a/sound/soc/codecs/simple-amplifier.c
> +++ b/sound/soc/codecs/simple-amplifier.c
> @@ -12,6 +13,9 @@
>  #include <sound/soc.h>
>  
>  struct simple_amp_data {
> +	unsigned int supports;
> +#define SIMPLE_AUDIO_SUPPORT_PGA		BIT(0)
> +

This isn't a bug, but should this macro be defined outside the struct?

Macros are handled by the preprocessor and have file-wide scope, so placing it
inside the struct can be misleading and goes against standard kernel coding
style.

>  	const struct snd_soc_dapm_widget *dapm_widgets;

[ ... ]

> +static const struct snd_soc_dapm_route simple_amp_stereo_pga_dapm_routes[] = {
> +	{ "PGA", NULL, "INL" },
> +	{ "PGA", NULL, "INR" },
> +	{ "PGA", NULL, "vdd" },
> +	{ "OUTL", NULL, "PGA" },
> +	{ "OUTR", NULL, "PGA" },
> +};

Does this routing mix the independent stereo audio paths?

By routing both INL and INR into a single PGA widget, and then routing that
PGA to both OUTL and OUTR, DAPM will see valid audio paths crossing over
(INL to OUTR, and INR to OUTL). 

Could this cause incorrect power management behavior where playing audio on
just the left channel keeps the right channel's downstream widgets powered
up unnecessarily?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513081702.317117-1-herve.codina@bootlin.com?part=9

  reply	other threads:[~2026-05-14  3:51 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-13  8:16 [PATCH v3 00/17] ASoC: Add support for GPIOs driven amplifiers Herve Codina
2026-05-13  8:16 ` [PATCH v3 01/17] of: Introduce of_property_read_s32_index() Herve Codina
2026-05-13  8:16 ` [PATCH v3 02/17] ASoC: dt-bindings: Add support for the GPIOs driven amplifier Herve Codina
2026-05-13  8:16 ` [PATCH v3 03/17] ASoC: simple-amplifier: Remove DRV_NAME defined value Herve Codina
2026-05-13  8:16 ` [PATCH v3 04/17] ASoC: simple-amplifier: Add missing headers Herve Codina
2026-05-13  8:16 ` [PATCH v3 05/17] ASoC: simple-amplifier: Remove CONFIG_OF flag and of_match_ptr() Herve Codina
2026-05-13  8:16 ` [PATCH v3 06/17] ASoC: simple-amplifier: Rename drv_event() function Herve Codina
2026-05-13  8:16 ` [PATCH v3 07/17] ASoC: simple-amplifier: Use 'simple_amp' variable name instead of 'priv' Herve Codina
2026-05-13  8:16 ` [PATCH v3 08/17] ASoC: simple-amplifier: Remove DAPM widgets and routes from the ASoC component driver Herve Codina
2026-05-14  3:32   ` sashiko-bot
2026-05-13  8:16 ` [PATCH v3 09/17] ASoC: simple-amplifier: Introduce support for gpio-audio-amp Herve Codina
2026-05-14  3:51   ` sashiko-bot [this message]
2026-05-13  8:16 ` [PATCH v3 10/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for extra power supplies Herve Codina
2026-05-13  8:16 ` [PATCH v3 11/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for mute gpio Herve Codina
2026-05-13  8:16 ` [PATCH v3 12/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for bypass gpio Herve Codina
2026-05-14  4:54   ` sashiko-bot
2026-05-13  8:16 ` [PATCH v3 13/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for basic gain Herve Codina
2026-05-13  8:16 ` [PATCH v3 14/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for gain-ranges Herve Codina
2026-05-14  6:10   ` sashiko-bot
2026-05-13  8:16 ` [PATCH v3 15/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for gain-labels Herve Codina
2026-05-13  8:17 ` [PATCH v3 16/17] ASoC: simple-amplifier: Update author and copyright Herve Codina
2026-05-14  6:49   ` sashiko-bot
2026-05-13  8:17 ` [PATCH v3 17/17] MAINTAINERS: Add the ASoC gpio audio amplifier entry Herve Codina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260514035157.81CEEC2BCB7@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=herve.codina@bootlin.com \
    --cc=krzk+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox