> On 31. 3. 2022, at 14:34, Mark Brown <broonie@xxxxxxxxxx> wrote: > > On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote: > >> I put together a machine-level ASoC driver for recent Apple Macs (the >> ones with ARM64 SoCs) and want to gauge opinions. > > This would be a bit easier to review with a description of the hardware. The typical hardware configuration the driver is supposed to be used with is this: * SoC with couple of I2S ports. * Array of speakers with individual speaker amp chips, hooked to a single I2S bus or split between two of the SoC’s I2S ports. Speakers can be one, two, four or six in total. The speaker amp chips resemble either TAS2770 or TAS2764. * Jack codec hooked to a separate I2S port, operating independently. (Codec driver supports set_jack.) The example in the binding patch describes an actual arrangement on one piece of hardware. >> Commit 2 adds a new ASoC card method (filter_controls) to let the card >> prevent some codec kcontrols from being visible to userspace. For example >> the TAS2770 speaker amp driver would be happy to expose TDM slot selection >> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to >> make the patch acceptable to upstream. > > 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 > removed. Rather than getting in the way of people's policy arguments > about how to set things we expose them to userspace and let userspace > worry about it, usually with the help of UCM files. The general > userspace model is that people interact with their sound server more > than the hardware card. This is also helpful for people developing use > cases, it means they're not having to get the kernel rebuilt to tune > things. 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. > 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. 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. Now I can actually make sure the I2S data lines are always zeroed out in the ASoC platform driver, but I would rather not even have to tie these loose ends for a control there’s no reason to expose in the first place (again, on this system). 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 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. That’s the reasoning anyway. To reiterate, seems to me the controls are useless/confusing at best and dangerous at worst. Martin
