Re: [PATCH v2 1/2] platform/x86: Add x86-acpi-irq-helpers.h

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Dec 21, 2021 at 07:58:26PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 16:27, Andy Shevchenko wrote:
> > On Tue, Dec 21, 2021 at 5:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Add helper code to get Linux IRQ numbers given a description of the IRQ
> >> source (either IOAPIC index, or GPIO chip name + pin-number).
> >>
> >> This is intended to be used to lookup Linux IRQ numbers in cases where the
> >> ACPI description for a device somehow lacks this info. This is only meant
> >> for use on x86 ACPI platforms.
> >>
> >> This code is big/complex enough to warrant sharing, but too small to live
> >> in its own module, therefor x86_acpi_irq_helper_get() is defined as
> >> a static inline helper function.
> > 
> > ...
> > 
> >> +/* For gpio_get_desc which is EXPORT_SYMBOL_GPL() */
> > 
> > gpio_get_desc()
> 
> Fixed in my local version.
> 
> > and honestly I don't like this kind of includes (yes,
> > I know sometimes it's the best compromise).
> > 
> >> +#include "../../gpio/gpiolib.h"
> > 
> > ...
> > 
> >> +               /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
> >> +               chip = gpiochip_find(data->gpio_chip, x86_acpi_irq_helper_gpiochip_find);
> >> +               if (!chip)
> >> +                       return -EPROBE_DEFER;
> >> +
> >> +               gpiod = gpiochip_get_desc(chip, data->index);
> >> +               if (IS_ERR(gpiod)) {
> >> +                       ret = PTR_ERR(gpiod);
> >> +                       pr_err("error %d getting GPIO %s %d\n", ret,
> >> +                              data->gpio_chip, data->index);
> >> +                       return ret;
> >> +               }
> >> +
> >> +               irq = gpiod_to_irq(gpiod);
> >> +               if (irq < 0) {
> >> +                       pr_err("error %d getting IRQ %s %d\n", irq,
> >> +                              data->gpio_chip, data->index);
> >> +                       return irq;
> >> +               }
> >> +
> >> +               irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
> >> +               if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
> >> +                       irq_set_irq_type(irq, irq_type);
> >> +
> >> +               return irq;
> > 
> > I'm wondering why it can't be a part of the GPIO ACPI library?
> 
> Interesting suggestion, but this really is only intended for the
> special case when the DSDT is missing this info. I'm a bit worried
> that having this available as a generic helper may lead to it
> getting used too much. But I guess we can just put a comment on it
> explaining that normally its use should be avoided.
> 
> I've added Mika to the Cc, Mika, what do you think about adding this
> as a new helper to the GPIO ACPI library ?

Preferably no :-) Reason is that even if we add comment to the function
you don't remember it after two weeks so the next patch adding another
user will not be noticed by reviewers (unless tha name of the function
clearly says it is a quirk/workaround).