| Message ID | 20240219033835.11369-3-raag.jadav@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | DesignWare PWM improvements | expand |
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
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
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
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
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)