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

[Hans de Goede]

Hi,

On 12/29/20 2:06 PM, Charles Keepax wrote:
> On Mon, Dec 28, 2020 at 04:28:07PM +0000, Mark Brown wrote:
>> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
>>
>>> 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 .
>>
>> Has Android been fixed to use the ALSA/input layer interfaces?  That's
>> why that code is there, long term the goal was to have ALSA generate
>> extcon events too so userspace could fall over to using that.  The basic
>> thing at the time was that nobody liked any of the existing interfaces
>> (the input layer thing is a total bodge stemming from it having been
>> easy to hack in a key for GPIO detection and using ALSA controls means
>> having to link against alsa-lib which is an awful faff for system level
>> UI stuff) and there were three separate userspace interfaces used by
>> different software stacks which needed to be joined together, extcon was
>> felt to be a bit more designed and is a superset so that was the
>> direction we were heading in.
> 
> Android has been updated to have the option to catch input events
> for jack detection now.
> 
> I have always been slightly confused between extcon and the ALSA
> jack reporting and have been unsure as to what is the longer term
> plan here. I vaguely thought there was a gentle plan to move to
> extcon, it is interesting to see Hans basically saying the
> opposite that extcon is intended to be paritially deprecated. I
> assume you just mean with respect to audio jacks, not other
> connector types?

No I mean that afaik extcon is being deprecated in general. Extcon
is mostly meant for kernel internal use, to allow things like
charger-type-detection done by e.g. a fsa micro-usb mux or a
Type-C PD controller to be hooked up to the actual charger chip
and set the input-current-limit based on this.

But there is no clean way to express these relation-ships
in extcon terms in devicetree. At least for new device with
Type-C functionality this is all being moved to OF graph bindings
combined with using the power_supply class to provide info like
negotiated voltage and current to the charger-IC.

Note this is mostly my 2 cents / my impression about where extcon
is heading. I have actually tried to use extcon for Type-C stuff
and this was frowned upon by both the devicetree bindings people and
the USB Type-C code controller people.

Also despite its age, the extcon code is still not really in good
shape. E.g. a driver can do extcon_get_extcon_dev, but that does
not return a device-reference, that just does a lookup and returns
the requested extcon-device, but there are 0 guarantees that the
extcon device will not go away and dereferencing the returned
pointer after doing a rmmod of the extcon-driver will result in
a nasty crash.

So all this suggests to me that extcon should not be used for
new code / projects.

> I would agree with Mark though that if extcon exists for external
> connectors it seems odd that audio jacks would have their own
> special way rather than just using the connector stuff.

Well as I said above in me experience the extcon code is (was) mostly
meant for kernel internal use. The sysfs API is more of a debugging
tool then anything else (IMHO).

Also the kernel has support for a lot of sound devices, including
many with jack-detection support. Yet a grep for EXTCON_JACK_HEADPHONE
over the entire mainline kernel tree shows that only extcon-arizona.c
is using it. So given that we have dozens of drivers providing jack
functionality through the sound/core/jack.c core and only 1 driver
using the extcon interface I believe that the ship on how to export
this to userspace has long sailed, since most userspace code will
clearly expect the sound/core/jack.c way of doing things to be used.

Arguably we should/could maybe even drop the extcon part of extcon-arizona.c
but I did not do that as I did not want to regress existing userspace
code which may depend on this (on specific embedded/android devices).

Regards,

Hans