| Message ID | 5256A186.5030708@efe-gmbh.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
On Thu, Oct 10, 2013 at 7:50 AM, Jens Renner <renner@efe-gmbh.de> 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
On Oct 10, 2013 3:51 PM, "Mark Brown" <broonie@sirena.org.uk> 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
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;
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@efe-gmbh.de> --- drivers/spi/spi-xilinx.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) ------------------------------------------------------------------------------ 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