Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Linus Walleij]

On Sat, Dec 2, 2017 at 12:28 AM, Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:

>         /* Query the number of GPIOs from ACPI */
>         ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios);
>         if (ret < 0) {
> -               dev_warn(&pdev->dev, "missing num-gpios property\n");
> +               dev_err(&pdev->dev, "missing 'num-gpios' property\n");
>                 return ret;
>         }

It's unfortunate that this driver uses the undocumented "num-gpios"
when the device tree bindings already has standardized "ngpios"
as the name for this.

Maybe it was not standardized back in 2015 when this driver was merged.

Or we were all sloppy :/

> +               /* The number of GPIOs in the approved list */
> +               ret = device_property_read_u16_array(&pdev->dev, "gpios",
> +                                                    NULL, 0);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "missing 'gpios' property\n");
> +                       return ret;
> +               }

This is in direct conflict with the existing "gpios" binding in device tree.

Where is this name coming from? ACPI standards?

If device tree and ACPI start defining things which are in direct conflict
we can just shut down this device_property() business altogether,
it will never work that way.

I would try to merge a DT bindings doc defining "valid-gpios" or something
like this, can we proceed like that?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Andy Shevchenko]

On Tue, 2017-12-12 at 11:42 +0100, Linus Walleij wrote:
> On Sat, Dec 2, 2017 at 12:28 AM, Timur Tabi <timur@codeaurora.org>
> wrote:

> > +               /* The number of GPIOs in the approved list */
> > +               ret = device_property_read_u16_array(&pdev->dev,
> > "gpios",
> > +                                                    NULL, 0);
> > +               if (ret < 0) {
> > +                       dev_err(&pdev->dev, "missing 'gpios'
> > property\n");
> > +                       return ret;
> > +               }
> 
> This is in direct conflict with the existing "gpios" binding in device
> tree.
> 
> Where is this name coming from? ACPI standards?

Not ACPI standards as of my knowledge. ACPI standard defines a common
scheme how to define properties, it doesn't tell anything about property
names or any mappings between names to values or names to "OS
subsystem").

As for GPIO we just follow *de facto* what DT has right now, i.e. "xxx-
gpio" or "xxx-gpios" pattern is used to map ACPI standard resource to a
GPIO name. That's how GPIO ACPI lib is being developed.

> If device tree and ACPI start defining things which are in direct
> conflict
> we can just shut down this device_property() business altogether,
> it will never work that way.

This is fully understandable. Also it works in other direction, i.e. if
DT will break the established thing it will break also ACPI and built-in 
device properties.

We are keeping an eye on this not to happen as much as we can in any
direction.

So, summarize above, I don't see any impediments (except maybe very
broken ARM64 firmware that is already on devices on market) to make it
properly from the beginning.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/12/2017 05:07 AM, Andy Shevchenko wrote:

> Not ACPI standards as of my knowledge. ACPI standard defines a common
> scheme how to define properties, it doesn't tell anything about property
> names or any mappings between names to values or names to "OS
> subsystem").

There was an attempt a while back to standardize this like we do for 
device tree, but it fell apart.  Device-specific ACPI-only properties 
are not standarized.  This driver is initialized only on ACPI systems. 
It has no device tree binding.

> As for GPIO we just follow *de facto* what DT has right now, i.e. "xxx-
> gpio" or "xxx-gpios" pattern is used to map ACPI standard resource to a
> GPIO name. That's how GPIO ACPI lib is being developed.

GPIOs in device tree are defined completely differently than in ACPI. 
On DT, the kernel controls the pin muxing.  On ACPI, pins are muxed by 
firmware and never re-muxed by the operating system.  So all this driver 
does is expose a few pins as simple GPIOs.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Andy Shevchenko]

On Tue, 2017-12-12 at 14:27 -0600, Timur Tabi wrote:
> On 12/12/2017 05:07 AM, Andy Shevchenko wrote:
> 
> > Not ACPI standards as of my knowledge. ACPI standard defines a
> > common
> > scheme how to define properties, it doesn't tell anything about
> > property
> > names or any mappings between names to values or names to "OS
> > subsystem").
> 
> There was an attempt a while back to standardize this like we do for 
> device tree, but it fell apart.  Device-specific ACPI-only properties 
> are not standarized.  This driver is initialized only on ACPI
> systems. 
> It has no device tree binding.

It should follow DT *de facto* standard bindings like "ngpios" (though
it's not needed in ACPI case IIRC) and other properties.

> > As for GPIO we just follow *de facto* what DT has right now, i.e.
> > "xxx-
> > gpio" or "xxx-gpios" pattern is used to map ACPI standard resource
> > to a
> > GPIO name. That's how GPIO ACPI lib is being developed.
> 
> GPIOs in device tree are defined completely differently than in ACPI. 
> On DT, the kernel controls the pin muxing.  On ACPI, pins are muxed
> by 
> firmware and never re-muxed by the operating system.  So all this
> driver 
> does is expose a few pins as simple GPIOs.

Wait, runtime muxing is a matter of requesting another function (usually
GPIO) and putting it back afterwards. Do you really need anything like
this at *runtime*?

Pin control design is not compatible with hardware (too abstract), but
that is the problem of DT as well: I'm referring here to not carefully
designed so called "pin states". This is another story and has nothing
specific for ACPI.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/4] [v4] pinctrl: qcom: qdf2xxx: add support for new ACPI HID QCOM8002

[Timur Tabi]

On 12/13/2017 08:36 AM, Andy Shevchenko wrote:
> Wait, runtime muxing is a matter of requesting another function (usually
> GPIO) and putting it back afterwards. Do you really need anything like
> this at*runtime*?

No, there is no runtime muxing on ACPI platforms.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.