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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Dec 22, 2021 at 2:47 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Dec 21, 2021 at 07:58:26PM +0100, Hans de Goede wrote:
> > 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).

Oh, we have a solution for this already, it's called an export
namespace. We may export symbols in its own namespace and any user
must to import it. It will show immediately who is trying to do "bad
things". Code duplication makes kernel bigger and harder to maintain.
Imagine if there is an issue or refactoring happening in one copy of
the code and missed in the other. How long would it take to discover
that and fix it?

-- 
With Best Regards,
Andy Shevchenko