Message-Id: <CCE4A06E-6D6F-457D-B3C5-C36209BF38D3@cutebit.org>
Date:   Thu, 31 Mar 2022 15:28:12 +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 14:34, Mark Brown <broonie@...nel.org> 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

Powered by blists - more mailing lists