Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Dec 2022 11:58:13 +0000
From:   Lee Jones <lee@kernel.org>
To:     Henning Schild <henning.schild@siemens.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the
 GPIO providing driver
Message-ID: <Y6WX1Y9GZmvxqlCc@google.com>
References: <20221007153323.1326-1-henning.schild@siemens.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221007153323.1326-1-henning.schild@siemens.com>
Precedence: bulk

On Fri, 07 Oct 2022, Henning Schild wrote:

> If we register a "leds-gpio" platform device for GPIO pins that do not
> exist we get a -EPROBE_DEFER and the probe will be tried again later.
> If there is no driver to provide that pin we will poll forever and also
> create a lot of log messages.
> 
> So check if that GPIO driver is configured, if so it will come up
> eventually. If not, we exit our probe function early and do not even
> bother registering the "leds-gpio". This method was chosen over "Kconfig
> depends" since this way we can add support for more devices and GPIO
> backends more easily without "depends":ing on all GPIO backends.
> 
> Fixes: a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO version of Siemens driver")
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---

What happened in versions 1 through 3?  Please provide a change-log.

>  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> index b9eeb8702df0..fb8d427837db 100644
> --- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> @@ -77,6 +77,8 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_127E:
> +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> +			return -ENODEV;

I see that there is an unfortunate precedent for this in the lines
below.  However, I also see that the commit which added it was not
reviewed by Pavel.

This is an interesting problem, due to the different devices we're
attempting to support in this single driver using different
GPIO/PINCTRL drivers, which is unusual.  We usually resolve these kinds of
issues as a Kconfig 'depends' line which covers the whole driver.

Would 'depends GPIO_F7188X || PINCTRL_BROXTON' be a suitable
replacement, I wonder?  If it's possible for SIMATIC_IPC_DEVICE_127E to
be probing when only GPIO_F7188X is enabled?  If so, this would result
in the same scenario.

It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely in
some valid circumstances dependencies are never satisfied?

>  		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e;
>  		break;
>  	case SIMATIC_IPC_DEVICE_227G:
> -- 
> 2.35.1
> 

-- 
Lee Jones [李琼斯]