Messages in this thread | | | Date | Thu, 21 Dec 2023 16:58:07 +0800 | From | Tzung-Bi Shih <> | Subject | Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq |
| |
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.
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c [...] > +static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = { > + { > + .ident = "Brya", > + .matches = { > + DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya") > + } > + }, > + { > + .ident = "Brask", > + .matches = { > + DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask") > + } > + }, > + { } > +} > +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
Does it really need `MODULE_DEVICE_TABLE`?
> +static bool cros_ec_should_force_irq_wake_capable(void)
Suggestion: either drop "cros_ec_" prefix or use "cros_ec_lpc_" prefix.
> @@ -428,20 +453,36 @@ static int cros_ec_lpc_probe(struct platform_device *pdev) > * Some boards do not have an IRQ allotted for cros_ec_lpc, > * which makes ENXIO an expected (and safe) scenario. > */ > - 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 (cros_ec_should_force_irq_wake_capable())
Please see suggestion above.
> ret = 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.
> + if (irq_wake) { > + ret = device_init_wakeup(dev, true); > + if (ret) { > + dev_err_probe(dev, ret, "Failed to init device for wakeup"); > + return ret; > + } > + ret = dev_pm_set_wake_irq(dev, irq); > + if (ret) > + return ret; > + } [...] > @@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_device *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_probe()?
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c [...] > -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > +static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct spi_device *spi) > { > - struct device_node *np = dev->of_node; > + struct cros_ec_device *ec_dev = spi_get_drvdata(spi); > + struct device_node *np = spi->dev.of_node;
struct spi_device *spi = ec_spi->spi; [1]
[1]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L751
> + if (!np) > + return; > +
The change is an improvement (or rather say it could change behavior). But strictly speaking, the change is irrelevant to the series.
> @@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); > if (!ret) > ec_spi->end_of_msg_delay = 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?
> @@ -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 = 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.
> @@ -776,19 +789,31 @@ static int cros_ec_spi_probe(struct spi_device *spi) > > err = cros_ec_register(ec_dev); > if (err) { > - dev_err(dev, "cannot register EC\n"); > + dev_err_probe(dev, err, "cannot register EC\n");
The change is irrelevant to the series.
> - device_init_wakeup(&spi->dev, true); > + if (ec_spi->irq_wake) { > + err = device_init_wakeup(dev, true); > + if (err) { > + dev_err_probe(dev, err, "Failed to init device for wakeup\n"); > + return err; > + } > + err = dev_pm_set_wake_irq(dev, ec_dev->irq); > + if (err) > + dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);
The part is different from what the patch changed in cros_ec_lpc.c. Better to be consistent. - Just return vs. dev_err_probe(). - %i vs. %d.
> static void cros_ec_spi_remove(struct spi_device *spi) > { > struct cros_ec_device *ec_dev = spi_get_drvdata(spi); > + struct device *dev = ec_dev->dev; > > + dev_pm_clear_wake_irq(dev); > + device_init_wakeup(dev, false);
Ditto, is it safe to just call them regardless of `ec_spi->irq_wake`?
> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c [...] > @@ -301,13 +307,31 @@ static int cros_ec_uart_probe(struct serdev_device *serdev) > > serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops); > > - return cros_ec_register(ec_dev); > + /* Register a new cros_ec device */ > + ret = cros_ec_register(ec_dev); > + if (ret) { > + dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
From reading the changes above, I thought it would use dev_err_probe().
> + if (ec_uart->irq_wake) { > + ret = device_init_wakeup(dev, true); > + if (ret) { > + dev_err_probe(dev, ret, "Failed to init device for wakeup"); > + return ret; > + } > + ret = dev_pm_set_wake_irq(dev, ec_uart->irq);
Ditto, better to be consistent.
> static void cros_ec_uart_remove(struct serdev_device *serdev) > { > struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev); > + struct device *dev = ec_dev->dev; > > + dev_pm_clear_wake_irq(dev); > + device_init_wakeup(dev, false);
Ditto, is it safe to just call them regardless of `ec_uart->irq_wake`?
| |