2021-11-10 08:55:44

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:
> The fwnode framework did not have means to obtain the IRQ number from
> the name of a node.
> Add that now, in form of the fwnode_irq_get_byname() function.

...

> +int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name)
> +{
> + int index;
> +
> + if (unlikely(!name))
> + return -EINVAL;

> + index = fwnode_property_match_string(fwnode, "interrupt-names", name);
> + if (index < 0)
> + return index;

It won't work like this. The ACPI table quite likely won't have this in them.
Also it doesn't cover the GPIO interrupts in ACPI case.

> + return fwnode_irq_get(fwnode, index);

Neither this covers GPIO IRQs.

> +}
> +EXPORT_SYMBOL(fwnode_irq_get_byname);

So, first you need to provide a design for this how ACPI cases can be handled.

Imagine these cases (at least) for _CRS method in ACPI:
1/ Single GSI

Interrupt()

2/ Single GPIO IRQ

GpioInt()

3/ Both in different orders
a)
Interrupt()
GpioInt()

b)
GpioInt()
Interrupt()

4/ Mixed (complicated cases)

Interrupt()
Interrupt()
GpioInt()
Interrupt()
GpioInt()

Obvious question, what does the index mean in all these cases?

Next one is, how can we quirk out the platforms with the old ACPI tables
where no properties are provided? For GPIO there is struct acpi_gpio_params
which goes deep into ACPI glue layer.

Luckily, the GPIO IRQ case has already available APIs for indexing and naming
match: acpi_dev_gpio_irq_get_by().

Hence, the main task is to define index in cases like 4 and see what can be
done for the GSI cases.

--
With Best Regards,
Andy Shevchenko



2021-11-10 17:05:03

by Puranjay Mohan

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

On Wed, Nov 10, 2021 at 2:23 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:
> > The fwnode framework did not have means to obtain the IRQ number from
> > the name of a node.
> > Add that now, in form of the fwnode_irq_get_byname() function.
>
> ...
>
> > +int fwnode_irq_get_byname(struct fwnode_handle *fwnode, const char *name)
> > +{
> > + int index;
> > +
> > + if (unlikely(!name))
> > + return -EINVAL;
>
> > + index = fwnode_property_match_string(fwnode, "interrupt-names", name);
> > + if (index < 0)
> > + return index;
>
> It won't work like this. The ACPI table quite likely won't have this in them.
> Also it doesn't cover the GPIO interrupts in ACPI case.
>
> > + return fwnode_irq_get(fwnode, index);
>
> Neither this covers GPIO IRQs.
>
> > +}
> > +EXPORT_SYMBOL(fwnode_irq_get_byname);
>
> So, first you need to provide a design for this how ACPI cases can be handled.
>
> Imagine these cases (at least) for _CRS method in ACPI:
> 1/ Single GSI
>
> Interrupt()
>
> 2/ Single GPIO IRQ
>
> GpioInt()
>
> 3/ Both in different orders
> a)
> Interrupt()
> GpioInt()
>
> b)
> GpioInt()
> Interrupt()
>
> 4/ Mixed (complicated cases)
>
> Interrupt()
> Interrupt()
> GpioInt()
> Interrupt()
> GpioInt()
>
> Obvious question, what does the index mean in all these cases?
>
> Next one is, how can we quirk out the platforms with the old ACPI tables
> where no properties are provided? For GPIO there is struct acpi_gpio_params
> which goes deep into ACPI glue layer.
>
> Luckily, the GPIO IRQ case has already available APIs for indexing and naming
> match: acpi_dev_gpio_irq_get_by().
>
> Hence, the main task is to define index in cases like 4 and see what can be
> done for the GSI cases.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Andy,
I wrote this function keeping the device tree in mind. I will have to
look into ACPI and see how the cases you mentioned can be implemented.
Let's see how far I can get with understanding the ACPI.

--
Thanks and Regards

Yours Truly,

Puranjay Mohan

2021-11-10 17:27:25

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] device property: Add fwnode_irq_get_byname()

On Wed, Nov 10, 2021 at 10:34:43PM +0530, Puranjay Mohan wrote:
> On Wed, Nov 10, 2021 at 2:23 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Nov 10, 2021 at 01:38:39AM +0530, Puranjay Mohan wrote:

> I wrote this function keeping the device tree in mind. I will have to
> look into ACPI and see how the cases you mentioned can be implemented.
> Let's see how far I can get with understanding the ACPI.

Yeah.

What you need to have is
1) expand fwnode_irq_get() to support ACPI GPIO IRQ resources.
2) provide a simple version of the fwnode_irq_get_by_name() like

if (is_of_node())
return of_...();

return acpi_dev_gpio_irq_get_by();

3) establish understanding about naming for ACPI and
4) extend fwnode_irq_get_by_name() by it

if (is_of_node())
return of_...();

ret = acpi_irq_get_by_name();
if (ret > 0)
return ret;

return acpi_dev_gpio_irq_get_by();

As I mentioned, items 1 and 2 are easy to achieve with currently existing APIs.

--
With Best Regards,
Andy Shevchenko