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:   Tue, 3 Jan 2023 21:20:59 +0100
From:   Henning Schild <henning.schild@siemens.com>
To:     Lee Jones <lee@kernel.org>
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: <20230103212059.5c80fecb@md1za8fc.ad001.siemens.net>
In-Reply-To: <20230102162227.523d2a73@md1za8fc.ad001.siemens.net>
References: <20221007153323.1326-1-henning.schild@siemens.com>
        <Y6WX1Y9GZmvxqlCc@google.com>
        <20230102162227.523d2a73@md1za8fc.ad001.siemens.net>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?hYS2sXKP6iv+xMoeHGd/hXzjKVrx4Z2S+JmeMrujPXn9mK8tn9AlqvAWtpAF?=
 =?us-ascii?Q?/5RIK9E483qBGuN/X+T85n7tn6W9uswnC90M0hu7JMFMHOeVCQCbASbYhEpb?=
 =?us-ascii?Q?ubmUBzzyyWJJ8HJ1qLRYeDWJbNUsr4RpxtvT192xzf9M6O/rQA/ahvUbrkzb?=
 =?us-ascii?Q?gNJ27fTF3cu40yqppxVFCPnDLi4jBhOUP6JXWeLL05UHP/WWr437305nwUU9?=
 =?us-ascii?Q?zNVCF5e3xbrTlspRlu1r73/XhogAp6/nOF7dgKAFETCu45pvbyDGQz/KLb6d?=
 =?us-ascii?Q?NOMfzwQgqZBSheX8rnY9ZKLYhhCJ+qcLrjz6ElV/RoMg6zYK5vlXgQ9Eufav?=
 =?us-ascii?Q?lA835aN+utj+1VDVFlThgWue3khaYQNVsSF7ztMi5O5h/edaRVh0469KrANv?=
 =?us-ascii?Q?jdPyfg91Ta7b09DG9gxzqajDGTiQ+RMO/ZUw/ng0fr08LRFPRVVbv1AbCNel?=
 =?us-ascii?Q?daBqd3WWAaVI3Pc+6ITewMtjtLQIyFkBm9tUAUTyd3YdaTRrz8EVQB2yBjyJ?=
 =?us-ascii?Q?NKFIG0Yiwm4myfuhLSDJr6YUnlHj3vzkIIxFunBR7ujCIKbC/sv1T0d1Y54E?=
 =?us-ascii?Q?TbMhjvcZ06dnqnLe0+9FIeFuv72dA3J0+9rBlo+WUcP2PwMkdV/PIWXqIXE8?=
 =?us-ascii?Q?RFxJJ+k7O1sQt973BtTTKZw3/yroLfoB79jrvEfvUitF1+db3Qksi64FZqhX?=
 =?us-ascii?Q?QOgq14MGv5vA6lDkPfKZZ1Pk+MSvMfH3p9yY2aSb0BitpgAKTGubLpBVNQ3o?=
 =?us-ascii?Q?qiPDQoJ8klaqW5uyrbOsFco3qzbM1lvBiYDTzE7NxvKB1TB83tAXMNjCE1OS?=
 =?us-ascii?Q?QlaWmQU/ES1wEZlDp5pHTiVu0NImY9kjlvrN2t7mxzIyQDXDCMIeaUwwBrYE?=
 =?us-ascii?Q?wVzm+t+gDR3r7T3Lzlt4FGNeLLrfCLZYuhiQM3JpnU3jr3QQlAckJfjljaR0?=
 =?us-ascii?Q?570E1RmeRNeTD0d4bTIxBw8p0IVqx+euApu14i1AZa8dnwaF2G0zOenV1l2L?=
 =?us-ascii?Q?7hGgZpPgwGJoykGbC2hApUirzmm0x0rKuh3b3myZURKmrnluNNNDqX6M62Ri?=
 =?us-ascii?Q?C2TSjHR8Mlr3xYjSP5cYmx2N8+CQfMODAHL69xRGUo7a7OgHQ/LWL29DNL91?=
 =?us-ascii?Q?ucnp42Wo3m+Cl3bYRC2cuagOBuLiYP9my+Wyd541YCw9/j6i1B48RqR+4JMZ?=
 =?us-ascii?Q?z3fuA4k5xYMiYF0uOwdpSLJtZwCBTNuNHMp1+PPJVrAp20Au+orLfkFMfJXx?=
 =?us-ascii?Q?gQcfOArJdnseXnQuCUSVsBNZfBAOWh9q3SzcGn9lLkMeS8RkOYTqNovTUXOF?=
 =?us-ascii?Q?rT4kxlFKqsGrwsP/UjAtYN4+hnw95fkjD8G498EHAw7tg6x8VVleyakFLLQn?=
 =?us-ascii?Q?fscGLYghcpCE4PNzUxE8Kd8eybgolYthjn5KiIBXIcMvbkIAu/tr04klokp/?=
 =?us-ascii?Q?sfTDSG0n/63mwEyP/FywOhvfS6lXjx29LjyxWkjV2P/vS0Y6GO+XcJoH+wQV?=
 =?us-ascii?Q?1+D5FOQp8IMF+BL/WvARq7RH8PXJNVjtGhrgeIAwfC7D0vLhokVJegYKoshH?=
 =?us-ascii?Q?8EnCVneh72V/e711FHMPXMLA1yBjjiHWkEhgo25rQBQhn6YWHjLGK7aeVz0Y?=
 =?us-ascii?Q?m3AgYfIFv99XWkrXL2LzgCnXVnW1dAhv3DqZG68g6anIW8HuRZUoMrPsxCR9?=
 =?us-ascii?Q?QN8ZPw=3D=3D?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b6037ce9-6782-4d32-24b5-08daedc809da
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5780.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jan 2023 20:21:04.2442
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 38ae3bcd-9579-4fd4-adda-b42e1495d55a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SUUGAV5PSYVFVmN0XGn7DkrnepWMeMxgSnwiS5KVDQBTY6vGs1K707X3HaKnRwnk/eFN+Sgrszisx5qgK/aOkJH0b0cxXHXxli07hKv9wDc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR10MB3350
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am Mon, 2 Jan 2023 16:22:27 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Fri, 23 Dec 2022 11:58:13 +0000
> schrieb Lee Jones <lee@kernel.org>:
> 
> > 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.  
> 
> Not too much really, but i will write a changelog and cover letter
> when sending again. Mostly commit message stuff and later a rebase.

Lee please let me know if you insist on that changelog, in which case i
would send that same patch again with a cover-letter that will carry a
not too spectacular changelog.

Or get back on the rest of what i wrote earlier, maybe we need another
version of the patch and not just the same one again with only a
changelog added.

Henning

> > >  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.  
> 
> Right i think that might have been you in the end.
> 
> > 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.  
> 
> This was tried but the result was not too nice. It is really the same
> gpio led driver implemented on top of multiple possible gpio chip
> drivers. Making it depend on both pulls in too much in case one wants
> a minimal config, writing a new driver for each backend would
> duplicate too much code.
> 
> But maybe a splitting out a -common or moving stuff into headers could
> help with the duplication if we want to go the "one driver for one
> device" road. I would not want that and what we currently see was
> discussed and approved as part of another series, when i introduced
> x27G.
>  
> > 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.  
> 
> No that would not work. Depending on which board we are on we depend
> on another pin provider. "&&" would be but it would be kind of
> overkill and not allow for a minimal kernel config in case someone
> wanted a special minimal kernel for either one.
> 
> > It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely
> > in some valid circumstances dependencies are never satisfied?  
> 
> Well that is what i would guess as well. But that infinite loop
> waiting for a pin to appear endlessly is a part of "leds-gpio". If
> "leds-gpio" had some magic to eventually bail out (maybe say we give
> it X runs with some sleep back-off) i would not have to do anything
> here. I consider that patch a workaround for a shortcoming in
> "leds-gpio", which busy loops and fills up your disk quickly with logs
> if you mention a pin that never comes. Which i imagine can quickly
> happen if you have a typo on your device tree or a kernel config not
> enabling a pin provider driver.
> 
> I am not sure there are no other valid reasons. And i think that indef
> loop needs fixing at some point. Hopefully by a LEDs maintainer or
> maybe i will even help out.
> 
> Until that day i would like to have the proposed patch merged to not
> have users run into a known issue. The pattern is established and has
> been discussed before and the patch it rather trivial.
> 
> Later we can see about improving and ask fundamental questions again.
> 
> Henning
> 
> > >  		simatic_ipc_led_gpio_table =
> > > &simatic_ipc_led_gpio_table_127e; break;
> > >  	case SIMATIC_IPC_DEVICE_227G:
> > > -- 
> > > 2.35.1
> > >     
> >   
>