* [PATCH v1 0/3] DesignWare PWM improvements
@ 2024-01-22 3:02 Raag Jadav
2024-01-22 3:02 ` [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake Raag Jadav
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Raag Jadav @ 2024-01-22 3:02 UTC (permalink / raw)
To: u.kleine-koenig, jarkko.nikula, mika.westerberg,
andriy.shevchenko, lakshmi.sowjanya.d
Cc: linux-pwm, linux-kernel, Raag Jadav
This series adds 16 channel support for Intel Elkhart Lake and simplifies
code using standard helpers in DesignWare PWM driver.
Raag Jadav (3):
pwm: dwc: Add 16 channel support for Intel Elkhart Lake
pwm: dwc: simplify error handling
pwm: dwc: use to_pci_dev() helper
drivers/pwm/pwm-dwc.c | 71 +++++++++++++++++++++++++++++--------------
drivers/pwm/pwm-dwc.h | 5 +++
2 files changed, 54 insertions(+), 22 deletions(-)
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
--
2.35.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-01-22 3:02 [PATCH v1 0/3] DesignWare PWM improvements Raag Jadav
@ 2024-01-22 3:02 ` Raag Jadav
2024-01-28 14:53 ` Andy Shevchenko
2024-01-22 3:02 ` [PATCH v1 2/3] pwm: dwc: simplify error handling Raag Jadav
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-01-22 3:02 UTC (permalink / raw)
To: u.kleine-koenig, jarkko.nikula, mika.westerberg,
andriy.shevchenko, lakshmi.sowjanya.d
Cc: linux-pwm, linux-kernel, 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>
---
drivers/pwm/pwm-dwc.c | 51 +++++++++++++++++++++++++++++++++++--------
drivers/pwm/pwm-dwc.h | 5 +++++
2 files changed, 47 insertions(+), 9 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 4929354f8cd9..fd64313cb38d 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -25,16 +25,48 @@
#include "pwm-dwc.h"
+/* Elkhart Lake */
+static const struct dwc_pwm_info ehl_pwm_info = {
+ .nr = 2,
+ .size = 0x1000,
+};
+
+static int dwc_pwm_init(struct device *dev, const struct dwc_pwm_info *info, void __iomem *base)
+{
+ /* Default values for single instance devices */
+ unsigned int nr = 1, size = 0;
+ int i, ret;
+
+ /* Fill up values from driver_data, if any */
+ if (info) {
+ nr = info->nr;
+ size = info->size;
+ }
+
+ for (i = 0; i < nr; i++) {
+ struct dwc_pwm *dwc;
+
+ dwc = dwc_pwm_alloc(dev);
+ if (!dwc)
+ return -ENOMEM;
+
+ dwc->base = base + (i * size);
+
+ ret = devm_pwmchip_add(dev, &dwc->chip);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
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;
- struct dwc_pwm *dwc;
+ void __iomem *base;
int ret;
- dwc = dwc_pwm_alloc(dev);
- if (!dwc)
- return -ENOMEM;
-
ret = pcim_enable_device(pci);
if (ret) {
dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
@@ -49,13 +81,14 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
return ret;
}
- dwc->base = pcim_iomap_table(pci)[0];
- if (!dwc->base) {
+ base = pcim_iomap_table(pci)[0];
+ if (!base) {
dev_err(dev, "Base address missing\n");
return -ENOMEM;
}
- ret = devm_pwmchip_add(dev, &dwc->chip);
+ info = (const struct dwc_pwm_info *)id->driver_data;
+ ret = dwc_pwm_init(dev, info, base);
if (ret)
return ret;
@@ -109,7 +142,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-22 3:02 [PATCH v1 0/3] DesignWare PWM improvements Raag Jadav
2024-01-22 3:02 ` [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake Raag Jadav
@ 2024-01-22 3:02 ` Raag Jadav
2024-01-28 14:48 ` Andy Shevchenko
2024-01-22 3:02 ` [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper Raag Jadav
2024-01-24 9:45 ` [PATCH v1 0/3] DesignWare PWM improvements Jarkko Nikula
3 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-01-22 3:02 UTC (permalink / raw)
To: u.kleine-koenig, jarkko.nikula, mika.westerberg,
andriy.shevchenko, lakshmi.sowjanya.d
Cc: linux-pwm, linux-kernel, Raag Jadav
Simplify error handling in ->probe() function using dev_err_probe() helper.
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
drivers/pwm/pwm-dwc.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index fd64313cb38d..8f8035b047c1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -68,24 +68,18 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
int 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 (%pe)\n", ERR_PTR(ret));
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 (%pe)\n", ERR_PTR(ret));
base = pcim_iomap_table(pci)[0];
- if (!base) {
- dev_err(dev, "Base address missing\n");
- return -ENOMEM;
- }
+ if (!base)
+ return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
info = (const struct dwc_pwm_info *)id->driver_data;
ret = dwc_pwm_init(dev, info, base);
--
2.35.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper
2024-01-22 3:02 [PATCH v1 0/3] DesignWare PWM improvements Raag Jadav
2024-01-22 3:02 ` [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake Raag Jadav
2024-01-22 3:02 ` [PATCH v1 2/3] pwm: dwc: simplify error handling Raag Jadav
@ 2024-01-22 3:02 ` Raag Jadav
2024-01-28 14:46 ` Andy Shevchenko
2024-01-24 9:45 ` [PATCH v1 0/3] DesignWare PWM improvements Jarkko Nikula
3 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-01-22 3:02 UTC (permalink / raw)
To: u.kleine-koenig, jarkko.nikula, mika.westerberg,
andriy.shevchenko, lakshmi.sowjanya.d
Cc: linux-pwm, linux-kernel, Raag Jadav
Use to_pci_dev() helper to get pci device reference.
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
drivers/pwm/pwm-dwc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 8f8035b047c1..f5fcea4e69e2 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -100,7 +100,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 pci_dev *pdev = to_pci_dev(dev);
struct dwc_pwm *dwc = pci_get_drvdata(pdev);
int i;
@@ -120,7 +120,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 pci_dev *pdev = to_pci_dev(dev);
struct dwc_pwm *dwc = pci_get_drvdata(pdev);
int i;
--
2.35.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v1 0/3] DesignWare PWM improvements
2024-01-22 3:02 [PATCH v1 0/3] DesignWare PWM improvements Raag Jadav
` (2 preceding siblings ...)
2024-01-22 3:02 ` [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper Raag Jadav
@ 2024-01-24 9:45 ` Jarkko Nikula
3 siblings, 0 replies; 19+ messages in thread
From: Jarkko Nikula @ 2024-01-24 9:45 UTC (permalink / raw)
To: Raag Jadav, u.kleine-koenig, mika.westerberg, andriy.shevchenko,
lakshmi.sowjanya.d
Cc: linux-pwm, linux-kernel
On 1/22/24 05:02, Raag Jadav wrote:
> This series adds 16 channel support for Intel Elkhart Lake and simplifies
> code using standard helpers in DesignWare PWM driver.
>
> Raag Jadav (3):
> pwm: dwc: Add 16 channel support for Intel Elkhart Lake
> pwm: dwc: simplify error handling
> pwm: dwc: use to_pci_dev() helper
>
> drivers/pwm/pwm-dwc.c | 71 +++++++++++++++++++++++++++++--------------
> drivers/pwm/pwm-dwc.h | 5 +++
> 2 files changed, 54 insertions(+), 22 deletions(-)
>
For the patchset:
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper
2024-01-22 3:02 ` [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper Raag Jadav
@ 2024-01-28 14:46 ` Andy Shevchenko
2024-01-28 16:55 ` Uwe Kleine-König
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-01-28 14:46 UTC (permalink / raw)
To: Raag Jadav
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Mon, Jan 22, 2024 at 08:32:38AM +0530, Raag Jadav wrote:
> Use to_pci_dev() helper to get pci device reference.
PCI
...
> static int dwc_pwm_suspend(struct device *dev)
> {
> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> + struct pci_dev *pdev = to_pci_dev(dev);
> struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> int i;
>
> @@ -120,7 +120,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 pci_dev *pdev = to_pci_dev(dev);
> struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> int i;
I don't see how pdev is being used. That said, why dev_get_drvdata() is not
suffice?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-22 3:02 ` [PATCH v1 2/3] pwm: dwc: simplify error handling Raag Jadav
@ 2024-01-28 14:48 ` Andy Shevchenko
2024-01-28 16:58 ` Uwe Kleine-König
2024-01-30 8:31 ` Raag Jadav
0 siblings, 2 replies; 19+ messages in thread
From: Andy Shevchenko @ 2024-01-28 14:48 UTC (permalink / raw)
To: Raag Jadav
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote:
> Simplify error handling in ->probe() function using dev_err_probe() helper.
...
> 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 (%pe)\n", ERR_PTR(ret));
>
> base = pcim_iomap_table(pci)[0];
> - if (!base) {
> - dev_err(dev, "Base address missing\n");
> - return -ENOMEM;
> - }
> + if (!base)
> + return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
This check is bogus. Just remove it completely.
The pcim_iomap_table() fails IFF pcim_iomap_regions() fails.
You have checked the latter already.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-01-22 3:02 ` [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake Raag Jadav
@ 2024-01-28 14:53 ` Andy Shevchenko
2024-01-30 10:30 ` Raag Jadav
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-01-28 14:53 UTC (permalink / raw)
To: Raag Jadav
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
> Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
> function with 8 channels each. Add support for the remaining channels.
...
> +static int dwc_pwm_init(struct device *dev, const struct dwc_pwm_info *info, void __iomem *base)
> +{
> + /* Default values for single instance devices */
> + unsigned int nr = 1, size = 0;
> + int i, ret;
> +
> + /* Fill up values from driver_data, if any */
> + if (info) {
> + nr = info->nr;
> + size = info->size;
> + }
> +
> + for (i = 0; i < nr; i++) {
> + struct dwc_pwm *dwc;
> +
> + dwc = dwc_pwm_alloc(dev);
> + if (!dwc)
> + return -ENOMEM;
> +
> + dwc->base = base + (i * size);
> +
> + ret = devm_pwmchip_add(dev, &dwc->chip);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Why not doing this slightly differently?
First option: Always provide driver data (info is never NULL).
Second option, have the body of the for-loop be factored to a helper
dwc_pwm_init_one() and here
if (!info)
return dwc_pwm_init_one(..., 1, 0);
for (i = 0; i < info->nr; i++) {
ret = dwc_pwm_init_one(...);
if (ret)
return ret;
}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper
2024-01-28 14:46 ` Andy Shevchenko
@ 2024-01-28 16:55 ` Uwe Kleine-König
2024-01-31 6:54 ` Raag Jadav
0 siblings, 1 reply; 19+ messages in thread
From: Uwe Kleine-König @ 2024-01-28 16:55 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Raag Jadav, jarkko.nikula, mika.westerberg, lakshmi.sowjanya.d,
linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]
Hello,
On Sun, Jan 28, 2024 at 04:46:48PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2024 at 08:32:38AM +0530, Raag Jadav wrote:
> > Use to_pci_dev() helper to get pci device reference.
>
> PCI
>
> ...
>
> > static int dwc_pwm_suspend(struct device *dev)
> > {
> > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> > int i;
> >
> > @@ -120,7 +120,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 pci_dev *pdev = to_pci_dev(dev);
> > struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> > int i;
>
> I don't see how pdev is being used. That said, why dev_get_drvdata() is not
> suffice?
I would even consider using dev_get_drvdata() a nice cleanup given that
pci_get_drvdata() works because dwc_pwm_alloc() called dev_set_drvdata()
(and not pci_set_drvdata()).
Not so long ago (i.e. before commit a357d1493f0c ("pwm: dwc: Move memory
allocation to own function")) the dwc driver was pci only and used
pci_set_drvdata(). Then the upside of first converting the struct device
to a struct pci_dev was not to hard code knowledge about the
implementation of pci_[gs]et_drvdata().
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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-28 14:48 ` Andy Shevchenko
@ 2024-01-28 16:58 ` Uwe Kleine-König
2024-02-01 11:22 ` Andy Shevchenko
2024-01-30 8:31 ` Raag Jadav
1 sibling, 1 reply; 19+ messages in thread
From: Uwe Kleine-König @ 2024-01-28 16:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Raag Jadav, jarkko.nikula, mika.westerberg, lakshmi.sowjanya.d,
linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]
Hello,
On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote:
> > Simplify error handling in ->probe() function using dev_err_probe() helper.
>
> ...
>
> > 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 (%pe)\n", ERR_PTR(ret));
> >
> > base = pcim_iomap_table(pci)[0];
>
> > - if (!base) {
> > - dev_err(dev, "Base address missing\n");
> > - return -ENOMEM;
> > - }
> > + if (!base)
> > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
>
> This check is bogus. Just remove it completely.
This would be a separate patch though. IMHO mechanically converting to
dev_err_probe() is fine.
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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-28 14:48 ` Andy Shevchenko
2024-01-28 16:58 ` Uwe Kleine-König
@ 2024-01-30 8:31 ` Raag Jadav
2024-02-01 11:24 ` Andy Shevchenko
1 sibling, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-01-30 8:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote:
> > Simplify error handling in ->probe() function using dev_err_probe() helper.
>
> ...
>
> > 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 (%pe)\n", ERR_PTR(ret));
> >
> > base = pcim_iomap_table(pci)[0];
>
> > - if (!base) {
> > - dev_err(dev, "Base address missing\n");
> > - return -ENOMEM;
> > - }
> > + if (!base)
> > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
>
> This check is bogus. Just remove it completely.
>
> The pcim_iomap_table() fails IFF pcim_iomap_regions() fails.
> You have checked the latter already.
I'm no expert on devres but I found a few NULL returns in alloc_dr()
call path. In the interest of learning more about iomap, wouldn't we
need to handle them (just in some odd case)?
Raag
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-01-28 14:53 ` Andy Shevchenko
@ 2024-01-30 10:30 ` Raag Jadav
2024-02-01 11:26 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-01-30 10:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Sun, Jan 28, 2024 at 04:53:24PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
> > Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
> > function with 8 channels each. Add support for the remaining channels.
>
> ...
>
> > +static int dwc_pwm_init(struct device *dev, const struct dwc_pwm_info *info, void __iomem *base)
> > +{
> > + /* Default values for single instance devices */
> > + unsigned int nr = 1, size = 0;
> > + int i, ret;
> > +
> > + /* Fill up values from driver_data, if any */
> > + if (info) {
> > + nr = info->nr;
> > + size = info->size;
> > + }
> > +
> > + for (i = 0; i < nr; i++) {
> > + struct dwc_pwm *dwc;
> > +
> > + dwc = dwc_pwm_alloc(dev);
> > + if (!dwc)
> > + return -ENOMEM;
> > +
> > + dwc->base = base + (i * size);
> > +
> > + ret = devm_pwmchip_add(dev, &dwc->chip);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> Why not doing this slightly differently?
>
> First option: Always provide driver data (info is never NULL).
Allowing empty driver_data would save us from adding dummy info
for single instance devices in the future.
> Second option, have the body of the for-loop be factored to a helper
> dwc_pwm_init_one() and here
>
> if (!info)
> return dwc_pwm_init_one(..., 1, 0);
>
> for (i = 0; i < info->nr; i++) {
> ret = dwc_pwm_init_one(...);
> if (ret)
> return ret;
> }
Considering above, we're looking at something like this.
static int dwc_pwm_init_one(struct device *dev, void __iomem *base, unsigned int size)
{
struct dwc_pwm *dwc;
dwc = dwc_pwm_alloc(dev);
if (!dwc)
return -ENOMEM;
dwc->base = base + size;
return devm_pwmchip_add(dev, &dwc->chip);
}
...
if (info) {
for (i = 0; i < info->nr; i++) {
ret = dwc_pwm_init_one(dev, base, i * info->size);
if (ret)
return ret;
}
} else {
ret = dwc_pwm_init_one(dev, base, 0);
if (ret)
return ret;
}
...
Raag
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper
2024-01-28 16:55 ` Uwe Kleine-König
@ 2024-01-31 6:54 ` Raag Jadav
0 siblings, 0 replies; 19+ messages in thread
From: Raag Jadav @ 2024-01-31 6:54 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andy Shevchenko, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Sun, Jan 28, 2024 at 05:55:00PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Sun, Jan 28, 2024 at 04:46:48PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 22, 2024 at 08:32:38AM +0530, Raag Jadav wrote:
> > > Use to_pci_dev() helper to get pci device reference.
> >
> > PCI
> >
> > ...
> >
> > > static int dwc_pwm_suspend(struct device *dev)
> > > {
> > > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > > + struct pci_dev *pdev = to_pci_dev(dev);
> > > struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> > > int i;
> > >
> > > @@ -120,7 +120,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 pci_dev *pdev = to_pci_dev(dev);
> > > struct dwc_pwm *dwc = pci_get_drvdata(pdev);
> > > int i;
> >
> > I don't see how pdev is being used. That said, why dev_get_drvdata() is not
> > suffice?
>
> I would even consider using dev_get_drvdata() a nice cleanup given that
> pci_get_drvdata() works because dwc_pwm_alloc() called dev_set_drvdata()
> (and not pci_set_drvdata()).
Makes sense. Will update in v2.
Raag
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-28 16:58 ` Uwe Kleine-König
@ 2024-02-01 11:22 ` Andy Shevchenko
0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2024-02-01 11:22 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Raag Jadav, jarkko.nikula, mika.westerberg, lakshmi.sowjanya.d,
linux-pwm, linux-kernel
On Sun, Jan 28, 2024 at 05:58:38PM +0100, Uwe Kleine-König wrote:
> On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote:
> > > Simplify error handling in ->probe() function using dev_err_probe() helper.
...
> > > 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 (%pe)\n", ERR_PTR(ret));
> > >
> > > base = pcim_iomap_table(pci)[0];
> >
> > > - if (!base) {
> > > - dev_err(dev, "Base address missing\n");
> > > - return -ENOMEM;
> > > - }
> > > + if (!base)
> > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
> >
> > This check is bogus. Just remove it completely.
>
> This would be a separate patch though. IMHO mechanically converting to
> dev_err_probe() is fine.
Sure, that's what I meant. First patch to remove, followed by dev_err_probe()
conversion.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/3] pwm: dwc: simplify error handling
2024-01-30 8:31 ` Raag Jadav
@ 2024-02-01 11:24 ` Andy Shevchenko
0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2024-02-01 11:24 UTC (permalink / raw)
To: Raag Jadav
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Tue, Jan 30, 2024 at 10:31:02AM +0200, Raag Jadav wrote:
> On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote:
> > > Simplify error handling in ->probe() function using dev_err_probe() helper.
...
> > > 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 (%pe)\n", ERR_PTR(ret));
> > >
> > > base = pcim_iomap_table(pci)[0];
> >
> > > - if (!base) {
> > > - dev_err(dev, "Base address missing\n");
> > > - return -ENOMEM;
> > > - }
> > > + if (!base)
> > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n");
> >
> > This check is bogus. Just remove it completely.
> >
> > The pcim_iomap_table() fails IFF pcim_iomap_regions() fails.
> > You have checked the latter already.
>
> I'm no expert on devres but I found a few NULL returns in alloc_dr()
> call path. In the interest of learning more about iomap, wouldn't we
> need to handle them (just in some odd case)?
It has nothing to do with devres. The logic is implemented in PCI core.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-01-30 10:30 ` Raag Jadav
@ 2024-02-01 11:26 ` Andy Shevchenko
2024-02-02 4:02 ` Raag Jadav
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2024-02-01 11:26 UTC (permalink / raw)
To: Raag Jadav
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Tue, Jan 30, 2024 at 12:30:23PM +0200, Raag Jadav wrote:
> On Sun, Jan 28, 2024 at 04:53:24PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
> > > Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
> > > function with 8 channels each. Add support for the remaining channels.
...
> > First option: Always provide driver data (info is never NULL).
>
> Allowing empty driver_data would save us from adding dummy info
> for single instance devices in the future.
Which may be too premature "optimisation". Why? Because if we ever have
something like pci_dev_get_match_data(), the empty will mean NULL, and
we may not get difference between empty and missing one.
> > Second option, have the body of the for-loop be factored to a helper
> > dwc_pwm_init_one() and here
> >
> > if (!info)
> > return dwc_pwm_init_one(..., 1, 0);
> >
> > for (i = 0; i < info->nr; i++) {
> > ret = dwc_pwm_init_one(...);
> > if (ret)
> > return ret;
> > }
>
> Considering above, we're looking at something like this.
As one option, yes.
> static int dwc_pwm_init_one(struct device *dev, void __iomem *base, unsigned int size)
> {
> struct dwc_pwm *dwc;
>
> dwc = dwc_pwm_alloc(dev);
> if (!dwc)
> return -ENOMEM;
>
> dwc->base = base + size;
>
> return devm_pwmchip_add(dev, &dwc->chip);
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-02-01 11:26 ` Andy Shevchenko
@ 2024-02-02 4:02 ` Raag Jadav
2024-02-07 11:48 ` Raag Jadav
0 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-02-02 4:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Thu, Feb 01, 2024 at 01:26:53PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 30, 2024 at 12:30:23PM +0200, Raag Jadav wrote:
> > On Sun, Jan 28, 2024 at 04:53:24PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
> > > > Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
> > > > function with 8 channels each. Add support for the remaining channels.
>
> ...
>
> > > First option: Always provide driver data (info is never NULL).
> >
> > Allowing empty driver_data would save us from adding dummy info
> > for single instance devices in the future.
>
> Which may be too premature "optimisation". Why? Because if we ever have
> something like pci_dev_get_match_data(), the empty will mean NULL, and
> we may not get difference between empty and missing one.
Not sure if I'm able to find such a helper as of now, but fair.
I can change it in v2 if Jarkko is okay with it.
Raag
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-02-02 4:02 ` Raag Jadav
@ 2024-02-07 11:48 ` Raag Jadav
2024-02-07 13:11 ` Jarkko Nikula
0 siblings, 1 reply; 19+ messages in thread
From: Raag Jadav @ 2024-02-07 11:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: u.kleine-koenig, jarkko.nikula, mika.westerberg,
lakshmi.sowjanya.d, linux-pwm, linux-kernel
On Fri, Feb 02, 2024 at 06:02:46AM +0200, Raag Jadav wrote:
> On Thu, Feb 01, 2024 at 01:26:53PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 30, 2024 at 12:30:23PM +0200, Raag Jadav wrote:
> > > On Sun, Jan 28, 2024 at 04:53:24PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
> > > > > Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
> > > > > function with 8 channels each. Add support for the remaining channels.
> >
> > ...
> >
> > > > First option: Always provide driver data (info is never NULL).
> > >
> > > Allowing empty driver_data would save us from adding dummy info
> > > for single instance devices in the future.
> >
> > Which may be too premature "optimisation". Why? Because if we ever have
> > something like pci_dev_get_match_data(), the empty will mean NULL, and
> > we may not get difference between empty and missing one.
>
> Not sure if I'm able to find such a helper as of now, but fair.
> I can change it in v2 if Jarkko is okay with it.
Hi Jarkko,
If you agree with Andy's comments, please let me know.
Will send out a v2 accordingly.
Raag
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake
2024-02-07 11:48 ` Raag Jadav
@ 2024-02-07 13:11 ` Jarkko Nikula
0 siblings, 0 replies; 19+ messages in thread
From: Jarkko Nikula @ 2024-02-07 13:11 UTC (permalink / raw)
To: Raag Jadav, Andy Shevchenko
Cc: u.kleine-koenig, mika.westerberg, lakshmi.sowjanya.d, linux-pwm,
linux-kernel
On 2/7/24 13:48, Raag Jadav wrote:
> On Fri, Feb 02, 2024 at 06:02:46AM +0200, Raag Jadav wrote:
>> On Thu, Feb 01, 2024 at 01:26:53PM +0200, Andy Shevchenko wrote:
>>> On Tue, Jan 30, 2024 at 12:30:23PM +0200, Raag Jadav wrote:
>>>> On Sun, Jan 28, 2024 at 04:53:24PM +0200, Andy Shevchenko wrote:
>>>>> On Mon, Jan 22, 2024 at 08:32:36AM +0530, Raag Jadav wrote:
>>>>>> Intel Elkhart Lake PSE includes two instances of PWM as a single PCI
>>>>>> function with 8 channels each. Add support for the remaining channels.
>>>
>>> ...
>>>
>>>>> First option: Always provide driver data (info is never NULL).
>>>>
>>>> Allowing empty driver_data would save us from adding dummy info
>>>> for single instance devices in the future.
>>>
>>> Which may be too premature "optimisation". Why? Because if we ever have
>>> something like pci_dev_get_match_data(), the empty will mean NULL, and
>>> we may not get difference between empty and missing one.
>>
>> Not sure if I'm able to find such a helper as of now, but fair.
>> I can change it in v2 if Jarkko is okay with it.
>
> Hi Jarkko,
>
> If you agree with Andy's comments, please let me know.
> Will send out a v2 accordingly.
>
Ah, sorry, I didn't have opinion.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-02-07 13:11 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-22 3:02 [PATCH v1 0/3] DesignWare PWM improvements Raag Jadav
2024-01-22 3:02 ` [PATCH v1 1/3] pwm: dwc: Add 16 channel support for Intel Elkhart Lake Raag Jadav
2024-01-28 14:53 ` Andy Shevchenko
2024-01-30 10:30 ` Raag Jadav
2024-02-01 11:26 ` Andy Shevchenko
2024-02-02 4:02 ` Raag Jadav
2024-02-07 11:48 ` Raag Jadav
2024-02-07 13:11 ` Jarkko Nikula
2024-01-22 3:02 ` [PATCH v1 2/3] pwm: dwc: simplify error handling Raag Jadav
2024-01-28 14:48 ` Andy Shevchenko
2024-01-28 16:58 ` Uwe Kleine-König
2024-02-01 11:22 ` Andy Shevchenko
2024-01-30 8:31 ` Raag Jadav
2024-02-01 11:24 ` Andy Shevchenko
2024-01-22 3:02 ` [PATCH v1 3/3] pwm: dwc: use to_pci_dev() helper Raag Jadav
2024-01-28 14:46 ` Andy Shevchenko
2024-01-28 16:55 ` Uwe Kleine-König
2024-01-31 6:54 ` Raag Jadav
2024-01-24 9:45 ` [PATCH v1 0/3] DesignWare PWM improvements Jarkko Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).