Re: [RFC PATCH] gpio: support for GPIO forwarding

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Wednesday, January 14, 2015 08:32:12 AM Darren Hart wrote:
> 
> On 1/14/15 4:58 AM, Linus Walleij wrote:
> > On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > 
> >> This makes it possible to assign GPIOs at runtime. The
> >> motivation for it is because of need to forward GPIOs from
> >> one device to an other. That feature may be useful for
> >> example with some mfd devices, but initially it is needed
> 
> +Samuel Ortiz for his thoughts on applicability to MFD.
> 
> >> because on some Intel Braswell based ACPI platforms the GPIO
> >> resources controlling signals of the USB PHY are given to
> >> the controller device object for whatever reason, so the
> >> driver of that controller needs be able to pass them to the
> >> PHY device somehow.
> >>
> >> So basically this is meant to allow patching of bad (bad
> >> from Linux kernels point of view) hardware descriptions
> >> provided by system fw in the drivers.
> >>
> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >> ---
> >>
> >> Hi,
> >>
> >> I'm sending this first as a RFC in case you guys have some better
> >> idea how to solve our problem. I actually already have couple
> >> platforms where the GPIO resources are given to the "wrong" device
> >> objects now.
> >>
> >> Originally we were thinking about simply handling our problem with
> >> hacks to the PHY drivers, basically also checking if the parent has
> >> GPIOs. That would only work if the controller is always the parent,
> >> which it's not, but even if it was, it would be too risky. The PHY
> >> drivers don't know which controller they are attached to, so what is
> >> to say that the GPIOs aren't really attached to the controller.
> >>
> >> So the safest way to handle our problem is to deal with it in quirks
> >> in the controller drivers. Solving it by bouncing the GPIOs did not
> >> feel ideal there doesn't seem to be any other way. The API is copied
> >> from clkdev (clk_register_clkdev). In the end it's really only one
> >> function for adding the lookups and one for removing them.
> >>
> >> The way it's implemented is by modifying the current style of handling
> >> the lookups a little bit. The per device lookup table is basically
> >> reverted back to the single linked-list format since it seems that
> >> these lookups may have to be assigned from several sources.
> > 
> > Oh ain't that great.
> > 
> > So now we start patching around the kernel because the ACPI
> > tables are erroneous for GPIOs. I'd like some feedback from
> > Rafael &| Darren on this patch, i.e. if you two think this is a good
> > way of accounting for bad GPIO descriptions in ACPI tables then
> > ACK this patch.
> > 
> > I guess the other option would be to fix up the ACPI DSDT
> > itself to put resources right, correct? Is this not possible?
> 
> This is my gut reaction as well.

Pretty much agreed.

> Heikki, why can't we correct the firmware tables? That needs to be made clear
> before we start adding hacks to the kernel.

We need to start working in that direction really.  Fixing problems in ACPI
tables via kernel code is not going to scale sufficiently IMO.

> You said, "Bad for Linux", why is this not a problem for other operating
> systems?

Good question. :-)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Thu, Jan 15, 2015 at 10:28:03AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, January 14, 2015 08:32:12 AM Darren Hart wrote:
> > 
> > On 1/14/15 4:58 AM, Linus Walleij wrote:
> > > On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > 
> > >> This makes it possible to assign GPIOs at runtime. The
> > >> motivation for it is because of need to forward GPIOs from
> > >> one device to an other. That feature may be useful for
> > >> example with some mfd devices, but initially it is needed
> > 
> > +Samuel Ortiz for his thoughts on applicability to MFD.
> > 
> > >> because on some Intel Braswell based ACPI platforms the GPIO
> > >> resources controlling signals of the USB PHY are given to
> > >> the controller device object for whatever reason, so the
> > >> driver of that controller needs be able to pass them to the
> > >> PHY device somehow.
> > >>
> > >> So basically this is meant to allow patching of bad (bad
> > >> from Linux kernels point of view) hardware descriptions
> > >> provided by system fw in the drivers.
> > >>
> > >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >> ---
> > >>
> > >> Hi,
> > >>
> > >> I'm sending this first as a RFC in case you guys have some better
> > >> idea how to solve our problem. I actually already have couple
> > >> platforms where the GPIO resources are given to the "wrong" device
> > >> objects now.
> > >>
> > >> Originally we were thinking about simply handling our problem with
> > >> hacks to the PHY drivers, basically also checking if the parent has
> > >> GPIOs. That would only work if the controller is always the parent,
> > >> which it's not, but even if it was, it would be too risky. The PHY
> > >> drivers don't know which controller they are attached to, so what is
> > >> to say that the GPIOs aren't really attached to the controller.
> > >>
> > >> So the safest way to handle our problem is to deal with it in quirks
> > >> in the controller drivers. Solving it by bouncing the GPIOs did not
> > >> feel ideal there doesn't seem to be any other way. The API is copied
> > >> from clkdev (clk_register_clkdev). In the end it's really only one
> > >> function for adding the lookups and one for removing them.
> > >>
> > >> The way it's implemented is by modifying the current style of handling
> > >> the lookups a little bit. The per device lookup table is basically
> > >> reverted back to the single linked-list format since it seems that
> > >> these lookups may have to be assigned from several sources.
> > > 
> > > Oh ain't that great.
> > > 
> > > So now we start patching around the kernel because the ACPI
> > > tables are erroneous for GPIOs. I'd like some feedback from
> > > Rafael &| Darren on this patch, i.e. if you two think this is a good
> > > way of accounting for bad GPIO descriptions in ACPI tables then
> > > ACK this patch.
> > > 
> > > I guess the other option would be to fix up the ACPI DSDT
> > > itself to put resources right, correct? Is this not possible?
> > 
> > This is my gut reaction as well.
> 
> Pretty much agreed.
> 
> > Heikki, why can't we correct the firmware tables? That needs to be made clear
> > before we start adding hacks to the kernel.
> 
> We need to start working in that direction really.  Fixing problems in ACPI
> tables via kernel code is not going to scale sufficiently IMO.

Fixing the DSDT produced by the firmware was my first suggesting for
this, but it just does not seem to be happening. These products are
already out on the market (this is one of them [1]) and what I have
understood is that the firmware just is what it is. It's almost as if
there is nobody even developing it for these products anymore. Even
fixes would not go in and this is not even considered a fix. Things
work just find for them with their hacked kernel.

So the firmware and the ACPI tables it provides are not going to be
fixed. What else can we do? Can we expect the users to always use
custom DSDT, or maybe somekind of custom AML snipped (is something
like that even possible), when using these products?

> > You said, "Bad for Linux", why is this not a problem for other operating
> > systems?
> 
> Good question. :-)

I would imagine certain operating systems consider a component like
PHY as something that should always be handled by the controller
driver. It does not seem to be a problem for them even if every second
product using the same SoC happens to have different PHY. All of them
will have a custom controller driver in any case. Even though the
example product below is an Android tablet, to me it looks like the
style of writing software comes straight from those "other" operation
systems.

At least it seems clear that these guys don't understand that it's not
possible to do things like write a new driver for every product using
the same SoC in Linux.

[1] http://www.trekstor.de/detail-surftabs-en/product/surftab-xintron-i-70.html

Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> I am not really fond of this idea since it adds complexity to the
> (already too complex) GPIO lookup, and only solves to a local level
> (GPIO) what is a more global problem (bad ACPI tables that can affect
> any subsystem).
(...)
> it
> seems more to-the-point to find a way to fix/patch the ACPI tables at
> runtime, if that is possible at all, to provide a more general
> solution to this issue.

This is my position as well, until proven that this cannot be done.
In device tree the same mechanism is called "device tree overlays"
and I just have some vague feeling that such stuff is patched around in
some Intel platforms already, but maybe that involves replacing
the whole DSDT from userspace, surely the mechanism can be
refined?

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> 
> > I am not really fond of this idea since it adds complexity to the
> > (already too complex) GPIO lookup, and only solves to a local level
> > (GPIO) what is a more global problem (bad ACPI tables that can affect
> > any subsystem).
> (...)
> > it
> > seems more to-the-point to find a way to fix/patch the ACPI tables at
> > runtime, if that is possible at all, to provide a more general
> > solution to this issue.
> 
> This is my position as well, until proven that this cannot be done.

Well, that goes against the current practice, mind you, which *is* to put
workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
that, but it has been done for several years now.

Also someone may say to that: "Why don't *you* demonstrate that it can be done
in the first place?"  And what if it can be done, but is too complex to be
practical or similar?

My personal opinion is that having a way to apply a fix on top of broken ACPI
tables (or an extension on top of correct ones for that matter) without touching
the kernel would be very useful indeed, but making it secure may be somewhat
challenging, because in principle there's no reason why the kernel should trust
such "external" fixes.

> In device tree the same mechanism is called "device tree overlays"
> and I just have some vague feeling that such stuff is patched around in
> some Intel platforms already, but maybe that involves replacing
> the whole DSDT from userspace,

>From initramfs rather than from user space, but yes, it does.

> surely the mechanism can be refined?

Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
going to take some time.  Once we've done that, we'll see how painful it is to
"patch" ACPI tables this way in practice.

Also there is an ecosystem problem related to distributing such "patches".
Today, distributions don't need to worry about patching buggy platform
firmware, because they get workarounds in the kernel, but if we switch over
to the model in which platform firmware "overlays" need to be provided in
addition to it, then suddenly questions arise about who should be responsible
for making them available, how to avoid duplication of efforts between
distributions etc.

All of that needs to be clarified before we start making hard statements like
"No in-kernel workarounds for that!"

And, of course, there's the question of what the kernel should do if the given
firmware patch is not effective, so it doesn't really fix the problem it is
supposed to fix or it fixes that problem only partially or, worse yet, it
introuces more bugs than it fixes.  Should the kernel simply fail then (and
in what way if so) or should it try to carry out some default "sanitization"
of what the firmare (and patch) tells it and try to continue on the best
effort basis?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Jan 21, 2015 at 6:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
>> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>
>> > I am not really fond of this idea since it adds complexity to the
>> > (already too complex) GPIO lookup, and only solves to a local level
>> > (GPIO) what is a more global problem (bad ACPI tables that can affect
>> > any subsystem).
>> (...)
>> > it
>> > seems more to-the-point to find a way to fix/patch the ACPI tables at
>> > runtime, if that is possible at all, to provide a more general
>> > solution to this issue.
>>
>> This is my position as well, until proven that this cannot be done.
>
> Well, that goes against the current practice, mind you, which *is* to put
> workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
> that, but it has been done for several years now.
>
> Also someone may say to that: "Why don't *you* demonstrate that it can be done
> in the first place?"  And what if it can be done, but is too complex to be
> practical or similar?
>
> My personal opinion is that having a way to apply a fix on top of broken ACPI
> tables (or an extension on top of correct ones for that matter) without touching
> the kernel would be very useful indeed, but making it secure may be somewhat
> challenging, because in principle there's no reason why the kernel should trust
> such "external" fixes.
>
>> In device tree the same mechanism is called "device tree overlays"
>> and I just have some vague feeling that such stuff is patched around in
>> some Intel platforms already, but maybe that involves replacing
>> the whole DSDT from userspace,
>
> From initramfs rather than from user space, but yes, it does.
>
>> surely the mechanism can be refined?
>
> Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> going to take some time.  Once we've done that, we'll see how painful it is to
> "patch" ACPI tables this way in practice.
>
> Also there is an ecosystem problem related to distributing such "patches".
> Today, distributions don't need to worry about patching buggy platform
> firmware, because they get workarounds in the kernel, but if we switch over
> to the model in which platform firmware "overlays" need to be provided in
> addition to it, then suddenly questions arise about who should be responsible
> for making them available, how to avoid duplication of efforts between
> distributions etc.
>
> All of that needs to be clarified before we start making hard statements like
> "No in-kernel workarounds for that!"
>
> And, of course, there's the question of what the kernel should do if the given
> firmware patch is not effective, so it doesn't really fix the problem it is
> supposed to fix or it fixes that problem only partially or, worse yet, it
> introuces more bugs than it fixes.  Should the kernel simply fail then (and
> in what way if so) or should it try to carry out some default "sanitization"
> of what the firmare (and patch) tells it and try to continue on the best
> effort basis?

If we decide to go ahead with the solution proposed by this patch for
practical reasons (which are good reasons indeed), I still have one
problem with its current form.

As the discussion highlighted, this is an ACPI problem, so I'd very
much like it to be confined to the ACPI GPIO code, to be enabled only
when ACPI is, and to use function names that start with acpi_gpio. The
current implementation leverages platform lookup, making said lookup
less efficient in the process and bringing confusion about its
purpose. Although the two processes are indeed similar, they are
separate things: one is a legitimate way to map GPIOs, the other is a
fixup for broken firmware.

I suppose we all agree this is a hackish fix, so let's confine it as
much as we can.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Tue, Jan 20, 2015 at 10:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> going to take some time.  Once we've done that, we'll see how painful it is to
> "patch" ACPI tables this way in practice.
>
> Also there is an ecosystem problem related to distributing such "patches".
> Today, distributions don't need to worry about patching buggy platform
> firmware, because they get workarounds in the kernel, but if we switch over
> to the model in which platform firmware "overlays" need to be provided in
> addition to it, then suddenly questions arise about who should be responsible
> for making them available, how to avoid duplication of efforts between
> distributions etc.
>
> All of that needs to be clarified before we start making hard statements like
> "No in-kernel workarounds for that!"

OK so why can't the patching happen in the kernel?

If the kernel anyway has to supply some kind of workaround for
the issue, it is more a question of where to place it. Whether it does
so by patching the ACPI tables or by detecting a bad ACPI thing
and working around it at runtime in a certain driver doesn't really
matter, does it? They are both in-kernel ACPI fixes, just that one
of the mechanisms is generic.

I don't understand why this obsession with userspace having
to do the ACPI table patching - if kernels should "just work" then
put this stuff behind Kconfig and have it in the kernel.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:
> On Tue, Jan 20, 2015 at 10:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> > Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> > going to take some time.  Once we've done that, we'll see how painful it is to
> > "patch" ACPI tables this way in practice.
> >
> > Also there is an ecosystem problem related to distributing such "patches".
> > Today, distributions don't need to worry about patching buggy platform
> > firmware, because they get workarounds in the kernel, but if we switch over
> > to the model in which platform firmware "overlays" need to be provided in
> > addition to it, then suddenly questions arise about who should be responsible
> > for making them available, how to avoid duplication of efforts between
> > distributions etc.
> >
> > All of that needs to be clarified before we start making hard statements like
> > "No in-kernel workarounds for that!"
> 
> OK so why can't the patching happen in the kernel?
> 
> If the kernel anyway has to supply some kind of workaround for
> the issue, it is more a question of where to place it. Whether it does
> so by patching the ACPI tables or by detecting a bad ACPI thing
> and working around it at runtime in a certain driver doesn't really
> matter, does it?

It needs to know what to patch and how so the result is still consistent.

How do you think the kernel is going to figure that out?

> They are both in-kernel ACPI fixes, just that one
> of the mechanisms is generic.

I'm not following you here, sorry.

> I don't understand why this obsession with userspace having
> to do the ACPI table patching - if kernels should "just work" then
> put this stuff behind Kconfig and have it in the kernel.

This is not an obsession and your suggestion here leads to having custom
per-board kernels which is not supportable in the long term.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> On Wed, Jan 21, 2015 at 6:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
> >> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >>
> >> > I am not really fond of this idea since it adds complexity to the
> >> > (already too complex) GPIO lookup, and only solves to a local level
> >> > (GPIO) what is a more global problem (bad ACPI tables that can affect
> >> > any subsystem).
> >> (...)
> >> > it
> >> > seems more to-the-point to find a way to fix/patch the ACPI tables at
> >> > runtime, if that is possible at all, to provide a more general
> >> > solution to this issue.
> >>
> >> This is my position as well, until proven that this cannot be done.
> >
> > Well, that goes against the current practice, mind you, which *is* to put
> > workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
> > that, but it has been done for several years now.
> >
> > Also someone may say to that: "Why don't *you* demonstrate that it can be done
> > in the first place?"  And what if it can be done, but is too complex to be
> > practical or similar?
> >
> > My personal opinion is that having a way to apply a fix on top of broken ACPI
> > tables (or an extension on top of correct ones for that matter) without touching
> > the kernel would be very useful indeed, but making it secure may be somewhat
> > challenging, because in principle there's no reason why the kernel should trust
> > such "external" fixes.
> >
> >> In device tree the same mechanism is called "device tree overlays"
> >> and I just have some vague feeling that such stuff is patched around in
> >> some Intel platforms already, but maybe that involves replacing
> >> the whole DSDT from userspace,
> >
> > From initramfs rather than from user space, but yes, it does.
> >
> >> surely the mechanism can be refined?
> >
> > Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> > going to take some time.  Once we've done that, we'll see how painful it is to
> > "patch" ACPI tables this way in practice.
> >
> > Also there is an ecosystem problem related to distributing such "patches".
> > Today, distributions don't need to worry about patching buggy platform
> > firmware, because they get workarounds in the kernel, but if we switch over
> > to the model in which platform firmware "overlays" need to be provided in
> > addition to it, then suddenly questions arise about who should be responsible
> > for making them available, how to avoid duplication of efforts between
> > distributions etc.
> >
> > All of that needs to be clarified before we start making hard statements like
> > "No in-kernel workarounds for that!"
> >
> > And, of course, there's the question of what the kernel should do if the given
> > firmware patch is not effective, so it doesn't really fix the problem it is
> > supposed to fix or it fixes that problem only partially or, worse yet, it
> > introuces more bugs than it fixes.  Should the kernel simply fail then (and
> > in what way if so) or should it try to carry out some default "sanitization"
> > of what the firmare (and patch) tells it and try to continue on the best
> > effort basis?
> 
> If we decide to go ahead with the solution proposed by this patch for
> practical reasons (which are good reasons indeed), I still have one
> problem with its current form.
> 
> As the discussion highlighted, this is an ACPI problem, so I'd very
> much like it to be confined to the ACPI GPIO code, to be enabled only
> when ACPI is, and to use function names that start with acpi_gpio.

I can agree with that.

> The current implementation leverages platform lookup, making said lookup
> less efficient in the process and bringing confusion about its
> purpose. Although the two processes are indeed similar, they are
> separate things: one is a legitimate way to map GPIOs, the other is a
> fixup for broken firmware.
> 
> I suppose we all agree this is a hackish fix, so let's confine it as
> much as we can.

OK

Heikki, any comments?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Hi guys,

On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > If we decide to go ahead with the solution proposed by this patch for
> > practical reasons (which are good reasons indeed), I still have one
> > problem with its current form.
> > 
> > As the discussion highlighted, this is an ACPI problem, so I'd very
> > much like it to be confined to the ACPI GPIO code, to be enabled only
> > when ACPI is, and to use function names that start with acpi_gpio.
> 
> I can agree with that.
> 
> > The current implementation leverages platform lookup, making said lookup
> > less efficient in the process and bringing confusion about its
> > purpose. Although the two processes are indeed similar, they are
> > separate things: one is a legitimate way to map GPIOs, the other is a
> > fixup for broken firmware.
> > 
> > I suppose we all agree this is a hackish fix, so let's confine it as
> > much as we can.
> 
> OK
> 
> Heikki, any comments?

I'm fine with that.

That actually makes me think that we could then drop the lookup tables
completely and use device properties instead with the help of "generic
property" (attached):

We would just need to agree on the format how to describe a gpio
property, document it and of course convert the current users as
usual. The format could be something like this as an example (I'm
writing this out of my head so don't shoot me if you can see it would
not work. Just an example):

static const u32 example_gpio[] = { <gpio>, <flags>, };

static struct dev_gen_prop example_prop[] =
        {
                .type = DEV_PROP_U32,
                .name = "gpio,<con_id>",
                .nval = 2,
                .num = &example_gpio,
        },
        { },
};

static struct platform_device example_pdev = {
        ...
        .dev = {
                .gen_prop = &example_prop,
        },
}


In gpiolib.c we would then, instead of going through the lookups,
simply ask for that property:

        ...
        sprintf(propname, "gpio,%s", con_id);
        device_property_read_u32_array(dev, propname, &val, 2);
        ...
        desc = gpio_to_desc(val[0]);
        flags = val[1];
        ...


So this is just and idea. I think it would be relatively easy to
implement. What do you guys think?


Cheers,

-- 
heikki

[-- Attachment #2: 0001-driver-core-property-support-for-generic-property.patch --]
[-- Type: text/plain, Size: 7947 bytes --]

>From bb1146d8ca7c39386dfc53043051d389cba49cbe Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 7 Jan 2015 10:57:44 +0200
Subject: [PATCH] driver core: property: support for generic property

This extends the unified device property interface by adding
"Generic Property" to it for cases where device tree or ACPI
are not being used.

That makes the unified device property interface cover also
most of the cases where information is extracted from custom
platform_data in the drivers. So if before we had to check
separately is there custom platform_data for a driver:

	if (pdata)
		bar = pdata->bar;
	else
		device_property_read_u32(dev, "foo", &bar);

we can now drop that check and simply always use the unified
device property interface.

That makes it possible to drop a lot of boiler plate from
the drivers, plus quite a few header files under
include/linux/ describing those driver specific platform
data structures can be removed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c  | 135 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/device.h   |   3 ++
 include/linux/property.h |  17 ++++++
 3 files changed, 143 insertions(+), 12 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c458458..4ea6d27 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -15,6 +15,108 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 
+static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name)
+{
+	struct dev_gen_prop *prop;
+
+	if (!dev->gen_prop)
+		return NULL;
+
+	for (prop = dev->gen_prop; prop->name; prop++)
+		if (!strcmp(name, prop->name))
+			return prop;
+	return NULL;
+}
+
+static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u16(u16 *src, u16 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u32(u32 *src, u32 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u64(u64 *src, u64 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_string(const char **src, const char **val,
+				      size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_read_array(struct device *dev, const char *name,
+			       enum dev_prop_type type, void *val, size_t nval)
+{
+	struct dev_gen_prop *prop;
+	int ret = 0;
+
+	prop = dev_prop_get(dev, name);
+	if (!prop)
+		return -ENODATA;
+
+	if (prop->type != type)
+		return -EPROTO;
+
+	if (prop->nval < nval)
+		return -EOVERFLOW;
+
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		ret = dev_prop_copy_array_u8((u8 *)prop->num, (u8 *)val,
+					     nval);
+		break;
+	case DEV_PROP_U16:
+		ret = dev_prop_copy_array_u16((u16 *)prop->num, (u16 *)val,
+					      nval);
+		break;
+	case DEV_PROP_U32:
+		ret = dev_prop_copy_array_u32((u32 *)prop->num, (u32 *)val,
+					      nval);
+		break;
+	case DEV_PROP_U64:
+		ret = dev_prop_copy_array_u64(prop->num, (u64 *)val, nval);
+		break;
+	case DEV_PROP_STRING:
+		ret = dev_prop_copy_array_string(prop->str,
+						 (const char **)val, nval);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
@@ -26,8 +128,9 @@ bool device_property_present(struct device *dev, const char *propname)
 {
 	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
 		return of_property_read_bool(dev->of_node, propname);
-
-	return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
+	if (ACPI_HANDLE(dev))
+		return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
+	return !!dev_prop_get(dev, propname);
 }
 EXPORT_SYMBOL_GPL(device_property_present);
 
@@ -55,8 +158,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_present);
 	IS_ENABLED(CONFIG_OF) && _dev_->of_node ? \
 		(OF_DEV_PROP_READ_ARRAY(_dev_->of_node, _propname_, _type_, \
 					_val_, _nval_)) : \
-		acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
-				   _proptype_, _val_, _nval_)
+		ACPI_HANDLE(_dev_) ? \
+			acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
+					   _proptype_, _val_, _nval_) : \
+			dev_prop_read_array(_dev_, _propname_, _proptype_, \
+					   _val_, _nval_)
 
 /**
  * device_property_read_u8_array - return a u8 array property of a device
@@ -169,10 +275,13 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array);
 int device_property_read_string_array(struct device *dev, const char *propname,
 				      const char **val, size_t nval)
 {
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		of_property_read_string_array(dev->of_node, propname, val, nval) :
-		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
-				   DEV_PROP_STRING, val, nval);
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_property_read_string_array(dev->of_node, propname,
+						     val, nval);
+	if (ACPI_HANDLE(dev))
+		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
+					  DEV_PROP_STRING, val, nval);
+	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string_array);
 
@@ -193,10 +302,12 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array);
 int device_property_read_string(struct device *dev, const char *propname,
 				const char **val)
 {
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		of_property_read_string(dev->of_node, propname, val) :
-		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
-				   DEV_PROP_STRING, val, 1);
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_property_read_string(dev->of_node, propname, val);
+	if (ACPI_HANDLE(dev))
+		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
+					  DEV_PROP_STRING, val, 1);
+	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, 1);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..738dc25 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -27,6 +27,7 @@
 #include <linux/ratelimit.h>
 #include <linux/uidgid.h>
 #include <linux/gfp.h>
+#include <linux/property.h>
 #include <asm/device.h>
 
 struct device;
@@ -704,6 +705,7 @@ struct acpi_dev_node {
  * @archdata:	For arch-specific additions.
  * @of_node:	Associated device tree node.
  * @acpi_node:	Associated ACPI device node.
+ * @gen_prop:	Generic device property
  * @devt:	For creating the sysfs "dev".
  * @id:		device instance
  * @devres_lock: Spinlock to protect the resource of the device.
@@ -780,6 +782,7 @@ struct device {
 
 	struct device_node	*of_node; /* associated device tree node */
 	struct acpi_dev_node	acpi_node; /* associated ACPI device node */
+	struct dev_gen_prop	*gen_prop; /* generic device property */
 
 	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
 	u32			id;	/* device instance */
diff --git a/include/linux/property.h b/include/linux/property.h
index a6a3d98..44e875f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -26,6 +26,23 @@ enum dev_prop_type {
 	DEV_PROP_MAX,
 };
 
+/**
+ * struct dev_gen_prop - Generic Device Property
+ * @name: property name
+ * @nval: number of elements in the array
+ * @str: value array of strings
+ * @num: value array of numbers
+ *
+ * Used when of_node and acpi_node are missing.
+ */
+struct dev_gen_prop {
+	enum dev_prop_type type;
+	const char *name;
+	size_t nval;
+	const char **str;
+	u64 *num;
+};
+
 bool device_property_present(struct device *dev, const char *propname);
 int device_property_read_u8_array(struct device *dev, const char *propname,
 				  u8 *val, size_t nval);
-- 
2.1.4

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Friday, January 23, 2015 01:21:22 PM Heikki Krogerus wrote:
> 
> --Nq2Wo0NMKNjxTN9z
> Content-Type: text/plain; charset=iso-8859-1
> Content-Disposition: inline
> Content-Transfer-Encoding: 8bit
> 
> Hi guys,
> 
> On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > > If we decide to go ahead with the solution proposed by this patch for
> > > practical reasons (which are good reasons indeed), I still have one
> > > problem with its current form.
> > > 
> > > As the discussion highlighted, this is an ACPI problem, so I'd very
> > > much like it to be confined to the ACPI GPIO code, to be enabled only
> > > when ACPI is, and to use function names that start with acpi_gpio.
> > 
> > I can agree with that.
> > 
> > > The current implementation leverages platform lookup, making said lookup
> > > less efficient in the process and bringing confusion about its
> > > purpose. Although the two processes are indeed similar, they are
> > > separate things: one is a legitimate way to map GPIOs, the other is a
> > > fixup for broken firmware.
> > > 
> > > I suppose we all agree this is a hackish fix, so let's confine it as
> > > much as we can.
> > 
> > OK
> > 
> > Heikki, any comments?
> 
> I'm fine with that.
> 
> That actually makes me think that we could then drop the lookup tables
> completely and use device properties instead with the help of "generic
> property" (attached):

Which reminds me that I've lost track of this one.

Can you please resend it and CC something like linux-acpi?

Also I'm not sure what you mean by "drop the lookup tables completely".

> We would just need to agree on the format how to describe a gpio
> property, document it and of course convert the current users as
> usual. The format could be something like this as an example (I'm
> writing this out of my head so don't shoot me if you can see it would
> not work. Just an example):
> 
> static const u32 example_gpio[] = { <gpio>, <flags>, };
> 
> static struct dev_gen_prop example_prop[] =
>         {
>                 .type = DEV_PROP_U32,
>                 .name = "gpio,<con_id>",
>                 .nval = 2,
>                 .num = &example_gpio,
>         },
>         { },
> };
> 
> static struct platform_device example_pdev = {
>         ...
>         .dev = {
>                 .gen_prop = &example_prop,
>         },
> }
> 
> 
> In gpiolib.c we would then, instead of going through the lookups,
> simply ask for that property:
> 
>         ...
>         sprintf(propname, "gpio,%s", con_id);
>         device_property_read_u32_array(dev, propname, &val, 2);
>         ...
>         desc = gpio_to_desc(val[0]);
>         flags = val[1];
>         ...
> 
> 
> So this is just and idea. I think it would be relatively easy to
> implement. What do you guys think?

Well, I need some time to think about that.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Fri, Jan 23, 2015 at 04:14:13PM +0100, Rafael J. Wysocki wrote:
> > That actually makes me think that we could then drop the lookup tables
> > completely and use device properties instead with the help of "generic
> > property" (attached):
> 
> Which reminds me that I've lost track of this one.
> 
> Can you please resend it and CC something like linux-acpi?

OK, I'll resend it.

> Also I'm not sure what you mean by "drop the lookup tables completely".

So I meant removing struct gpio_lookup and struct gpio_lookup_table.

> > We would just need to agree on the format how to describe a gpio
> > property, document it and of course convert the current users as
> > usual. The format could be something like this as an example (I'm
> > writing this out of my head so don't shoot me if you can see it would
> > not work. Just an example):
> > 
> > static const u32 example_gpio[] = { <gpio>, <flags>, };
> > 
> > static struct dev_gen_prop example_prop[] =
> >         {
> >                 .type = DEV_PROP_U32,
> >                 .name = "gpio,<con_id>",
> >                 .nval = 2,
> >                 .num = &example_gpio,
> >         },
> >         { },
> > };
> > 
> > static struct platform_device example_pdev = {
> >         ...
> >         .dev = {
> >                 .gen_prop = &example_prop,
> >         },
> > }
> > 
> > 
> > In gpiolib.c we would then, instead of going through the lookups,
> > simply ask for that property:
> > 
> >         ...
> >         sprintf(propname, "gpio,%s", con_id);
> >         device_property_read_u32_array(dev, propname, &val, 2);
> >         ...
> >         desc = gpio_to_desc(val[0]);
> >         flags = val[1];
> >         ...
> > 
> > 
> > So this is just and idea. I think it would be relatively easy to
> > implement. What do you guys think?
> 
> Well, I need some time to think about that.


Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Thu, Jan 22, 2015 at 5:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:

>> If the kernel anyway has to supply some kind of workaround for
>> the issue, it is more a question of where to place it. Whether it does
>> so by patching the ACPI tables or by detecting a bad ACPI thing
>> and working around it at runtime in a certain driver doesn't really
>> matter, does it?
>
> It needs to know what to patch and how so the result is still consistent.
>
> How do you think the kernel is going to figure that out?

Isn't every DSDT (etc) unique? So you could detect one by making
a checksum of the binary or something.

And then you'd know that the table with this checksum needs
patching?

>> They are both in-kernel ACPI fixes, just that one
>> of the mechanisms is generic.
>
> I'm not following you here, sorry.

As per above? Fix the firmware table, not work around the
fact that it is buggy.

>> I don't understand why this obsession with userspace having
>> to do the ACPI table patching - if kernels should "just work" then
>> put this stuff behind Kconfig and have it in the kernel.
>
> This is not an obsession and your suggestion here leads to having custom
> per-board kernels which is not supportable in the long term.

Not at all. The other suggestion leads to having custom per-board
fixes in every driver for which broken ACPI descriptions exists,
which is just as bad, isn't it? Instead of collecting the problem
in one place (patch any broken ACPI table) it is distrubuted across
N drivers, where each and every one has to detect that it is
being malinformed by a broken ACPI table.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> On Thu, Jan 22, 2015 at 5:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:
> 
> >> If the kernel anyway has to supply some kind of workaround for
> >> the issue, it is more a question of where to place it. Whether it does
> >> so by patching the ACPI tables or by detecting a bad ACPI thing
> >> and working around it at runtime in a certain driver doesn't really
> >> matter, does it?
> >
> > It needs to know what to patch and how so the result is still consistent.
> >
> > How do you think the kernel is going to figure that out?
> 
> Isn't every DSDT (etc) unique?

Formally, it doesn't have to be.

And it doesn't have to be a DSDT, it may be an SSDT too or even a plain
table that's buggy.

> So you could detect one by making a checksum of the binary or something.
> 
> And then you'd know that the table with this checksum needs patching?

At a single table level it is generally difficult to say whether or not
things are going to work.

What needs to work is the namespace which is built from all of the tables
provided combined.  So the namespace needs to be populated first and then
fixes applied on top of that (presumably by deleting, adding or replacing
objects).

Now, in theory, you *may* be able to figure out that combination of tables
A produces namespace B which then will require fix X if the system is Y,
but quite frankly I wouldn't count on that.

Moreover, fixups (or "patches" as I called them, but that wasn't exactly
correct) need to be provided in the form of AML definition blocks to apply on
top of an already populated namespace and if you want to use a binary kernel image,
you can't really afford putting all that stuff for all systems it can possibly
run on into it.  This means that distros need to be able to combine a fixup for
the ACPI tables with the binary kernel and install the result into the system's
boot medium (whatever it is).  Also it should be possible to update the fixup
and the kernel image separately if necessary.

Now from the kernel's perspective that raises the question: "What if the
ACPI tables fixup provided by the distro is not sufficient?"

That needs to be addressed somehow in the code.

> >> They are both in-kernel ACPI fixes, just that one
> >> of the mechanisms is generic.
> >
> > I'm not following you here, sorry.
> 
> As per above? Fix the firmware table, not work around the
> fact that it is buggy.
> 
> >> I don't understand why this obsession with userspace having
> >> to do the ACPI table patching - if kernels should "just work" then
> >> put this stuff behind Kconfig and have it in the kernel.
> >
> > This is not an obsession and your suggestion here leads to having custom
> > per-board kernels which is not supportable in the long term.
> 
> Not at all. The other suggestion leads to having custom per-board
> fixes in every driver for which broken ACPI descriptions exists,

So I'm not arguing for that.

> which is just as bad, isn't it? Instead of collecting the problem
> in one place (patch any broken ACPI table) it is distrubuted across
> N drivers, where each and every one has to detect that it is
> being malinformed by a broken ACPI table.

In the general-purpose binary kernel image distribution model drivers generally
need to treat information provided by the platform firmware, be it ACPI or a DT
or what-not, as untrusted input that needs to be validated if possible.  That
is not always possible and in those cases we have no choice but to try to use
that information, fingers crossed.  Sometimes we can validate it, though, and
then we should and do something if it turns out to be invalid.

Overall, my view on that is that (1) there needs to be a way for a distro to
provide an ACPI tables fixup for the kernel to apply on top of the already
populated ACPI namespace on a given system and (2) drivers need to be careful
about using firmware data and possibly be able to recover from errors in there.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:

>> So you could detect one by making a checksum of the binary or something.
>>
>> And then you'd know that the table with this checksum needs patching?
>
> At a single table level it is generally difficult to say whether or not
> things are going to work.
>
> What needs to work is the namespace which is built from all of the tables
> provided combined.  So the namespace needs to be populated first and then
> fixes applied on top of that (presumably by deleting, adding or replacing
> objects).
>
> Now, in theory, you *may* be able to figure out that combination of tables
> A produces namespace B which then will require fix X if the system is Y,
> but quite frankly I wouldn't count on that.
>
> Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> correct) need to be provided in the form of AML definition blocks to apply on
> top of an already populated namespace and if you want to use a binary kernel image,
> you can't really afford putting all that stuff for all systems it can possibly
> run on into it.  This means that distros need to be able to combine a fixup for
> the ACPI tables with the binary kernel and install the result into the system's
> boot medium (whatever it is).  Also it should be possible to update the fixup
> and the kernel image separately if necessary.
>
> Now from the kernel's perspective that raises the question: "What if the
> ACPI tables fixup provided by the distro is not sufficient?"
>
> That needs to be addressed somehow in the code.

Yeah I guess I'm convinced that we need to handle this particular
weirdness in the gpio-acpi code... if it can be contained there as
expressed by Alexandre.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> 
> >> So you could detect one by making a checksum of the binary or something.
> >>
> >> And then you'd know that the table with this checksum needs patching?
> >
> > At a single table level it is generally difficult to say whether or not
> > things are going to work.
> >
> > What needs to work is the namespace which is built from all of the tables
> > provided combined.  So the namespace needs to be populated first and then
> > fixes applied on top of that (presumably by deleting, adding or replacing
> > objects).
> >
> > Now, in theory, you *may* be able to figure out that combination of tables
> > A produces namespace B which then will require fix X if the system is Y,
> > but quite frankly I wouldn't count on that.
> >
> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> > correct) need to be provided in the form of AML definition blocks to apply on
> > top of an already populated namespace and if you want to use a binary kernel image,
> > you can't really afford putting all that stuff for all systems it can possibly
> > run on into it.  This means that distros need to be able to combine a fixup for
> > the ACPI tables with the binary kernel and install the result into the system's
> > boot medium (whatever it is).  Also it should be possible to update the fixup
> > and the kernel image separately if necessary.
> >
> > Now from the kernel's perspective that raises the question: "What if the
> > ACPI tables fixup provided by the distro is not sufficient?"
> >
> > That needs to be addressed somehow in the code.
> 
> Yeah I guess I'm convinced that we need to handle this particular
> weirdness in the gpio-acpi code... if it can be contained there as
> expressed by Alexandre.

I'm still fine if we want to confine this "gpio forwarding" to acpi
if you guys want it, but I was looking at the Sound SoC drivers and I
realised that we do have places which pass gpio descriptors to other
devices in platform data. And these of course aren't always used on
acpi platforms. By greping gpio_desc I saw at least these files are
passing it in platform data structures:

include/sound/soc.h
include/linux/leds.h
include/linux/usb/usb_phy_generic.h

There are probable others but I checked those. And of course now there
is nothing preventing people from adding more of them.

So we could use my original idea with them. I think most of the
drivers using those are tied to separate handling for dt and pdata
only because of the way the gpio descriptor is delivered to them.

What do you guys think?


Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> Hi guys,
>
> On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
>> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
>> > If we decide to go ahead with the solution proposed by this patch for
>> > practical reasons (which are good reasons indeed), I still have one
>> > problem with its current form.
>> >
>> > As the discussion highlighted, this is an ACPI problem, so I'd very
>> > much like it to be confined to the ACPI GPIO code, to be enabled only
>> > when ACPI is, and to use function names that start with acpi_gpio.
>>
>> I can agree with that.
>>
>> > The current implementation leverages platform lookup, making said lookup
>> > less efficient in the process and bringing confusion about its
>> > purpose. Although the two processes are indeed similar, they are
>> > separate things: one is a legitimate way to map GPIOs, the other is a
>> > fixup for broken firmware.
>> >
>> > I suppose we all agree this is a hackish fix, so let's confine it as
>> > much as we can.
>>
>> OK
>>
>> Heikki, any comments?
>
> I'm fine with that.
>
> That actually makes me think that we could then drop the lookup tables
> completely and use device properties instead with the help of "generic
> property" (attached):
>
> We would just need to agree on the format how to describe a gpio
> property, document it and of course convert the current users as
> usual. The format could be something like this as an example (I'm
> writing this out of my head so don't shoot me if you can see it would
> not work. Just an example):
>
> static const u32 example_gpio[] = { <gpio>, <flags>,爙;
>
> static struct dev_gen_prop example_prop[] =
>         {
>                 .type = DEV_PROP_U32,
>                 .name = "gpio,<con_id>",
>                 .nval = 2,
>                 .num = &example_gpio,
>         },
>         { },
> };
>
> static struct platform_device example_pdev = {
>         ...
>         .dev = {
>                 .gen_prop = &example_prop,
>         },
> }
>
>
> In gpiolib.c we would then, instead of going through the lookups,
> simply ask for that property:
>
>         ...
>         sprintf(propname, "gpio,%s", con_id);
>         device_property_read_u32_array(dev, propname, &val, 2);
>         ...
>         desc = gpio_to_desc(val[0]);
>         flags = val[1];
>         ...
>
>
> So this is just and idea. I think it would be relatively easy to
> implement. What do you guys think?

At first sight, that looks like a very good idea and a great use of
the device properties API. Are you willing to explore it further?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
>> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
>>
>> >> So you could detect one by making a checksum of the binary or something.
>> >>
>> >> And then you'd know that the table with this checksum needs patching?
>> >
>> > At a single table level it is generally difficult to say whether or not
>> > things are going to work.
>> >
>> > What needs to work is the namespace which is built from all of the tables
>> > provided combined.  So the namespace needs to be populated first and then
>> > fixes applied on top of that (presumably by deleting, adding or replacing
>> > objects).
>> >
>> > Now, in theory, you *may* be able to figure out that combination of tables
>> > A produces namespace B which then will require fix X if the system is Y,
>> > but quite frankly I wouldn't count on that.
>> >
>> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
>> > correct) need to be provided in the form of AML definition blocks to apply on
>> > top of an already populated namespace and if you want to use a binary kernel image,
>> > you can't really afford putting all that stuff for all systems it can possibly
>> > run on into it.  This means that distros need to be able to combine a fixup for
>> > the ACPI tables with the binary kernel and install the result into the system's
>> > boot medium (whatever it is).  Also it should be possible to update the fixup
>> > and the kernel image separately if necessary.
>> >
>> > Now from the kernel's perspective that raises the question: "What if the
>> > ACPI tables fixup provided by the distro is not sufficient?"
>> >
>> > That needs to be addressed somehow in the code.
>>
>> Yeah I guess I'm convinced that we need to handle this particular
>> weirdness in the gpio-acpi code... if it can be contained there as
>> expressed by Alexandre.
>
> I'm still fine if we want to confine this "gpio forwarding" to acpi
> if you guys want it, but I was looking at the Sound SoC drivers and I
> realised that we do have places which pass gpio descriptors to other
> devices in platform data. And these of course aren't always used on
> acpi platforms. By greping gpio_desc I saw at least these files are
> passing it in platform data structures:
>
> include/sound/soc.h
> include/linux/leds.h
> include/linux/usb/usb_phy_generic.h
>
> There are probable others but I checked those. And of course now there
> is nothing preventing people from adding more of them.

For sound/soc.h, the member is indeed public but I don't see it being
used to pass descriptors around between drivers.

For linux/leds.h, I think this is the reason why we introduced
devm_get_gpiod_from_child()

These looks more like a bad usage of GPIO descriptors, but AFAICT they
can be fixed by fixing the drivers and, by all means, this is where we
should do it.

This is a different situation from yours where we are trying to deal
with broken firmware and need to resort to tricks at one point or the
other.

Or am I missing your point?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Tuesday, February 10, 2015 06:32:46 PM Alexandre Courbot wrote:
> On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > Hi guys,
> >
> > On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> >> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> >> > If we decide to go ahead with the solution proposed by this patch for
> >> > practical reasons (which are good reasons indeed), I still have one
> >> > problem with its current form.
> >> >
> >> > As the discussion highlighted, this is an ACPI problem, so I'd very
> >> > much like it to be confined to the ACPI GPIO code, to be enabled only
> >> > when ACPI is, and to use function names that start with acpi_gpio.
> >>
> >> I can agree with that.
> >>
> >> > The current implementation leverages platform lookup, making said lookup
> >> > less efficient in the process and bringing confusion about its
> >> > purpose. Although the two processes are indeed similar, they are
> >> > separate things: one is a legitimate way to map GPIOs, the other is a
> >> > fixup for broken firmware.
> >> >
> >> > I suppose we all agree this is a hackish fix, so let's confine it as
> >> > much as we can.
> >>
> >> OK
> >>
> >> Heikki, any comments?
> >
> > I'm fine with that.
> >
> > That actually makes me think that we could then drop the lookup tables
> > completely and use device properties instead with the help of "generic
> > property" (attached):
> >
> > We would just need to agree on the format how to describe a gpio
> > property, document it and of course convert the current users as
> > usual. The format could be something like this as an example (I'm
> > writing this out of my head so don't shoot me if you can see it would
> > not work. Just an example):
> >
> > static const u32 example_gpio[] = { <gpio>, <flags>,爙;
> >
> > static struct dev_gen_prop example_prop[] =
> >         {
> >                 .type = DEV_PROP_U32,
> >                 .name = "gpio,<con_id>",
> >                 .nval = 2,
> >                 .num = &example_gpio,
> >         },
> >         { },
> > };
> >
> > static struct platform_device example_pdev = {
> >         ...
> >         .dev = {
> >                 .gen_prop = &example_prop,
> >         },
> > }
> >
> >
> > In gpiolib.c we would then, instead of going through the lookups,
> > simply ask for that property:
> >
> >         ...
> >         sprintf(propname, "gpio,%s", con_id);
> >         device_property_read_u32_array(dev, propname, &val, 2);
> >         ...
> >         desc = gpio_to_desc(val[0]);
> >         flags = val[1];
> >         ...
> >
> >
> > So this is just and idea. I think it would be relatively easy to
> > implement. What do you guys think?
> 
> At first sight, that looks like a very good idea and a great use of
> the device properties API. Are you willing to explore it further?

I guess Heikki is waiting for my feedback on his core patch that I'm
going to provide later today.

And yes, this is an interesting idea.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Tue, Feb 10, 2015 at 06:44:34PM +0900, Alexandre Courbot wrote:
> On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
> >> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> >>
> >> >> So you could detect one by making a checksum of the binary or something.
> >> >>
> >> >> And then you'd know that the table with this checksum needs patching?
> >> >
> >> > At a single table level it is generally difficult to say whether or not
> >> > things are going to work.
> >> >
> >> > What needs to work is the namespace which is built from all of the tables
> >> > provided combined.  So the namespace needs to be populated first and then
> >> > fixes applied on top of that (presumably by deleting, adding or replacing
> >> > objects).
> >> >
> >> > Now, in theory, you *may* be able to figure out that combination of tables
> >> > A produces namespace B which then will require fix X if the system is Y,
> >> > but quite frankly I wouldn't count on that.
> >> >
> >> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> >> > correct) need to be provided in the form of AML definition blocks to apply on
> >> > top of an already populated namespace and if you want to use a binary kernel image,
> >> > you can't really afford putting all that stuff for all systems it can possibly
> >> > run on into it.  This means that distros need to be able to combine a fixup for
> >> > the ACPI tables with the binary kernel and install the result into the system's
> >> > boot medium (whatever it is).  Also it should be possible to update the fixup
> >> > and the kernel image separately if necessary.
> >> >
> >> > Now from the kernel's perspective that raises the question: "What if the
> >> > ACPI tables fixup provided by the distro is not sufficient?"
> >> >
> >> > That needs to be addressed somehow in the code.
> >>
> >> Yeah I guess I'm convinced that we need to handle this particular
> >> weirdness in the gpio-acpi code... if it can be contained there as
> >> expressed by Alexandre.
> >
> > I'm still fine if we want to confine this "gpio forwarding" to acpi
> > if you guys want it, but I was looking at the Sound SoC drivers and I
> > realised that we do have places which pass gpio descriptors to other
> > devices in platform data. And these of course aren't always used on
> > acpi platforms. By greping gpio_desc I saw at least these files are
> > passing it in platform data structures:
> >
> > include/sound/soc.h
> > include/linux/leds.h
> > include/linux/usb/usb_phy_generic.h
> >
> > There are probable others but I checked those. And of course now there
> > is nothing preventing people from adding more of them.
> 
> For sound/soc.h, the member is indeed public but I don't see it being
> used to pass descriptors around between drivers.
> 
> For linux/leds.h, I think this is the reason why we introduced
> devm_get_gpiod_from_child()
> 
> These looks more like a bad usage of GPIO descriptors, but AFAICT they
> can be fixed by fixing the drivers and, by all means, this is where we
> should do it.
> 
> This is a different situation from yours where we are trying to deal
> with broken firmware and need to resort to tricks at one point or the
> other.
> 
> Or am I missing your point?

Well, in this case the motivation would be to make it possible to live
without platform data with these drivers, but in the end the situation
would be exactly the same. We need to give a gpio descriptor to
some other device in a driver. So the goal would be that in the actual
consumer driver of the gpio we could request it always in one way,
ideally always with gpiod_get() call.

But if there is no real need in other places for this thing, then
let's forget about it.


Thanks,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Tue, Feb 10, 2015 at 04:10:04PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 10, 2015 06:32:46 PM Alexandre Courbot wrote:
> > On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > Hi guys,
> > >
> > > On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> > >> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > >> > If we decide to go ahead with the solution proposed by this patch for
> > >> > practical reasons (which are good reasons indeed), I still have one
> > >> > problem with its current form.
> > >> >
> > >> > As the discussion highlighted, this is an ACPI problem, so I'd very
> > >> > much like it to be confined to the ACPI GPIO code, to be enabled only
> > >> > when ACPI is, and to use function names that start with acpi_gpio.
> > >>
> > >> I can agree with that.
> > >>
> > >> > The current implementation leverages platform lookup, making said lookup
> > >> > less efficient in the process and bringing confusion about its
> > >> > purpose. Although the two processes are indeed similar, they are
> > >> > separate things: one is a legitimate way to map GPIOs, the other is a
> > >> > fixup for broken firmware.
> > >> >
> > >> > I suppose we all agree this is a hackish fix, so let's confine it as
> > >> > much as we can.
> > >>
> > >> OK
> > >>
> > >> Heikki, any comments?
> > >
> > > I'm fine with that.
> > >
> > > That actually makes me think that we could then drop the lookup tables
> > > completely and use device properties instead with the help of "generic
> > > property" (attached):
> > >
> > > We would just need to agree on the format how to describe a gpio
> > > property, document it and of course convert the current users as
> > > usual. The format could be something like this as an example (I'm
> > > writing this out of my head so don't shoot me if you can see it would
> > > not work. Just an example):
> > >
> > > static const u32 example_gpio[] = { <gpio>, <flags>,爙;
> > >
> > > static struct dev_gen_prop example_prop[] =
> > >         {
> > >                 .type = DEV_PROP_U32,
> > >                 .name = "gpio,<con_id>",
> > >                 .nval = 2,
> > >                 .num = &example_gpio,
> > >         },
> > >         { },
> > > };
> > >
> > > static struct platform_device example_pdev = {
> > >         ...
> > >         .dev = {
> > >                 .gen_prop = &example_prop,
> > >         },
> > > }
> > >
> > >
> > > In gpiolib.c we would then, instead of going through the lookups,
> > > simply ask for that property:
> > >
> > >         ...
> > >         sprintf(propname, "gpio,%s", con_id);
> > >         device_property_read_u32_array(dev, propname, &val, 2);
> > >         ...
> > >         desc = gpio_to_desc(val[0]);
> > >         flags = val[1];
> > >         ...
> > >
> > >
> > > So this is just and idea. I think it would be relatively easy to
> > > implement. What do you guys think?
> > 
> > At first sight, that looks like a very good idea and a great use of
> > the device properties API. Are you willing to explore it further?

Yes. If I get green light for the generic property idea, I can start
thinking about this.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

Hi,

> If we decide to go ahead with the solution proposed by this patch for
> practical reasons (which are good reasons indeed), I still have one
> problem with its current form.
> 
> As the discussion highlighted, this is an ACPI problem, so I'd very
> much like it to be confined to the ACPI GPIO code, to be enabled only
> when ACPI is, and to use function names that start with acpi_gpio. The
> current implementation leverages platform lookup, making said lookup
> less efficient in the process and bringing confusion about its
> purpose. Although the two processes are indeed similar, they are
> separate things: one is a legitimate way to map GPIOs, the other is a
> fixup for broken firmware.
> 
> I suppose we all agree this is a hackish fix, so let's confine it as
> much as we can.

Are we considering MFD cases hackish as well?
i.e. if we have a driver that needs to register children devices and this
driver needs to pass GPIO to a child.

Br, David

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> Hi,
>
>> If we decide to go ahead with the solution proposed by this patch for
>> practical reasons (which are good reasons indeed), I still have one
>> problem with its current form.
>>
>> As the discussion highlighted, this is an ACPI problem, so I'd very
>> much like it to be confined to the ACPI GPIO code, to be enabled only
>> when ACPI is, and to use function names that start with acpi_gpio. The
>> current implementation leverages platform lookup, making said lookup
>> less efficient in the process and bringing confusion about its
>> purpose. Although the two processes are indeed similar, they are
>> separate things: one is a legitimate way to map GPIOs, the other is a
>> fixup for broken firmware.
>>
>> I suppose we all agree this is a hackish fix, so let's confine it as
>> much as we can.
>
> Are we considering MFD cases hackish as well?
> i.e. if we have a driver that needs to register children devices and this
> driver needs to pass GPIO to a child.

In that case wouldn't the GPIO be best defined in the child node
itself, for the child device's driver to directly probe?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

On Wed, Feb 25, 2015 at 10:34:45AM +0900, Alexandre Courbot wrote:
> On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> > Hi,
> >
> >> If we decide to go ahead with the solution proposed by this patch for
> >> practical reasons (which are good reasons indeed), I still have one
> >> problem with its current form.
> >>
> >> As the discussion highlighted, this is an ACPI problem, so I'd very
> >> much like it to be confined to the ACPI GPIO code, to be enabled only
> >> when ACPI is, and to use function names that start with acpi_gpio. The
> >> current implementation leverages platform lookup, making said lookup
> >> less efficient in the process and bringing confusion about its
> >> purpose. Although the two processes are indeed similar, they are
> >> separate things: one is a legitimate way to map GPIOs, the other is a
> >> fixup for broken firmware.
> >>
> >> I suppose we all agree this is a hackish fix, so let's confine it as
> >> much as we can.
> >
> > Are we considering MFD cases hackish as well?
> > i.e. if we have a driver that needs to register children devices and this
> > driver needs to pass GPIO to a child.
> 
> In that case wouldn't the GPIO be best defined in the child node
> itself, for the child device's driver to directly probe?

In my case [1] I need 2 "virtual devices" (and more in future) to be
part of an USB OTG port control. I call it virtual because they are too
simple components connected to no bus and controlled by GPIOs:
- a fixed regulator controlled by GPIO
- a generic mux controlled by GPIO

I'd need to request official ACPI HID for them in order to make them
self-sufficient.

I can go ahead with this approach, but we have many examples of drivers
on upstream that are platform driver expecting to receive gpio via
platform data (e.g. extcon-gpio). The ACPI table of some products on
market were defined following this concept and won't change anymore.

Br, David

[1] https://lkml.org/lkml/2015/2/19/411

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Wed, Feb 25, 2015 at 7:25 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> In my case [1] I need 2 "virtual devices" (and more in future) to be
> part of an USB OTG port control. I call it virtual because they are too
> simple components connected to no bus and controlled by GPIOs:
> - a fixed regulator controlled by GPIO
> - a generic mux controlled by GPIO
>
> I'd need to request official ACPI HID for them in order to make them
> self-sufficient.
>
> I can go ahead with this approach, but we have many examples of drivers
> on upstream that are platform driver expecting to receive gpio via
> platform data (e.g. extcon-gpio). The ACPI table of some products on
> market were defined following this concept and won't change anymore.

So it's not just going to be GPIOs I take it?

There is going to be regulator forwarding, clock forwarding, pin control
forwarding, IRQ forwarding and DMA channel forwarding etc at the end
of the day?

I think it's strange that we see this so much, is the real problem that
ACPI and the kernel have different ideas of what constitutes a device?
And how come the DT seems to be a much better fit and not experience
this? Because we haven't had to deal with deployed device trees with
resources (GPIOs, regulators, etc) bound to some complex MFD device?

Yours,
Linus Walleij