Received-SPF: pass (google.com: domain of linux-kernel+bounces-12274-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231226192149.1830592-1-markhas@chromium.org>
 <20231226122113.v3.24.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid> <ZYxgQn8L7ENkc0AJ@smile.fi.intel.com>
In-Reply-To: <ZYxgQn8L7ENkc0AJ@smile.fi.intel.com>
From: Mark Hasemeyer <markhas@chromium.org>
Date: Wed, 27 Dec 2023 14:29:16 -0700
Message-ID: <CANg-bXAN-DOZNsea7NNO4bFoZ6un44_upkseynS_dg5Odg6Whg@mail.gmail.com>
Subject: Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Rob Herring <robh@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Sudeep Holla <sudeep.holla@arm.com>, 
	Raul Rangel <rrangel@chromium.org>, Tzung-Bi Shih <tzungbi@kernel.org>, 
	Benson Leung <bleung@chromium.org>, Bhanu Prakash Maiya <bhanumaiya@chromium.org>, 
	Chen-Yu Tsai <wenst@chromium.org>, Guenter Roeck <groeck@chromium.org>, Lee Jones <lee@kernel.org>, 
	Prashant Malani <pmalani@chromium.org>, Rob Barnes <robbarnes@google.com>, 
	Stephen Boyd <swboyd@chromium.org>, chrome-platform@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"

> > -     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).