| Message ID | 20210317155925.297680-1-jarkko.nikula@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: dwc: Use dev_get_drvdata() directly in PM callbacks | expand |
Hello Jarkko, On Wed, Mar 17, 2021 at 05:59:25PM +0200, Jarkko Nikula wrote: > Instead of figuring out struct pci_dev pointer from device pointer and > then pci_get_drvdata() we can use directly dev_get_drvdata() to get the > pointer to struct dwc_pwm. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@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 f6c98e0d57c2..4d59a035c0c9 100644 > --- a/drivers/pwm/pwm-dwc.c > +++ b/drivers/pwm/pwm-dwc.c > @@ -258,8 +258,7 @@ static void dwc_pwm_remove(struct pci_dev *pci) > #ifdef CONFIG_PM_SLEEP > 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; I'm a bit ambivalent here. I'd consider it an implementation detail of the PCI framework that pci_get_drvdata is dev_get_drvdata on the related struct device. So even though the PCI guys probably will never change that, it feels a bit like a layer violation to rely on this behaviour. As additionally the status quo isn't less effective (unless I miss something) than the alternative proposed in your patch, I tend to not like your change. Best regards Uwe
On 3/17/21 10:09 PM, Uwe Kleine-König wrote: > Hello Jarkko, > > On Wed, Mar 17, 2021 at 05:59:25PM +0200, Jarkko Nikula wrote: >> Instead of figuring out struct pci_dev pointer from device pointer and >> then pci_get_drvdata() we can use directly dev_get_drvdata() to get the >> pointer to struct dwc_pwm. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@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 f6c98e0d57c2..4d59a035c0c9 100644 >> --- a/drivers/pwm/pwm-dwc.c >> +++ b/drivers/pwm/pwm-dwc.c >> @@ -258,8 +258,7 @@ static void dwc_pwm_remove(struct pci_dev *pci) >> #ifdef CONFIG_PM_SLEEP >> 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; > > I'm a bit ambivalent here. I'd consider it an implementation detail of > the PCI framework that pci_get_drvdata is dev_get_drvdata on the related > struct device. So even though the PCI guys probably will never change > that, it feels a bit like a layer violation to rely on this behaviour. > > As additionally the status quo isn't less effective (unless I miss > something) than the alternative proposed in your patch, I tend to not > like your change. > Yeah, agree, it is a bit confusing to see mix of pci_set_drvdata() and dev_get_drvdata(). Got the idea for this patch from another driver and similar commits. Better would be to switch entirely to dev_set_drvdata() in probe and dev_get_drvdata() in all other functions. Perhaps not worth of effort (does here 4 insertions(+), 6 deletions(-)) and not many PCI drivers seem to use dev_set_drvdata() in their probe (additional confusion?). Jarkko
Hello, On Thu, Mar 18, 2021 at 10:06:37AM +0200, Jarkko Nikula wrote: > On 3/17/21 10:09 PM, Uwe Kleine-König wrote: > > On Wed, Mar 17, 2021 at 05:59:25PM +0200, Jarkko Nikula wrote: > > > - 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; > > > > I'm a bit ambivalent here. I'd consider it an implementation detail of > > the PCI framework that pci_get_drvdata is dev_get_drvdata on the related > > struct device. So even though the PCI guys probably will never change > > that, it feels a bit like a layer violation to rely on this behaviour. > > > > As additionally the status quo isn't less effective (unless I miss > > something) than the alternative proposed in your patch, I tend to not > > like your change. > > > Yeah, agree, it is a bit confusing to see mix of pci_set_drvdata() and > dev_get_drvdata(). Got the idea for this patch from another driver and > similar commits. > > Better would be to switch entirely to dev_set_drvdata() in probe and > dev_get_drvdata() in all other functions. Perhaps not worth of effort (does > here 4 insertions(+), 6 deletions(-)) and not many PCI drivers seem to use > dev_set_drvdata() in their probe (additional confusion?). ok, I'll discard this patch in our patchwork. Best regards Uwe
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c index f6c98e0d57c2..4d59a035c0c9 100644 --- a/drivers/pwm/pwm-dwc.c +++ b/drivers/pwm/pwm-dwc.c @@ -258,8 +258,7 @@ static void dwc_pwm_remove(struct pci_dev *pci) #ifdef CONFIG_PM_SLEEP 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++) { @@ -278,8 +277,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++) {
Instead of figuring out struct pci_dev pointer from device pointer and then pci_get_drvdata() we can use directly dev_get_drvdata() to get the pointer to struct dwc_pwm. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/pwm/pwm-dwc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)