On Mon, 24 May 2021 09:13:30 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sun, May 23, 2021 at 7:24 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > The driver previous supported using GPIO requests to retrieve
>
> previously
>
> > multiple interrupt lines. As existing firmware may be using
> > this method, we need to continue to support it. However, that doesn't
> > stop us also supporting just getting irqs directly.
> >
> > The handling of irqflags has to take into account the fact that using
> > a GPIO method to identify the interrupt does not convey direction of
> > the trigger that fwnode_irq_get() will. So we need to set the
> > IRQF_TRIGGER_RISING in that path but not otherwise, where it will
> > cause an issue if we reprobe the driver after removal.
>
> ...
>
> > + /* fwnode_irq_get() returns 0 for not present on OF, and -EINVAL for ACPI */
> > + if (ret == 0 || ret == -EINVAL) {
> > + gpio = devm_gpiod_get_index(dev, NULL, i, GPIOD_IN);
> > + if (IS_ERR(gpio)) {
>
> > + dev_err(dev, "gpio get index failed\n");
> > + return PTR_ERR(gpio);
>
> This should be dev_err_probe().
> (I guess you need to prepend this patch with one that switches to
> dev_err_probe() API)
>
> > + }
> > +
> > + ret = gpiod_to_irq(gpio);
> > + if (ret < 0)
> > + return ret;
>
> > + /* GPIO interrupt does npt have a specified direction */
Gah. What is it with me and spelling in comments...
> > + irqflags |= IRQF_TRIGGER_RISING;
>
> I'm not sure I understand this part. If we are talking about the ACPI
> GpioInt() resource, then it should have this flag. If GpioIo() is in
> use (which is already a sign of either using the line in dual
> direction mode, but this needs to be described in the data sheet and
> thus used in the driver, or misdesigned ACPI tables). DT, I suppose,
> should have all necessary information.
Honestly I have no idea. I didn't want to change the exiting flags without
any visibility of what the ACPI tables look like (assuming they exist).
Given I'm proposing killing of the ID, chances are ACPI is broken anyway
now :) So, more risky is DT out there that just specifies this as a
GPIO.
Plan B would be to just drop the GPIO support entirely.
Would GpioInt() get picked up by the the fwnode_irq_get() path?
I'm guessing these were on a dev board 6+ years ago, but whilst I can
find references to the mma9553 on some freescale platforms, not finding
much on the mma9551.
Looking a bit deeper they are both listed as obsolete parts now (according to
digikey as I can't find status on nxp.com)
... So plan C is just remove the drivers on the basis they are significantly
odd and we don't know of a platform anyone cares about with them on.
Mind you, aside from having a lack of documented bindings (which was what was
annoying me, they aren't doing any harm or causing any real maintenance burden.)
More than possible someone out there is using them. The mm9953 appears on the
warpboard.org reference platform, but seems the sensor was never enabled upstream.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/arch/arm/boot/dts/imx6sl-warp.dts
https://revotics.com/warp?v=a284e24d5f46
Also, only some passing references in there, so I'd guess it got dropped in
later revisions? Shaun, any ideas?
Jonathan
>
> > + }
>
>
>
