[PATCH] spidev: add platform driver support

[PATCH] spidev: add platform driver support

[Christian Gmeiner]

This makes it possible to use spidev in combination with the
MFD subsystem. The MFD subsystem add platform_driver devices.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/spi/spidev.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index f56e0e975a46..fb7b483ff70d 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -25,6 +25,8 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
 
+#include <linux/platform_device.h>
+
 #include <linux/uaccess.h>
 
 
@@ -827,6 +829,40 @@ static struct spi_driver spidev_spi_driver = {
 	 */
 };
 
+static int spidev_platform_probe(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct spi_device *spi;
+
+	if (strcmp(parent->bus->name, "spi"))
+		return -ENODEV;
+
+	spi = to_spi_device(parent);
+
+	/* This only works if no drvdata is stored */
+	if (spi_get_drvdata(spi)) {
+		dev_err(&pdev->dev, "drvdata is not NULL\n");
+		return -EOPNOTSUPP;
+	}
+
+	return spidev_probe(spi);
+}
+
+static int spidev_platform_remove(struct platform_device *pdev)
+{
+	struct spi_device *spi = to_spi_device(pdev->dev.parent);
+
+	return spidev_remove(spi);
+}
+
+static struct platform_driver spidev_platfoem_driver = {
+	.probe = spidev_platform_probe,
+	.remove = spidev_platform_remove,
+	.driver = {
+		.name = "spidev",
+	},
+};
+
 /*-------------------------------------------------------------------------*/
 
 static int __init spidev_init(void)
@@ -853,12 +889,21 @@ static int __init spidev_init(void)
 		class_destroy(spidev_class);
 		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
 	}
+
+	status = platform_driver_register(&spidev_platfoem_driver);
+	if (status < 0) {
+		spi_unregister_driver(&spidev_spi_driver);
+		class_destroy(spidev_class);
+		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
+	}
+
 	return status;
 }
 module_init(spidev_init);
 
 static void __exit spidev_exit(void)
 {
+	platform_driver_unregister(&spidev_platfoem_driver);
 	spi_unregister_driver(&spidev_spi_driver);
 	class_destroy(spidev_class);
 	unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
-- 
2.31.1

Re: [PATCH] spidev: add platform driver support

[Mark Brown]

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

On Thu, May 27, 2021 at 10:45:15AM +0200, Christian Gmeiner wrote:

> This makes it possible to use spidev in combination with the
> MFD subsystem. The MFD subsystem add platform_driver devices.

This is a really strange thing to want to do so it needs a
changelog which explains what the goal is and why this is a good
way of accomplishing that goal.

> +static int spidev_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct spi_device *spi;
> +
> +	if (strcmp(parent->bus->name, "spi"))
> +		return -ENODEV;
> +
> +	spi = to_spi_device(parent);
> +
> +	/* This only works if no drvdata is stored */
> +	if (spi_get_drvdata(spi)) {
> +		dev_err(&pdev->dev, "drvdata is not NULL\n");

Why?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	return spidev_probe(spi);

This really does not seem like a good idea, this is exposing the
entire device to userspace in a completely unstructured fashion
while there will be other drivers controlling the same hardware.
That seems like it's asking for trouble, there's absolutely
nothing ensuring that userspace doesn't break things the drivers
are doing.

I really don't think it makes sense to mix kernel drivers with
unmoderated userspace access to the hardware in a single driver.

> +static struct platform_driver spidev_platfoem_driver = {

platfoem?

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

Re: [PATCH] spidev: add platform driver support

[Christian Gmeiner]

ping

Am Do., 27. Mai 2021 um 10:45 Uhr schrieb Christian Gmeiner
<christian.gmeiner@gmail.com>:
>
> This makes it possible to use spidev in combination with the
> MFD subsystem. The MFD subsystem add platform_driver devices.
>
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/spi/spidev.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index f56e0e975a46..fb7b483ff70d 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -25,6 +25,8 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spidev.h>
>
> +#include <linux/platform_device.h>
> +
>  #include <linux/uaccess.h>
>
>
> @@ -827,6 +829,40 @@ static struct spi_driver spidev_spi_driver = {
>          */
>  };
>
> +static int spidev_platform_probe(struct platform_device *pdev)
> +{
> +       struct device *parent = pdev->dev.parent;
> +       struct spi_device *spi;
> +
> +       if (strcmp(parent->bus->name, "spi"))
> +               return -ENODEV;
> +
> +       spi = to_spi_device(parent);
> +
> +       /* This only works if no drvdata is stored */
> +       if (spi_get_drvdata(spi)) {
> +               dev_err(&pdev->dev, "drvdata is not NULL\n");
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return spidev_probe(spi);
> +}
> +
> +static int spidev_platform_remove(struct platform_device *pdev)
> +{
> +       struct spi_device *spi = to_spi_device(pdev->dev.parent);
> +
> +       return spidev_remove(spi);
> +}
> +
> +static struct platform_driver spidev_platfoem_driver = {
> +       .probe = spidev_platform_probe,
> +       .remove = spidev_platform_remove,
> +       .driver = {
> +               .name = "spidev",
> +       },
> +};
> +
>  /*-------------------------------------------------------------------------*/
>
>  static int __init spidev_init(void)
> @@ -853,12 +889,21 @@ static int __init spidev_init(void)
>                 class_destroy(spidev_class);
>                 unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
>         }
> +
> +       status = platform_driver_register(&spidev_platfoem_driver);
> +       if (status < 0) {
> +               spi_unregister_driver(&spidev_spi_driver);
> +               class_destroy(spidev_class);
> +               unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
> +       }
> +
>         return status;
>  }
>  module_init(spidev_init);
>
>  static void __exit spidev_exit(void)
>  {
> +       platform_driver_unregister(&spidev_platfoem_driver);
>         spi_unregister_driver(&spidev_spi_driver);
>         class_destroy(spidev_class);
>         unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
> --
> 2.31.1
>


-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH] spidev: add platform driver support

[Mark Brown]

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

On Wed, Jun 16, 2021 at 09:16:44PM +0200, Christian Gmeiner wrote:

> ping

I replied to your mail some time ago...

Also:

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

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

Re: [PATCH] spidev: add platform driver support

[Christian Gmeiner]

Hi Mark,

Am Mi., 16. Juni 2021 um 21:44 Uhr schrieb Mark Brown <broonie@kernel.org>:
>
> On Wed, Jun 16, 2021 at 09:16:44PM +0200, Christian Gmeiner wrote:
>
> > ping
>
> I replied to your mail some time ago...
>

It did not land in my in-box and I do not see your mail here:
https://patches.linaro.org/patch/449250/
In the end I found it only here:
https://lore.kernel.org/lkml/YK9wDd%2F+c1uAjwk7@sirena.org.uk/

This makes it basically quite hard to answer your question. So will do it here:

> > This makes it possible to use spidev in combination with the
> > MFD subsystem. The MFD subsystem add platform_driver devices.

> This is a really strange thing to want to do so it needs a
> changelog which explains what the goal is and why this is a good
> way of accomplishing that goal.

I am currently working on a system controller that is connected via
SPI. The system controller
provides a handful of LEDs, a hex switch and some GPIOs. All of this
can be modeled quite easily
with the MFD subsystem but the e.g. LED subsystem.

The biggest pain point is that this system controller or the used FPGA
can be updated via SPI. And
I see no suitable subsystem to handle this update case.

That's why I have chosen to expose the SPI device also via spidev.

>
> Also:
>
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be addressed.
>

I am aware of the rules.. but as I told you I got nothing in my in-box
for 2 weeks.

> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, so sending again is generally a better approach though there are
> some other maintainers who like them - if in doubt look at how patches
> for the subsystem are normally handled.
>
> Please don't top post, reply in line with needed context.  This allows
> readers to readily follow the flow of conversation and understand what
> you are talking about and also helps ensure that everything in the
> discussion is being addressed.

Yeah.. I should never ever send a mail from my smartphone.


--
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH] spidev: add platform driver support

[Mark Brown]

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

On Mon, Jun 21, 2021 at 03:57:02PM +0200, Christian Gmeiner wrote:
> Am Mi., 16. Juni 2021 um 21:44 Uhr schrieb Mark Brown <broonie@kernel.org>:
> > On Wed, Jun 16, 2021 at 09:16:44PM +0200, Christian Gmeiner wrote:

> > I replied to your mail some time ago...

> It did not land in my in-box and I do not see your mail here:
> https://patches.linaro.org/patch/449250/

Speak to Linaro about their systems, they're nothing to do with me.

> In the end I found it only here:
> https://lore.kernel.org/lkml/YK9wDd%2F+c1uAjwk7@sirena.org.uk/

Lore provides links to download mboxes FWIW.

> The biggest pain point is that this system controller or the used FPGA
> can be updated via SPI. And
> I see no suitable subsystem to handle this update case.

> That's why I have chosen to expose the SPI device also via spidev.

OK, so you need a driver or subsystem that implements FPGA updates then
- depending on what that looks like the FPGA or MTD subsystems seem
likely to be candidates, or perhaps the MFD for your device should
provide something as part of the core functionality.  All the issues I
raised earlier apply here, spidev is rarely a good idea for production
use and certainly not in cases where there are actual drivers sharing
the device.

> > Please don't send content free pings and please allow a reasonable time
> > for review.  People get busy, go on holiday, attend conferences and so
> > on so unless there is some reason for urgency (like critical bug fixes)
> > please allow at least a couple of weeks for review.  If there have been
> > review comments then people may be waiting for those to be addressed.

> I am aware of the rules.. but as I told you I got nothing in my in-box
> for 2 weeks.

You need to take this up with your mail provider.

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