Re: [PATCH] drivers/platform/x86: Add driver for INT0002 ACPI device

[Takashi Iwai <tiwai@suse.de>]

On Fri, 21 Apr 2017 10:27:32 +0200,
Andy Shevchenko wrote:
> 
> On Fri, Apr 21, 2017 at 11:17 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 21 Apr 2017 09:41:57 +0200,
> > Hans de Goede wrote:
> >>
> >> Some peripherals on Baytrail and Cherrytrail platforms signal PME to the
> >> PMC to wakeup the system. When this happens software needs to clear the
> >> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
> >>
> >> This is modelled in ACPI through the INT0002 ACPI device.
> >>
> >> This commit adds a driver which will bind to that device, call the
> >> ACPI event handler for the wakeup and clear the interrupt source
> >> avoiding the irq storm.
> 
> >> +     char ev_name[5];
> >
> > Are 5 bytes enough?  I see the code below:
> >
> >> +             snprintf(data->ev_name, sizeof(data->ev_name), "_%c%02X",
> >> +                     res->data.gpio.triggering ? 'E' : 'L',
> >> +                     res->data.gpio.pin_table[0]);
> >
> > So it counts 6 including NUL.
> 
> How? 4 + NUL = 5.

Well, "_E00X" is 5 letters, IIUC.

> OTOH it looks like code duplication with existing drivers (GPIO ACPI
> library IIRC) which might make sense to make generic.
> 
> >> +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >> +     if (!data) {
> >> +             dev_err(dev, "can't allocate memory for int0002\n");
> >
> > The error message is mostly superfluous.
> >
> >> +static int int0002_runtime_suspend(struct device *dev)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static int int0002_runtime_resume(struct device *dev)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops int0002_pm_ops = {
> >> +     .runtime_suspend = int0002_runtime_suspend,
> >> +     .runtime_resume = int0002_runtime_resume,
> >> +};
> >
> > Do we need these runtime PM?  If not, we can remove the header
> > inclusion, too.
> 
> Yeah, and it needs attention when built with !CONFIG_PM.

Practically seen, we may build this only with CONFIG_PM, too.
The virtual GPIO thing happens only when the machine gets resumed.


Takashi