Re: [PATCH v5 06/11] devm-helpers: Add resource managed version of irq_create_mapping()

["Marek Behún" <kabel@kernel.org>]

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

> On 3/23/24 18:43, Marek Behún wrote:
> > Add resource managed version of irq_create_mapping(), to help drivers
> > automatically dispose a linux irq mapping when driver is detached.
> > 
> > The new function devm_irq_create_mapping() is not yet used, but the
> > action function can be used in the FSL CAAM driver.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >   drivers/crypto/caam/jr.c     |  8 ++----
> >   include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 56 insertions(+), 6 deletions(-)
> > 
> > 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
> >    */
> >   
> > +#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;
> >   }
> >   
> > -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 *pdev)
> >   		return -EINVAL;
> >   	}
> >   
> > -	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> > +	error = 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 @@
> >    */
> >   
> >   #include <linux/device.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/irqdomain.h>
> >   #include <linux/workqueue.h>  
> 
> My confidence level is not terribly high today, so I am likely to accept 
> just about any counter arguments :) But ... More I think of this whole 
> header, less convinced I am that this (the header) is a great idea. I 
> wonder who has authored a concept like this... :rolleyes:
> 
> Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in 
> one header has potential to be including a lot of unneeded stuff to the 
> users. I am under impression this can be bad for example for the build 
> times.
> 
> I think that ideally the devm-APIs should live close to their non-devm 
> counterparts, and this header should be just used as a last resort, when 
> all the other options fail :) May I assume all other options have failed 
> for the IRQ stuff?
> 
> Well, I will leave the big picture to the bigger minds. When just 
> looking at the important things like the function names and coding style 
> - 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