[PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Tobias Klauser]

From: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Until now we let the code in spi.c assign us the bus number if the platform
device didn't specify its id. This patch adds the possibility to explicitely
specify the bus number via device tree.

Signed-off-by: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
---
 .../devicetree/bindings/spi/spi_altera.txt         |    3 +++
 drivers/spi/spi-altera.c                           |    9 +++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi_altera.txt b/Documentation/devicetree/bindings/spi/spi_altera.txt
index dda3759..3e0e921 100644
--- a/Documentation/devicetree/bindings/spi/spi_altera.txt
+++ b/Documentation/devicetree/bindings/spi/spi_altera.txt
@@ -2,3 +2,6 @@ Altera SPI
 
 Required properties:
 - compatible : should be "ALTR,spi-1.0".
+
+Optional property:
+- bus-number : SPI bus number
diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index 4813a63..d5bf52b 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -226,6 +226,15 @@ static int __devinit altera_spi_probe(struct platform_device *pdev)
 	if (!master)
 		return err;
 
+	/* bus_num is dynamically assigned when undefined (see spi.c)  */
+	/* look for a DTS entry to override this */
+	if (pdev->id == -1 && pdev->dev.of_node != 0) {
+		u32 tmp;
+
+		if (of_property_read_u32(pdev->dev.of_node, "bus-number", &tmp) == 0)
+			pdev->id = tmp;
+	}
+
 	/* setup the master state. */
 	master->bus_num = pdev->id;
 	master->num_chipselect = 16;
-- 
1.7.5.4

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Tue, Jan 31, 2012 at 04:54:06PM +0100, Tobias Klauser wrote:
> From: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Until now we let the code in spi.c assign us the bus number if the platform
> device didn't specify its id. This patch adds the possibility to explicitely
> specify the bus number via device tree.

Why would this be required?  Bus numbers are a Linux specific thing...

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Frederic LAMBERT]

Because this bus number is used to create the device name on
/sys/bus/spi/..., name that the user app must know to work with.

Le 1 févr. 2012 14:55, "Mark Brown" <broonie-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> a écrit :
>
> On Tue, Jan 31, 2012 at 04:54:06PM +0100, Tobias Klauser wrote:
> > From: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > Until now we let the code in spi.c assign us the bus number if the
platform
> > device didn't specify its id. This patch adds the possibility to
explicitely
> > specify the bus number via device tree.
>
> Why would this be required?  Bus numbers are a Linux specific thing...
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Wed, Feb 01, 2012 at 03:01:53PM +0100, Frederic LAMBERT wrote:

Don't top post!

> Because this bus number is used to create the device name on
> /sys/bus/spi/..., name that the user app must know to work with.

Why must the user application know this?  What is missing to allow the
application to discover an appropriate device to work with automatically?

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Frederic LAMBERT]

> Don't top post!
I do what I can with my Google phone :-)

> > Because this bus number is used to create the device name on
> > /sys/bus/spi/..., name that the user app must know to work with.
>
> Why must the user application know this?  What is missing to allow the
> application to discover an appropriate device to work with automatically?

Good question. I have a nvSRAM that I access using the AT25 driver, and I
don't know how access it without its /sys/bus/ name. If there is another
way, it is worse to consider it.

I think a similar way is used to read/write GPIOs...

Before DTS, it was possible to set the SPI bus number in
arch/nios2/kernel/config.c (in the case of our NIOS2 Linux port), but now,
without such a patch, it's not possible anymore. For me, it is a
regression!


Fred
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Wed, Feb 01, 2012 at 03:25:33PM +0100, Frederic LAMBERT wrote:

> > > Because this bus number is used to create the device name on
> > > /sys/bus/spi/..., name that the user app must know to work with.

> > Why must the user application know this?  What is missing to allow the
> > application to discover an appropriate device to work with automatically?

> Good question. I have a nvSRAM that I access using the AT25 driver, and I
> don't know how access it without its /sys/bus/ name. If there is another
> way, it is worse to consider it.

For this I'd expect the application to be able to enumerate all the SPI
devices and offer the user a selection of those with the appropriate
driver bound (though I can see that simple scripts might not have
bothered).

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Frederic LAMBERT]

>
> > Good question. I have a nvSRAM that I access using the AT25 driver, and I
> > don't know how access it without its /sys/bus/ name. If there is another
> > way, it is worse to consider it.
>
> For this I'd expect the application to be able to enumerate all the SPI
> devices and offer the user a selection of those with the appropriate
> driver bound (though I can see that simple scripts might not have
> bothered).
>

???
I wrote a piece of code that uses this nvSRAM as a persistent storage.
This runs in a softcore embedded in an FPGA, in an equipment.
The PC version of this application uses a simple file to emulate this
behavior.
The simulated version, using an x86 port embedded in a VirtualBox uses a
piece of virtual disk with /dev/sdb.

To sumarize, I have a same code that works for the 3 architecture, the
single difference being in the device name:
/sys/proc/spi/drivers/at25/spi0.0/eeprom for Nios2
dataBase.dat on Linux
/dev/sdb on the Virtual Machine

Why should I have to break that, which work nice, to use a bus enumeration
which would be specific to the real hardware version, because I'm using a
new tools, DTS, which is supposed to simplify my life.
Sorry to insist, but this incapacity to set the bus number is a *real*
regression (for me).

I know I could use the name chosen by the kernel (it mus be something like
/sys/.../spi253.0/...), but I have no control on it. If tomorrow someone
changes something in the spi bus number definition, a kernel upgrade will
have an impact on the user stuff...
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Wed, Feb 01, 2012 at 03:53:00PM +0100, Frederic LAMBERT wrote:

> I wrote a piece of code that uses this nvSRAM as a persistent storage.
> This runs in a softcore embedded in an FPGA, in an equipment.
> The PC version of this application uses a simple file to emulate this
> behavior.
> The simulated version, using an x86 port embedded in a VirtualBox uses a
> piece of virtual disk with /dev/sdb.

So it's just a basic block device?

> To sumarize, I have a same code that works for the 3 architecture, the
> single difference being in the device name:
> /sys/proc/spi/drivers/at25/spi0.0/eeprom for Nios2
> dataBase.dat on Linux
> /dev/sdb on the Virtual Machine

In this case surely the mechanisms used to identify regular disks (UUIDs
and so on) will also work (the .dat file will have to be an override,
but otherwise...)?  Names like sdb aren't stable either - they can
change with either software or hardware changes since they just come
from the order of discovery.

> Why should I have to break that, which work nice, to use a bus enumeration
> which would be specific to the real hardware version, because I'm using a
> new tools, DTS, which is supposed to simplify my life.
> Sorry to insist, but this incapacity to set the bus number is a *real*
> regression (for me).

> I know I could use the name chosen by the kernel (it mus be something like
> /sys/.../spi253.0/...), but I have no control on it. If tomorrow someone
> changes something in the spi bus number definition, a kernel upgrade will
> have an impact on the user stuff...

We went through exactly the same routine whenever people noticed that
the names for disks weren't as stable as people had been assuming, for
example with the SATA transition from IDE to SCSI, with network devices,
and in a few other places too.  Coming up with fixed names for things
that don't depend on implementation details is really rather hard and
tends to break down eventually either due to software changes or due to
newer hardware.

The other issue with your patch is that setting a bus number is
obviously not a device specific thing, it's something that will apply to
any SPI controller on Linux, and so shouldn't be something driver
specific but should instead be a change to the SPI core.

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Frederic LAMBERT]

>
> > I wrote a piece of code that uses this nvSRAM as a persistent storage.
> > This runs in a softcore embedded in an FPGA, in an equipment.
> > The PC version of this application uses a simple file to emulate this
> > behavior.
> > The simulated version, using an x86 port embedded in a VirtualBox uses a
> > piece of virtual disk with /dev/sdb.
>
> So it's just a basic block device?
>

It is the purpose of AT25, yes.


>  > To sumarize, I have a same code that works for the 3 architecture, the
> > single difference being in the device name:
> > /sys/proc/spi/drivers/at25/spi0.0/eeprom for Nios2
> > dataBase.dat on Linux
> > /dev/sdb on the Virtual Machine
>
> In this case surely the mechanisms used to identify regular disks (UUIDs
> and so on) will also work (the .dat file will have to be an override,
> but otherwise...)?  Names like sdb aren't stable either - they can
> change with either software or hardware changes since they just come
> from the order of discovery.
>

Sure there are mechanisms, but that means that the code must be either
specific to the arch, either have the code for all of them. Not simple !

The other issue with your patch is that setting a bus number is
> obviously not a device specific thing, it's something that will apply to
> any SPI controller on Linux, and so shouldn't be something driver
> specific but should instead be a change to the SPI core.
>

Although I tend to agree you on the fact that SPI bus num is a SPI core
matter, but the fact is that, for now, it is the controller that must fill
the spi_master structure to call spi_master_get(), and that this structure
contains a field 'bus_num' that is always initialized to -1 since DTS...

-- 
Fred
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Wed, Feb 01, 2012 at 04:29:25PM +0100, Frederic LAMBERT wrote:

> >  > To sumarize, I have a same code that works for the 3 architecture, the
> > > single difference being in the device name:
> > > /sys/proc/spi/drivers/at25/spi0.0/eeprom for Nios2
> > > dataBase.dat on Linux
> > > /dev/sdb on the Virtual Machine

> > In this case surely the mechanisms used to identify regular disks (UUIDs
> > and so on) will also work (the .dat file will have to be an override,
> > but otherwise...)?  Names like sdb aren't stable either - they can
> > change with either software or hardware changes since they just come
> > from the order of discovery.

> Sure there are mechanisms, but that means that the code must be either
> specific to the arch, either have the code for all of them. Not simple !

Well, something that walks all block devices on Linux is very
straightforward...  It sounds like you just want logic along the lines
of "find a block device and offer the option of overriding by providing
a specific path for use with a file", is that about right?

> The other issue with your patch is that setting a bus number is
> > obviously not a device specific thing, it's something that will apply to
> > any SPI controller on Linux, and so shouldn't be something driver
> > specific but should instead be a change to the SPI core.

> Although I tend to agree you on the fact that SPI bus num is a SPI core
> matter, but the fact is that, for now, it is the controller that must fill
> the spi_master structure to call spi_master_get(), and that this structure
> contains a field 'bus_num' that is always initialized to -1 since DTS...

This sounds like a simple matter of proramming to change.  Clearly
providing a standard feature of the SPI subsystem will involve
modification of the SPI core, for example to change the interpretation
of -1 to go look at the device tree and override it.

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Frederic LAMBERT]

2012/2/1 Mark Brown <broonie-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>

>
> Well, something that walks all block devices on Linux is very
> straightforward...  It sounds like you just want logic along the lines
> of "find a block device and offer the option of overriding by providing
> a specific path for use with a file", is that about right?
>

Well, yes, excepted that the file *is* equivalent to a block device for my
piece of code!

 > Although I tend to agree you on the fact that SPI bus num is a SPI core
> > matter, but the fact is that, for now, it is the controller that must
> fill
> > the spi_master structure to call spi_master_get(), and that this
> structure
> > contains a field 'bus_num' that is always initialized to -1 since DTS...
>
> This sounds like a simple matter of proramming to change.  Clearly
> providing a standard feature of the SPI subsystem will involve
> modification of the SPI core, for example to change the interpretation
> of -1 to go look at the device tree and override it.
>

Yes, that seems simple said like that.
It was thus more simple for me, at that time, to modify a specific driver
for a specific port (Nios2), than to modify a core used by every one.
Question of time, and confidence in my own capacities :-).
The other point being the change acceptation (and the time it takes) by the
community...

-- 
Fred
------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Grant Likely]

On Wed, Feb 1, 2012 at 8:52 AM, Frederic LAMBERT <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2012/2/1 Mark Brown <broonie-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
>
>>
>> Well, something that walks all block devices on Linux is very
>> straightforward...  It sounds like you just want logic along the lines
>> of "find a block device and offer the option of overriding by providing
>> a specific path for use with a file", is that about right?
>>
>
> Well, yes, excepted that the file *is* equivalent to a block device for my
> piece of code!

The answer here is to use the data provided by the kernel to find the
correct device.  Hard coding bus numbers (or any other globally
visible number space for that matter) has all kinds of problems and it
is by design that spi bus drivers are not allowed to choose a bus
number when they are created from DT data.

Instead, your code should either be using udev, or doing the same
thing that udev does to find a specific hardware device.  The uevent
attribute in devices that appear under /dev/bus/spi/devices should be
used to compare with the full device tree path.  I've just written a
patch that should give you the information that you need and I'll cc:
you when I post it.  I'll try to get it merged for v3.4.  This
approach will allow you to find the correct device in a reliable way
that won't be affected by bus numbering changes.

g.

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Grant Likely]

On Tue, Jan 31, 2012 at 8:54 AM, Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org> wrote:
> From: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Until now we let the code in spi.c assign us the bus number if the platform
> device didn't specify its id. This patch adds the possibility to explicitely
> specify the bus number via device tree.
>
> Signed-off-by: Frederic Lambert <frdrc66-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
> ---
> diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
> index 4813a63..d5bf52b 100644
> --- a/drivers/spi/spi-altera.c
> +++ b/drivers/spi/spi-altera.c
> @@ -226,6 +226,15 @@ static int __devinit altera_spi_probe(struct platform_device *pdev)
>        if (!master)
>                return err;
>
> +       /* bus_num is dynamically assigned when undefined (see spi.c)  */
> +       /* look for a DTS entry to override this */
> +       if (pdev->id == -1 && pdev->dev.of_node != 0) {
> +               u32 tmp;
> +
> +               if (of_property_read_u32(pdev->dev.of_node, "bus-number", &tmp) == 0)
> +                       pdev->id = tmp;

BTW, it is absolutely illegal for device drivers to modify the
pdev->id field.  Once a device is registered, the id and name cannot
be changed.

g.

Re: [PATCH] spi/spi-altera: Allow to explicitely override bus number via dts

[Mark Brown]

On Wed, Feb 01, 2012 at 04:52:14PM +0100, Frederic LAMBERT wrote:

> Yes, that seems simple said like that.
> It was thus more simple for me, at that time, to modify a specific driver
> for a specific port (Nios2), than to modify a core used by every one.
> Question of time, and confidence in my own capacities :-).
> The other point being the change acceptation (and the time it takes) by the
> community...

In this case doing something generic would be easier to get merged
(modulo the "don't do that" issue) - people are being a lot more wary
about driver specific changes that should be generic than they used to
be.