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

[Mark Brown <broonie@xxxxxxxxxx>]

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: signature.asc
Description: PGP signature