[PATCH v2 0/6] mmc: sdhci-acpi: A few cleanups

[PATCH v2 0/6] mmc: sdhci-acpi: A few cleanups

[Andy Shevchenko]

A few almost independent cleanups for the driver because of
new available APIs.

In v2:
- added patch 1 to solve compilation error (LKP)
- split patch 3 out of (previous version of) patch 4 (Christophe)
- added patches 5 and 6

Andy Shevchenko (6):
  mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions
  mmc: sdhci-acpi: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and
    pm_ptr()
  mmc: sdhci-acpi: Remove not so useful error message
  mmc: sdhci-acpi: Use devm_platform_ioremap_resource()
  mmc: sdhci-acpi: Tidy up ACPI ID table
  mmc: sdhci-acpi: Don't use "proxy" headers

 drivers/mmc/host/sdhci-acpi.c | 92 ++++++++++++++---------------------
 drivers/mmc/host/sdhci.c      | 14 ++----
 drivers/mmc/host/sdhci.h      |  2 -
 3 files changed, 40 insertions(+), 68 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Andy Shevchenko]

Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
for exporting PM functions. This helps cleaning up the other
SDHCI drivers in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci.c | 14 ++++----------
 drivers/mmc/host/sdhci.h |  2 --
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4a7733a8ad2..2214280ca5fb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3733,8 +3733,6 @@ EXPORT_SYMBOL_GPL(sdhci_thread_irq);
  *                                                                           *
 \*****************************************************************************/
 
-#ifdef CONFIG_PM
-
 static bool sdhci_cd_irq_can_wakeup(struct sdhci_host *host)
 {
 	return mmc_card_is_removable(host->mmc) &&
@@ -3814,8 +3812,7 @@ int sdhci_suspend_host(struct sdhci_host *host)
 
 	return 0;
 }
-
-EXPORT_SYMBOL_GPL(sdhci_suspend_host);
+EXPORT_PM_FN_GPL(sdhci_suspend_host);
 
 int sdhci_resume_host(struct sdhci_host *host)
 {
@@ -3853,8 +3850,7 @@ int sdhci_resume_host(struct sdhci_host *host)
 
 	return ret;
 }
-
-EXPORT_SYMBOL_GPL(sdhci_resume_host);
+EXPORT_PM_FN_GPL(sdhci_resume_host);
 
 int sdhci_runtime_suspend_host(struct sdhci_host *host)
 {
@@ -3876,7 +3872,7 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(sdhci_runtime_suspend_host);
+EXPORT_PM_FN_GPL(sdhci_runtime_suspend_host);
 
 int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
 {
@@ -3927,9 +3923,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
-
-#endif /* CONFIG_PM */
+EXPORT_PM_FN_GPL(sdhci_runtime_resume_host);
 
 /*****************************************************************************\
  *                                                                           *
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cd0e35a80542..4ee2695b0202 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -874,12 +874,10 @@ irqreturn_t sdhci_thread_irq(int irq, void *dev_id);
 void sdhci_adma_write_desc(struct sdhci_host *host, void **desc,
 			   dma_addr_t addr, int len, unsigned int cmd);
 
-#ifdef CONFIG_PM
 int sdhci_suspend_host(struct sdhci_host *host);
 int sdhci_resume_host(struct sdhci_host *host);
 int sdhci_runtime_suspend_host(struct sdhci_host *host);
 int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset);
-#endif
 
 void sdhci_cqe_enable(struct mmc_host *mmc);
 void sdhci_cqe_disable(struct mmc_host *mmc, bool recovery);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 2/6] mmc: sdhci-acpi: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()

[Andy Shevchenko]

SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() are deprecated now
and require an ifdeffery protection against unused function warnings.
The usage of pm_ptr() and SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() allows
the compiler to see the functions, thus suppressing the warning. Thus
drop the ifdeffery protection.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index d1ce9193ece9..5241c5c26891 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -1006,8 +1006,6 @@ static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
 	}
 }
 
-#ifdef CONFIG_PM_SLEEP
-
 static int sdhci_acpi_suspend(struct device *dev)
 {
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
@@ -1034,10 +1032,6 @@ static int sdhci_acpi_resume(struct device *dev)
 	return sdhci_resume_host(c->host);
 }
 
-#endif
-
-#ifdef CONFIG_PM
-
 static int sdhci_acpi_runtime_suspend(struct device *dev)
 {
 	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
@@ -1064,12 +1058,9 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
 	return sdhci_runtime_resume_host(c->host, 0);
 }
 
-#endif
-
 static const struct dev_pm_ops sdhci_acpi_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(sdhci_acpi_suspend, sdhci_acpi_resume)
-	SET_RUNTIME_PM_OPS(sdhci_acpi_runtime_suspend,
-			sdhci_acpi_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(sdhci_acpi_suspend, sdhci_acpi_resume)
+	RUNTIME_PM_OPS(sdhci_acpi_runtime_suspend, sdhci_acpi_runtime_resume, NULL)
 };
 
 static struct platform_driver sdhci_acpi_driver = {
@@ -1077,7 +1068,7 @@ static struct platform_driver sdhci_acpi_driver = {
 		.name			= "sdhci-acpi",
 		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,
 		.acpi_match_table	= sdhci_acpi_ids,
-		.pm			= &sdhci_acpi_pm_ops,
+		.pm			= pm_ptr(&sdhci_acpi_pm_ops),
 	},
 	.probe	= sdhci_acpi_probe,
 	.remove = sdhci_acpi_remove,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 3/6] mmc: sdhci-acpi: Remove not so useful error message

[Andy Shevchenko]

First of all, this error message is just informative and doesn't prevent
driver from going on. Second, the ioremap() on many architectures just
works on page size granularity, which is higher than 256 bytes. Last,
but not lease, this is an impediment for furhter cleanups, hence remove
it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 5241c5c26891..f861e9a3a911 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -849,9 +849,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	len = resource_size(iomem);
-	if (len < 0x100)
-		dev_err(dev, "Invalid iomem size!\n");
-
 	if (!devm_request_mem_region(dev, iomem->start, len, dev_name(dev)))
 		return -ENOMEM;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 4/6] mmc: sdhci-acpi: Use devm_platform_ioremap_resource()

[Andy Shevchenko]

The struct resource is not used for anything else, so we can simplify
the code a bit by using the helper function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index f861e9a3a911..8e265b53d7ce 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -822,8 +822,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
-	struct resource *iomem;
-	resource_size_t len;
 	size_t priv_size;
 	int quirks = 0;
 	int err;
@@ -844,14 +842,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
 
-	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iomem)
-		return -ENOMEM;
-
-	len = resource_size(iomem);
-	if (!devm_request_mem_region(dev, iomem->start, len, dev_name(dev)))
-		return -ENOMEM;
-
 	priv_size = slot ? slot->priv_size : 0;
 	host = sdhci_alloc_host(dev, sizeof(struct sdhci_acpi_host) + priv_size);
 	if (IS_ERR(host))
@@ -873,10 +863,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
-	host->ioaddr = devm_ioremap(dev, iomem->start,
-					    resource_size(iomem));
-	if (host->ioaddr == NULL) {
-		err = -ENOMEM;
+	host->ioaddr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(host->ioaddr)) {
+		err = PTR_ERR(host->ioaddr);
 		goto err_free;
 	}
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 5/6] mmc: sdhci-acpi: Tidy up ACPI ID table

[Andy Shevchenko]

Tidy up ACPI ID table:
- sort entries alphabetically for better maintenance
- drop comma in the terminator entry

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 8e265b53d7ce..c931f506c1b0 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -682,33 +682,35 @@ struct sdhci_acpi_uid_slot {
 };
 
 static const struct sdhci_acpi_uid_slot sdhci_acpi_uids[] = {
-	{ "80865ACA", NULL, &sdhci_acpi_slot_int_sd },
-	{ "80865ACC", NULL, &sdhci_acpi_slot_int_emmc },
-	{ "80865AD0", NULL, &sdhci_acpi_slot_int_sdio },
 	{ "80860F14" , "1" , &sdhci_acpi_slot_int_emmc },
 	{ "80860F14" , "2" , &sdhci_acpi_slot_int_sdio },
 	{ "80860F14" , "3" , &sdhci_acpi_slot_int_sd   },
 	{ "80860F16" , NULL, &sdhci_acpi_slot_int_sd   },
+	{ "80865ACA", NULL, &sdhci_acpi_slot_int_sd },
+	{ "80865ACC", NULL, &sdhci_acpi_slot_int_emmc },
+	{ "80865AD0", NULL, &sdhci_acpi_slot_int_sdio },
+	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc },
+	{ "AMDI0041", NULL, &sdhci_acpi_slot_amd_emmc },
 	{ "INT33BB"  , "2" , &sdhci_acpi_slot_int_sdio },
 	{ "INT33BB"  , "3" , &sdhci_acpi_slot_int_sd },
 	{ "INT33C6"  , NULL, &sdhci_acpi_slot_int_sdio },
 	{ "INT3436"  , NULL, &sdhci_acpi_slot_int_sdio },
 	{ "INT344D"  , NULL, &sdhci_acpi_slot_int_sdio },
-	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
 	{ "PNP0D40"  },
+	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
 	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
 	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
-	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc },
-	{ "AMDI0041", NULL, &sdhci_acpi_slot_amd_emmc },
-	{ },
+	{ }
 };
 
 static const struct acpi_device_id sdhci_acpi_ids[] = {
+	{ "80860F14" },
+	{ "80860F16" },
 	{ "80865ACA" },
 	{ "80865ACC" },
 	{ "80865AD0" },
-	{ "80860F14" },
-	{ "80860F16" },
+	{ "AMDI0040" },
+	{ "AMDI0041" },
 	{ "INT33BB"  },
 	{ "INT33C6"  },
 	{ "INT3436"  },
@@ -716,9 +718,7 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
 	{ "PNP0D40"  },
 	{ "QCOM8051" },
 	{ "QCOM8052" },
-	{ "AMDI0040" },
-	{ "AMDI0041" },
-	{ },
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 6/6] mmc: sdhci-acpi: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use) principle.
While at it, sort them alphabetically for better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index c931f506c1b0..fcb69a509c1e 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -5,27 +5,30 @@
  * Copyright (c) 2012, Intel Corporation.
  */
 
+#include <linux/acpi.h>
 #include <linux/bitfield.h>
-#include <linux/init.h>
-#include <linux/export.h>
-#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/compiler.h>
+#include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
-#include <linux/ioport.h>
-#include <linux/io.h>
-#include <linux/dma-mapping.h>
-#include <linux/compiler.h>
-#include <linux/stddef.h>
-#include <linux/bitops.h>
-#include <linux/types.h>
-#include <linux/err.h>
-#include <linux/interrupt.h>
-#include <linux/acpi.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
-#include <linux/delay.h>
-#include <linux/dmi.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/pm.h>
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 0/6] mmc: sdhci-acpi: A few cleanups

[Ulf Hansson]

On Fri, 1 Nov 2024 at 11:14, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> A few almost independent cleanups for the driver because of
> new available APIs.
>
> In v2:
> - added patch 1 to solve compilation error (LKP)
> - split patch 3 out of (previous version of) patch 4 (Christophe)
> - added patches 5 and 6
>
> Andy Shevchenko (6):
>   mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions
>   mmc: sdhci-acpi: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and
>     pm_ptr()
>   mmc: sdhci-acpi: Remove not so useful error message
>   mmc: sdhci-acpi: Use devm_platform_ioremap_resource()
>   mmc: sdhci-acpi: Tidy up ACPI ID table
>   mmc: sdhci-acpi: Don't use "proxy" headers
>
>  drivers/mmc/host/sdhci-acpi.c | 92 ++++++++++++++---------------------
>  drivers/mmc/host/sdhci.c      | 14 ++----
>  drivers/mmc/host/sdhci.h      |  2 -
>  3 files changed, 40 insertions(+), 68 deletions(-)
>
> --

This looks good to me, but deferring to apply it a few more days to
allow Adrian to share his opinion.

Kind regards
Uffe

Re: [PATCH v2 0/6] mmc: sdhci-acpi: A few cleanups

[Andy Shevchenko]

On Mon, Dec 02, 2024 at 04:24:00PM +0100, Ulf Hansson wrote:
> On Fri, 1 Nov 2024 at 11:14, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > A few almost independent cleanups for the driver because of
> > new available APIs.
> >
> > In v2:
> > - added patch 1 to solve compilation error (LKP)
> > - split patch 3 out of (previous version of) patch 4 (Christophe)
> > - added patches 5 and 6

> This looks good to me, but deferring to apply it a few more days to
> allow Adrian to share his opinion.

Sure, no hurry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
> for exporting PM functions. This helps cleaning up the other
> SDHCI drivers in the future.

It seems sdhci is the first code in the kernel to use
EXPORT_PM_FN_NS_GPL() but it was not asked for ;-)

As such, can you fill in a little background.  I am not
sure what it achieves.  Why have CONFIG_PM if not to
#ifdef dependent code behind it?

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mmc/host/sdhci.c | 14 ++++----------
>  drivers/mmc/host/sdhci.h |  2 --
>  2 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..2214280ca5fb 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3733,8 +3733,6 @@ EXPORT_SYMBOL_GPL(sdhci_thread_irq);
>   *                                                                           *
>  \*****************************************************************************/
>  
> -#ifdef CONFIG_PM
> -
>  static bool sdhci_cd_irq_can_wakeup(struct sdhci_host *host)
>  {
>  	return mmc_card_is_removable(host->mmc) &&
> @@ -3814,8 +3812,7 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  
>  	return 0;
>  }
> -
> -EXPORT_SYMBOL_GPL(sdhci_suspend_host);
> +EXPORT_PM_FN_GPL(sdhci_suspend_host);
>  
>  int sdhci_resume_host(struct sdhci_host *host)
>  {
> @@ -3853,8 +3850,7 @@ int sdhci_resume_host(struct sdhci_host *host)
>  
>  	return ret;
>  }
> -
> -EXPORT_SYMBOL_GPL(sdhci_resume_host);
> +EXPORT_PM_FN_GPL(sdhci_resume_host);
>  
>  int sdhci_runtime_suspend_host(struct sdhci_host *host)
>  {
> @@ -3876,7 +3872,7 @@ int sdhci_runtime_suspend_host(struct sdhci_host *host)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(sdhci_runtime_suspend_host);
> +EXPORT_PM_FN_GPL(sdhci_runtime_suspend_host);
>  
>  int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
>  {
> @@ -3927,9 +3923,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(sdhci_runtime_resume_host);
> -
> -#endif /* CONFIG_PM */
> +EXPORT_PM_FN_GPL(sdhci_runtime_resume_host);
>  
>  /*****************************************************************************\
>   *                                                                           *
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index cd0e35a80542..4ee2695b0202 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -874,12 +874,10 @@ irqreturn_t sdhci_thread_irq(int irq, void *dev_id);
>  void sdhci_adma_write_desc(struct sdhci_host *host, void **desc,
>  			   dma_addr_t addr, int len, unsigned int cmd);
>  
> -#ifdef CONFIG_PM
>  int sdhci_suspend_host(struct sdhci_host *host);
>  int sdhci_resume_host(struct sdhci_host *host);
>  int sdhci_runtime_suspend_host(struct sdhci_host *host);
>  int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset);
> -#endif
>  
>  void sdhci_cqe_enable(struct mmc_host *mmc);
>  void sdhci_cqe_disable(struct mmc_host *mmc, bool recovery);

Re: [PATCH v2 2/6] mmc: sdhci-acpi: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and pm_ptr()

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() are deprecated now
> and require an ifdeffery protection against unused function warnings.
> The usage of pm_ptr() and SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() allows
> the compiler to see the functions, thus suppressing the warning. Thus
> drop the ifdeffery protection.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Seems to depend on patch 1

> ---
>  drivers/mmc/host/sdhci-acpi.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index d1ce9193ece9..5241c5c26891 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -1006,8 +1006,6 @@ static void __maybe_unused sdhci_acpi_reset_signal_voltage_if_needed(
>  	}
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -
>  static int sdhci_acpi_suspend(struct device *dev)
>  {
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> @@ -1034,10 +1032,6 @@ static int sdhci_acpi_resume(struct device *dev)
>  	return sdhci_resume_host(c->host);
>  }
>  
> -#endif
> -
> -#ifdef CONFIG_PM
> -
>  static int sdhci_acpi_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_acpi_host *c = dev_get_drvdata(dev);
> @@ -1064,12 +1058,9 @@ static int sdhci_acpi_runtime_resume(struct device *dev)
>  	return sdhci_runtime_resume_host(c->host, 0);
>  }
>  
> -#endif
> -
>  static const struct dev_pm_ops sdhci_acpi_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(sdhci_acpi_suspend, sdhci_acpi_resume)
> -	SET_RUNTIME_PM_OPS(sdhci_acpi_runtime_suspend,
> -			sdhci_acpi_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(sdhci_acpi_suspend, sdhci_acpi_resume)
> +	RUNTIME_PM_OPS(sdhci_acpi_runtime_suspend, sdhci_acpi_runtime_resume, NULL)
>  };
>  
>  static struct platform_driver sdhci_acpi_driver = {
> @@ -1077,7 +1068,7 @@ static struct platform_driver sdhci_acpi_driver = {
>  		.name			= "sdhci-acpi",
>  		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,
>  		.acpi_match_table	= sdhci_acpi_ids,
> -		.pm			= &sdhci_acpi_pm_ops,
> +		.pm			= pm_ptr(&sdhci_acpi_pm_ops),
>  	},
>  	.probe	= sdhci_acpi_probe,
>  	.remove = sdhci_acpi_remove,

Re: [PATCH v2 3/6] mmc: sdhci-acpi: Remove not so useful error message

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> First of all, this error message is just informative and doesn't prevent
> driver from going on. Second, the ioremap() on many architectures just
> works on page size granularity, which is higher than 256 bytes. Last,
> but not lease, this is an impediment for furhter cleanups, hence remove

lease -> least
furhter -> further

> it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-acpi.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 5241c5c26891..f861e9a3a911 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -849,9 +849,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	len = resource_size(iomem);
> -	if (len < 0x100)
> -		dev_err(dev, "Invalid iomem size!\n");
> -
>  	if (!devm_request_mem_region(dev, iomem->start, len, dev_name(dev)))
>  		return -ENOMEM;
>  

Re: [PATCH v2 4/6] mmc: sdhci-acpi: Use devm_platform_ioremap_resource()

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> The struct resource is not used for anything else, so we can simplify
> the code a bit by using the helper function.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-acpi.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index f861e9a3a911..8e265b53d7ce 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -822,8 +822,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	struct acpi_device *device;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
> -	struct resource *iomem;
> -	resource_size_t len;
>  	size_t priv_size;
>  	int quirks = 0;
>  	int err;
> @@ -844,14 +842,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  	if (sdhci_acpi_byt_defer(dev))
>  		return -EPROBE_DEFER;
>  
> -	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!iomem)
> -		return -ENOMEM;
> -
> -	len = resource_size(iomem);
> -	if (!devm_request_mem_region(dev, iomem->start, len, dev_name(dev)))
> -		return -ENOMEM;
> -
>  	priv_size = slot ? slot->priv_size : 0;
>  	host = sdhci_alloc_host(dev, sizeof(struct sdhci_acpi_host) + priv_size);
>  	if (IS_ERR(host))
> @@ -873,10 +863,9 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>  		goto err_free;
>  	}
>  
> -	host->ioaddr = devm_ioremap(dev, iomem->start,
> -					    resource_size(iomem));
> -	if (host->ioaddr == NULL) {
> -		err = -ENOMEM;
> +	host->ioaddr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(host->ioaddr)) {
> +		err = PTR_ERR(host->ioaddr);
>  		goto err_free;
>  	}
>  

Re: [PATCH v2 5/6] mmc: sdhci-acpi: Tidy up ACPI ID table

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> Tidy up ACPI ID table:
> - sort entries alphabetically for better maintenance

Not a fan of alphabetical order just for the sake of it.
In this case, it seems to me more useful to keep different
vendors IDs together.

> - drop comma in the terminator entry
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mmc/host/sdhci-acpi.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 8e265b53d7ce..c931f506c1b0 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -682,33 +682,35 @@ struct sdhci_acpi_uid_slot {
>  };
>  
>  static const struct sdhci_acpi_uid_slot sdhci_acpi_uids[] = {
> -	{ "80865ACA", NULL, &sdhci_acpi_slot_int_sd },
> -	{ "80865ACC", NULL, &sdhci_acpi_slot_int_emmc },
> -	{ "80865AD0", NULL, &sdhci_acpi_slot_int_sdio },
>  	{ "80860F14" , "1" , &sdhci_acpi_slot_int_emmc },
>  	{ "80860F14" , "2" , &sdhci_acpi_slot_int_sdio },
>  	{ "80860F14" , "3" , &sdhci_acpi_slot_int_sd   },
>  	{ "80860F16" , NULL, &sdhci_acpi_slot_int_sd   },
> +	{ "80865ACA", NULL, &sdhci_acpi_slot_int_sd },
> +	{ "80865ACC", NULL, &sdhci_acpi_slot_int_emmc },
> +	{ "80865AD0", NULL, &sdhci_acpi_slot_int_sdio },
> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc },
> +	{ "AMDI0041", NULL, &sdhci_acpi_slot_amd_emmc },
>  	{ "INT33BB"  , "2" , &sdhci_acpi_slot_int_sdio },
>  	{ "INT33BB"  , "3" , &sdhci_acpi_slot_int_sd },
>  	{ "INT33C6"  , NULL, &sdhci_acpi_slot_int_sdio },
>  	{ "INT3436"  , NULL, &sdhci_acpi_slot_int_sdio },
>  	{ "INT344D"  , NULL, &sdhci_acpi_slot_int_sdio },
> -	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
>  	{ "PNP0D40"  },
> +	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
> -	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc },
> -	{ "AMDI0041", NULL, &sdhci_acpi_slot_amd_emmc },
> -	{ },
> +	{ }
>  };
>  
>  static const struct acpi_device_id sdhci_acpi_ids[] = {
> +	{ "80860F14" },
> +	{ "80860F16" },
>  	{ "80865ACA" },
>  	{ "80865ACC" },
>  	{ "80865AD0" },
> -	{ "80860F14" },
> -	{ "80860F16" },
> +	{ "AMDI0040" },
> +	{ "AMDI0041" },
>  	{ "INT33BB"  },
>  	{ "INT33C6"  },
>  	{ "INT3436"  },
> @@ -716,9 +718,7 @@ static const struct acpi_device_id sdhci_acpi_ids[] = {
>  	{ "PNP0D40"  },
>  	{ "QCOM8051" },
>  	{ "QCOM8052" },
> -	{ "AMDI0040" },
> -	{ "AMDI0041" },
> -	{ },
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>  

Re: [PATCH v2 6/6] mmc: sdhci-acpi: Don't use "proxy" headers

[Adrian Hunter]

On 1/11/24 12:11, Andy Shevchenko wrote:
> Update header inclusions to follow IWYU (Include What You Use) principle.
> While at it, sort them alphabetically for better maintenance.

Not a fan of alphabetical order for include files.
In this case it makes it hard to see what actually
changed.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mmc/host/sdhci-acpi.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index c931f506c1b0..fcb69a509c1e 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -5,27 +5,30 @@
>   * Copyright (c) 2012, Intel Corporation.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/bitfield.h>
> -#include <linux/init.h>
> -#include <linux/export.h>
> -#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmi.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
>  #include <linux/pinctrl/pinconf-generic.h>
>  #include <linux/platform_device.h>
> -#include <linux/ioport.h>
> -#include <linux/io.h>
> -#include <linux/dma-mapping.h>
> -#include <linux/compiler.h>
> -#include <linux/stddef.h>
> -#include <linux/bitops.h>
> -#include <linux/types.h>
> -#include <linux/err.h>
> -#include <linux/interrupt.h>
> -#include <linux/acpi.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/delay.h>
> -#include <linux/dmi.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +#include <linux/uuid.h>
>  
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/pm.h>

Re: [PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Andy Shevchenko]

On Mon, Dec 09, 2024 at 12:38:59PM +0200, Adrian Hunter wrote:
> On 1/11/24 12:11, Andy Shevchenko wrote:
> > Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
> > for exporting PM functions. This helps cleaning up the other
> > SDHCI drivers in the future.
> 
> It seems sdhci is the first code in the kernel to use
> EXPORT_PM_FN_NS_GPL() but it was not asked for ;-)
> 
> As such, can you fill in a little background.  I am not
> sure what it achieves.  Why have CONFIG_PM if not to
> #ifdef dependent code behind it?

It makes sure that the code elimination happens at compile time and
at the same time gives developer less uglified (by ifdeffery) code.
It means there is less risk to miss anything of that which make become
a compile-time warning of unused function, or even issues during linking
with modules, etc.

Should I update a commit message with that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] mmc: sdhci-acpi: Tidy up ACPI ID table

[Andy Shevchenko]

On Mon, Dec 09, 2024 at 01:29:19PM +0200, Adrian Hunter wrote:
> On 1/11/24 12:11, Andy Shevchenko wrote:
> > Tidy up ACPI ID table:
> > - sort entries alphabetically for better maintenance
> 
> Not a fan of alphabetical order just for the sake of it.

It's not for the sake of it, that makes easier to see what's going on with the
IDs. It's not the first time I stumbled over (unsorted) ID list and questioning
what the ID belongs to (vendor wise) and how many of them from the same vendor
and how many of them are invalid.

> In this case, it seems to me more useful to keep different
> vendors IDs together.

Agree, but also note, alphabetical order roughly gives the different vendor IDs
together with also grouping PNP/ACPI/invalid ACPI IDs.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] mmc: sdhci-acpi: Don't use "proxy" headers

[Andy Shevchenko]

On Mon, Dec 09, 2024 at 01:32:36PM +0200, Adrian Hunter wrote:
> On 1/11/24 12:11, Andy Shevchenko wrote:
> > Update header inclusions to follow IWYU (Include What You Use) principle.
> > While at it, sort them alphabetically for better maintenance.
> 
> Not a fan of alphabetical order for include files.
> In this case it makes it hard to see what actually
> changed.

Huh?! It will be quite easy to see if anything is being added or removed and
makes code robust against (possible) duplications. Much easier to maintain!

If you are talking about _this_ change, then it's true, but only for _this_
change, and not for the future changes. Alphabetical order is natural for
humans (and we write code for humans) which our brains are trained for. So
the processing time of the ordered data is higher and less resource-consuming.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Adrian Hunter]

On 9/12/24 18:36, Andy Shevchenko wrote:
> On Mon, Dec 09, 2024 at 12:38:59PM +0200, Adrian Hunter wrote:
>> On 1/11/24 12:11, Andy Shevchenko wrote:
>>> Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
>>> for exporting PM functions. This helps cleaning up the other
>>> SDHCI drivers in the future.
>>
>> It seems sdhci is the first code in the kernel to use
>> EXPORT_PM_FN_NS_GPL() but it was not asked for ;-)
>>
>> As such, can you fill in a little background.  I am not
>> sure what it achieves.  Why have CONFIG_PM if not to
>> #ifdef dependent code behind it?
> 
> It makes sure that the code elimination happens at compile time and

Does it eliminate the code?  Maybe I am missing something,
but it looks like it is still there:

$ grep CONFIG_PM .config
# CONFIG_PM is not set
# CONFIG_PMIC_OPREGION is not set
# CONFIG_PMBUS is not set
# CONFIG_PMIC_ADP5520 is not set
# CONFIG_PMIC_DA903X is not set
CONFIG_PM_DEVFREQ=y
# CONFIG_PM_DEVFREQ_EVENT is not set
CONFIG_PM_OPP=y
$ objdump -d drivers/mmc/host/sdhci.ko | grep sdhci_suspend_host
00000000000089f0 <__pfx_sdhci_suspend_host>:
0000000000008a00 <sdhci_suspend_host>:
    8a16:       e8 00 00 00 00          call   8a1b <sdhci_suspend_host+0x1b>
    8a29:       74 0c                   je     8a37 <sdhci_suspend_host+0x37>
    8a35:       75 54                   jne    8a8b <sdhci_suspend_host+0x8b>
    8a4c:       0f 85 06 01 00 00       jne    8b58 <sdhci_suspend_host+0x158>
    8a66:       0f 85 00 01 00 00       jne    8b6c <sdhci_suspend_host+0x16c>
    8a7b:       e8 00 00 00 00          call   8a80 <sdhci_suspend_host+0x80>
    8a86:       e9 00 00 00 00          jmp    8a8b <sdhci_suspend_host+0x8b>
    8a92:       75 0a                   jne    8a9e <sdhci_suspend_host+0x9e>
    8a98:       0f 84 87 00 00 00       je     8b25 <sdhci_suspend_host+0x125>
    8aa5:       74 90                   je     8a37 <sdhci_suspend_host+0x37>
    8ab8:       0f 85 f5 00 00 00       jne    8bb3 <sdhci_suspend_host+0x1b3>
    8ad8:       0f 85 c0 00 00 00       jne    8b9e <sdhci_suspend_host+0x19e>
    8af0:       0f 85 93 00 00 00       jne    8b89 <sdhci_suspend_host+0x189>
    8b06:       e8 00 00 00 00          call   8b0b <sdhci_suspend_host+0x10b>
    8b14:       0f 85 1d ff ff ff       jne    8a37 <sdhci_suspend_host+0x37>
    8b20:       e9 00 00 00 00          jmp    8b25 <sdhci_suspend_host+0x125>
    8b25:       e8 00 00 00 00          call   8b2a <sdhci_suspend_host+0x12a>
    8b2c:       75 52                   jne    8b80 <sdhci_suspend_host+0x180>
    8b53:       e9 55 ff ff ff          jmp    8aad <sdhci_suspend_host+0xad>
    8b62:       e8 00 00 00 00          call   8b67 <sdhci_suspend_host+0x167>
    8b67:       e9 ef fe ff ff          jmp    8a5b <sdhci_suspend_host+0x5b>
    8b76:       e8 00 00 00 00          call   8b7b <sdhci_suspend_host+0x17b>
    8b7b:       e9 f5 fe ff ff          jmp    8a75 <sdhci_suspend_host+0x75>
    8b84:       e9 15 ff ff ff          jmp    8a9e <sdhci_suspend_host+0x9e>
    8b94:       e8 00 00 00 00          call   8b99 <sdhci_suspend_host+0x199>
    8b99:       e9 60 ff ff ff          jmp    8afe <sdhci_suspend_host+0xfe>
    8ba9:       e8 00 00 00 00          call   8bae <sdhci_suspend_host+0x1ae>
    8bae:       e9 32 ff ff ff          jmp    8ae5 <sdhci_suspend_host+0xe5>
    8bbb:       e8 00 00 00 00          call   8bc0 <sdhci_suspend_host+0x1c0>
    8bc0:       e9 03 ff ff ff          jmp    8ac8 <sdhci_suspend_host+0xc8>
$ 

> at the same time gives developer less uglified (by ifdeffery) code.
> It means there is less risk to miss anything of that which make become
> a compile-time warning of unused function, or even issues during linking
> with modules, etc.
> 
> Should I update a commit message with that?

Re: [PATCH v2 0/6] mmc: sdhci-acpi: A few cleanups

[Ulf Hansson]

On Fri, 1 Nov 2024 at 11:14, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> A few almost independent cleanups for the driver because of
> new available APIs.
>
> In v2:
> - added patch 1 to solve compilation error (LKP)
> - split patch 3 out of (previous version of) patch 4 (Christophe)
> - added patches 5 and 6
>
> Andy Shevchenko (6):
>   mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions
>   mmc: sdhci-acpi: Switch to SYSTEM_SLEEP_PM_OPS()/RUNTIME_PM_OPS() and
>     pm_ptr()
>   mmc: sdhci-acpi: Remove not so useful error message
>   mmc: sdhci-acpi: Use devm_platform_ioremap_resource()
>   mmc: sdhci-acpi: Tidy up ACPI ID table
>   mmc: sdhci-acpi: Don't use "proxy" headers
>
>  drivers/mmc/host/sdhci-acpi.c | 92 ++++++++++++++---------------------
>  drivers/mmc/host/sdhci.c      | 14 ++----
>  drivers/mmc/host/sdhci.h      |  2 -
>  3 files changed, 40 insertions(+), 68 deletions(-)
>

Patch 3 and 4 applied for next, deferring the others.

Kind regards
Uffe

Re: [PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Andy Shevchenko]

On Mon, Dec 09, 2024 at 07:11:41PM +0200, Adrian Hunter wrote:
> On 9/12/24 18:36, Andy Shevchenko wrote:
> > On Mon, Dec 09, 2024 at 12:38:59PM +0200, Adrian Hunter wrote:
> >> On 1/11/24 12:11, Andy Shevchenko wrote:
> >>> Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
> >>> for exporting PM functions. This helps cleaning up the other
> >>> SDHCI drivers in the future.
> >>
> >> It seems sdhci is the first code in the kernel to use
> >> EXPORT_PM_FN_NS_GPL() but it was not asked for ;-)
> >>
> >> As such, can you fill in a little background.  I am not
> >> sure what it achieves.  Why have CONFIG_PM if not to
> >> #ifdef dependent code behind it?
> > 
> > It makes sure that the code elimination happens at compile time and
> 
> Does it eliminate the code?  Maybe I am missing something,
> but it looks like it is still there:

Hmm... Indeed. My tests show the same. I believe these new macros were never
tested (and we have no users in the kernel).

Richard?

> > at the same time gives developer less uglified (by ifdeffery) code.
> > It means there is less risk to miss anything of that which make become
> > a compile-time warning of unused function, or even issues during linking
> > with modules, etc.
> > 
> > Should I update a commit message with that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] mmc: sdhci-acpi: Tidy up ACPI ID table

[Andy Shevchenko]

On Mon, Dec 09, 2024 at 01:29:19PM +0200, Adrian Hunter wrote:
> On 1/11/24 12:11, Andy Shevchenko wrote:
> > Tidy up ACPI ID table:
> > - sort entries alphabetically for better maintenance
> 
> Not a fan of alphabetical order just for the sake of it.
> In this case, it seems to me more useful to keep different
> vendors IDs together.

But it will be natural outcome.
Should I specifically put them into groups?

> > - drop comma in the terminator entry

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] mmc: sdhci: Use EXPORT_PM_FN_NS_GPL() for exporting PM functions

[Richard Fitzgerald]

On 16/01/2025 3:14 pm, Andy Shevchenko wrote:
> On Mon, Dec 09, 2024 at 07:11:41PM +0200, Adrian Hunter wrote:
>> On 9/12/24 18:36, Andy Shevchenko wrote:
>>> On Mon, Dec 09, 2024 at 12:38:59PM +0200, Adrian Hunter wrote:
>>>> On 1/11/24 12:11, Andy Shevchenko wrote:
>>>>> Switch from ugly ifdeffery to using EXPORT_PM_FN_NS_GPL()
>>>>> for exporting PM functions. This helps cleaning up the other
>>>>> SDHCI drivers in the future.
>>>>
>>>> It seems sdhci is the first code in the kernel to use
>>>> EXPORT_PM_FN_NS_GPL() but it was not asked for ;-)
>>>>
>>>> As such, can you fill in a little background.  I am not
>>>> sure what it achieves.  Why have CONFIG_PM if not to
>>>> #ifdef dependent code behind it?
>>>
>>> It makes sure that the code elimination happens at compile time and
>>
>> Does it eliminate the code?  Maybe I am missing something,
>> but it looks like it is still there:
> 
> Hmm... Indeed. My tests show the same. I believe these new macros were never
> tested (and we have no users in the kernel).
> 
> Richard?
> 

As I recall the intention wasn't to eliminate the code. It was to
eliminate the EXPORT for code that had already been eliminated by other
macros.

A bunch of ugly macros were added a while back by someone to create PM
callback functions so that the code would be eliminated if CONFIG_PM=n,
without having to use __maybe_unused or ifdefs in the .c, And together
with this, drivers were being zealously changed to use these macros to
eliminated unused PM callbacks.

But the macros assumed the PM functions were always local static.
Sometimes a family of drivers for similar hardware share common code and
the PM functions are exported so I added these macros to make the EXPORT
also disappear. It's not great but it was based on an existing similar
pattern.

I did also have patches that used the macros with the PM wrapper macros
to eliminate the code and exports in the cs35l56 codec driver. But they
got held up behind a large backlog of other work and are basically still
sitting in a dark corner somewhere. At this point it doesn't seem worth
the trouble to eliminate the PM callbacks, especially as we don't know
of any platform that is using codecs like cs35l56 without PM. So really
I don't mind if you remove these EXPORT macros.

>>> at the same time gives developer less uglified (by ifdeffery) code.
>>> It means there is less risk to miss anything of that which make become
>>> a compile-time warning of unused function, or even issues during linking
>>> with modules, etc.
>>>
>>> Should I update a commit message with that?
>