spi/xilinx: Use DT information for bits_per_word value, fix bus_num value
Subsystems (2)
PATCH 1/1 spi/xilinx: Use DT information for bits_per_word value, fix bus_num value
This patch overrides the default value of bits_per_word with the actual value of "xlnx,num-transfer-bits" from the DTS file to allow for 16 and 32 bit word lengths.
Also, bus_num always was (and probably should still be) derived from pdev->id. Otherwise this could lead to problems when using more than one SPI master.
--- drivers/spi/spi-xilinx.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/spi/spi-xilinx.c +3 -1
@@ -356,6 +356,8 @@ static int xilinx_spi_probe(struct platform_device *pdev) }else {of_property_read_u32 (pdev->dev.of_node,"xlnx,num-ss-bits" , &num_cs); +of_property_read_u32 (pdev->dev.of_node,"xlnx,num-transfer-bits" , + &bits_per_word); }if (!num_cs) {
@@ -385,7 +387,7 @@ static int xilinx_spi_probe(struct platform_device *pdev) goto put_master; } - master->bus_num = pdev->dev.id; + master->bus_num = pdev->id; master->num_chipselect = num_cs; master->dev.of_node = pdev->dev.of_node;
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH RESEND 1/1 spi/xilinx: Use DT information for bits_per_word value, fix bus_num value
This patch overrides the default value of bits_per_word with the actual value of "xlnx,num-transfer-bits" from the DTS file to allow for 16 and 32 bit word lengths.
Also, bus_num always was (and probably should still be) derived from pdev->id. Otherwise this could lead to problems when using more than one SPI master.
Tested on our own Spartan-6 / Microblaze platform with two SPI controllers.
--- drivers/spi/spi-xilinx.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/spi/spi-xilinx.c +3 -1
@@ -356,6 +356,8 @@ static int xilinx_spi_probe(struct platform_device *pdev) }else {of_property_read_u32 (pdev->dev.of_node,"xlnx,num-ss-bits" , &num_cs); +of_property_read_u32 (pdev->dev.of_node,"xlnx,num-transfer-bits" , + &bits_per_word); }if (!num_cs) {
@@ -385,7 +387,7 @@ static int xilinx_spi_probe(struct platform_device *pdev) goto put_master; } - master->bus_num = pdev->dev.id; + master->bus_num = pdev->id; master->num_chipselect = num_cs; master->dev.of_node = pdev->dev.of_node;
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Am 10.10.2013 16:12, schrieb Mark Brown:
Quoted 8 lines · Jens Renner
drivers/spi/spi-xilinx.c static int xilinx_spi_probe(struct platform_device *pdev) · line 360
Quoted 3 lines
drivers/spi/spi-xilinx.c +2
-0+of_property_read_u32 (pdev->dev.of_node,"xlnx,num-transfer-bits" , + &bits_per_word);This new property needs to be documented in the binding document (and sent to the DT maintainers for review though in this case it's probably OK).
So far there is no binding documentation for spi/xilinx at all, but I prepared a file spi-xilinx.txt which documents everything. I will send it to the DT maintainers.
Quoted 11 lines
drivers/spi/spi-xilinx.c static int xilinx_spi_probe(struct platform_device *pdev) · line 392
Quoted 8 lines
drivers/spi/spi-xilinx.c +1
-1@@ -385,7 +387,7 @@ static int xilinx_spi_probe(struct platform_device *pdev) goto put_master; } - master->bus_num = pdev->dev.id; + master->bus_num = pdev->id; master->num_chipselect = num_cs; master->dev.of_node = pdev->dev.of_node;This looks like an unrelated change (and buggy?).
Related insofar as I mentioned the modification in the patch title and description. Buggy, yes, I resent the patch ...
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 6 lines · Jens Renner
Am 10.10.2013 16:12, schrieb Mark Brown:
Quoted 5 lines · Jens Renner
Quoted 3 lines
drivers/spi/spi-xilinx.c +2
-0+of_property_read_u32 (pdev->dev.of_node,"xlnx,num-transfer-bits" , + &bits_per_word);
Quoted 7 lines
Quoted 3 lines
This new property needs to be documented in the binding document (and sent to the DT maintainers for review though in this case it's probably OK).
So far there is no binding documentation for spi/xilinx at all, but I prepared a file spi-xilinx.txt which documents everything. I will send it to the DT maintainers.
In the normal Linux SPI layer, bits_per_word is a field for a spi_device (slave). Different slaves on the same master could use different bits per word values, which can be changed by the slave for different transfers.
It looks like maybe this master can only support one bits_per_word value? But unlike many devices that just support 8-bits, it can support one of 8, 16, or 32 depending on "something", and that something is specified in the device tree here? No way to query the device?
Because if you're just trying to set the default bits per word value it shouldn't be a property of the master, but rather a generic slave property like spi-max-frequency or spi-cs-high.
Anyway, the driver should use this device tree/platform data property to set the master's bits_per_word_mask field. Then you can remove the bits_per_word checking code from the driver (in xilinx_spi_setup_transfer()) since the spi core will do it for you.
BTW, you might consider what happens if a new version of the IP supports multiple bits_per_words when deciding on how the DT property should be design. Maybe a list of values or a mask?
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Mark Brown
Quoted 5 lines · Trent Piepho
It looks like maybe this master can only support one
bits_per_wordvalue? But unlike many devices that just support 8-bits, it can support one of 8, 16, or 32 depending on "something", and that something is specified in the device tree here? No way to query the device?
Quoted 6 lines
I have to say that (not having looked at the driver or the patch really) I'd have hoped that this was giving a maximum not a fixed number, if the hardware is wired to only support a single value that's anything other than 8 bits per word most devices won't work.
From my look at the driver, it's definitely a single fixed value. I don't
know this hardware, but my guess is it's chosen when a FPGA image is designed. So the hardware itself is not strictly fixed, but Linux is stuck with whatever the IP core was configured as when the image was made. Maybe someone from Xilinx can clarify.
Quoted 8 lines
Quoted 4 lines
Because if you're just trying to set the default bits per word value it shouldn't be a property of the master, but rather a generic slave property like spi-max-frequency or spi-cs-high.
It shouldn't be a property at all I'd expect, I'd expect it to be something that's hard coded into the client driver mostly - unless the controller really is brain dead.
Probably why no one's needed the property. There is spidev, which wouldn't know what value to use, but that can be set via ioctl, which makes more sense.
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk