Received-SPF: pass (google.com: domain of linux-crypto+bounces-2851-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 25 Mar 2024 10:57:42 +0100
From: Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>, Gregory CLEMENT
 <gregory.clement@bootlin.com>, soc@kernel.org, arm@kernel.org, Hans de
 Goede <hdegoede@redhat.com>, Horia =?UTF-8?B?R2VhbnTEgw==?=
 <horia.geanta@nxp.com>, Pankaj Gupta <pankaj.gupta@nxp.com>, Gaurav Jain
 <gaurav.jain@nxp.com>, linux-crypto@vger.kernel.org, Herbert Xu
 <herbert@gondor.apana.org.au>
Subject: Re: [PATCH v5 06/11] devm-helpers: Add resource managed version of
 irq_create_mapping()
Message-ID: <20240325105742.0e1c8906@dellmb>
In-Reply-To: <dd4c655f-39c4-47c1-b5fb-4d6fc94cc430@gmail.com>
References: <20240323164359.21642-1-kabel@kernel.org>
	<20240323164359.21642-7-kabel@kernel.org>
	<dd4c655f-39c4-47c1-b5fb-4d6fc94cc430@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 25 Mar 2024 11:40:20 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/23/24 18:43, Marek Beh=C3=BAn wrote:
> > Add resource managed version of irq_create_mapping(), to help drivers
> > automatically dispose a linux irq mapping when driver is detached.
> >=20
> > The new function devm_irq_create_mapping() is not yet used, but the
> > action function can be used in the FSL CAAM driver.
> >=20
> > Signed-off-by: Marek Beh=C3=BAn <kabel@kernel.org>
> > ---
> >   drivers/crypto/caam/jr.c     |  8 ++----
> >   include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 56 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> > index 26eba7de3fb0..ad0295b055f8 100644
> > --- a/drivers/crypto/caam/jr.c
> > +++ b/drivers/crypto/caam/jr.c
> > @@ -7,6 +7,7 @@
> >    * Copyright 2019, 2023 NXP
> >    */
> >  =20
> > +#include <linux/devm-helpers.h>
> >   #include <linux/of_irq.h>
> >   #include <linux/of_address.h>
> >   #include <linux/platform_device.h>
> > @@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
> >   	return error;
> >   }
> >  =20
> > -static void caam_jr_irq_dispose_mapping(void *data)
> > -{
> > -	irq_dispose_mapping((unsigned long)data);
> > -}
> > -
> >   /*
> >    * Probe routine for each detected JobR subsystem.
> >    */
> > @@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pd=
ev)
> >   		return -EINVAL;
> >   	}
> >  =20
> > -	error =3D devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> > +	error =3D devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
> >   					 (void *)(unsigned long)jrpriv->irq);
> >   	if (error)
> >   		return error;
> > diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> > index 74891802200d..3805551fd433 100644
> > --- a/include/linux/devm-helpers.h
> > +++ b/include/linux/devm-helpers.h
> > @@ -24,6 +24,8 @@
> >    */
> >  =20
> >   #include <linux/device.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/irqdomain.h>
> >   #include <linux/workqueue.h> =20
>=20
> My confidence level is not terribly high today, so I am likely to accept=
=20
> just about any counter arguments :) But ... More I think of this whole=20
> header, less convinced I am that this (the header) is a great idea. I=20
> wonder who has authored a concept like this... :rolleyes:
>=20
> Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in=
=20
> one header has potential to be including a lot of unneeded stuff to the=20
> users. I am under impression this can be bad for example for the build=20
> times.
>=20
> I think that ideally the devm-APIs should live close to their non-devm=20
> counterparts, and this header should be just used as a last resort, when=
=20
> all the other options fail :) May I assume all other options have failed=
=20
> for the IRQ stuff?
>=20
> Well, I will leave the big picture to the bigger minds. When just=20
> looking at the important things like the function names and coding style=
=20
> - this change looks Ok to me ;)

If the authors of devm-helpers or someone else decide it should not
exist (due for example of long build times), I am OK with that.

But currently this seems to me to be the proper place to put this into.

Marek