Date:   Tue, 24 Jan 2023 14:52:48 +0100
From:   Henning Schild <henning.schild@siemens.com>
To:     Lee Jones <lee@kernel.org>
Cc:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the
 GPIO providing driver
Message-ID: <20230124145248.3fffa6f0@md1za8fc.ad001.siemens.net>
In-Reply-To: <Y8+zD6AxuJVy5b7Y@google.com>
References: <20221007153323.1326-1-henning.schild@siemens.com>
        <Y8mv8PzL1UsP9gNh@google.com>
        <20230123214859.725cd1c3@md1za8fc.ad001.siemens.net>
        <CAHp75VfSHgdikX5=Qba62BwWofVf7gHhS2hq2OuBwHFz9riCWQ@mail.gmail.com>
        <Y8+zD6AxuJVy5b7Y@google.com>
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?pfcfGeg/GRtoRcDe/8Ga2VfNdPsRNDeO0BiZoG0H3qx6b6kP1bee6/eYf/5a?=
 =?us-ascii?Q?xmZf3BtsTjWN6hlWmCDOmfxNE6DS0IKKoFUsw6+lmviExz0Zl4bGxbhh9gQ6?=
 =?us-ascii?Q?XGyBHqzgsn8H0dyQfEIL7pphx0VQ/Xa4ik7j5a13awiYcRLzsJbFQAwfnFr+?=
 =?us-ascii?Q?cd4K5wEBU446k0ADqP9SG1+CAoFJ5xMZQG8GtmWz8NgCDeuQdGz5H/t7F1yB?=
 =?us-ascii?Q?ht0LVl1SOHlfHZX+1VU6ZS9JfGZiJc10omPDjw/wXcJvgVv4eh9tpHCSVl9Y?=
 =?us-ascii?Q?Mi4IZwsJrIkj508iC1QDgNNytzuZLbGCuc6VXMbOPNXxKwW/znv9pC2dAUNL?=
 =?us-ascii?Q?3ddMqHNz1tBvIWFF839jEBmq7+FXXvL6WSW7Opixc8ZUbWbCRPhiQduC5xTV?=
 =?us-ascii?Q?NT7xeiKYKQ609AY+n+IPaE66huznuy1bWzd768IWMWKPV90oe670ZgPK3SA7?=
 =?us-ascii?Q?xLS5Fxns8xgs1mWdIeyH0O0dSE6rKzmHx0An39okTENewy5iBF7fKoDoG/rN?=
 =?us-ascii?Q?KVSH/6HI+yF2dkfJ3v9vLra0in0C6ORrt0we0ooXYRhLr8kCOWjnmVJixseT?=
 =?us-ascii?Q?O+ny2uzfCT/lUMbKq2Q1xQt35DNG4uNLCNzqjCVbwbEKredAuGuw6kbRZ1pJ?=
 =?us-ascii?Q?7cKcvXWhAkDX+9vV7B5BENbnFKcU5Qe55oVf6tH3KQ3QV4Pb4k30+RBdfpot?=
 =?us-ascii?Q?3RQ85V3CpO/zrsw0KVQO+Etv1Fcd4FMN4GnkkLmf6npdasoEvfrzQOBK6JGc?=
 =?us-ascii?Q?G06CbtNr5a8hzsaMJggxHUWvSSHYejUPP22Qy3K/IdbinP0U9EIAlYfquURc?=
 =?us-ascii?Q?LuJN+ou6U2H9ybn34KmV8J9cpVF7l/1dqajeYpW6z9ZOx7739xWMbRFoL6Vv?=
 =?us-ascii?Q?lhvYb2CmF8UC1FYW4+JWStWFQhom9DIo30wWLHMl/nYLGGkdMDqtaksNFvdU?=
 =?us-ascii?Q?IRNvzHlleyd2CM4nr9PR1JPcLt5/7/8lE9WTveZFUQwvw4cO3aLaITgCKz2v?=
 =?us-ascii?Q?W7JnmNkYwLf3ZdV8uazuUa75bXpWP0OqWU66udTCKxUmme6PnSwf4es7ZEkD?=
 =?us-ascii?Q?y2GiAqe6NGdeJz3puNx6i39NQnDL/L2sddj+VMTawgpu62oS3DzVlm1SPFN3?=
 =?us-ascii?Q?RpQ7z07+8U2CC0uCqq6gIk2dRDDKs7HFivbPqBiYW6eOaR4709Q0e0R0z0FL?=
 =?us-ascii?Q?chujq1o9/bQZoky6eb/NJ945I8k9hl7OFFlZp/6T72eiuUix8M2NX8oSDDi8?=
 =?us-ascii?Q?nupIPnmFs8rfxeZx6r2Pq9+jpqbkadmdBcVRsuWfQX8QGkqGHVdMHd4zGgwF?=
 =?us-ascii?Q?Z5uc8yKRsKYZhjm8Y40V14BxyR3/wMGRsq/WdLrpJCLKgw8YhFIf5G9/3kV8?=
 =?us-ascii?Q?IbdjqMja7rYuSSvLQWeopYVbn22htX+l+jUkK/BQpf8a6P4fuIDRK2NMqM70?=
 =?us-ascii?Q?lpwUdMZo/GZ7knbQ0b5jRfzGvv8qziLI6b7B3Zim+smqVteZjs7doVUBbWZJ?=
 =?us-ascii?Q?QJWzPsm+CB54Ss5s59N9DzqG5TtwiIOCFW6dzXl1rRvtsZPYCubXYZpKymHL?=
 =?us-ascii?Q?ektGNXhhFzCI45QdOGAIzisXTLhH1dP5eTeesfSe1vL/XfWHgOQRNIO9cwke?=
 =?us-ascii?Q?5wDVclECtRnUsCafE80aQloTabSb3AfVEwIhOBRGDJhyucXyVbQisS2WowYW?=
 =?us-ascii?Q?Ei/y/A=3D=3D?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 904e55de-9bab-4e09-cdc3-08dafe124b4f
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5780.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2023 13:52:55.6483
 (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: DkP518/k0Rfd17B1/INwkLWZY56a3NkyjmyvVKEkPRiURlgnWfvUoFMkIO+8MkHHoLtyKsQyVi0jhjs8Xk1uVWPdJ/S0mY9vu/NMPn8alo0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR10MB8028
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am Tue, 24 Jan 2023 10:29:35 +0000
schrieb Lee Jones <lee@kernel.org>:

> On Tue, 24 Jan 2023, Andy Shevchenko wrote:
> 
> > On Mon, Jan 23, 2023 at 10:49 PM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> > > Am Thu, 19 Jan 2023 21:02:40 +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> ---
> > > > >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)  
> > > >
> > > > FYI: I'm going to try my best not to take another one like
> > > > this.  
> > >
> > > understood!
> > >  
> > > > Please try to improve the whole situation for you next
> > > > submission.  
> > >
> > > When i have to touch this again, which i will, i will propose
> > > either "depend on all possible GPIO drivers" or introduce "#ifdef
> > > CONFIG"s. Caring most about big configs as seen in distros like
> > > debian, even for embedded systems ... i think i would prefer the
> > > first option, as it will also be easier to maintain.
> > >
> > > I do not see the whole infinite loop story on my plate, but if
> > > that got fixed i would follow up taking the fix into account.  
> 
> I still don't really know what you mean by this.  Probe deferring
> should not work this way.  Do you know why the loop is infinite on
> your platform?  What keeps triggering the re-probe?  Are you
> continually binding and unbinding drivers, forever?  Also, what is
> printing out the failure?  Maybe it should be silent?

It has been a while and i would have to reproduce this. But basically
what happened is that i registered a leds-gpio platform device with a
lookup table, no errors returned and my "driver" would be done and
leds-gpio takes over.

All GPIO_LOOKUP_IDXs point to not yet exisiting pins, potentially never
existing when the providing driver never comes up. Now leds-gpio
internally tries again and again with a high frequency (busy?) and
printing stuff (would have to try again to see what).

I think one could modifiy any other leds-gpio and totally invalidate
the lookup table by introducing typos in the chip name of each entry.

But i will rty again and get back with a better description. Maybe the
bug was fixed in the meantime or i am doing something wrong when
registering that platform-device.

Henning

> > AFAICS another possible (not sure if it's preferable) solution is to
> > split this driver to subdrivers and each of them will be dependent
> > on the corresponding pin control in Kconfig. It will satisfy both
> > of your requirements, right? Something like
> > 
> > simatic-leds-core.c
> > simatic-leds-127e.c (config ..._127E depends on PINCTRL_BROXTON)  
> 
> In theory, yes it would.  You could also introduce a core driver to
> contain all of the shared code.  Duplication would also be a travesty.
>