Adds get/set driver data helpers for auxiliary devices.
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Mark Gross <[email protected]>
---
V2
- No changes
include/linux/auxiliary_bus.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index fc51d45f106b..a8338d456e81 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -28,6 +28,16 @@ struct auxiliary_driver {
const struct auxiliary_device_id *id_table;
};
+static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
+{
+ return dev_get_drvdata(&auxdev->dev);
+}
+
+static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
+{
+ dev_set_drvdata(&auxdev->dev, data);
+}
+
static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
{
return container_of(dev, struct auxiliary_device, dev);
--
2.25.1
On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> Adds get/set driver data helpers for auxiliary devices.
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: David E. Box <[email protected]>
> Reviewed-by: Mark Gross <[email protected]>
> ---
> V2
> - No changes
>
> include/linux/auxiliary_bus.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index fc51d45f106b..a8338d456e81 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -28,6 +28,16 @@ struct auxiliary_driver {
> const struct auxiliary_device_id *id_table;
> };
>
> +static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
> +{
> + return dev_get_drvdata(&auxdev->dev);
> +}
> +
> +static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
> +{
> + dev_set_drvdata(&auxdev->dev, data);
> +}
> +
> static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
> {
> return container_of(dev, struct auxiliary_device, dev);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> Adds get/set driver data helpers for auxiliary devices.
>
> Signed-off-by: David E. Box <[email protected]>
> Reviewed-by: Mark Gross <[email protected]>
> ---
> V2
> - No changes
>
> include/linux/auxiliary_bus.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
I would really like to see an explanation why such obfuscation is really
needed. dev_*_drvdata() is a standard way to access driver data.
Thanks
>
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index fc51d45f106b..a8338d456e81 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -28,6 +28,16 @@ struct auxiliary_driver {
> const struct auxiliary_device_id *id_table;
> };
>
> +static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
> +{
> + return dev_get_drvdata(&auxdev->dev);
> +}
> +
> +static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
> +{
> + dev_set_drvdata(&auxdev->dev, data);
> +}
> +
> static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
> {
> return container_of(dev, struct auxiliary_device, dev);
> --
> 2.25.1
>
On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > Adds get/set driver data helpers for auxiliary devices.
> >
> > Signed-off-by: David E. Box <[email protected]>
> > Reviewed-by: Mark Gross <[email protected]>
> > ---
> > V2
> > - No changes
> >
> > include/linux/auxiliary_bus.h | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
>
> I would really like to see an explanation why such obfuscation is really
> needed. dev_*_drvdata() is a standard way to access driver data.
Lots of busses have this helper. This is nothing new at all, and is
nice to have. Look at all of the calls to dev_get_drvdata() in
include/linux/ for the examples.
thanks,
greg k-h
On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > >
> > > Signed-off-by: David E. Box <[email protected]>
> > > Reviewed-by: Mark Gross <[email protected]>
> > > ---
> > > V2
> > > - No changes
> > >
> > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> >
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.
>
> Lots of busses have this helper. This is nothing new at all, and is
> nice to have. Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.
I looked and this is why I asked. From the point of person who does
reviews and refactoring, such obfuscations are evil.
If I understand your correctly, the explanation is just copy/paste, right?
Thanks
>
> thanks,
>
> greg k-h
On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > >
> > > > Signed-off-by: David E. Box <[email protected]>
> > > > Reviewed-by: Mark Gross <[email protected]>
> > > > ---
> > > > V2
> > > > - No changes
> > > >
> > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > 1 file changed, 10 insertions(+)
> > >
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> >
> > Lots of busses have this helper. This is nothing new at all, and is
> > nice to have. Look at all of the calls to dev_get_drvdata() in
> > include/linux/ for the examples.
>
> I looked and this is why I asked. From the point of person who does
> reviews and refactoring, such obfuscations are evil.
Then you must consider about 80 busses evil :)
Again, this is a very common helper pattern in the kernel, one that we
have had for decades now. It allows a driver writer to only worry about
the bus apis for that specific bus, and not have to dive down into the
driver core functions at all. What is wrong with that?
> If I understand your correctly, the explanation is just copy/paste, right?
I do not understand what you mean by this, sorry.
greg k-h
On Wed, 08 Dec 2021, Greg KH wrote:
> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > >
> > > Signed-off-by: David E. Box <[email protected]>
> > > Reviewed-by: Mark Gross <[email protected]>
> > > ---
> > > V2
> > > - No changes
> > >
> > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> >
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.
I wouldn't call it obfuscation, but it does looks like abstraction for
the sake of abstraction, which I usually push back on. What are the
technical benefits over using the dev_*() variant?
> Lots of busses have this helper. This is nothing new at all, and is
> nice to have. Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
>
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > >
> > > > Signed-off-by: David E. Box <[email protected]>
> > > > Reviewed-by: Mark Gross <[email protected]>
> > > > ---
> > > > V2
> > > > - No changes
> > > >
> > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > 1 file changed, 10 insertions(+)
> > >
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
>
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on. What are the
> technical benefits over using the dev_*() variant?
See my response at:
https://lore.kernel.org/r/[email protected]
for why it is a good thing to do.
In short, driver authors should not have to worry about mixing
bus-specific and low-level driver core functions.
thanks,
greg k-h
On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > >
> > > > > Signed-off-by: David E. Box <[email protected]>
> > > > > Reviewed-by: Mark Gross <[email protected]>
> > > > > ---
> > > > > V2
> > > > > - No changes
> > > > >
> > > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > > 1 file changed, 10 insertions(+)
> > > >
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > >
> > > Lots of busses have this helper. This is nothing new at all, and is
> > > nice to have. Look at all of the calls to dev_get_drvdata() in
> > > include/linux/ for the examples.
> >
> > I looked and this is why I asked. From the point of person who does
> > reviews and refactoring, such obfuscations are evil.
>
> Then you must consider about 80 busses evil :)
>
> Again, this is a very common helper pattern in the kernel, one that we
> have had for decades now. It allows a driver writer to only worry about
> the bus apis for that specific bus, and not have to dive down into the
> driver core functions at all. What is wrong with that?
What is wrong?
The idea that you have two APIs which do the same thing, one is
obfuscated version of another.
If you don't want from people to use driver core function and structures,
you shouldn't expose them in global headers.
Thanks
On Wed, 08 Dec 2021, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> >
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > >
> > > > > Signed-off-by: David E. Box <[email protected]>
> > > > > Reviewed-by: Mark Gross <[email protected]>
> > > > > ---
> > > > > V2
> > > > > - No changes
> > > > >
> > > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > > 1 file changed, 10 insertions(+)
> > > >
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> >
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on. What are the
> > technical benefits over using the dev_*() variant?
>
> See my response at:
> https://lore.kernel.org/r/[email protected]
> for why it is a good thing to do.
I saw this after I'd sent my query.
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.
Okay, that makes sense.
I guess my view abstraction for the sake of it is slightly higher
level as I vehemently dislike it when driver-set writers create their
own APIs, such as; (just off the top of my head, not a real example)
cros_get_data() or cros_write() which are usually abstractions of top
level APIs like platform_get_data() and regmap_write() respectively.
Abstracting at *real* API level does seem like the right thing to do.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
>
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > >
> > > > Signed-off-by: David E. Box <[email protected]>
> > > > Reviewed-by: Mark Gross <[email protected]>
> > > > ---
> > > > V2
> > > > - No changes
> > > >
> > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > 1 file changed, 10 insertions(+)
> > >
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
>
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on. What are the
> technical benefits over using the dev_*() variant?
You can see it in Greg's answer, there is no technical benefits in any
variant. It is simple copy/paste pattern from other buses.
Maybe it is not clear from my response, I don't care if this patch is
going to be applied or not, but I would like to hear someone explains
to me what are the benefits of such one liners.
Thanks
On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
...
> The idea that you have two APIs which do the same thing, one is
> obfuscated version of another.
>
> If you don't want from people to use driver core function and structures,
> you shouldn't expose them in global headers.
For all these APIs the rationale is very simple. If you have callback that
takes a pointer to the container (*), you better use the APIs related to
this container (no need to have an explicit dereferencing). Otherwise you
use dev_*() APIs (when it's pointer to the pure struct device).
The value is to have coherent APIs around struct device containers.
*) under container here I assume the data structure that has the embedded
struct device in it.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> >
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > >
> > > > > Signed-off-by: David E. Box <[email protected]>
> > > > > Reviewed-by: Mark Gross <[email protected]>
> > > > > ---
> > > > > V2
> > > > > - No changes
> > > > >
> > > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > > 1 file changed, 10 insertions(+)
> > > >
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> >
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on. What are the
> > technical benefits over using the dev_*() variant?
>
> See my response at:
> https://lore.kernel.org/r/[email protected]
> for why it is a good thing to do.
>
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.
Right! I just answered to Leon with the similar view behind (using
different words).
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 08, 2021 at 12:14:41PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
>
> ...
>
> > The idea that you have two APIs which do the same thing, one is
> > obfuscated version of another.
> >
> > If you don't want from people to use driver core function and structures,
> > you shouldn't expose them in global headers.
>
> For all these APIs the rationale is very simple. If you have callback that
> takes a pointer to the container (*), you better use the APIs related to
> this container (no need to have an explicit dereferencing). Otherwise you
> use dev_*() APIs (when it's pointer to the pure struct device).
>
> The value is to have coherent APIs around struct device containers.
>
> *) under container here I assume the data structure that has the embedded
> struct device in it.
Thanks
>
--
> With Best Regards,
> Andy Shevchenko
>
>
[+cc Rafael, since I used generic PM as an example]
On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > >
> > > > > Signed-off-by: David E. Box <[email protected]>
> > > > > Reviewed-by: Mark Gross <[email protected]>
> > > > > ---
> > > > > V2
> > > > > - No changes
> > > > >
> > > > > include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > > 1 file changed, 10 insertions(+)
> > > >
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> >
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on. What are the
> > technical benefits over using the dev_*() variant?
>
> See my response at:
> https://lore.kernel.org/r/[email protected]
> for why it is a good thing to do.
>
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.
In the very common situation of PCI drivers that use generic power
management, authors *do* have to use both (example from [1]):
ioh_gpio_probe(struct pci_dev *pdev) # pci_driver.probe()
pci_set_drvdata(pdev, chip);
ioh_gpio_remove(struct pci_dev *pdev) # pci_driver.remove()
struct ioh_gpio *chip = pci_get_drvdata(pdev);
ioh_gpio_suspend(struct device *dev) # pci_driver.driver.pm.suspend()
struct ioh_gpio *chip = dev_get_drvdata(dev); <--
The pci_driver methods receive a struct pci_dev and use the
pci_get_drvdata() wrapper.
The generic power management methods receive a struct device and use
the underlying dev_get_drvdata().
It's kind of ugly that readers have to know that pci_get_drvdata()
gives you the same thing as dev_get_drvdata().
I guess the generic PM methods could do something like:
pci_get_drvdata(to_pci_dev(dev));
but that seems a little bit circuitous. It's slightly wordier, but I
might prefer to just use this everywhere and skip the pci_* wrappers:
dev_get_drvdata(&pdev->dev);
Bjorn
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505
On Thu, Dec 09, 2021 at 10:32:45AM -0600, Bjorn Helgaas wrote:
> [+cc Rafael, since I used generic PM as an example]
> On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
...
Okay, more bikeshedding :-)
> In the very common situation of PCI drivers that use generic power
> management, authors *do* have to use both (example from [1]):
>
> ioh_gpio_probe(struct pci_dev *pdev) # pci_driver.probe()
> pci_set_drvdata(pdev, chip);
>
> ioh_gpio_remove(struct pci_dev *pdev) # pci_driver.remove()
> struct ioh_gpio *chip = pci_get_drvdata(pdev);
>
> ioh_gpio_suspend(struct device *dev) # pci_driver.driver.pm.suspend()
> struct ioh_gpio *chip = dev_get_drvdata(dev); <--
>
> The pci_driver methods receive a struct pci_dev and use the
> pci_get_drvdata() wrapper.
>
> The generic power management methods receive a struct device and use
> the underlying dev_get_drvdata().
>
> It's kind of ugly that readers have to know that pci_get_drvdata()
> gives you the same thing as dev_get_drvdata().
>
> I guess the generic PM methods could do something like:
>
> pci_get_drvdata(to_pci_dev(dev));
>
> but that seems a little bit circuitous. It's slightly wordier, but I
> might prefer to just use this everywhere and skip the pci_* wrappers:
>
> dev_get_drvdata(&pdev->dev);
Strictly speaking the
<$BUS)_get_drvdata(<$CONTAINER>) != dev_get_drvdata(dev)
it's completely up to the container handling code what to do.
In 99% (or 100%?) cases it's equal, but it's not obliged to be so.
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505
--
With Best Regards,
Andy Shevchenko