On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote:

...

On 31. 3. 2022, at 14:34, Mark Brown broonie@kernel.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.