[PATCH] spi: Add spi-bits-per-word binding.

[PATCH] spi: Add spi-bits-per-word binding.

[Adrian Fiergolski]

If an SPI controller doesn't support 8 bit transfers
(master->bits_per_word_mask), it will be never registered (tested with
spidev):
of_register_spi_device calls spi_add_device which calls spi_setup. The last
takes as an argument spi_device struct, which, in case of spidev, has
bits_per_word set to 0. Thus, the spi_setup function will set it to default
8. Further, the same function will call __spi_validate_bits_per_word which
will fail the whole registration for controllers not supporting 8 bit
transfers (i.e. xilinx-spi).

Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt | 40 ++++++++++++-----------
 drivers/spi/spi.c                                 | 18 ++++++++++
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 4b1d6e7..8401741 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -43,26 +43,28 @@ cs3 : &gpio1 2 0
 
 SPI slave nodes must be children of the SPI master node and can
 contain the following properties.
-- reg             - (required) chip select address of device.
-- compatible      - (required) name of SPI device following generic names
-		recommended practice.
+- reg               - (required) chip select address of device.
+- compatible        - (required) name of SPI device following generic names
+		      recommended practice.
 - spi-max-frequency - (required) Maximum SPI clocking speed of device in Hz.
-- spi-cpol        - (optional) Empty property indicating device requires
-		inverse clock polarity (CPOL) mode.
-- spi-cpha        - (optional) Empty property indicating device requires
-		shifted clock phase (CPHA) mode.
-- spi-cs-high     - (optional) Empty property indicating device requires
-		chip select active high.
-- spi-3wire       - (optional) Empty property indicating device requires
-		3-wire mode.
-- spi-lsb-first   - (optional) Empty property indicating device requires
-		LSB first mode.
-- spi-tx-bus-width - (optional) The bus width (number of data wires) that is
-                      used for MOSI. Defaults to 1 if not present.
-- spi-rx-bus-width - (optional) The bus width (number of data wires) that is
-                      used for MISO. Defaults to 1 if not present.
-- spi-rx-delay-us  - (optional) Microsecond delay after a read transfer.
-- spi-tx-delay-us  - (optional) Microsecond delay after a write transfer.
+- spi-cpol          - (optional) Empty property indicating device requires
+		      inverse clock polarity (CPOL) mode.
+- spi-cpha          - (optional) Empty property indicating device requires
+		      shifted clock phase (CPHA) mode.
+- spi-cs-high       - (optional) Empty property indicating device requires
+		      chip select active high.
+- spi-3wire         - (optional) Empty property indicating device requires
+		      3-wire mode.
+- spi-lsb-first     - (optional) Empty property indicating device requires
+		      LSB first mode.
+- spi-tx-bus-width  - (optional) The bus width (number of data wires) that is
+                       used for MOSI. Defaults to 1 if not present.
+- spi-rx-bus-width  - (optional) The bus width (number of data wires) that is
+                       used for MISO. Defaults to 1 if not present.
+- spi-rx-delay-us   - (optional) Microsecond delay after a read transfer.
+- spi-tx-delay-us   - (optional) Microsecond delay after a write transfer.
+- spi-bits-per-word - (optional) Word size for a data transfer. Defaults is 8
+                      if not present.
 
 Some SPI controllers and devices support Dual and Quad SPI transfer mode.
 It allows data in the SPI system to be transferred using 2 wires (DUAL) or 4
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 656dd3e..57fe058 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1594,6 +1594,17 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 	}
 	spi->max_speed_hz = value;
 
+	/* Device bits-per-word */
+	if (!of_property_read_u32(nc, "spi-bits-per-word", &value)) {
+		if (value > 32)
+			dev_warn(&master->dev,
+				 "bits-per-word %d not supported\n",
+				 value);
+		else
+			spi->bits_per_word = value;
+	}
+
+
 	/* Store a pointer to the node in the device structure */
 	of_node_get(nc);
 	spi->dev.of_node = nc;
@@ -1673,6 +1684,13 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 
 			spi->max_speed_hz = sb->connection_speed;
 
+			if (sb->data_bit_length > 32)
+				dev_warn(&master->dev,
+					 "bits-per-word %d not supported\n",
+					 sb->data_bit_length);
+			else
+				spi->bits_per_word = sb->data_bit_length;
+
 			if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
 				spi->mode |= SPI_CPHA;
 			if (sb->clock_polarity == ACPI_SPI_START_HIGH)
-- 
2.9.3

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Mon, Feb 20, 2017 at 04:35:12PM +0100, Adrian Fiergolski wrote:
> If an SPI controller doesn't support 8 bit transfers
> (master->bits_per_word_mask), it will be never registered (tested with
> spidev):
> of_register_spi_device calls spi_add_device which calls spi_setup. The last
> takes as an argument spi_device struct, which, in case of spidev, has
> bits_per_word set to 0. Thus, the spi_setup function will set it to default
> 8. Further, the same function will call __spi_validate_bits_per_word which
> will fail the whole registration for controllers not supporting 8 bit
> transfers (i.e. xilinx-spi).

This doens't make much sense, if a device driver requires a given number
of bits per word forcing a different value via device tree is not going
to result in the driver actually working.  If you want to use a device
with a controller with restricted bits per word support you need to
change the device driver to support the limited capabilities of the
controller.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Adrian Fiergolski]

Hi Mark,

I think, we misunderstand. Any line of spidev (or device driver in
general) is called, if master, setting its bit_per_word_mask, doesn't
cover 8 bits per word. Thus, in such a case, any device driver has a
possibility to be registered and handle the "limited capabilities of the
controller". IMHO, the problem is in the spi core itself. I will try to
be more clear.

In my case, xilinx_spi_probe function (of spi-xilinx controller) sets
bits_per_word_mask of spi_master struct only to 16 bits support. Later,
xilinx_spi_probe calls of_register_spi_devices, which calls
of_register_spi_devices. The last one allocates an empty spi_device
struct and configures different options of the spi_device according to a
device tree. bits_per_word are not covered here (why?), thus it is left
0 (value after allocation), which, by convention, means 8 bits support.
At the end, the same function (of_register_spi_device) calls
spi_add_device which finally calls spi_setup. The last call, according
to convention, changes bits_per_word to 8 and calls
__spi_validate_bits_per_word which fails, as master doesn't support 8
bit transmission. This fails registration sequence of a device driver.
As you see, the device driver doesn't have possibility to modify
bits_per_word during the registration process, thus it can't provide
support for such limited controllers.

Regards,
Adrian


On 21.02.2017 at 20:08, Mark Brown wrote:
> On Mon, Feb 20, 2017 at 04:35:12PM +0100, Adrian Fiergolski wrote:
>> If an SPI controller doesn't support 8 bit transfers
>> (master->bits_per_word_mask), it will be never registered (tested with
>> spidev):
>> of_register_spi_device calls spi_add_device which calls spi_setup. The last
>> takes as an argument spi_device struct, which, in case of spidev, has
>> bits_per_word set to 0. Thus, the spi_setup function will set it to default
>> 8. Further, the same function will call __spi_validate_bits_per_word which
>> will fail the whole registration for controllers not supporting 8 bit
>> transfers (i.e. xilinx-spi).
> This doens't make much sense, if a device driver requires a given number
> of bits per word forcing a different value via device tree is not going
> to result in the driver actually working.  If you want to use a device
> with a controller with restricted bits per word support you need to
> change the device driver to support the limited capabilities of the
> controller.

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

On Mon, Mar 13, 2017 at 06:25:53PM +0100, Adrian Fiergolski wrote:
> Hi Mark,

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets
> bits_per_word_mask of spi_master struct only to 16 bits support. Later,
> xilinx_spi_probe calls of_register_spi_devices, which calls
> of_register_spi_devices. The last one allocates an empty spi_device
> struct and configures different options of the spi_device according to a
> device tree. bits_per_word are not covered here (why?), thus it is left
> 0 (value after allocation), which, by convention, means 8 bits support.
> At the end, the same function (of_register_spi_device) calls
> spi_add_device which finally calls spi_setup. The last call, according
> to convention, changes bits_per_word to 8 and calls
> __spi_validate_bits_per_word which fails, as master doesn't support 8
> bit transmission. This fails registration sequence of a device driver.
> As you see, the device driver doesn't have possibility to modify
> bits_per_word during the registration process, thus it can't provide
> support for such limited controllers.

I can't see any way in which it follows from the above that it's a good
idea to try to override bits per word settings in the device tree, that
just wastes user time and is an abstraction failure.  We need better
handling of defaults done purely in the kernel.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Adrian Fiergolski]

On 13.03.2017 at 18:55, Mark Brown wrote:
>
>> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets
>> bits_per_word_mask of spi_master struct only to 16 bits support. Later,
>> xilinx_spi_probe calls of_register_spi_devices, which calls
>> of_register_spi_devices. The last one allocates an empty spi_device
>> struct and configures different options of the spi_device according to a
>> device tree. bits_per_word are not covered here (why?), thus it is left
>> 0 (value after allocation), which, by convention, means 8 bits support.
>> At the end, the same function (of_register_spi_device) calls
>> spi_add_device which finally calls spi_setup. The last call, according
>> to convention, changes bits_per_word to 8 and calls
>> __spi_validate_bits_per_word which fails, as master doesn't support 8
>> bit transmission. This fails registration sequence of a device driver.
>> As you see, the device driver doesn't have possibility to modify
>> bits_per_word during the registration process, thus it can't provide
>> support for such limited controllers.
> I can't see any way in which it follows from the above that it's a good
> idea to try to override bits per word settings in the device tree, that
> just wastes user time and is an abstraction failure.  We need better
> handling of defaults done purely in the kernel.
If enforcing by device tree specific for a given device driver SPI_CPHA,
SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction
point of view, why it doesn't apply to bits_per_word ?

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Geert Uytterhoeven]

Hi Adrian,

On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski
<Adrian.Fiergolski@cern.ch> wrote:
> On 13.03.2017 at 18:55, Mark Brown wrote:
>>> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets
>>> bits_per_word_mask of spi_master struct only to 16 bits support. Later,
>>> xilinx_spi_probe calls of_register_spi_devices, which calls
>>> of_register_spi_devices. The last one allocates an empty spi_device
>>> struct and configures different options of the spi_device according to a
>>> device tree. bits_per_word are not covered here (why?), thus it is left
>>> 0 (value after allocation), which, by convention, means 8 bits support.
>>> At the end, the same function (of_register_spi_device) calls
>>> spi_add_device which finally calls spi_setup. The last call, according
>>> to convention, changes bits_per_word to 8 and calls
>>> __spi_validate_bits_per_word which fails, as master doesn't support 8
>>> bit transmission. This fails registration sequence of a device driver.
>>> As you see, the device driver doesn't have possibility to modify
>>> bits_per_word during the registration process, thus it can't provide
>>> support for such limited controllers.
>> I can't see any way in which it follows from the above that it's a good
>> idea to try to override bits per word settings in the device tree, that
>> just wastes user time and is an abstraction failure.  We need better
>> handling of defaults done purely in the kernel.
> If enforcing by device tree specific for a given device driver SPI_CPHA,
> SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction
> point of view, why it doesn't apply to bits_per_word ?

Because unlike polarity, phase, and speed, bits_per_word is a property
of the communication protocol.

E.g. you can talk to the same EEPROM using different polarities, phase, or
speed, but bits_per_word is fixed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Adrian Fiergolski]

Hi Geert,

On 13.03.2017 at 20:57, Geert Uytterhoeven wrote:
> On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski
> <Adrian.Fiergolski@cern.ch> wrote:
>> On 13.03.2017 at 18:55, Mark Brown wrote:
>>>> In my case, xilinx_spi_probe function (of spi-xilinx controller) sets
>>>> bits_per_word_mask of spi_master struct only to 16 bits support. Later,
>>>> xilinx_spi_probe calls of_register_spi_devices, which calls
>>>> of_register_spi_devices. The last one allocates an empty spi_device
>>>> struct and configures different options of the spi_device according to a
>>>> device tree. bits_per_word are not covered here (why?), thus it is left
>>>> 0 (value after allocation), which, by convention, means 8 bits support.
>>>> At the end, the same function (of_register_spi_device) calls
>>>> spi_add_device which finally calls spi_setup. The last call, according
>>>> to convention, changes bits_per_word to 8 and calls
>>>> __spi_validate_bits_per_word which fails, as master doesn't support 8
>>>> bit transmission. This fails registration sequence of a device driver.
>>>> As you see, the device driver doesn't have possibility to modify
>>>> bits_per_word during the registration process, thus it can't provide
>>>> support for such limited controllers.
>>> I can't see any way in which it follows from the above that it's a good
>>> idea to try to override bits per word settings in the device tree, that
>>> just wastes user time and is an abstraction failure.  We need better
>>> handling of defaults done purely in the kernel.
>> If enforcing by device tree specific for a given device driver SPI_CPHA,
>> SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction
>> point of view, why it doesn't apply to bits_per_word ?
> Because unlike polarity, phase, and speed, bits_per_word is a property
> of the communication protocol.
>
> E.g. you can talk to the same EEPROM using different polarities, phase, or
> speed, but bits_per_word is fixed.
In this case, currently, what is the proper way to handle SPI
controllers (spi-xilinx) without 8-bit transmission support ?

Re: [PATCH] spi: Add spi-bits-per-word binding.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Mon, Mar 13, 2017 at 09:26:04PM +0100, Adrian Fiergolski wrote:
> On 13.03.2017 at 20:57, Geert Uytterhoeven wrote:
> > On Mon, Mar 13, 2017 at 7:12 PM, Adrian Fiergolski
d
> >>> I can't see any way in which it follows from the above that it's a good
> >>> idea to try to override bits per word settings in the device tree, that
> >>> just wastes user time and is an abstraction failure.  We need better
> >>> handling of defaults done purely in the kernel.

> >> If enforcing by device tree specific for a given device driver SPI_CPHA,
> >> SPIC_CPOL, SPI_CS_HIGH, max_speed_hz, etc. if fine form the abstraction
> >> point of view, why it doesn't apply to bits_per_word ?

> > Because unlike polarity, phase, and speed, bits_per_word is a property
> > of the communication protocol.

> > E.g. you can talk to the same EEPROM using different polarities, phase, or
> > speed, but bits_per_word is fixed.

> In this case, currently, what is the proper way to handle SPI
> controllers (spi-xilinx) without 8-bit transmission support ?

As I said above we should fix the handling of defaults such that it is
possible to instantiate a 16 bit using device on a 16 bit supporting
controller; there should be no need to have anything about device tree
in this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]