Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1599662 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] spi: Add spi-bits-per-word binding. Mark Brown <broonie@kernel.org> - 2017-03-13 19:00 +0100
    Re: [PATCH] spi: Add spi-bits-per-word binding. Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-13 21:00 +0100
      Re: [PATCH] spi: Add spi-bits-per-word binding. Mark Brown <broonie@kernel.org> - 2017-03-17 22:30 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [next] | [standalone]

#1599744

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

[toc] | [prev] | [next] | [standalone]

#1603630

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web