[PATCH v4 0/4] DesignWare PWM improvements

[PATCH v4 0/4] 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 v3:
- Fix PM regression
- Add comment for removed error check
- Drop accepted patches
- Update tags

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 (4):
  pwm: dwc: Fix PM regression
  pwm: dwc: drop redundant error check
  pwm: dwc: Add 16 channel support for Intel Elkhart Lake
  pwm: dwc: simplify error handling

 drivers/pwm/pwm-dwc.c | 58 +++++++++++++++++++++++++++----------------
 drivers/pwm/pwm-dwc.h |  5 ++++
 2 files changed, 41 insertions(+), 22 deletions(-)


base-commit: 191fba33d814a855ac6a29d2c26a46fbe88d55b4
-- 
2.35.3

[PATCH v4 1/4] pwm: dwc: Fix PM regression

[Raag Jadav]

While preparing dwc driver for devm_pwmchip_alloc() usage, commit
df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
modified ->suspend() handle to use the pwm_chip as driver_data for
accessing struct dwc_pwm, but didn't modify ->resume() handle with
relevant changes. This results into illegal memory access during
device wakeup and causes a PM regression.

Fix this by correctly accessing struct dwc_pwm in ->resume() handle.

Fixes: df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/pwm/pwm-dwc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 8ca1c20a6aaf..c0e586688e57 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -95,7 +95,8 @@ static int dwc_pwm_suspend(struct device *dev)
 
 static int dwc_pwm_resume(struct device *dev)
 {
-	struct dwc_pwm *dwc = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct dwc_pwm *dwc = to_dwc_pwm(chip);
 	int i;
 
 	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
-- 
2.35.3

[PATCH v4 2/4] 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>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/pwm/pwm-dwc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index c0e586688e57..7dbb72c80ef5 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -51,11 +51,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return ret;
 	}
 
+	/* No need to check for failure, pcim_iomap_regions() does it for us. */
 	dwc->base = pcim_iomap_table(pci)[0];
-	if (!dwc->base) {
-		dev_err(dev, "Base address missing\n");
-		return -ENOMEM;
-	}
 
 	ret = devm_pwmchip_add(dev, chip);
 	if (ret)
-- 
2.35.3

[PATCH v4 3/4] 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: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
---
 drivers/pwm/pwm-dwc.c | 38 +++++++++++++++++++++++++++++---------
 drivers/pwm/pwm-dwc.h |  5 +++++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 7dbb72c80ef5..de95352081fa 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -25,17 +25,32 @@
 
 #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 pwm_chip *chip;
 	struct dwc_pwm *dwc;
-	int ret;
 
 	chip = dwc_pwm_alloc(dev);
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
+
 	dwc = to_dwc_pwm(chip);
+	dwc->base = base + offset;
+
+	return devm_pwmchip_add(dev, 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) {
@@ -51,12 +66,17 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return ret;
 	}
 
-	/* No need to check for failure, pcim_iomap_regions() does it for us. */
-	dwc->base = pcim_iomap_table(pci)[0];
+	info = (const struct dwc_pwm_info *)id->driver_data;
 
-	ret = devm_pwmchip_add(dev, chip);
-	if (ret)
-		return ret;
+	for (i = 0; i < info->nr; i++) {
+		/*
+		 * No need to check for pcim_iomap_table() failure,
+		 * pcim_iomap_regions() already does it for us.
+		 */
+		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);
@@ -108,7 +128,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 5887371803fd..a8b074841ae8 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 v4 4/4] 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>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.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 de95352081fa..676eaf8d7a53 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -53,18 +53,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

Re: [PATCH v4 2/4] pwm: dwc: drop redundant error check

[Thorsten Scherer]

Hello,

On Mon, Feb 19, 2024 at 09:08:33AM +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>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
>  drivers/pwm/pwm-dwc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index c0e586688e57..7dbb72c80ef5 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -51,11 +51,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  		return ret;
>  	}
>  
> +	/* No need to check for failure, pcim_iomap_regions() does it for us. */

IMHO this comment could be omitted.

>  	dwc->base = pcim_iomap_table(pci)[0];
> -	if (!dwc->base) {
> -		dev_err(dev, "Base address missing\n");
> -		return -ENOMEM;
> -	}
>  
>  	ret = devm_pwmchip_add(dev, chip);
>  	if (ret)
> -- 
> 2.35.3
> 
> 

Best regards
Thorsten

Re: [PATCH v4 1/4] pwm: dwc: Fix PM regression

[Uwe Kleine-König]

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

Hello Raag,

On Mon, Feb 19, 2024 at 09:08:32AM +0530, Raag Jadav wrote:
> While preparing dwc driver for devm_pwmchip_alloc() usage, commit
> df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
> modified ->suspend() handle to use the pwm_chip as driver_data for
> accessing struct dwc_pwm, but didn't modify ->resume() handle with
> relevant changes. This results into illegal memory access during
> device wakeup and causes a PM regression.
> 
> Fix this by correctly accessing struct dwc_pwm in ->resume() handle.
> 
> Fixes: df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/pwm/pwm-dwc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 8ca1c20a6aaf..c0e586688e57 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -95,7 +95,8 @@ static int dwc_pwm_suspend(struct device *dev)
>  
>  static int dwc_pwm_resume(struct device *dev)
>  {
> -	struct dwc_pwm *dwc = dev_get_drvdata(dev);
> +	struct pwm_chip *chip = dev_get_drvdata(dev);
> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
>  	int i;
>  
>  	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {

If you're ok I'd squash this into df41cd8bbcad adding

	Thanks to Raag Jadav for providing a hunk of this patch that Uwe
	missed during creation of this patch.

to the commit log.

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 --]

Re: [PATCH v4 2/4] pwm: dwc: drop redundant error check

[Uwe Kleine-König]

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

Hello,

On Mon, Feb 19, 2024 at 08:11:00AM +0100, Thorsten Scherer wrote:
> On Mon, Feb 19, 2024 at 09:08:33AM +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>
> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > ---
> >  drivers/pwm/pwm-dwc.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> > index c0e586688e57..7dbb72c80ef5 100644
> > --- a/drivers/pwm/pwm-dwc.c
> > +++ b/drivers/pwm/pwm-dwc.c
> > @@ -51,11 +51,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
> >  		return ret;
> >  	}
> >  
> > +	/* No need to check for failure, pcim_iomap_regions() does it for us. */
> 
> IMHO this comment could be omitted.

I like the comment (and even asked for it). Is it really only me who
doesn't know that after pcim_iomap_regions() calling pcim_iomap_table()
is unproblematic?

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 --]

Re: [PATCH v4 2/4] pwm: dwc: drop redundant error check

[Thorsten Scherer]

Hello,

On Mon, Feb 19, 2024 at 08:11:01AM +0100, Thorsten Scherer wrote:
> Hello,
> 
> On Mon, Feb 19, 2024 at 09:08:33AM +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>
> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > ---
> >  drivers/pwm/pwm-dwc.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> > index c0e586688e57..7dbb72c80ef5 100644
> > --- a/drivers/pwm/pwm-dwc.c
> > +++ b/drivers/pwm/pwm-dwc.c
> > @@ -51,11 +51,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
> >  		return ret;
> >  	}
> >  
> > +	/* No need to check for failure, pcim_iomap_regions() does it for us. */
> 
> IMHO this comment could be omitted.

I somehow overlooked the discussion in the v2.  Please ignore my
previous mail.

> >  	dwc->base = pcim_iomap_table(pci)[0];
> > -	if (!dwc->base) {
> > -		dev_err(dev, "Base address missing\n");
> > -		return -ENOMEM;
> > -	}
> >  
> >  	ret = devm_pwmchip_add(dev, chip);
> >  	if (ret)
> > -- 
> > 2.35.3
> > 
> > 
> 
> Best regards
> Thorsten

Best regards
Thorsten

Re: [PATCH v4 1/4] pwm: dwc: Fix PM regression

[Raag Jadav]

On Mon, Feb 19, 2024 at 08:22:15AM +0100, Uwe Kleine-König wrote:
> Hello Raag,
> 
> On Mon, Feb 19, 2024 at 09:08:32AM +0530, Raag Jadav wrote:
> > While preparing dwc driver for devm_pwmchip_alloc() usage, commit
> > df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
> > modified ->suspend() handle to use the pwm_chip as driver_data for
> > accessing struct dwc_pwm, but didn't modify ->resume() handle with
> > relevant changes. This results into illegal memory access during
> > device wakeup and causes a PM regression.
> > 
> > Fix this by correctly accessing struct dwc_pwm in ->resume() handle.
> > 
> > Fixes: df41cd8bbcad ("pwm: dwc: Prepare removing pwm_chip from driver data")
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  drivers/pwm/pwm-dwc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> > index 8ca1c20a6aaf..c0e586688e57 100644
> > --- a/drivers/pwm/pwm-dwc.c
> > +++ b/drivers/pwm/pwm-dwc.c
> > @@ -95,7 +95,8 @@ static int dwc_pwm_suspend(struct device *dev)
> >  
> >  static int dwc_pwm_resume(struct device *dev)
> >  {
> > -	struct dwc_pwm *dwc = dev_get_drvdata(dev);
> > +	struct pwm_chip *chip = dev_get_drvdata(dev);
> > +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> >  	int i;
> >  
> >  	for (i = 0; i < DWC_TIMERS_TOTAL; i++) {
> 
> If you're ok I'd squash this into df41cd8bbcad adding
> 
> 	Thanks to Raag Jadav for providing a hunk of this patch that Uwe
> 	missed during creation of this patch.
> 
> to the commit log.

Works for me.

Raag

Re: [PATCH v4 2/4] pwm: dwc: drop redundant error check

[Raag Jadav]

On Mon, Feb 19, 2024 at 08:27:43AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 19, 2024 at 08:11:00AM +0100, Thorsten Scherer wrote:
> > On Mon, Feb 19, 2024 at 09:08:33AM +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>
> > > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > ---
> > >  drivers/pwm/pwm-dwc.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> > > index c0e586688e57..7dbb72c80ef5 100644
> > > --- a/drivers/pwm/pwm-dwc.c
> > > +++ b/drivers/pwm/pwm-dwc.c
> > > @@ -51,11 +51,8 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
> > >  		return ret;
> > >  	}
> > >  
> > > +	/* No need to check for failure, pcim_iomap_regions() does it for us. */
> > 
> > IMHO this comment could be omitted.
> 
> I like the comment (and even asked for it). Is it really only me who
> doesn't know that after pcim_iomap_regions() calling pcim_iomap_table()
> is unproblematic?

Neither did I :) (Check the v1 discussion)

Raag

Re: [PATCH v4 0/4] DesignWare PWM improvements

[Uwe Kleine-König]

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

Hello,

On Mon, Feb 19, 2024 at 09:08:31AM +0530, Raag Jadav wrote:
> This series implements 16 channel PWM support for Intel Elkhart Lake
> along with minor cleanups for DesignWare PWM driver.
> 
> Changes since v3:
> - Fix PM regression
> - Add comment for removed error check
> - Drop accepted patches
> - Update tags
> 
> 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 (4):
>   pwm: dwc: Fix PM regression
>   pwm: dwc: drop redundant error check
>   pwm: dwc: Add 16 channel support for Intel Elkhart Lake
>   pwm: dwc: simplify error handling

I squashed patch #1 into the offending commit and applied patches #2 - #4 now to

https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next

Thanks
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 --]