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
Simplify error handling in ->probe() function using dev_err_probe() helper.
Signed-off-by: Raag Jadav <[email protected]>
---
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
Use to_pci_dev() helper to get pci device reference.
Signed-off-by: Raag Jadav <[email protected]>
---
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
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 <[email protected]>
Tested-by: Jarkko Nikula <[email protected]>
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
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
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/ |
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/ |
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
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
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
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