Message-Id: <6D199EAB-FE14-4030-96A7-2E0E89D25FAB@cutebit.org>
Date:   Thu, 31 Mar 2022 17:04:32 +0200
From:   Martin Povišer <povik@...ebit.org>
To:     Mark Brown <broonie@...nel.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 31. 3. 2022, at 16:18, Mark Brown <broonie@...nel.org> wrote:
> 
> 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.

Granted. If we make sure the volume caps are there to prevent damage
under arbitrary input (which we should anyway) that covers slot
misconfiguration too.

>> 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.

I don’t know. I would have to go study the details of this. Should I see
if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings
that would lead to driver conflict on one of the models, or is there
a chance we could hide those controls just on the basis of ‘it doesn’t
do anything usable and is possibly dangerous’?

>> 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.

Yeah, agreed there’s no (damage) issue as we should to proper volume
caps anyway.

Martin

Powered by blists - more mailing lists