[PATCH v3 0/5] DesignWare PWM improvements

[PATCH v3 0/5] DesignWare PWM improvements

[Raag Jadav]

This series implements 16 channel PWM support for Intel Elkhart Lake
along with minor cleanups for DesignWare PWM driver.

Changes since v2:
- Remove error code duplication from dev_err_probe()
- Update tags

Changes since v1:
- Drop redundant error check
- Provide dwc_pwm_init_one() to initialize one PWM instance
- Use dev_get_drvdata() instead of pci_get_drvdata()
- Use pm_sleep_ptr() instead of use pm_ptr()

Raag Jadav (5):
  pwm: dwc: use pm_sleep_ptr() macro
  pwm: dwc: drop redundant error check
  pwm: dwc: Add 16 channel support for Intel Elkhart Lake
  pwm: dwc: simplify error handling
  pwm: dwc: access driver_data using dev_get_drvdata()

 drivers/pwm/pwm-dwc.c | 59 ++++++++++++++++++++++++-------------------
 drivers/pwm/pwm-dwc.h |  5 ++++
 2 files changed, 38 insertions(+), 26 deletions(-)


base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.35.3

[PATCH v3 1/5] pwm: dwc: use pm_sleep_ptr() macro

[Raag Jadav]

Since we don't have runtime PM handles here, we should be using
pm_sleep_ptr() macro, so that the compiler can discard it in case
CONFIG_PM_SLEEP=n.

Fixes: 30b5b066fa83 ("pwm: dwc: Use DEFINE_SIMPLE_DEV_PM_OPS for PM functions")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-dwc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 4929354f8cd9..a4a057ae03ea 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -120,7 +120,7 @@ static struct pci_driver dwc_pwm_driver = {
 	.remove = dwc_pwm_remove,
 	.id_table = dwc_pwm_id_table,
 	.driver = {
-		.pm = pm_ptr(&dwc_pwm_pm_ops),
+		.pm = pm_sleep_ptr(&dwc_pwm_pm_ops),
 	},
 };
 
-- 
2.35.3

[PATCH v3 2/5] pwm: dwc: drop redundant error check

[Raag Jadav]

pcim_iomap_table() fails only if pcim_iomap_regions() fails. No need to
check for failure if the latter is already successful.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-dwc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index a4a057ae03ea..b9e18dbf7493 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -50,10 +50,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	}
 
 	dwc->base = pcim_iomap_table(pci)[0];
-	if (!dwc->base) {
-		dev_err(dev, "Base address missing\n");
-		return -ENOMEM;
-	}
 
 	ret = devm_pwmchip_add(dev, &dwc->chip);
 	if (ret)
-- 
2.35.3

[PATCH v3 3/5] pwm: dwc: Add 16 channel support for Intel Elkhart Lake

[Raag Jadav]

Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
function with 8 channels each. Add support for the remaining channels.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Tested-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-dwc.c | 33 +++++++++++++++++++++++++--------
 drivers/pwm/pwm-dwc.h |  5 +++++
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index b9e18dbf7493..6d922afcb20a 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -25,16 +25,31 @@
 
 #include "pwm-dwc.h"
 
-static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
+/* Elkhart Lake */
+static const struct dwc_pwm_info ehl_pwm_info = {
+	.nr = 2,
+	.size = 0x1000,
+};
+
+static int dwc_pwm_init_one(struct device *dev, void __iomem *base, unsigned int offset)
 {
-	struct device *dev = &pci->dev;
 	struct dwc_pwm *dwc;
-	int ret;
 
 	dwc = dwc_pwm_alloc(dev);
 	if (!dwc)
 		return -ENOMEM;
 
+	dwc->base = base + offset;
+
+	return devm_pwmchip_add(dev, &dwc->chip);
+}
+
+static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	const struct dwc_pwm_info *info;
+	struct device *dev = &pci->dev;
+	int i, ret;
+
 	ret = pcim_enable_device(pci);
 	if (ret) {
 		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
@@ -49,11 +64,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return ret;
 	}
 
-	dwc->base = pcim_iomap_table(pci)[0];
+	info = (const struct dwc_pwm_info *)id->driver_data;
 
-	ret = devm_pwmchip_add(dev, &dwc->chip);
-	if (ret)
-		return ret;
+	for (i = 0; i < info->nr; i++) {
+		ret = dwc_pwm_init_one(dev, pcim_iomap_table(pci)[0], i * info->size);
+		if (ret)
+			return ret;
+	}
 
 	pm_runtime_put(dev);
 	pm_runtime_allow(dev);
@@ -105,7 +122,7 @@ static int dwc_pwm_resume(struct device *dev)
 static DEFINE_SIMPLE_DEV_PM_OPS(dwc_pwm_pm_ops, dwc_pwm_suspend, dwc_pwm_resume);
 
 static const struct pci_device_id dwc_pwm_id_table[] = {
-	{ PCI_VDEVICE(INTEL, 0x4bb7) }, /* Elkhart Lake */
+	{ PCI_VDEVICE(INTEL, 0x4bb7), (kernel_ulong_t)&ehl_pwm_info },
 	{  }	/* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(pci, dwc_pwm_id_table);
diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h
index 64795247c54c..c9bbfc77b568 100644
--- a/drivers/pwm/pwm-dwc.h
+++ b/drivers/pwm/pwm-dwc.h
@@ -33,6 +33,11 @@ MODULE_IMPORT_NS(dwc_pwm);
 #define DWC_TIM_CTRL_INT_MASK	BIT(2)
 #define DWC_TIM_CTRL_PWM	BIT(3)
 
+struct dwc_pwm_info {
+	unsigned int nr;
+	unsigned int size;
+};
+
 struct dwc_pwm_ctx {
 	u32 cnt;
 	u32 cnt2;
-- 
2.35.3

[PATCH v3 4/5] pwm: dwc: simplify error handling

[Raag Jadav]

Simplify error handling in ->probe() function using dev_err_probe() helper
and while at it, drop error codes from the message to prevent duplication.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/pwm/pwm-dwc.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 6d922afcb20a..56fac8655c7b 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -51,18 +51,14 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	int i, ret;
 
 	ret = pcim_enable_device(pci);
-	if (ret) {
-		dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable device\n");
 
 	pci_set_master(pci);
 
 	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
-	if (ret) {
-		dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to iomap PCI BAR\n");
 
 	info = (const struct dwc_pwm_info *)id->driver_data;
 
-- 
2.35.3

[PATCH v3 5/5] pwm: dwc: access driver_data using dev_get_drvdata()

[Raag Jadav]

Now that we're setting driver_data using dev_set_drvdata(), we can use
dev_get_drvdata() for accessing it.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-dwc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 56fac8655c7b..ed56b796b670 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -82,8 +82,7 @@ static void dwc_pwm_remove(struct pci_dev *pci)
 
 static int dwc_pwm_suspend(struct device *dev)
 {
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
-	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
+	struct dwc_pwm *dwc = dev_get_drvdata(dev);
 	int i;
 
 	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
@@ -102,8 +101,7 @@ static int dwc_pwm_suspend(struct device *dev)
 
 static int dwc_pwm_resume(struct device *dev)
 {
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
-	struct dwc_pwm *dwc = pci_get_drvdata(pdev);
+	struct dwc_pwm *dwc = dev_get_drvdata(dev);
 	int i;
 
 	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
-- 
2.35.3

Re: [PATCH v3 0/5] DesignWare PWM improvements

[Jarkko Nikula]

On 2/12/24 15:02, Raag Jadav wrote:
> This series implements 16 channel PWM support for Intel Elkhart Lake
> along with minor cleanups for DesignWare PWM driver.
> 
> Changes since v2:
> - Remove error code duplication from dev_err_probe()
> - Update tags
> 
> Changes since v1:
> - Drop redundant error check
> - Provide dwc_pwm_init_one() to initialize one PWM instance
> - Use dev_get_drvdata() instead of pci_get_drvdata()
> - Use pm_sleep_ptr() instead of use pm_ptr()
> 
> Raag Jadav (5):
>    pwm: dwc: use pm_sleep_ptr() macro
>    pwm: dwc: drop redundant error check
>    pwm: dwc: Add 16 channel support for Intel Elkhart Lake
>    pwm: dwc: simplify error handling
>    pwm: dwc: access driver_data using dev_get_drvdata()
> 
>   drivers/pwm/pwm-dwc.c | 59 ++++++++++++++++++++++++-------------------
>   drivers/pwm/pwm-dwc.h |  5 ++++
>   2 files changed, 38 insertions(+), 26 deletions(-)
> 
I tested on Elkhart lake this patchset and it adds another 8 channel PWM 
instance and the PCI device continue switching between D0 and D3 power 
states depending is some channel enabled or all idle.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v3 2/5] pwm: dwc: drop redundant error check

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

Hello,

On Mon, Feb 12, 2024 at 06:32:44PM +0530, Raag Jadav wrote:
> pcim_iomap_table() fails only if pcim_iomap_regions() fails. No need to
> check for failure if the latter is already successful.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/pwm-dwc.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index a4a057ae03ea..b9e18dbf7493 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -50,10 +50,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  	}
>  
>  	dwc->base = pcim_iomap_table(pci)[0];
> -	if (!dwc->base) {
> -		dev_err(dev, "Base address missing\n");
> -		return -ENOMEM;
> -	}

As just written in reply to v2, I'd like to have a comment here saying
that pcim_iomap_table() won't fail after pcim_iomap_table() to prevent
someone sending a patch that undoes this change with the reasoning that
pcim_iomap_table()'s return value should be checked.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]