Re: [PATCH v7 6/9] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

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

On Wed, 24 Apr 2024 22:47:10 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote:
> > On Wed, 24 Apr 2024 21:33:44 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:  
> > > On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote:  
> 
> ...
> 
> > > > +static void omnia_irq_mapping_drop(void *res)
> > > > +{
> > > > +	irq_dispose_mapping((unsigned int)(unsigned long)res);
> > > > +}  
> > > 
> > > Leftover?  
> > 
> > What do you mean? I dropped the devm-helpers.h changes, now I do
> > devm_add_action_or_reset() manually, with this function as the action.  
> 
> But why?
> 
> ...
> 
> > > > +	irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
> > > > +	irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > > > +	if (irq < 0)
> > > > +		return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  
> 
> > > > +	err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop,
> > > > +				       (void *)(unsigned long)irq);
> > > > +	if (err)
> > > > +		return err;  
> > > 
> > > Are you sure it's correct now?  
> > 
> > Yes, why wouldn't it?  
> 
> For what purpose? I don't see drivers doing that. Are you expecting that
> the same IRQ mapping will be reused for something else? Can you elaborate
> how? (I can imagine one theoretical / weird case how to achieve that,
> but impractical.)

I do a lot of binding/unbinding of that driver. I was under the
impression that all resources should be dropped on driver unbind.

> Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care
> about this, it should be provided by GPIO library.
> 

Something like the following?

From 5aac93d55f6fb750726f7e879672142956981a4c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org>
Date: Thu, 25 Apr 2024 11:33:33 +0200
Subject: [PATCH] gpiolib: devres: Add resource managed version of
 gpiod_to_irq()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add devm_gpiod_to_irq(), a resource managed version of gpiod_to_irq().
The release function calls irq_dispose_mapping().

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/gpio/gpiolib-devres.c | 27 +++++++++++++++++++++++++++
 include/linux/gpio/consumer.h | 10 ++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 4987e62dcb3d..98a40492e596 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -12,6 +12,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/device.h>
 #include <linux/gfp.h>
+#include <linux/irqdomain.h>
 
 #include "gpiolib.h"
 
@@ -427,3 +428,29 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, vo
 	return devm_add_action_or_reset(dev, devm_gpio_chip_release, gc);
 }
 EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key);
+
+static void devm_gpiod_irq_release(void *data)
+{
+	irq_dispose_mapping((unsigned int)(unsigned long)data);
+}
+
+/**
+ * devm_gpiod_to_irq() - Resource managed devm_gpiod_to_irq()
+ * @dev: pointer to the device that gpio_chip belongs to.
+ * @desc: gpio whose IRQ will be returned
+ *
+ * Return the IRQ corresponding to the passed GPIO, or an error code in case of
+ * error.
+ */
+int devm_gpiod_to_irq(struct device *dev, const struct gpio_desc *desc)
+{
+	int virq;
+
+	virq = gpiod_to_irq(desc);
+	if (virq < 0)
+		return virq;
+
+	return devm_add_action_or_reset(dev, devm_gpiod_irq_release,
+					(void *)(unsigned long)virq);
+}
+EXPORT_SYMBOL_GPL(devm_gpiod_to_irq);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edb..e8f4829538f6 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -165,6 +165,8 @@ int gpiod_is_active_low(const struct gpio_desc *desc);
 int gpiod_cansleep(const struct gpio_desc *desc);
 
 int gpiod_to_irq(const struct gpio_desc *desc);
+int devm_gpiod_to_irq(struct device *dev, const struct gpio_desc *desc);
+
 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name);
 
 /* Convert between the old gpio_ and new gpiod_ interfaces */
@@ -519,6 +521,14 @@ static inline int gpiod_to_irq(const struct gpio_desc *desc)
 	return -EINVAL;
 }
 
+static inline int devm_gpiod_to_irq(struct device *dev,
+				    const struct gpio_desc *desc)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(desc);
+	return -EINVAL;
+}
+
 static inline int gpiod_set_consumer_name(struct gpio_desc *desc,
 					  const char *name)
 {
-- 
2.43.2