Re: [PATCH 1/8] extcon: gpio: Add DT bindings

[Linus Walleij]

On Sun, Sep 24, 2017 at 9:56 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
> On Sun, Sep 24, 2017 at 9:56 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>> Add some reasonable device tree bindings and also add the cable defines
>> to the extcon include in <dt-bindings/extcon/connectors.h> since
>> the GPIO extcon definately need to specify which cable/connector it is
>> detecting.
>>
>> Adding the include file makes the (as it happens) Linux numbers into an
>> ABI, but I do not see any better method. It is possible to define strings
>> for all cable types but it seems like overkill, just reusing Linux'
>> enumerators seems like a good idea.
>>
>> The binding supports any number of GPIOs and connectors, but the driver
>> currently only supports one connector on one GPIO line.
>
> My view of extcon binding is it is fundamentally broken. I've
> expressed this multiple times before.

Sorry, I'm a newcomer in this area, so I was not aware of this.

Since this is a new binding consumer, is it something we can use
as role model to fix it?

If I understand correctly from reading up on the mailing list the root of the
problem is something like this:

"extcon" is a linuxism and ambiguous.

This driver should probe to "gpio-connector" or "gpio-switch" or something
like that if it should be generic. Or use very domain-specific compatible
strings (as you describe below), all supported maybe by the same driver
in the end.

The reason it is its own subsystem and not part of input (IIUC) is that
other drivers need to subscribe to events from these connectors,
they are not intended for userspace input such as keyboard or mice
or similar.

In the DTS file you will find stuff using extcons as resources with
 = <&extcon>; phandles so they can look them up and subscribe
to events.

Input has a whole slew of "events" that correspond to some of these
but a different usecase, but that usecase is just a linuxism in turn, there
is nothing saying another operating system could have a more versatile
defintion of "input".

Maybe from a hardware description PoV these should all move over
to devicetree/bindings/input - they all provice an input signal to the
system. The fact that Linux split a subset of these into "extcon"
is of no concern to the DT bindings.

Analogous with that some of the stuff in input/ should likely be
moved to a new output/ directory. Such as pwm-beeper,
pwm-vibrator etc. The fact that these are in the "input" subsystem
in Linux is just another linuxism.

> TL;DR: Anything extending the existing extcon binding will be NAKed.

That can be a reasonable stance.

I'm just trying to get this into a state where the code does not stand in
the way of kernel clean-up. (Especially I want to get rid of the call
to gpio_to_desc())

As stated in the cover letter the alternative will be to simply delete
the driver. But it's better if I can fix the situation, we can't have it
like this.

>> +External Connector Using GPIO
>
> What kind of connector? All connectors external to something... And
> GPIO is not a kind of connector on its own, but an implementation.

Yeah that is a problem with the whole subsystem I guess. Should
I add a paragraph describing the usecases?

The whole thing is a bit
of Androidism and is named like this because Android named it like
this in their kernel tree.

>> +Required properties:
>> +- compatible: should be "extcon-gpio"
>> +- extcon-gpios: the GPIO lines used for the external connectors
>
> This doesn't tell me what the GPIOs functions are and should. For
> example we have hpd-gpios for HDMI hotplug detect in HDMI connector
> binding.

The idea is that this array corresponds to the extcon-connector-types
right below, I'll clarify that if you think the overall idea is OK.

>> +  See gpio/gpio.txt
>> +- extcon-connector-types: set to an unsigned integer value arrat representing the types
>> +  of this connector, matched to the corresponding GPIO lines in the previous array.
>
> This should be determined by the compatible string.

So a GPIO connector is very versatile. It is "general purpose" by definition,
so it needs to be subclassed.

One possibility is to allow just one GPIO and have one comptible-string per
use case.
compatible = "gpio-usb-connector";
compatible = "gpio-charger-connector";
compatible = "gpio-headphone-connector";
etc etc

These bindings on the other hand, assume that the driver will be able
to handle an array of gpios with an associated array of connector types,
which reflects how many of the existing extcon-type driver hardware works:
a single PMIC providing some 5 misc extcons for example.

For this reason there is a generic compatible string and then the device
is subclassed per-gpio with the per-gpio connector type.

>> +/* USB external connector */
>> +#define EXTCON_USB             1
>> +#define EXTCON_USB_HOST                2
>> +#define EXTCON_CHG_USB_SDP     5       /* Standard Downstream Port */
>> +#define EXTCON_CHG_USB_DCP     6       /* Dedicated Charging Port */
>> +#define EXTCON_CHG_USB_CDP     7       /* Charging Downstream Port */
>> +#define EXTCON_CHG_USB_ACA     8       /* Accessory Charger Adapter */
>> +#define EXTCON_CHG_USB_FAST    9
>> +#define EXTCON_CHG_USB_SLOW    10
>> +#define EXTCON_CHG_WPT         11      /* Wireless Power Transfer */
>> +#define EXTCON_CHG_USB_PD      12      /* USB Power Delivery */
>
> These don't all look to be mutually exclusive.
>
> For USB PD, isn't that discoverable?

Someone else would have to answer, this is based on the current
connector types supported by the Linux kernel.

>> +/* Display external connector */
>> +#define EXTCON_DISP_HDMI       40      /* High-Definition Multimedia Interface */
>> +#define EXTCON_DISP_MHL                41      /* Mobile High-Definition Link */
>> +#define EXTCON_DISP_DVI                42      /* Digital Visual Interface */
>> +#define EXTCON_DISP_VGA                43      /* Video Graphics Array */
>> +#define EXTCON_DISP_DP         44      /* Display Port */
>> +#define EXTCON_DISP_HMD                45      /* Head-Mounted Display */
>
> We already have connector bindings for most of these. Use those as a
> model for whatever you want to do.

I guess they are not in Documentation/devicetree/bindings/extcon/*

Please point me in the right direction and I'll check it out.

Yours,
Linus Walleij