Re: [PATCH 2/2] HID: i2c-hid: Add support for GPIO interrupts

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Tue, Jan 27, 2015 at 03:57:47PM +0000, Mark Rutland wrote:
> On Tue, Jan 27, 2015 at 03:21:27PM +0000, Mika Westerberg wrote:
> > On Tue, Jan 27, 2015 at 03:06:06PM +0000, Mark Rutland wrote:
> > > On Tue, Jan 27, 2015 at 02:41:27PM +0000, Mika Westerberg wrote:
> > > > On Tue, Jan 27, 2015 at 02:33:34PM +0000, Mark Rutland wrote:
> > > > > Ok, that allays my fear w.r.t. ordering of the resources.
> > > > > 
> > > > > As I see it, the fact that we convert GpioInt entries to GPIOs rather
> > > > > than irqs when parsing _CRS is the issue here, and to me it makes no
> > > > > sense that we do so. Were we to treat them as interrupts, the binding is
> > > > > fine as-is, and we'd do the same thing in DT and ACPI.
> > > > > 
> > > > > The reason GpioInt is separate from GpioIo is that a GpioInt _is_ an
> > > > > interrupt (which happens to be backed by a GPIO), and is not something
> > > > > that necessarily makes sense as a GPIO.
> > > > 
> > > > I would rather say that GpioInt *is* a GPIO. That can then used as an
> > > > interrupt but it should not prevent you from using it as GPIO instead.
> > > > For example if you just want to poll that something is 0 or 1. That
> > > > should be possible as well and nothing say that you cannot do that for
> > > > GpioInt().
> > > 
> > > >From my POV a GpioInt is logically an interrupt, or it would be a
> > > GpioIo. That doesn't necessarily mean it's invalid to try to query its
> > > state as a GPIO, but I do not think that it makes sense to handle it by
> > > default as a GPIO given that it was handed to us as a GpioInt so that it
> > > can be used as an interrupt.
> > > 
> > > If it's just the case that ACPI and DT differ w.r.t. how this case (an
> > > interrupt line wired to a GPIO) is described, that in itself is fine;
> > > different standards have different models.
> > > 
> > > However, I do not think we must change the DT binding and violate
> > > established DT practice simply becuase on the ACPI side things are
> > > different (nor would it make sense to do things the other way around).
> > > If the two have different rules, then we should handle those rules
> > > separately rather than trying to force the two together when they
> > > clearly don't fit.
> > 
> > I agree.
> > 
> > > In the driver that can easily be achieved with separate probe paths.
> > 
> > Yes, that's what this patch is doing. It has different paths for
> > interrupt and GPIO cases. If we find that there is an interrupt number
> > already, then we use that directly.
> > 
> > If not we try to look for a GPIO which we could use as an interrupt.
> 
> My comment about separate probe paths was w.r.t. ACPI and DT. In DT,
> acquiring a GPIO here shouldn't be necessary (and ideally shouldn't be
> allowed) because the GPIO should be described as an interrupt (we need
> this to have the correct flags).
> 
> > The reason why this is not done only in the ACPI probe path is that
> > there is nothing ACPI specific in gpiod_get() and friends.
> 
> While the high-level APIs are not specific to ACPI or DT, the behaviour
> we want for DT is subtly different top the behaviour you want for ACPI.
> 
> In DT we don't necessarily have the relevant flags (e.g. edge vs level,
> active high vs active low) unless the GPIO is described as an interrupt,
> so permitting it to be described as a GPIO permits situations which we
> cannot handle correctly. In the DT case therefore we must not acquire
> the interrupt GPIO as a GPIO; it must be described as an interrupt.
> 
> So if we need to acquire a GPIO in the ACPI case, this should be limited
> to the ACPI case.

How about cases where platform data (no ACPI or DT) is used? You can
also provide lookup table to the GPIOs from platform data.

> Looking further at ACPI, the flags issue also seems to be a problem
> there. If the interrupt GPIO were described as a GpioIo, then you could
> acquire it as a GPIO with gpiod_get(), but wouldn't have the EdgeLevel
> and ActiveLevel properties, and therefore cannot necessarily correctly
> convert the GPIO to an irq, no?

That's right but typically what I've seen the drivers configure the GPIO
as they think is necessary.

Anyway, I think I'll give up now and leave the i2c-hid.c driver to
support only interrupts for now.