Re: [PATCH] surface: aggregator: propagate -EPROBE_DEFER from ssam_irq_setup()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

+ Konrad, Hans, Andy.

On Fri, 23 Oct 2026, Chris Simpson wrote:

> Hi,
> 
> Same failure here on a Surface Pro 9 (Tiger Lake, INTC1055 driven by
> pinctrl_tigerlake as a module): whenever surface_aggregator is loaded from
> the initramfs about 30 ms after /init, before udev has bound the GPIO
> controller, probe ends with
> 
>   surface_serial_hub serial0-0: error -EINVAL: failed to setup IRQ
> 
> and is never retried, so Type Cover, battery and tablet-mode switch are
> gone until a manual rebind. linux-surface has several reports of this
> since v6.12 (linux-surface/linux-surface#2219, #1645, #2064, #2039), and
> it is a regression from b27622f13172 ("platform/surface: Add OF support"),
> which used to propagate the gpiod_get() error.
> 
> Janus, are you planning a v2? If not, below is a variant that follows
> Ilpo's comment by using gpiod_get_optional(): only "no GPIO described"
> falls back to fwnode_irq_get(), every other error including
> -EPROBE_DEFER is passed on. I'm happy to send it properly (Fixes:
> b27622f13172, Cc: stable, your patch credited), or to test your v2 on
> the Pro 9 instead, whichever you prefer.
> 
> I've tested it on the Pro 9 (Fedora 43, 7.2.12 with the linux-surface
> patches), with surface_aggregator forced into the initramfs. Unpatched,
> the probe fails with the -EINVAL above and the Type Cover, battery and
> tablet-mode switch are missing. Patched, the probe defers three times
> and completes about 0.2 s later, with all of them present and the
> device no longer listed in devices_deferred. Suspend/resume behaves as
> on an unpatched kernel. The analysis and the diff were produced with an
> AI coding assistant (Claude Code), and reviewed and tested by me.
> 
> Chris Simpson
> 
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -2753,12 +2753,20 @@
>  	 */
>  	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>  
> -	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
> -	if (IS_ERR(gpiod)) {
> -		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> -	} else {
> +	/*
> +	 * Only fall back to the fwnode IRQ if no wakeup GPIO is described
> +	 * at all. Any other error, in particular -EPROBE_DEFER while the
> +	 * GPIO controller has not been registered yet, must reach the probe.
> +	 */
> +	gpiod = gpiod_get_optional(dev, "ssam_wakeup-int", GPIOD_ASIS);
> +	if (IS_ERR(gpiod))
> +		return PTR_ERR(gpiod);
> +
> +	if (gpiod) {
>  		irq = gpiod_to_irq(gpiod);
>  		gpiod_put(gpiod);
> +	} else {
> +		irq = fwnode_irq_get(dev_fwnode(dev), 0);

Hi,

I have to admit this was not exactly what I meant (I only meant to not 
use -Exx literal in the return statement), but now that I see this v2, 
I'm not saying your approach is wrong either (I'm left unsure).

Are there some legimate cases where error other than -EDEFER_PROBE should 
end up into this else like it previously did? The return on error was 
removed by b27622f13172 ("platform/surface: Add OF support") so I assume 
there was some good reason for that but the commit message isn't helpful 
at all... :-(

Konrad, you did that change, can you explain the reason why the return was 
removed from the error path?

-- 
 i.