Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

> > -     irq = platform_get_irq_optional(pdev, 0);
> > -     if (irq > 0)
> > +     irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> > +     if (irq > 0) {
> >               ec_dev->irq = irq;
> > -     else if (irq != -ENXIO) {
> > +             if (should_force_irq_wake_capable())
> > +                     ec_dev->irq_wake = true;
> > +             else
> > +                     ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +     } else if (irq != -ENXIO) {
> >               dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >               return irq;
> >       }
>
> Still I do not like ambiguity behind irq > 0 vs. irqres.start.
>
> For this, and if needed others, return plain error.
> Seems I gave the tag for the previous patch, consider
> that tag conditional (it seems I missed this).

What "others" do you mean? Modify platform_get_irq_resource_optional()
to return success/err? Or do you mean to modify all irq resource based
functions? of_irq_to_resource() already existed and returns the irq
number on success. Modifying it would mean updating all references to
it, in addition to modifying the fwnode abstraction layer (and its
references). I'm open to modifying
platform_get_irq_resource_optional(), but would like to avoid blowing
up this patch train any further.

> ...
>
> >       u16 proto_version;
> >       void *priv;
> >       int irq;
> > +     bool irq_wake;
> >       u8 *din;
> >       u8 *dout;
> >       int din_size;
> >       int dout_size;
> > -     bool wake_enabled;
> >       bool suspended;
> >       int (*cmd_xfer)(struct cros_ec_device *ec,
> >                       struct cros_ec_command *msg);
>
> Have you run pahole on this (before and after)?

Yes I did. The structure is not fully optimized, but this change keps
the overall size unchanged (328 bytes).