Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Salah Triki <salah.triki@gmail.com>]

On Tue, Apr 28, 2026 at 06:33:05PM +0200, Crt Mori wrote:
> On Tue, 28 Apr 2026 at 18:04, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 28 Apr 2026 09:38:47 +0100
> > Salah Triki <salah.triki@gmail.com> wrote:
> >
> > > On Tue, Apr 28, 2026 at 11:00:04AM +0300, Andy Shevchenko wrote:
> > > > On Mon, Apr 27, 2026 at 10:58:00PM +0100, Salah Triki wrote:
> > > > > The functions gpiod_direction_output() and gpiod_direction_input() can
> > > > > fail, but their return values were previously ignored.
> > > > >
> > > > > If an error occurs during the GPIO configuration, the function should
> > > > > abort the wake-up sequence and return the error code. More importantly,
> > > > > failing to check these values could lead to the I2C bus remaining
> > > > > locked if an error occurs after i2c_lock_bus() is called.
> > > > >
> > > > > Add return value checks and ensure the I2C bus is properly unlocked
> > > > > via a goto label in case of failure.
> > > >
> > > > ...
> > > >
> > > > > - gpiod_direction_output(data->wakeup_gpio, 0);
> > > > > +
> > > > > + ret = gpiod_direction_output(data->wakeup_gpio, 0);
> > > > > + if (ret)
> > > > > +         goto out_unlock;
> > > > > +
> > > > >   msleep(chip_info->wakeup_delay_ms);
> > > > > - gpiod_direction_input(data->wakeup_gpio);
> > > > > +
> > > > > + ret = gpiod_direction_input(data->wakeup_gpio);
> > > > > + if (ret)
> > > > > +         goto out_unlock;
> > > >
> > > > While technically it sounds correct, the potential problem here is that you may
> > > > fail this in case CONFIG_GPIOLIB=n. Is this GPIO optional?
> > > > What may happen if GPIO is not optional, but for some reason setting it fails?
> > > >
> > > > TL;DR:
> > > > I am not sure about this patch. At least I'm not comfortable to take it without
> > > > testing on real HW.
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
> > >
> > > Thank you for your feedback. Since I don't have the physical hardware to
> > > perform the necessary tests and ensure there are no regressions, I agree
> > > that it is safer to drop this patch. I don't want to risk breaking the
> > > driver for a minor cleanup.
> > >
> > Maybe Crt has the hardware to test. For now I'll assume this is not
> > going anywhere wrt to tracking in patchwork.
> >
> I will dig the hardware to test as it was off my desk for some time
> now. It seems like backwards compatible, but I am wondering why would
> we want to be informed of this failing? The problem is that sensor
> uses wakeup line (same as for i2c) to receive wakeup pulse, so in
> theory this is already a workaround. @Salah did you see any issues in
> the wild?
> 

Thanks for digging out the hardware to test this!

To answer your question: I haven't observed this failing in the wild on
this specific sensor. The patch was initially motivated by a static
analysis and general robustness cleanup, ensuring that we don't ignore
errors from gpiod_* functions (especially in the context of keeping the
I2C bus locked).

However, as Andy pointed out, turning this into a fatal error might not
be desirable if the GPIO is optional or if the failure is transient on some
platforms. The goal was simply to improve visibility if the GPIO
configuration fails.

Best Regards,
--
Salah Triki