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

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.

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.

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.