All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused
@ 2021-06-11 11:55 Jarkko Nikula
  2021-06-11 11:55 ` [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper Jarkko Nikula
  2021-06-11 17:23 ` [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jonathan Cameron
  0 siblings, 2 replies; 11+ messages in thread
From: Jarkko Nikula @ 2021-06-11 11:55 UTC (permalink / raw)
  To: linux-iio; +Cc: William Breathitt Gray, Jarkko Nikula, Jonathan Cameron

Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/counter/intel-qep.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index ab10ba33f46a..a8d3dccecc0f 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
 	intel_qep_writel(qep, INTEL_QEPCON, 0);
 }
 
-#ifdef CONFIG_PM
-static int intel_qep_suspend(struct device *dev)
+static int __maybe_unused intel_qep_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
 	struct intel_qep *qep = pci_get_drvdata(pdev);
@@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
 	return 0;
 }
 
-static int intel_qep_resume(struct device *dev)
+static int __maybe_unused intel_qep_resume(struct device *dev)
 {
 	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
 	struct intel_qep *qep = pci_get_drvdata(pdev);
@@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
 			    intel_qep_suspend, intel_qep_resume, NULL);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-11 11:55 [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jarkko Nikula
@ 2021-06-11 11:55 ` Jarkko Nikula
  2021-06-13 10:36   ` Andy Shevchenko
  2021-06-11 17:23 ` [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jonathan Cameron
  1 sibling, 1 reply; 11+ messages in thread
From: Jarkko Nikula @ 2021-06-11 11:55 UTC (permalink / raw)
  To: linux-iio; +Cc: William Breathitt Gray, Jarkko Nikula, Jonathan Cameron

Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/counter/intel-qep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index a8d3dccecc0f..8d7ae28fbd67 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -475,7 +475,7 @@ static void intel_qep_remove(struct pci_dev *pci)
 
 static int __maybe_unused intel_qep_suspend(struct device *dev)
 {
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct intel_qep *qep = pci_get_drvdata(pdev);
 
 	qep->qepcon = intel_qep_readl(qep, INTEL_QEPCON);
@@ -487,7 +487,7 @@ static int __maybe_unused intel_qep_suspend(struct device *dev)
 
 static int __maybe_unused intel_qep_resume(struct device *dev)
 {
-	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct intel_qep *qep = pci_get_drvdata(pdev);
 
 	/*
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused
  2021-06-11 11:55 [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jarkko Nikula
  2021-06-11 11:55 ` [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper Jarkko Nikula
@ 2021-06-11 17:23 ` Jonathan Cameron
  2021-06-12  0:45   ` William Breathitt Gray
  1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2021-06-11 17:23 UTC (permalink / raw)
  To: Jarkko Nikula; +Cc: linux-iio, William Breathitt Gray

On Fri, 11 Jun 2021 14:55:57 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Both applied to the togreg branch of iio.git and pushed out
as testing to let 0-day poke at them.

William, if you want to give feedback on these, still time for
me to add tags etc. They just seem trivial enough its not worth
wasting your time :)

Jonathan


> ---
>  drivers/counter/intel-qep.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> index ab10ba33f46a..a8d3dccecc0f 100644
> --- a/drivers/counter/intel-qep.c
> +++ b/drivers/counter/intel-qep.c
> @@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
>  	intel_qep_writel(qep, INTEL_QEPCON, 0);
>  }
>  
> -#ifdef CONFIG_PM
> -static int intel_qep_suspend(struct device *dev)
> +static int __maybe_unused intel_qep_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>  	struct intel_qep *qep = pci_get_drvdata(pdev);
> @@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int intel_qep_resume(struct device *dev)
> +static int __maybe_unused intel_qep_resume(struct device *dev)
>  {
>  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>  	struct intel_qep *qep = pci_get_drvdata(pdev);
> @@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
>  			    intel_qep_suspend, intel_qep_resume, NULL);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused
  2021-06-11 17:23 ` [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jonathan Cameron
@ 2021-06-12  0:45   ` William Breathitt Gray
  0 siblings, 0 replies; 11+ messages in thread
From: William Breathitt Gray @ 2021-06-12  0:45 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: Jarkko Nikula, linux-iio

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

On Fri, Jun 11, 2021 at 06:23:34PM +0100, Jonathan Cameron wrote:
> On Fri, 11 Jun 2021 14:55:57 +0300
> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> 
> > Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Both applied to the togreg branch of iio.git and pushed out
> as testing to let 0-day poke at them.
> 
> William, if you want to give feedback on these, still time for
> me to add tags etc. They just seem trivial enough its not worth
> wasting your time :)
> 
> Jonathan

Yes, these are pretty trivial and both look good to me. Feel free to add
my Ack to both patches if there's still time.

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> > ---
> >  drivers/counter/intel-qep.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> > index ab10ba33f46a..a8d3dccecc0f 100644
> > --- a/drivers/counter/intel-qep.c
> > +++ b/drivers/counter/intel-qep.c
> > @@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
> >  	intel_qep_writel(qep, INTEL_QEPCON, 0);
> >  }
> >  
> > -#ifdef CONFIG_PM
> > -static int intel_qep_suspend(struct device *dev)
> > +static int __maybe_unused intel_qep_suspend(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> >  	struct intel_qep *qep = pci_get_drvdata(pdev);
> > @@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static int intel_qep_resume(struct device *dev)
> > +static int __maybe_unused intel_qep_resume(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> >  	struct intel_qep *qep = pci_get_drvdata(pdev);
> > @@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > -#endif
> >  
> >  static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
> >  			    intel_qep_suspend, intel_qep_resume, NULL);
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-11 11:55 ` [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper Jarkko Nikula
@ 2021-06-13 10:36   ` Andy Shevchenko
  2021-06-13 11:08     ` Jonathan Cameron
  2021-06-14  8:24     ` Jarkko Nikula
  0 siblings, 2 replies; 11+ messages in thread
From: Andy Shevchenko @ 2021-06-13 10:36 UTC (permalink / raw)
  To: Jarkko Nikula; +Cc: linux-iio, William Breathitt Gray, Jonathan Cameron

On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);

...

> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +       struct pci_dev *pdev = to_pci_dev(dev);
>         struct intel_qep *qep = pci_get_drvdata(pdev);

Why not change both lines to dev_get_drvdata()?

> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +       struct pci_dev *pdev = to_pci_dev(dev);
>         struct intel_qep *qep = pci_get_drvdata(pdev);

Ditto

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-13 10:36   ` Andy Shevchenko
@ 2021-06-13 11:08     ` Jonathan Cameron
  2021-06-14  8:24     ` Jarkko Nikula
  1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2021-06-13 11:08 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Jarkko Nikula, linux-iio, William Breathitt Gray

On Sun, 13 Jun 2021 13:36:16 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
> >
> > Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);  
> 
> ...
> 
> > -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> >         struct intel_qep *qep = pci_get_drvdata(pdev);  
> 
> Why not change both lines to dev_get_drvdata()?
> 
> > -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> >         struct intel_qep *qep = pci_get_drvdata(pdev);  
> 
> Ditto
> 
Question when doing this is whether it is better to pair pci_get_drvdata
with pci_set_drvdata rather than assuming it will always map to dev_get_drvdata().

I personally don't feel too strongly about this either way, but I know
others are unhappy with mixing them.

Of course, could use dev_set_drvdata() in the first place then it would be less
of an issue.

Jonathan


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-13 10:36   ` Andy Shevchenko
  2021-06-13 11:08     ` Jonathan Cameron
@ 2021-06-14  8:24     ` Jarkko Nikula
  2021-06-14  8:57       ` Andy Shevchenko
  1 sibling, 1 reply; 11+ messages in thread
From: Jarkko Nikula @ 2021-06-14  8:24 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-iio, William Breathitt Gray, Jonathan Cameron

On 6/13/21 1:36 PM, Andy Shevchenko wrote:
> On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
>>
>> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);
> 
> ...
> 
>> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>> +       struct pci_dev *pdev = to_pci_dev(dev);
>>          struct intel_qep *qep = pci_get_drvdata(pdev);
> 
> Why not change both lines to dev_get_drvdata()?
> 
I thought it before and Uwe had a good point why it isn't necessarily a 
good idea:

https://www.spinics.net/lists/linux-pwm/msg15325.html

Jarkko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-14  8:24     ` Jarkko Nikula
@ 2021-06-14  8:57       ` Andy Shevchenko
  2021-06-14  9:37         ` Uwe Kleine-König
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2021-06-14  8:57 UTC (permalink / raw)
  To: Jarkko Nikula, Uwe Kleine-König
  Cc: linux-iio, William Breathitt Gray, Jonathan Cameron

+Uwe Kleine-König

On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> On 6/13/21 1:36 PM, Andy Shevchenko wrote:
> > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> > <jarkko.nikula@linux.intel.com> wrote:
> >>
> >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);
> >
> > ...
> >
> >> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> >> +       struct pci_dev *pdev = to_pci_dev(dev);
> >>          struct intel_qep *qep = pci_get_drvdata(pdev);
> >
> > Why not change both lines to dev_get_drvdata()?
> >
> I thought it before and Uwe had a good point why it isn't necessarily a
> good idea:
>
> https://www.spinics.net/lists/linux-pwm/msg15325.html

I understand this point, but the problem is that we often use
different callbacks for different layers. For example, the PM
callbacks are operating with generic 'struct device' and using the PCI
device there is non-flexible layering violation, so in my opinion it's
the opposite to what Uwe says. I.o.w. we need to use corresponding API
to what we have in the callbacks. If the callback comes from generic
level ==> generic APIs more appropriate.

Anyway, it's such a minor detail, that I'm not going to insist on
either way independently on the arguments.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-14  8:57       ` Andy Shevchenko
@ 2021-06-14  9:37         ` Uwe Kleine-König
  2021-06-14 10:47           ` Jonathan Cameron
  0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2021-06-14  9:37 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jarkko Nikula, linux-iio, William Breathitt Gray, Jonathan Cameron

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

On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote:
> +Uwe Kleine-König
> 
> On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
> > On 6/13/21 1:36 PM, Andy Shevchenko wrote:
> > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> > > <jarkko.nikula@linux.intel.com> wrote:
> > >>
> > >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);
> > >
> > > ...
> > >
> > >> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > >> +       struct pci_dev *pdev = to_pci_dev(dev);
> > >>          struct intel_qep *qep = pci_get_drvdata(pdev);
> > >
> > > Why not change both lines to dev_get_drvdata()?
> > >
> > I thought it before and Uwe had a good point why it isn't necessarily a
> > good idea:
> >
> > https://www.spinics.net/lists/linux-pwm/msg15325.html
> 
> I understand this point, but the problem is that we often use
> different callbacks for different layers. For example, the PM
> callbacks are operating with generic 'struct device' and using the PCI
> device there is non-flexible layering violation, so in my opinion it's
> the opposite to what Uwe says. I.o.w. we need to use corresponding API
> to what we have in the callbacks. If the callback comes from generic
> level ==> generic APIs more appropriate.

Without having looked at the driver in question: I (think I) understand
both sides here and both variants have their own downside.

 - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata()
   is a wrapper around dev_set_drvdata for the pcidev's struct device.

 - Using pci_get_drvdata() adds overhead (for humans only though, the
   compiler doesn't care and creates the same code) and having the pci
   device in the callback isn't necessary.

My personal opinion is: The first is the graver layer violation because
it relies on an implementation detail in the PCI framework. The latter
is relying on a fact that is local to the driver only: All devices this
driver is bound to are pci devices. The main benefit of explicitly using
pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow
knowledge of the PCI stuff can easier match a pci_get_drvdata() to the
pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so
while it uses a function call/code line more, it is more explicit and
more obviously correct.

And regarding your argument about the matching API: I think the above
code uses the matching API, that is make a pci_dev from a device using
to_pci_dev().

So this is about weighting up- and downsides. How you judge them is
subjective. (Though my judgement is naturally the better one :-)

Just my 0.02€
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] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-14  9:37         ` Uwe Kleine-König
@ 2021-06-14 10:47           ` Jonathan Cameron
  2021-06-14 11:10             ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2021-06-14 10:47 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Andy Shevchenko, Jarkko Nikula, linux-iio, William Breathitt Gray

On Mon, 14 Jun 2021 11:37:22 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote:
> > +Uwe Kleine-König
> > 
> > On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula
> > <jarkko.nikula@linux.intel.com> wrote:  
> > > On 6/13/21 1:36 PM, Andy Shevchenko wrote:  
> > > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> > > > <jarkko.nikula@linux.intel.com> wrote:  
> > > >>
> > > >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev);  
> > > >
> > > > ...
> > > >  
> > > >> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > > >> +       struct pci_dev *pdev = to_pci_dev(dev);
> > > >>          struct intel_qep *qep = pci_get_drvdata(pdev);  
> > > >
> > > > Why not change both lines to dev_get_drvdata()?
> > > >  
> > > I thought it before and Uwe had a good point why it isn't necessarily a
> > > good idea:
> > >
> > > https://www.spinics.net/lists/linux-pwm/msg15325.html  
> > 
> > I understand this point, but the problem is that we often use
> > different callbacks for different layers. For example, the PM
> > callbacks are operating with generic 'struct device' and using the PCI
> > device there is non-flexible layering violation, so in my opinion it's
> > the opposite to what Uwe says. I.o.w. we need to use corresponding API
> > to what we have in the callbacks. If the callback comes from generic
> > level ==> generic APIs more appropriate.  
> 
> Without having looked at the driver in question: I (think I) understand
> both sides here and both variants have their own downside.
> 
>  - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata()
>    is a wrapper around dev_set_drvdata for the pcidev's struct device.
> 
>  - Using pci_get_drvdata() adds overhead (for humans only though, the
>    compiler doesn't care and creates the same code) and having the pci
>    device in the callback isn't necessary.
> 
> My personal opinion is: The first is the graver layer violation because
> it relies on an implementation detail in the PCI framework. The latter
> is relying on a fact that is local to the driver only: All devices this
> driver is bound to are pci devices. The main benefit of explicitly using
> pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow
> knowledge of the PCI stuff can easier match a pci_get_drvdata() to the
> pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so
> while it uses a function call/code line more, it is more explicit and
> more obviously correct.
> 
> And regarding your argument about the matching API: I think the above
> code uses the matching API, that is make a pci_dev from a device using
> to_pci_dev().
> 
> So this is about weighting up- and downsides. How you judge them is
> subjective. (Though my judgement is naturally the better one :-)

Personally I'm happy with either

dev_set_drvdata / dev_get_drvdata
or 
pci_set_drvdata / pci_get_drvdata

In this particular case there is a convenient struct device *dev local
variable anyway in the probe() (IIRC) so could have done option 1 with
no loss of readability and a tiny saving in code.

Not worth changing it though is my 0.02€

Jonathan

> 
> Just my 0.02€
> Uwe
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper
  2021-06-14 10:47           ` Jonathan Cameron
@ 2021-06-14 11:10             ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2021-06-14 11:10 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Uwe Kleine-König, Jarkko Nikula, linux-iio, William Breathitt Gray

On Mon, Jun 14, 2021 at 1:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 14 Jun 2021 11:37:22 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote:
> > > +Uwe Kleine-König
> > > On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula
> > > <jarkko.nikula@linux.intel.com> wrote:
> > > > On 6/13/21 1:36 PM, Andy Shevchenko wrote:
> > > > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula
> > > > > <jarkko.nikula@linux.intel.com> wrote:

...

> > > > >> -       struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> > > > >> +       struct pci_dev *pdev = to_pci_dev(dev);
> > > > >>          struct intel_qep *qep = pci_get_drvdata(pdev);
> > > > >
> > > > > Why not change both lines to dev_get_drvdata()?
> > > > >
> > > > I thought it before and Uwe had a good point why it isn't necessarily a
> > > > good idea:
> > > >
> > > > https://www.spinics.net/lists/linux-pwm/msg15325.html
> > >
> > > I understand this point, but the problem is that we often use
> > > different callbacks for different layers. For example, the PM
> > > callbacks are operating with generic 'struct device' and using the PCI
> > > device there is non-flexible layering violation, so in my opinion it's
> > > the opposite to what Uwe says. I.o.w. we need to use corresponding API
> > > to what we have in the callbacks. If the callback comes from generic
> > > level ==> generic APIs more appropriate.
> >
> > Without having looked at the driver in question: I (think I) understand
> > both sides here and both variants have their own downside.
> >
> >  - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata()
> >    is a wrapper around dev_set_drvdata for the pcidev's struct device.
> >
> >  - Using pci_get_drvdata() adds overhead (for humans only though, the
> >    compiler doesn't care and creates the same code) and having the pci
> >    device in the callback isn't necessary.
> >
> > My personal opinion is: The first is the graver layer violation because
> > it relies on an implementation detail in the PCI framework. The latter
> > is relying on a fact that is local to the driver only: All devices this
> > driver is bound to are pci devices. The main benefit of explicitly using
> > pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow
> > knowledge of the PCI stuff can easier match a pci_get_drvdata() to the
> > pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so
> > while it uses a function call/code line more, it is more explicit and
> > more obviously correct.
> >
> > And regarding your argument about the matching API: I think the above
> > code uses the matching API, that is make a pci_dev from a device using
> > to_pci_dev().
> >
> > So this is about weighting up- and downsides. How you judge them is
> > subjective. (Though my judgement is naturally the better one :-)
>
> Personally I'm happy with either
>
> dev_set_drvdata / dev_get_drvdata
> or
> pci_set_drvdata / pci_get_drvdata
>
> In this particular case there is a convenient struct device *dev local
> variable anyway in the probe() (IIRC) so could have done option 1 with
> no loss of readability and a tiny saving in code.

As I said this is unflexible.
For example, we have quite a few drivers that split in the way of
core part (as library) + glue driver(s)
How to implement callbacks that will use the same pairs of the callbacks?

I don't think it's possible in a good and neat way.

On top of that I think using the knowledge of the device nature in the
generic callbacks _is_ a layering violation.

TL;DR: the simple rule of thumb may be:
if the callback uses struct device, that dev_get_drvdata(), otherwise
based on what you have got as a parameter.
Does it make sense?

> Not worth changing it though is my 0.02€


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-06-14 11:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 11:55 [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jarkko Nikula
2021-06-11 11:55 ` [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper Jarkko Nikula
2021-06-13 10:36   ` Andy Shevchenko
2021-06-13 11:08     ` Jonathan Cameron
2021-06-14  8:24     ` Jarkko Nikula
2021-06-14  8:57       ` Andy Shevchenko
2021-06-14  9:37         ` Uwe Kleine-König
2021-06-14 10:47           ` Jonathan Cameron
2021-06-14 11:10             ` Andy Shevchenko
2021-06-11 17:23 ` [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused Jonathan Cameron
2021-06-12  0:45   ` William Breathitt Gray

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.