[PATCH] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[PATCH] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[Jens Renner]

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.

Signed-off-by: Jens Renner <renner-Xf229rFC5gsb1SvskN2V4Q@public.gmane.org>
---
 drivers/spi/spi-xilinx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 0bf1b2c..5a6d9c5 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -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

[PATCH][RESEND] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[Jens Renner]

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.

Signed-off-by: Jens Renner <renner-Xf229rFC5gsb1SvskN2V4Q@public.gmane.org>
---
 drivers/spi/spi-xilinx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 0bf1b2c..5a6d9c5 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -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

Re: [PATCH] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[Jens Renner]

Am 10.10.2013 16:12, schrieb Mark Brown:
> On Thu, Oct 10, 2013 at 02:45:58PM +0200, Jens Renner wrote:
> 
>> +		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.

>> @@ -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

Re: [PATCH] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[Trent Piepho]

On Thu, Oct 10, 2013 at 7:50 AM, Jens Renner <renner-Xf229rFC5gsb1SvskN2V4Q@public.gmane.org> wrote:
> Am 10.10.2013 16:12, schrieb Mark Brown:
>> On Thu, Oct 10, 2013 at 02:45:58PM +0200, Jens Renner wrote:
>>
>>> +            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.

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

Re: [PATCH] spi/xilinx: Use DT information for bits_per_word value, fix bus_num value

[Trent Piepho]

On Oct 10, 2013 3:51 PM, "Mark Brown" <broonie-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> wrote:
>
> On Thu, Oct 10, 2013 at 02:44:28PM -0700, Trent Piepho wrote:
>
> > 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?
>
> 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.

>
> > 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