Message-ID: <YkW4MPh8VWc8eSGg@sirena.org.uk>
Date:   Thu, 31 Mar 2022 15:18:24 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Martin Povišer <povik@...ebit.org>
Cc:     Martin Povišer <povik+lin@...ebit.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mark Kettenis <kettenis@...nbsd.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>
Subject: Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote:
> > On 31. 3. 2022, at 14:34, Mark Brown <broonie@...nel.org> wrote:

> > The broad issue here is that what you consider ridiculous someone else
> > might have some bright ideas for configuring dynamically - if things are
> > being exposed for dynamic configuration it's probably because someone
> > wanted them, if the control is genuinely useless then it should just be

> Well but these are codec drivers reused on different systems, it can both
> be 'not genuinely useless’ on some system and ridiculous to leave open on
> the systems I am trying to write drivers for.

It wouldn't be the first time that we've had someone turn up with a new
idea for how to configure an already existing bit of hardware, part of
the reason for this approach is that people do get surprised by user
creativity with their systems.

> > The TDM swap thing you're mentioning looks like it's a left/right
> > selection which people do use sometimes as a way of doing mono mixes and
> > reorientation.  The ISENSE/VSENSE is less obvious, though it's possible
> > there's issues with not having enough slots on a heavily used TDM bus or
> > sometimes disabling the speaker protection processing for whatever
> > reason.

> Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’
> meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell
> me it’s okay to change that behaviour and it won’t be considered backwards
> compatibility breaking, that would be part of the solution I am seeking
> here.

Having the default state be muted or not routed is quite common, UCM
files or equivalent are typically required for embedded style hardware
like this.

> But even then, what for example if the system has a single speaker (as it
> does on the Mac mini to be covered by this driver) and the I2S bus is left
> undriven for the duration of unused TDM slots? That may genuinely pose
> a risk of people blowing their speakers by switching something in alsamixer.

Right, so that's a more sensible and valid use case.  We do have the
platform_max feature available for precisely this reason - that's
probably more appropriate here since if there's a danger of people
blowing their speaker with a floating input they could also blow their
speaker with just a very loud audio signal so limiting the volume people
can set on the speaker driver seems sensible and would also cover them
for misrouting.  Whatever the device might pick up from noise on an
undriven bus could also be played as audio down the bus.  This does
become a little fun with speaker protection as we'd want to raise the
kernel limit so that userspace can dynamically manage the volume to
contorl power (though that might be done with software control), but
it's easy enoguh to raise limits later.

On the other hand it seems like userspace might reasonably choose to do
a mono mix for this output entirely in software, in which case telling
the speaker amp to pick up one channel would make sense, or to just play
out a stereo signal over I2S and have the amplifier do a mono mix and
I'm not seeing why we'd force one or the other in the machine driver.

> The ISENSE/VSENSE controls are also actually useless on these systems as we
> are not doing anything to pick up the measured values (which are sent back
> over the I2S lines). I don’t know if there can be driver conflict between

Presumably someone might want to work on figuring that out though, and
from a hardware safety point of view it would be better if they did.

> two speaker amps trying to drive the I2S lines at the same time should
> the user happen to enable SENSE facilities on more than one of them.
> Now I can grudgingly study that and rule it out but I would rather hide
> the controls altogether.

Yes, having two devices driving the bus at the same time wouldn't be
great.  How is the TDM slot selection for the signals done in the
hardware, I'm not seeing anything immediately obvious in the driver?
I'd have thought that things would be implemented such that you could
implement speaker protection on all speakers simultaneously but perhaps
not.

> That’s the reasoning anyway. To reiterate, seems to me the controls
> are useless/confusing at best and dangerous at worst.

I'm just not seeing an issue for the slot selection.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists