Hi Adrian,
On Mon, Jun 15, 2020 at 4:06 PM Adrian Fiergolski
<adrian.fiergolski@xxxxxxxxxxxxx> wrote:
> Sorry for the typo in the example device tree:
>
> On 15.06.2020 15:57, Adrian Fiergolski wrote:
> > On 15.06.2020 15:07, Geert Uytterhoeven wrote:
> >> On Mon, Jun 15, 2020 at 3:01 PM Adrian Fiergolski
> >> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
> >>> On 13.06.2020 09:33, Geert Uytterhoeven wrote:
> >>>> On Fri, Jun 12, 2020 at 6:26 PM Adrian Fiergolski
> >>>> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
> >>>> I have a daisy chain of three ltc2634 slaves (iio/dac/ltc2632.c)
> >>>> connected to a single chip select of the cadence-spi master. I have the
> >>>> impression such a configuration is supported by none of those two
> >>>> drivers. I could try to extend both, however, I haven't found any other
> >>>> SPI driver, where I could find implementation inspiration. Is it
> >>>> supported by kernel?
> >>>>
> >>>> drivers/gpio/gpio-max3191x.c supports "#daisy-chained-devices".
> >>>> drivers/gpio/gpio-74x164.c supports multiple shift registers through the
> >>>> "registers-number" DT property.
> >>>>
> >>>> So both drivers handle this in their SPI slave drivers.
> >>>>
> >>>> Of course this does not handle the mixed case, i.e. daisy-chaining
> >>>> different types of devices.
> >>>>
> >>>> The documentation mentions only about the common 'daisy-chained-devices'
> >>>> property (devicetree/bindings/common-properties.txt). However, in order
> >>>> to try to implement it in the master driver, IMHO, the spi subsystem
> >>>> would need to have a call 'no-operation' to other nodes on the
> >>>> daisy-chain, which are not addressed by the given SPI access. Is there
> >>>> any recommended approach to address this case?
> >>>>
> >>>> Supporting this in a generic way would indeed be nice, as it would mean
> >>>> individual SPI slave drivers no longer have to care about it.
> >>>> However, that may be difficult, as the master needs to known which
> >>>> dummy (no-op) data is safe to shift through the non-addresses SPI slaves.
> >>> In fact, the ultimate solution would be to have it solved at the level
> >>> of the spi subsystem:
> >>>
> >>> * /spi_device struct/ would contain extra callback which returns msg
> >>> to be sent for no operation.
> >>> * spi_device struct would contain a pointer to the list describing the
> >>> given daisy chain (list of spi_devices on the chain)
> >>> * /spi_device struct /would contain extra u8 daisy_chain_msg_length
> >>> indicating length of a command of the addressed device if it's on
> >>> the daisy chain
> >>> For example, in case of the ltc2634 device, the regular message
> >>> consists of 24 bits, but when device is a part of a daisy chain, the
> >>> messages are 32 bits. This 32 would be stored in
> >>> /daisy_chain_msg_length./
> >>> * When /spi_write/ was called (include/linux/spi/spi.h), the
> >>> /spi_message_init_with_transfer/ would create a msg of length equal
> >>> to a sum of /daisy_chain_msg_length/ of all devices on the chain.
> >>> Afterwards, in /spi_message_init_with_transfers/, the actual message
> >>> would be filled with the command of the addressed device on the
> >>> chain and no_operation content for all other devices on the chain
> >>> not being addressed
> >> Sounds good to me.
> >>
> >>> * I think in such a case, the /daisy-chained-devices /property would
> >>> be not used, as chains would be build basing on the assigned
> >>> chipselect (reg property).
> >> So you still have to describe the chain in DT in some way.
> >> As there can be only a single sub node with the same unit address
> >> (= chip select), you probably need a container with that address, which
> >> would contain all devices in the chain, in order (unit addresses 0, 1, ...).
> > Good point. So maybe at the level of the device tree, it could be
> > described like that (based on the spi-cadence example):
> >
> > spi0: spi@ff040000 {
> > compatible = "cdns,spi-r1p6";
> > status = "disabled";
> > interrupt-parent = <&gic>;
> > interrupts = <0 19 4>;
> > reg = <0x0 0xff040000 0x0 0x1000>;
> > clock-names = "ref_clk", "pclk";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > power-domains = <&zynqmp_firmware PD_SPI_0>;
> > daisy-chain0 : daisy_chain@0 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>;
> > daisy-chained-devices = 2;
> >
> > dac0: ltc2632@0 {
> > compatible = "lltc,ltc2634-l12";
> > reg = <0>;
> > spi-max-frequency = <1000000>;
> > };
> > dac1: ltc2632@1 {
> > compatible = "lltc,ltc2634-l12";
> > reg = <1>;
> > spi-max-frequency = <2000000>;
> > };
> > };
> > };
>
> spi0: spi@ff040000 {
> compatible = "cdns,spi-r1p6";
> status = "disabled";
> interrupt-parent = <&gic>;
> interrupts = <0 19 4>;
> reg = <0x0 0xff040000 0x0 0x1000>;
> clock-names = "ref_clk", "pclk";
> #address-cells = <1>;
> #size-cells = <0>;
> power-domains = <&zynqmp_firmware PD_SPI_0>;
> daisy-chain0 : daisy_chain@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> #daisy-chained-devices = <2>;
You probably want a proper "compatible" value here instead.
I don't think "#daisy-chained-devices" is needed, it can be derived
from the number of subnodes.
>
> reg = <0>;
>
> dac0: ltc2632@0 {
> compatible = "lltc,ltc2634-l12";
> reg = <0>;
> spi-max-frequency = <1000000>;
> };
> dac1: ltc2632@1 {
> compatible = "lltc,ltc2634-l12";
> reg = <1>;
> spi-max-frequency = <2000000>;
> };
> };
> };
>
> >
> > Once a node has daisy-chanied-devices property defined,
> > of_register_spi_device (spi.c) will interpret it as a daisy chain. I
> > will assume, that for the given chain the lowest frequency of the whole
> > chain should be used. When it comes to the mode, as in case of
> > incompatibility no much can be done anyway, the mode of the addressed
> > spi device will be used.
Don't the modes have to agree, too?
Else the dummy command may be interpreted differently than intended.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
