Re: [PATCH 01/14] mfd: arizona: Add jack pointer to struct arizona

[Hans de Goede]

Hi,

On 12/28/20 1:21 PM, Mark Brown wrote:
> On Sun, Dec 27, 2020 at 10:12:19PM +0100, Hans de Goede wrote:
>> The Linux Arizona driver uses the MFD framework to create several
>> sub-devices for the Arizona codec and then uses a driver per function.
>>
>> The jack-detect support for the Arizona codec is handled by the
>> extcon-arizona driver. This driver exports info about the jack state
>> to userspace through the standard extcon sysfs class interface.
>>
>> But standard Linux userspace does not monitor/use the extcon sysfs
>> interface for jack-detection.
> 
> This seems like the wrong layer to fix this problem at, this issue will
> apply to all extcon devices that can detect audio.

Well, the problem really is that using extcon to report jack-state is
rather unusual to do, extcon-arizona.c is the only extcon driver which
deals with jack-state (typically extcon is used for things like determining
the type of charger connected to an USB charging port):

[hans@x1 linux]$ grep -lr EXTCON_JACK_HEADPHONE drivers/extcon/
drivers/extcon/extcon-arizona.c
drivers/extcon/extcon.c

And more in general AFAIK extcon is sort of deprecated and it is
not advised to use it for new code. I would esp. not expect it to
be used for new jack-detection code since we already have standard
uAPI support for that through sound/core/jack.c .

So extcon-arizona really is the odd duck here and writing some
generic extcon to sound/core/jack.c glue seems unnecessary since
we are just trying dealing with one special case here.

Also at first I tried to use extcon-glue like code in
sound/soc/intel/boards/bytcr_wm5102.c making it listen for
extcon events and have sound/soc/intel/boards/bytcr_wm5102.c
report jack events instead of sharing the jack with extcon-arizona.c
through the shared MFD data struct. But that did not work, because
the extcon-arizona.c probe function already (before this patch-set)
has this:

        if (!arizona->dapm || !arizona->dapm->card)
                return -EPROBE_DEFER;

Which means that the sound/soc/intel/boards/bytcr_wm5102.c machine
driver must first complete calling devm_snd_soc_register_card() before
the extcon driver will bind and register the extcon device.

But listening to extcon events requires the machine driver to do an:
extcon_get_extcon_dev("arizona-extcon") and as long as that returns
NULL, return -EPROBE_DEFER.

So now we have the machine-driver's probe returning with -EPROBE_DEFER
until the extcon driver shows up and the other-way around, so neither
ever binds.

I could have fixed this by making the machine driver bind without the
extcon driver being bound and then poll every second for the extcon device
to show up, and once it has shown up stop polling and register the jack,
once it has the extcon device.

But that seems quite ugly, so I did not even try to implement that
coming up with this solution instead which is much more KISS really.

Also note that sharing the jack is necessary to avoid creating 2
separate input_device-s for the headset, which also looks weird / ugly.

Besides being ugly, there also is another potential problem with
polling to wait for the extcon device to show up: the jack must be
registered before the card registration completes otherwise
snd_jack_dev_register will not run, since we are post registration.
But I guess that the sound core might be smart enough to call
the dev_register op immediately if the card has already been
registered ?

TL;DR: writing a generic solution for what is a special case used
in just driver seems like overkill and also writing such a
generic solution is not easily possible because of probe ordering
issues. So instead I've gone with this approach which is a much
simpler solution and as such seems a better way to deal with this
special case.

Regards,

Hans