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

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(+)

-- 
2.26.2

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.

On 26/11/2020 09:28, Marc Zyngier wrote:
> 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);
>>

Hi Marc,

>> +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. 

Well we do have the virq available, but then we need to pass the virq 
and device irq index. But maybe I somehow reverse-lookup the ACPI res 
somehow from virq, such that we don't require the irq device index.

> 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>

Yes, ideally we can use virq.

> 
>> +
>> +    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.

This is just clearing the irq resource flags, but it could be reasonable 
(to promote).

Thanks,
John