Re: [PATCH v3 3/5] driver core: platform: Add platform_put_irq()

[Marc Zyngier <maz@kernel.org>]

On 2020-11-25 17:20, John Garry wrote:
> Add a function to tear down the work which was done in 
> platform_get_irq()
> for when the device driver is done with the irq.
> 
> For ACPI companion devices the irq resource is set as disabled, as this
> resource is configured from platform_get_irq()->acpi_irq_get() and 
> requires
> resetting.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/base/platform.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 88aef93eb4dd..3eeda3746701 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -289,6 +289,20 @@ int platform_irq_count(struct platform_device 
> *dev)
>  }
>  EXPORT_SYMBOL_GPL(platform_irq_count);
> 
> +void platform_put_irq(struct platform_device *dev, unsigned int num)
> +{
> +	unsigned int virq = platform_get_irq(dev, num);

I find it pretty odd to have to recompute the interrupt number,
which in turn results in a domain lookup. It things were refcounted
(they aren't yet), irq_dispose_mapping() would have no effect.

<pedant>
It also goes against the usual construct where if you obtain an object
based on some parameters, the release happens by specifying the object
itself, and not the parameters that lead to the object.
</pedant>

> +
> +	irq_dispose_mapping(virq);
> +	if (has_acpi_companion(&dev->dev)) {
> +		struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ,
> +							   num);
> +
> +		if (r)
> +			acpi_dev_irqresource_disabled(r, 0);

It looks to me that the ACPI thing is what needs to be promoted to a
first class function, releasing all the resources that have used by
a given device.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...