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

[Shimrra Shai <shimrrashai@gmail.com>]

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.