* [PATCH] spi: Add spi-bits-per-word binding. [not found] <20170217101151.hyzmz5a4lgdy7p5m> @ 2017-02-20 15:35 ` Adrian Fiergolski 2017-02-21 19:08 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Adrian Fiergolski @ 2017-02-20 15:35 UTC (permalink / raw) To: linux-spi Cc: broonie, robh+dt, mark.rutland, devicetree, linux-kernel, miguel.ojeda.sandonis, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-02-20 15:35 ` [PATCH] spi: Add spi-bits-per-word binding Adrian Fiergolski @ 2017-02-21 19:08 ` Mark Brown 2017-03-13 17:25 ` Adrian Fiergolski 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2017-02-21 19:08 UTC (permalink / raw) To: Adrian Fiergolski Cc: linux-spi, robh+dt, mark.rutland, devicetree, linux-kernel, miguel.ojeda.sandonis [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-02-21 19:08 ` Mark Brown @ 2017-03-13 17:25 ` Adrian Fiergolski 2017-03-13 17:55 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Adrian Fiergolski @ 2017-03-13 17:25 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, robh+dt, mark.rutland, devicetree, linux-kernel, miguel.ojeda.sandonis 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-03-13 17:25 ` Adrian Fiergolski @ 2017-03-13 17:55 ` Mark Brown 2017-03-13 18:12 ` Adrian Fiergolski 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2017-03-13 17:55 UTC (permalink / raw) To: Adrian Fiergolski Cc: linux-spi, robh+dt, mark.rutland, devicetree, linux-kernel, miguel.ojeda.sandonis [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-03-13 17:55 ` Mark Brown @ 2017-03-13 18:12 ` Adrian Fiergolski 2017-03-13 19:57 ` Geert Uytterhoeven 0 siblings, 1 reply; 8+ messages in thread From: Adrian Fiergolski @ 2017-03-13 18:12 UTC (permalink / raw) To: Mark Brown Cc: linux-spi, robh+dt, mark.rutland, devicetree, linux-kernel, miguel.ojeda.sandonis 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 ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-03-13 18:12 ` Adrian Fiergolski @ 2017-03-13 19:57 ` Geert Uytterhoeven 2017-03-13 20:26 ` Adrian Fiergolski 0 siblings, 1 reply; 8+ messages in thread From: Geert Uytterhoeven @ 2017-03-13 19:57 UTC (permalink / raw) To: Adrian Fiergolski Cc: Mark Brown, linux-spi, Rob Herring, Mark Rutland, devicetree, linux-kernel, Miguel Ojeda Sandonis 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-03-13 19:57 ` Geert Uytterhoeven @ 2017-03-13 20:26 ` Adrian Fiergolski 2017-03-17 21:21 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Adrian Fiergolski @ 2017-03-13 20:26 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Mark Brown, linux-spi, Rob Herring, Mark Rutland, devicetree, linux-kernel, Miguel Ojeda Sandonis 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 ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] spi: Add spi-bits-per-word binding. 2017-03-13 20:26 ` Adrian Fiergolski @ 2017-03-17 21:21 ` Mark Brown 0 siblings, 0 replies; 8+ messages in thread From: Mark Brown @ 2017-03-17 21:21 UTC (permalink / raw) To: Adrian Fiergolski Cc: Geert Uytterhoeven, linux-spi, Rob Herring, Mark Rutland, devicetree, linux-kernel, Miguel Ojeda Sandonis [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-17 21:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20170217101151.hyzmz5a4lgdy7p5m> 2017-02-20 15:35 ` [PATCH] spi: Add spi-bits-per-word binding Adrian Fiergolski 2017-02-21 19:08 ` Mark Brown 2017-03-13 17:25 ` Adrian Fiergolski 2017-03-13 17:55 ` Mark Brown 2017-03-13 18:12 ` Adrian Fiergolski 2017-03-13 19:57 ` Geert Uytterhoeven 2017-03-13 20:26 ` Adrian Fiergolski 2017-03-17 21:21 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).