spi: spidev: Add compatible for external SPI ports on Kontron boards
diff mbox series

Message ID 20200702141846.7752-1-frieder.schrempf@kontron.de
State New
Headers show
Series
  • spi: spidev: Add compatible for external SPI ports on Kontron boards
Related show

Commit Message

Frieder Schrempf July 2, 2020, 2:18 p.m. UTC
From: Frieder Schrempf <frieder.schrempf@kontron.de>

Allow external SPI ports on Kontron boards to use the spidev driver.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/spi/spidev.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Mark Brown July 2, 2020, 2:25 p.m. UTC | #1
On Thu, Jul 02, 2020 at 04:18:46PM +0200, Schrempf Frieder wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Allow external SPI ports on Kontron boards to use the spidev driver.

I'd have expected this to require loading a DT overlay for whatever's
attached?
Frieder Schrempf July 2, 2020, 2:46 p.m. UTC | #2
On 02.07.20 16:25, Mark Brown wrote:
> On Thu, Jul 02, 2020 at 04:18:46PM +0200, Schrempf Frieder wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Allow external SPI ports on Kontron boards to use the spidev driver.
> 
> I'd have expected this to require loading a DT overlay for whatever's
> attached?

My intention is to use the spidev driver in the default board DT for an 
interface that is routed to an extension connector and has no dedicated 
slave device attached onboard. So users can attach sensors, etc. with
userspace drivers without touching the kernel or DT.

See https://patchwork.kernel.org/patch/11639075/ for the boards DT.
Geert Uytterhoeven July 2, 2020, 2:57 p.m. UTC | #3
Hi Frieder,

On Thu, Jul 2, 2020 at 4:46 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
> On 02.07.20 16:25, Mark Brown wrote:
> > On Thu, Jul 02, 2020 at 04:18:46PM +0200, Schrempf Frieder wrote:
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> Allow external SPI ports on Kontron boards to use the spidev driver.
> >
> > I'd have expected this to require loading a DT overlay for whatever's
> > attached?
>
> My intention is to use the spidev driver in the default board DT for an
> interface that is routed to an extension connector and has no dedicated
> slave device attached onboard. So users can attach sensors, etc. with
> userspace drivers without touching the kernel or DT.
>
> See https://patchwork.kernel.org/patch/11639075/ for the boards DT.

You can bind "kontron,user-spi" devices to spidev from userspace:
[PATCH v2 0/3] device tree spidev solution - driver_override for SPI
https://spinics.net/lists/linux-spi/msg13951.html

Gr{oetje,eeting}s,

                        Geert
Mark Brown July 2, 2020, 3:07 p.m. UTC | #4
On Thu, Jul 02, 2020 at 04:46:09PM +0200, Frieder Schrempf wrote:

> My intention is to use the spidev driver in the default board DT for an
> interface that is routed to an extension connector and has no dedicated
> slave device attached onboard. So users can attach sensors, etc. with
> userspace drivers without touching the kernel or DT.

The expected way of doing this is to describe whatever was attached via
DT when it's attached - the device is what has the compatible, not some
connector in the middle of the connection.  The way you've got things
set up if the device has a driver then they won't be able to instantiate
the driver.
Frieder Schrempf July 2, 2020, 4:04 p.m. UTC | #5
Hi Geert,

On 02.07.20 16:57, Geert Uytterhoeven wrote:
> Hi Frieder,
> 
> On Thu, Jul 2, 2020 at 4:46 PM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>> On 02.07.20 16:25, Mark Brown wrote:
>>> On Thu, Jul 02, 2020 at 04:18:46PM +0200, Schrempf Frieder wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>
>>>> Allow external SPI ports on Kontron boards to use the spidev driver.
>>>
>>> I'd have expected this to require loading a DT overlay for whatever's
>>> attached?
>>
>> My intention is to use the spidev driver in the default board DT for an
>> interface that is routed to an extension connector and has no dedicated
>> slave device attached onboard. So users can attach sensors, etc. with
>> userspace drivers without touching the kernel or DT.
>>
>> See https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11639075%2F&amp;data=02%7C01%7Cfrieder.schrempf%40kontron.de%7C5ca9f0ba0ccb4ab0329f08d81e983d4e%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637292987071583819&amp;sdata=M8y9HYICQLocSgRmak6uYsx9Y%2FoukaqgmK2D0F%2FTV98%3D&amp;reserved=0 for the boards DT.
> 
> You can bind "kontron,user-spi" devices to spidev from userspace:
> [PATCH v2 0/3] device tree spidev solution - driver_override for SPI
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fspinics.net%2Flists%2Flinux-spi%2Fmsg13951.html&amp;data=02%7C01%7Cfrieder.schrempf%40kontron.de%7C5ca9f0ba0ccb4ab0329f08d81e983d4e%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637292987071583819&amp;sdata=D9pum1oTXWSt%2BY8Egb1J4DOgSa5KH3RwxsSmUl7LgUk%3D&amp;reserved=0

Thanks for pointing that out. I didn't know that this is possible.
I just tried like below it and it works like a charm:

 > echo "spidev" > 
/sys/devices/platform/soc@0/30800000.bus/30840000.spi/spi_master/spi2/spi2.0/driver_override
 > echo "spi2.0" > /sys/bus/spi/drivers/spidev/bind

Thanks,
Frieder
Frieder Schrempf July 2, 2020, 4:24 p.m. UTC | #6
On 02.07.20 17:07, Mark Brown wrote:
> On Thu, Jul 02, 2020 at 04:46:09PM +0200, Frieder Schrempf wrote:
> 
>> My intention is to use the spidev driver in the default board DT for an
>> interface that is routed to an extension connector and has no dedicated
>> slave device attached onboard. So users can attach sensors, etc. with
>> userspace drivers without touching the kernel or DT.
> 
> The expected way of doing this is to describe whatever was attached via
> DT when it's attached - the device is what has the compatible, not some
> connector in the middle of the connection.  The way you've got things
> set up if the device has a driver then they won't be able to instantiate
> the driver.

Ok thanks, got it. I will remove the spi device from the board DT and 
use an overlay if required. Now I got a reason to learn writing DT 
overlays ;)
Frieder Schrempf July 13, 2020, 1:19 p.m. UTC | #7
On 02.07.20 18:24, Frieder Schrempf wrote:
> On 02.07.20 17:07, Mark Brown wrote:
>> On Thu, Jul 02, 2020 at 04:46:09PM +0200, Frieder Schrempf wrote:
>>
>>> My intention is to use the spidev driver in the default board DT for an
>>> interface that is routed to an extension connector and has no dedicated
>>> slave device attached onboard. So users can attach sensors, etc. with
>>> userspace drivers without touching the kernel or DT.
>>
>> The expected way of doing this is to describe whatever was attached via
>> DT when it's attached - the device is what has the compatible, not some
>> connector in the middle of the connection.  The way you've got things
>> set up if the device has a driver then they won't be able to instantiate
>> the driver.
> 
> Ok thanks, got it. I will remove the spi device from the board DT and 
> use an overlay if required. Now I got a reason to learn writing DT 
> overlays ;)

I need to come back to this for another question. I would very much like 
to do it the way Mark described it above: Use a DT overlay to describe 
the device and load this overlay when the device is attached.

But if I get this correctly it requires me to write a specific driver 
that handles the loading of the overlay, which is way too much overhead 
for the simple issue I'm trying to solve.

I would have expected that there is some kind of existing userspace API 
to load an overlay manually, but it seems like there isn't!?

So what's the reasoning behind this? How can I solve this in a 
mainline-compliant way, meaning without either keeping downstream 
patches to bind spidev to my device or writing and maintaining code that 
does the overlay loading?
Mark Brown July 13, 2020, 3:11 p.m. UTC | #8
On Mon, Jul 13, 2020 at 03:19:52PM +0200, Frieder Schrempf wrote:

> I would have expected that there is some kind of existing userspace API to
> load an overlay manually, but it seems like there isn't!?

> So what's the reasoning behind this? How can I solve this in a
> mainline-compliant way, meaning without either keeping downstream patches to
> bind spidev to my device or writing and maintaining code that does the
> overlay loading?

Basically the reasoning is that nobody's done it rather than any grand
design not to do it.  There's some issues for more complex connectors
present on multiple boards with mapping the same connector onto multiple
boards where a resource on the connector might be provided by different
things on the base board so it's not quite as trivial for them as it
should be.
Frieder Schrempf July 14, 2020, 8:54 a.m. UTC | #9
On 13.07.20 17:11, Mark Brown wrote:
> On Mon, Jul 13, 2020 at 03:19:52PM +0200, Frieder Schrempf wrote:
> 
>> I would have expected that there is some kind of existing userspace API to
>> load an overlay manually, but it seems like there isn't!?
> 
>> So what's the reasoning behind this? How can I solve this in a
>> mainline-compliant way, meaning without either keeping downstream patches to
>> bind spidev to my device or writing and maintaining code that does the
>> overlay loading?
> 
> Basically the reasoning is that nobody's done it rather than any grand
> design not to do it.  There's some issues for more complex connectors
> present on multiple boards with mapping the same connector onto multiple
> boards where a resource on the connector might be provided by different
> things on the base board so it's not quite as trivial for them as it
> should be.

Ok, thanks. I'm afraid I'm currently not in the position to work on any 
generic solution to leverage the loading of DT overlays from userspace.

It would still be quite nice to benefit from the flexibility of DT 
overlays not only for the SPI use case. But before I come up with any 
custom solution, for now I will rather have the device in the DT statically.

I just wonder if I need to keep the DT node for the device in a separate 
patch in our own tree, or if a node with a custom compatible string like 
for example "kontron,user-spi" would be accepted upstream, without a 
matching driver?
Mark Brown July 14, 2020, 7:29 p.m. UTC | #10
On Tue, Jul 14, 2020 at 10:54:15AM +0200, Frieder Schrempf wrote:

> It would still be quite nice to benefit from the flexibility of DT overlays
> not only for the SPI use case. But before I come up with any custom
> solution, for now I will rather have the device in the DT statically.

> I just wonder if I need to keep the DT node for the device in a separate
> patch in our own tree, or if a node with a custom compatible string like for
> example "kontron,user-spi" would be accepted upstream, without a matching
> driver?

I'm having a hard time getting enthusiastic about it TBH - can you not
just use spidev and live with the warning?
Frieder Schrempf July 15, 2020, 7:26 a.m. UTC | #11
On 14.07.20 21:29, Mark Brown wrote:
> On Tue, Jul 14, 2020 at 10:54:15AM +0200, Frieder Schrempf wrote:
> 
>> It would still be quite nice to benefit from the flexibility of DT overlays
>> not only for the SPI use case. But before I come up with any custom
>> solution, for now I will rather have the device in the DT statically.
> 
>> I just wonder if I need to keep the DT node for the device in a separate
>> patch in our own tree, or if a node with a custom compatible string like for
>> example "kontron,user-spi" would be accepted upstream, without a matching
>> driver?
> 
> I'm having a hard time getting enthusiastic about it TBH - can you not
> just use spidev and live with the warning?

Ok, I can do that, but when I resend my patches and add "compatible = 
'spidev'" to my DT I expect someone to complain again as my DT does not 
describe the hardware.

Seeing that there are quite a few DTs that still do it like this, I 
probably will try it still and also keep a patch in our tree to remove 
the warning so customers won't be getting worried.

But for obvious reasons this can't be considered a good solution and it 
seems somewhat disturbing that the maintainer needs to propose it 
because of lack of proper solutions ;)
Mark Brown July 15, 2020, 11:36 a.m. UTC | #12
On Wed, Jul 15, 2020 at 09:26:29AM +0200, Frieder Schrempf wrote:
> On 14.07.20 21:29, Mark Brown wrote:
> > On Tue, Jul 14, 2020 at 10:54:15AM +0200, Frieder Schrempf wrote:

> > > patch in our own tree, or if a node with a custom compatible string like for
> > > example "kontron,user-spi" would be accepted upstream, without a matching
> > > driver?

> > I'm having a hard time getting enthusiastic about it TBH - can you not
> > just use spidev and live with the warning?

> Ok, I can do that, but when I resend my patches and add "compatible =
> 'spidev'" to my DT I expect someone to complain again as my DT does not
> describe the hardware.

That's the issue with kontron,user-spi too though :/

> But for obvious reasons this can't be considered a good solution and it
> seems somewhat disturbing that the maintainer needs to propose it because of
> lack of proper solutions ;)

Hey, I proposed other solutions you didn't want to implement!
Frieder Schrempf July 15, 2020, 11:45 a.m. UTC | #13
On 15.07.20 13:36, Mark Brown wrote:
> On Wed, Jul 15, 2020 at 09:26:29AM +0200, Frieder Schrempf wrote:
>> On 14.07.20 21:29, Mark Brown wrote:
>>> On Tue, Jul 14, 2020 at 10:54:15AM +0200, Frieder Schrempf wrote:
> 
>>>> patch in our own tree, or if a node with a custom compatible string like for
>>>> example "kontron,user-spi" would be accepted upstream, without a matching
>>>> driver?
> 
>>> I'm having a hard time getting enthusiastic about it TBH - can you not
>>> just use spidev and live with the warning?
> 
>> Ok, I can do that, but when I resend my patches and add "compatible =
>> 'spidev'" to my DT I expect someone to complain again as my DT does not
>> describe the hardware.
> 
> That's the issue with kontron,user-spi too though :/

Yes, true.

> 
>> But for obvious reasons this can't be considered a good solution and it
>> seems somewhat disturbing that the maintainer needs to propose it because of
>> lack of proper solutions ;)
> 
> Hey, I proposed other solutions you didn't want to implement!

Right, but you have to admit that the other solutions turned out to be 
rather out of scope for someone like me who merely wants to use the 
spidev driver.

But I don't blame you. I'm now having a better idea of how things are 
(or aren't) supposed to look like. So thanks for your patience!
Mark Brown July 15, 2020, 1:10 p.m. UTC | #14
On Wed, Jul 15, 2020 at 01:45:51PM +0200, Frieder Schrempf wrote:
> On 15.07.20 13:36, Mark Brown wrote:

> > Hey, I proposed other solutions you didn't want to implement!

> Right, but you have to admit that the other solutions turned out to be
> rather out of scope for someone like me who merely wants to use the spidev
> driver.

> But I don't blame you. I'm now having a better idea of how things are (or
> aren't) supposed to look like. So thanks for your patience!

I'm not sure platforms like this are a great fit for DT TBH - the
trouble with DT is that it turns things into ABIs regardless of if it's
really a finished thing, platform data based systems were a lot more
flexible here.
Frieder Schrempf July 15, 2020, 1:48 p.m. UTC | #15
On 15.07.20 15:10, Mark Brown wrote:
> On Wed, Jul 15, 2020 at 01:45:51PM +0200, Frieder Schrempf wrote:
>> On 15.07.20 13:36, Mark Brown wrote:
> 
>>> Hey, I proposed other solutions you didn't want to implement!
> 
>> Right, but you have to admit that the other solutions turned out to be
>> rather out of scope for someone like me who merely wants to use the spidev
>> driver.
> 
>> But I don't blame you. I'm now having a better idea of how things are (or
>> aren't) supposed to look like. So thanks for your patience!
> 
> I'm not sure platforms like this are a great fit for DT TBH - the
> trouble with DT is that it turns things into ABIs regardless of if it's
> really a finished thing, platform data based systems were a lot more
> flexible here.

I see your point. Still it seems like the overlays would provide a 
sufficient alternative in matters of flexibility. Raspberry Pi uses it 
for years, which made me assume that some generic loading API is 
available. But it seems like everything is RPi-specific and downstream.
Mark Brown July 15, 2020, 1:51 p.m. UTC | #16
On Wed, Jul 15, 2020 at 03:48:37PM +0200, Frieder Schrempf wrote:
> On 15.07.20 15:10, Mark Brown wrote:

> > I'm not sure platforms like this are a great fit for DT TBH - the
> > trouble with DT is that it turns things into ABIs regardless of if it's
> > really a finished thing, platform data based systems were a lot more
> > flexible here.

> I see your point. Still it seems like the overlays would provide a
> sufficient alternative in matters of flexibility. Raspberry Pi uses it for
> years, which made me assume that some generic loading API is available. But
> it seems like everything is RPi-specific and downstream.

Yes, overlays would be fine if there were a way to use them for
connectors like this.  The connectors are a real problem, it's not just
RPi that has their own loaders/infrastructure downstream.
Geert Uytterhoeven July 15, 2020, 7:06 p.m. UTC | #17
On Mon, Jul 13, 2020 at 5:11 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 13, 2020 at 03:19:52PM +0200, Frieder Schrempf wrote:
> > I would have expected that there is some kind of existing userspace API to
> > load an overlay manually, but it seems like there isn't!?
>
> > So what's the reasoning behind this? How can I solve this in a
> > mainline-compliant way, meaning without either keeping downstream patches to
> > bind spidev to my device or writing and maintaining code that does the
> > overlay loading?
>
> Basically the reasoning is that nobody's done it rather than any grand

Nah, it's been done, but a bit unsafe, if you don't know what you're doing
("with great power come great responsibilities").

Please check out https://elinux.org/R-Car/DT-Overlays
I do my best to keep topic/overlays branch up-to-date, and working.

> design not to do it.  There's some issues for more complex connectors
> present on multiple boards with mapping the same connector onto multiple
> boards where a resource on the connector might be provided by different
> things on the base board so it's not quite as trivial for them as it
> should be.

There's a big list of issues at
https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts
In other words: more work to be done, to polish it, and make it safe(r).

Gr{oetje,eeting}s,

                        Geert
Frieder Schrempf July 16, 2020, 7:53 a.m. UTC | #18
On 15.07.20 21:06, Geert Uytterhoeven wrote:
> On Mon, Jul 13, 2020 at 5:11 PM Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Jul 13, 2020 at 03:19:52PM +0200, Frieder Schrempf wrote:
>>> I would have expected that there is some kind of existing userspace API to
>>> load an overlay manually, but it seems like there isn't!?
>>
>>> So what's the reasoning behind this? How can I solve this in a
>>> mainline-compliant way, meaning without either keeping downstream patches to
>>> bind spidev to my device or writing and maintaining code that does the
>>> overlay loading?
>>
>> Basically the reasoning is that nobody's done it rather than any grand
> 
> Nah, it's been done, but a bit unsafe, if you don't know what you're doing
> ("with great power come great responsibilities").
> 
> Please check out https://elinux.org/R-Car/DT-Overlays
> I do my best to keep topic/overlays branch up-to-date, and working.

Thanks! That looks like what I was searching for, but couldn't find it. 
So there's some work being done. That's good to know. We probably can't 
use it in this state, but that's still a lot better than nothing.

> 
>> design not to do it.  There's some issues for more complex connectors
>> present on multiple boards with mapping the same connector onto multiple
>> boards where a resource on the connector might be provided by different
>> things on the base board so it's not quite as trivial for them as it
>> should be.
> 
> There's a big list of issues at
> https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts
> In other words: more work to be done, to polish it, and make it safe(r).

I'm now getting a rough idea about the efforts and issues behind this, 
thanks.

Patch
diff mbox series

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 59e07675ef86..058b08a3767d 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -677,6 +677,7 @@  static const struct of_device_id spidev_dt_ids[] = {
 	{ .compatible = "lwn,bk4" },
 	{ .compatible = "dh,dhcom-board" },
 	{ .compatible = "menlo,m53cpld" },
+	{ .compatible = "kontron,user-spi" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);