On Mon, 24 May 2021 10:27:36 +0100
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 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?
I'm going to gamble a bit here and just drop the gpio support entirely.
We don't have any known boards out there running this driver so I 'might'
break someones hobby board, but hopefully they'll fix up their DT.
Without a confirmed user I'm not keen to maintain the complexity.
Jonathan
>
> Jonathan
>
>
> >
> > > + }
> >
> >
> >
>
