Re: [PATCH 13/15] iio: proximity: sx9500: hand on probe deferral of GPIOs

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 27 Oct 2026 17:19:17 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> From: Josef Bacik <josef@toxicpanda.com>
> 
> sx9500_gpio_probe() treats every failure of devm_gpiod_get() as "the
> line does not exist".
>
>  If the GPIO controller has not been probed yet,
> the lookup fails with -EPROBE_DEFER, and the driver binds for good
> without the line:
> 
>  - without the "interrupt" GPIO client->irq stays 0, so no interrupt is
>    requested and no trigger is registered: the device has no events and
>    no data-ready trigger until it is unbound and bound again;
>  - without the "reset" GPIO the chip is not reset in
>    sx9500_init_device().
> 
> Which of the two happens depends only on the order in which the drivers
> are probed.  Return -EPROBE_DEFER so that the probe is repeated once the
> controller is there.  All other errors are handled as before.  The i2c
> core sets client->irq again at the start of every probe, so a deferral
> on the second line leaves nothing behind.
> 
> A system that describes one of the lines but never gets a driver for its
> GPIO controller now keeps the sensor in the deferred list instead of
> binding it without the line.
That's all a bit long winded though accurate.

The original code is rather odd.  If the intent is to actually have
these as optional gpios, why not use devm_gpiod_get_optional()
which documents that intent?

Anyhow, I suggest a bit more radical surgery given the driver
is fine without the irq and the code being fixes smells like it is
papering over ancient firmware. It definitely shouldn't run on
a modern system.

> 
> Fixes: 45fd5f8e10d3 ("iio: sx9500: add GPIO reset pin")
> Fixes: e53111ad5dea ("iio: proximity: sx9500: Assign interrupt from GpioIo()")
> Cc: stable <stable@kernel.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: "Nuno Sá" <nuno.sa@analog.com>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
> Cc: Siratul Islam <email@sirat.me>
> Cc: Vlad Dogaru <ddvlad@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/iio/proximity/sx9500.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index dadce9b44227..94cc577f4404 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -853,15 +853,15 @@ static const struct acpi_gpio_mapping acpi_sx9500_gpios[] = {
>  	{ }
>  };
>  
> -static void sx9500_gpio_probe(struct i2c_client *client,
> -			      struct sx9500_data *data)
> +static int sx9500_gpio_probe(struct i2c_client *client,
> +			     struct sx9500_data *data)
>  {
>  	struct gpio_desc *gpiod_int;
>  	struct device *dev;
>  	int ret;
>  
>  	if (!client)
> -		return;
> +		return 0;
>  
>  	dev = &client->dev;
>  
> @@ -871,6 +871,8 @@ static void sx9500_gpio_probe(struct i2c_client *client,
>  
>  	if (client->irq <= 0) {
>  		gpiod_int = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
> +		if (PTR_ERR(gpiod_int) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
>  		if (IS_ERR(gpiod_int))
>  			dev_err(dev, "gpio get irq failed\n");

This is 'novel'. I did some archaeology.  Goes back almost 11 years
and at the time the driver only had ACPI and i2c_device_id. So
assuming we weren't talking board files I suppose this is papering over
a broken ACPI firmware.

Given the driver works fine without the interrupt (despite that print
implying it is an error).
Shall we just rip this out?  The irq from any modern firmware will
be in client->irq anyway so this section doesn't run.

>  		else
> @@ -878,10 +880,14 @@ static void sx9500_gpio_probe(struct i2c_client *client,
>  	}

The next bit is valid, though with the above gone, I'd be tempted
to flatten this helper and just get the gpio inline.

>  
>  	data->gpiod_rst = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

	data->gpiod_rst = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
	if (PTR_ERR(data->gpiod_rst))
		return dev_err_probe(dev, "gpio get reset failed\n");
	if (!data->gpiod_rst)
		dev_warn(dev, "gpio reset not available\n:);

	return 0;

> +	if (PTR_ERR(data->gpiod_rst) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
>  	if (IS_ERR(data->gpiod_rst)) {
>  		dev_warn(dev, "gpio get reset pin failed\n");
>  		data->gpiod_rst = NULL;
>  	}
> +
> +	return 0;
>  }
>  
>  static int sx9500_probe(struct i2c_client *client)
> @@ -911,7 +917,9 @@ static int sx9500_probe(struct i2c_client *client)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	i2c_set_clientdata(client, indio_dev);
>  
> -	sx9500_gpio_probe(client, data);
> +	ret = sx9500_gpio_probe(client, data);
> +	if (ret)
> +		return ret;
>  
>  	ret = sx9500_init_device(indio_dev);
>  	if (ret < 0)
>