Received-SPF: pass (google.com: domain of linux-kernel+bounces-10076-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231220235459.2965548-1-markhas@chromium.org>
 <20231220165423.v2.22.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid> <ZYP-H27vsJvMTupH@google.com>
In-Reply-To: <ZYP-H27vsJvMTupH@google.com>
From: Mark Hasemeyer <markhas@chromium.org>
Date: Fri, 22 Dec 2023 15:19:56 -0700
Message-ID: <CANg-bXAUiw-v3D6u3dzve3i65b29wqPSAnw3uuZ2mFNsVgY9Hw@mail.gmail.com>
Subject: Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to
 manage wakeirq
To: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Raul Rangel <rrangel@chromium.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Andy Shevchenko <andriy.shevchenko@intel.com>, 
	Rob Herring <robh@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, 
	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"
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 21, 2023 at 1:58=E2=80=AFAM Tzung-Bi Shih <tzungbi@kernel.org> =
wrote:
>
> On Wed, Dec 20, 2023 at 04:54:36PM -0700, Mark Hasemeyer wrote:
> > The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
> > opposed to adding it to cros_ec.c because the i2c subsystem already
> > enables the wakirq (if applicable) on our behalf.
>
> The setting flow are all the same.  I think helper functions in cros_ec.c=
 help
> to deduplicate the code.

I'll see what I can do.

> > +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
>
> Does it really need `MODULE_DEVICE_TABLE`?

Nope. Will drop.

> >       ret =3D cros_ec_register(ec_dev);
> >       if (ret) {
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n"=
, ret);
>
> The change is irrelevant to the series.

I'll drop the use of dev_err_probe() to stay consistent with current
conventions. Perhaps it can be added in a follow-up patch.

> > @@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_devi=
ce *pdev)
> >               acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> >                                          cros_ec_lpc_acpi_notify);
> >
> > +     dev_pm_clear_wake_irq(dev);
> > +     device_init_wakeup(dev, false);
>
> Is it safe to call them anyway regardless of `irq_wake` in cros_ec_lpc_pr=
obe()?

According to bench tests, it's not a problem. That said, I am
refactoring the code to move the logic into cros_ec.c and will
conditionally call the cleanup functions.

> > +     if (!np)
> > +             return;
> > +
>
> The change is an improvement (or rather say it could change behavior).  B=
ut
> strictly speaking, the change is irrelevant to the series.

Will drop.

>
> > @@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_sp=
i *ec_spi, struct device *dev)
> >       ret =3D of_property_read_u32(np, "google,cros-ec-spi-msg-delay", =
&val);
> >       if (!ret)
> >               ec_spi->end_of_msg_delay =3D val;
> > +
> > +     if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")=
) {
>
> Or just use `spi->irq`[2].
>
> [2]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome=
/cros_ec_spi.c#L762
>
> They are the same, but does of_property_present() make more sense?

Yes it does. I'll use it.

> > @@ -768,6 +778,9 @@ static int cros_ec_spi_probe(struct spi_device *spi=
)
> >                          sizeof(struct ec_response_get_protocol_info);
> >       ec_dev->dout_size =3D sizeof(struct ec_host_request);
> >
> > +     /* Check for any DT properties */
> > +     cros_ec_spi_dt_probe(ec_spi, spi);
>
> `spi` is possibly not needed.  See comment above.

Agreed.