[PATCH 0/2] ASoC: es8323: Playback enablement for Firefly ITX-3588J and similar hardware

[PATCH 0/2] ASoC: es8323: Playback enablement for Firefly ITX-3588J and similar hardware

[Shimrra Shai]

This small pair of patches is needed to enable the audio output to
speaker using the ES8323 CODEC on boards like the Firefly ITX-3588J
that use the right-hand DAC mixer to connect the line out jack. I also
enable associated DAPM power widgets so that the volume can be
controlled in the ALSA mixer. This seems sufficient to me to produce
basic audio functionality on this board and any others that might use
the same features. I tested it with both ear buds and a pair of
speakers plugged into the green jack on the back of the board.

Shimrra Shai (2):
  ASoC: es8323: enable right-hand DAC-mixer connection on ES8323
  ASoC: es8323: enable DAPM power widgets for playback DAC and output

 sound/soc/codecs/es8323.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH 1/2] ASoC: es8323: enable right-hand DAC-mixer connection on ES8323

[Shimrra Shai]

Enable the right-hand DAC mixer connection in the same manner as the
left-hand one.

Signed-off-by: Shimrra Shai <shimrrashai@gmail.com>
---
 sound/soc/codecs/es8323.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/es8323.c b/sound/soc/codecs/es8323.c
index a98229981..3a91713bd 100644
--- a/sound/soc/codecs/es8323.c
+++ b/sound/soc/codecs/es8323.c
@@ -633,6 +633,7 @@ static int es8323_probe(struct snd_soc_component *component)
 	snd_soc_component_write(component, ES8323_CONTROL2, 0x60);
 	snd_soc_component_write(component, ES8323_CHIPPOWER, 0x00);
 	snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);
+	snd_soc_component_write(component, ES8323_DACCONTROL20, 0xB8);
 
 	return 0;
 }
-- 
2.48.1

[PATCH 2/2] ASoC: es8323: enable DAPM power widgets for playback DAC and output

[Shimrra Shai]

Enable DAPM widgets for power and volume control of playback.

Signed-off-by: Shimrra Shai <shimrrashai@gmail.com>
---
 sound/soc/codecs/es8323.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/es8323.c b/sound/soc/codecs/es8323.c
index 3a91713bd..8d360cb79 100644
--- a/sound/soc/codecs/es8323.c
+++ b/sound/soc/codecs/es8323.c
@@ -211,8 +211,8 @@ static const struct snd_soc_dapm_widget es8323_dapm_widgets[] = {
 
 	SND_SOC_DAPM_ADC("Right ADC", "Right Capture", SND_SOC_NOPM, 4, 1),
 	SND_SOC_DAPM_ADC("Left ADC", "Left Capture", SND_SOC_NOPM, 5, 1),
-	SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1),
-	SND_SOC_DAPM_DAC("Left DAC", "Left Playback", SND_SOC_NOPM, 7, 1),
+	SND_SOC_DAPM_DAC("Right DAC", "Right Playback", ES8323_DACPOWER, 6, 1),
+	SND_SOC_DAPM_DAC("Left DAC", "Left Playback", ES8323_DACPOWER, 7, 1),
 
 	SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0,
 			   &es8323_left_mixer_controls[0],
@@ -223,10 +223,10 @@ static const struct snd_soc_dapm_widget es8323_dapm_widgets[] = {
 
 	SND_SOC_DAPM_PGA("Right ADC Power", SND_SOC_NOPM, 6, 1, NULL, 0),
 	SND_SOC_DAPM_PGA("Left ADC Power", SND_SOC_NOPM, 7, 1, NULL, 0),
-	SND_SOC_DAPM_PGA("Right Out 2", SND_SOC_NOPM, 2, 0, NULL, 0),
-	SND_SOC_DAPM_PGA("Left Out 2", SND_SOC_NOPM, 3, 0, NULL, 0),
-	SND_SOC_DAPM_PGA("Right Out 1", SND_SOC_NOPM, 4, 0, NULL, 0),
-	SND_SOC_DAPM_PGA("Left Out 1", SND_SOC_NOPM, 5, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Right Out 2", ES8323_DACPOWER, 2, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Left Out 2", ES8323_DACPOWER, 3, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Right Out 1", ES8323_DACPOWER, 4, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Left Out 1", ES8323_DACPOWER, 5, 0, NULL, 0),
 	SND_SOC_DAPM_PGA("LAMP", ES8323_ADCCONTROL1, 4, 0, NULL, 0),
 	SND_SOC_DAPM_PGA("RAMP", ES8323_ADCCONTROL1, 0, 0, NULL, 0),
 
-- 
2.48.1

Re: [PATCH 1/2] ASoC: es8323: enable right-hand DAC-mixer connection on ES8323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Wed, Aug 13, 2025 at 08:47:31PM -0500, Shimrra Shai wrote:
> Enable the right-hand DAC mixer connection in the same manner as the
> left-hand one.

> @@ -633,6 +633,7 @@ static int es8323_probe(struct snd_soc_component *component)
>  	snd_soc_component_write(component, ES8323_CONTROL2, 0x60);
>  	snd_soc_component_write(component, ES8323_CHIPPOWER, 0x00);
>  	snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);
> +	snd_soc_component_write(component, ES8323_DACCONTROL20, 0xB8);

Neither of these should be unconditional writes, these should be user
visible controls.  We don't encode specific system's use cases into the
driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/2] ASoC: es8323: Playback enablement for Firefly ITX-3588J and similar hardware

[Mark Brown]

On Wed, 13 Aug 2025 20:26:49 -0500, Shimrra Shai wrote:
> This small pair of patches is needed to enable the audio output to
> speaker using the ES8323 CODEC on boards like the Firefly ITX-3588J
> that use the right-hand DAC mixer to connect the line out jack. I also
> enable associated DAPM power widgets so that the volume can be
> controlled in the ALSA mixer. This seems sufficient to me to produce
> basic audio functionality on this board and any others that might use
> the same features. I tested it with both ear buds and a pair of
> speakers plugged into the green jack on the back of the board.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[2/2] ASoC: es8323: enable DAPM power widgets for playback DAC and output
      commit: 258384d8ce365dddd6c5c15204de8ccd53a7ab0a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: Re: [PATCH 1/2] ASoC: es8323: enable right-hand DAC-mixer connection on ES8323

[Shimrra Shai]

On Thu, Aug 14, 2025 at 01:11:59 PM +0100, Mark Brown wrote:
> On Wed, Aug 13, 2025 at 08:47:31PM -0500, Shimrra Shai wrote:
> > Enable the right-hand DAC mixer connection in the same manner as the
> > left-hand one.
>
> > @@ -633,6 +633,7 @@ static int es8323_probe(struct snd_soc_component *component)
> >  	snd_soc_component_write(component, ES8323_CONTROL2, 0x60);
> >  	snd_soc_component_write(component, ES8323_CHIPPOWER, 0x00);
> >  	snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);
> > +	snd_soc_component_write(component, ES8323_DACCONTROL20, 0xB8);
>
> Neither of these should be unconditional writes, these should be user
> visible controls.  We don't encode specific system's use cases into the
> driver.

I was just following the precedent from the driver's prior
author(s), in the manner of the line above it. Presumably, enabling
the left-hand DAC-mixer connection only was a solution that worked
for some devices, and theoretically (as in, I don't know of them
personally), there could be devices where enabling the right-hand
DAC-mixer connection only works for them (the Firefly may be just
such a device; I have only tested it with both enabled at once,
so I can't say for sure. I know it definitely does not work with
the left-hand connection enabled alone, i.e. the line preceding
my addition). And perhaps also devices where there is some kind
of cross-inhibition effect, meaning setting both enabled would
generate problems on those devices, thus justifying your concern
instead of using a blanket for all devices as I thought. In that
case, that means the original author was also wrong, and so I need
to know exactly where it should be placed.

With regard of that though, I see these lines earlier in the driver:

/* Left Mixer */
static const struct snd_kcontrol_new es8323_left_mixer_controls[] = {
	SOC_DAPM_SINGLE("Left Playback Switch", SND_SOC_NOPM, 7, 1, 1),
	SOC_DAPM_SINGLE("Left Bypass Switch", ES8323_DACCONTROL17, 6, 1, 0),
};

/* Right Mixer */
static const struct snd_kcontrol_new es8323_right_mixer_controls[] = {
	SOC_DAPM_SINGLE("Right Playback Switch", SND_SOC_NOPM, 6, 1, 1),
	SOC_DAPM_SINGLE("Right Bypass Switch", ES8323_DACCONTROL20, 6, 1, 0),
};

which suggest it is in fact controllable already, but I wonder why it
is in the "bypass" switch only and not the "playback" switch, which
seems to do nothing (SND_SOC_NOPM). Would it perhaps be correct to
move these to the "playback" switch, or to have both switches
collapsed into a single switch?

In any case, if these are the correct places to enable this control
and it is already supported there, then it seems neither write command
in the setup is needed, viz. we should _delete_

snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);

too. What do you say?

  Thanks for any feedback,
  Shimrra Shai.

Re: Re: [PATCH 1/2] ASoC: es8323: enable right-hand DAC-mixer connection on ES8323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Thu, Aug 14, 2025 at 01:33:44PM -0500, Shimrra Shai wrote:
> On Thu, Aug 14, 2025 at 01:11:59 PM +0100, Mark Brown wrote:
> > On Wed, Aug 13, 2025 at 08:47:31PM -0500, Shimrra Shai wrote:

> > >  	snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);
> > > +	snd_soc_component_write(component, ES8323_DACCONTROL20, 0xB8);

> > Neither of these should be unconditional writes, these should be user
> > visible controls.  We don't encode specific system's use cases into the
> > driver.

> I was just following the precedent from the driver's prior
> author(s), in the manner of the line above it. Presumably, enabling
> the left-hand DAC-mixer connection only was a solution that worked

Yes, as I say this is very bad practice on the part of the original
authors which only escaped review due to the magic numbers.

> instead of using a blanket for all devices as I thought. In that
> case, that means the original author was also wrong, and so I need
> to know exactly where it should be placed.

Like I say these should be userspace controls, not just blind writes.
Probably wired up in DAPM, SOC_DAPM_SINGLE().

> which suggest it is in fact controllable already, but I wonder why it
> is in the "bypass" switch only and not the "playback" switch, which
> seems to do nothing (SND_SOC_NOPM). Would it perhaps be correct to
> move these to the "playback" switch, or to have both switches
> collapsed into a single switch?

Those will be different audio paths, it's almost certainly a bug due to
the hard coding of the enables.  Both DAC and bypass paths should be
normal user controllable things, from the sound of it what's needed is
to define the register for the DAC path.

> In any case, if these are the correct places to enable this control
> and it is already supported there, then it seems neither write command
> in the setup is needed, viz. we should _delete_

> snd_soc_component_write(component, ES8323_DACCONTROL17, 0xB8);

> too. What do you say?

Yes, ideally that shouldn't be there.  There's some risk that there
might be some userspace relying on having the mono channel enabled by
default though so perhaps it's safer to just leave that as is - the path
can still be configured by userspace, even if we end up with a weird
asymmetric default.  I guess there's also some other things in that
register which most likely should also be controllable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]