Additional SPI-master parameters per SPI-Device in device-tree

Hi!

How could we implement additional per spi-device parameters that are interesting to the spi-master.

Examples for the spi-bcm2835 master for example could be:
* set minimum transfer size for dma to something else but the default (of 96 bytes - setting for something high would disable it)
* change polling transfer limits (i.e disable it)

Implementing these on a spi-master level is easy during probe.

So I wonder if the following approach would be acceptable to implement device-tree parsing on the device for master specific parameters:

int bcm2835_spi_setup(struct spi_device *spi) {
struct bcm2835_spi_dev *cs = spi->controller_state;

  if (!cs) {
    cs = devm_kzalloc(&spi->dev, sizeof(struct bcm2835_spi_dev), 0);
    if (!cs)
        return -ENOMEM;
    spi->controller_state = cs;

    /* parse device-tree and assign to controller_state */
    cs->polling_limit = master_bs->polling_limit;
    of_property_read_u32(nc, "spi-bcm2835-polling-limit", 
&cs->polling_limit);
    cs->dma_limit = master_bs->dma_limit;
    of_property_read_u32(nc, "spi-bcm2835-dma-limit", &cs->polling_limit);

}
...
return 0;
}

Or would it be better if we included an additional master method that would get called from spi_add_device to do this kind of setup? say:
int add_device(struct spi_master *master, struct spi_device *spi);
(possibly also one called when removing the device)

Thanks,
Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Define parameters for the slave and read them from the slave device node at some appropriate point. Please send patches for review, your sketches look fine without context but when written as a proper patch other issues may become obvious. Given that setup() gets called repeatedly a one time call may be more sensible.

Why would these things be configured in the device tree in the first place? They don't look like things that should be device tree properties, they look like something the kernel should figure out at runtime - the system tuning may change as the kernel evolves or depending on userspace needs.

Sometimes it may be necessary to define different values than the "kernel" thinks may be necessary (hardcoded).

We just had one issue with fbtft devices where it would have been nice to change those values on a per device basis (even if it was just for some testing to see why it was behaving badly
- see the still un-merged patch:
"spi: bcm2835: set up spi-mode before asserting cs-gpio").

So I first thought of adding it as a module parameter for the driver and then exposing it via sysfs for modification, but now I think it might be even more beneficial to "tune" the parameters per device.

Say for fbtft devices to use DMA for any xfer-size above 4 bytes. But that policy may not apply to the corresponding touch screen device also connected to the same SPI bus.

But then - I now come to realize - that this "dt-property" idea may again break the "philosophy" of "device-tree only contains a description of the HW".

So maybe a per device sysfs approach together with udev-rules to change those settings may be the better approach.

But even those may require those device registration hooks I had mentioned.

So I was wondering what was feasible/acceptable.

Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

So improve the kernel or (if you're really desparate) provide runtime configuration - if you're just randomly bashing on things you can rebuild the kernel just as well as the DT. We don't want random magic number performance tuning becoming an ABI, that way if the kernel changes we're stuck with trying to work with the old magic numbers even if they no longer work or perform well.

I'm not finding that patch particularly enlightening here, sorry... can you be more explicit please?

There are properties that might make sense on a per device basis like descriptions of which chip select to use in the hardware, it's just that these don't seem like such properties.

Well, while trying to figure the issue out we tried to disable or tune different parameters to see where the issue really came from.

At that time we had to recompile the kernel several times, which is a much longer turn-arround than tuning parameters on the fly.

In the end the issue (which the patch fixes) turned out not to be related to any of those parameters, but a default-clock-polarity change while GPIO-CS was already asserted resulting in an extra level-change on the clock getting interpreted by the HW as a clock change, which the above mentioned patch fixes by setting up polarity before GPIO-CS.

That is where the idea of configuring it in DT or via sysfs came from.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Right, and runtime tuning is much easier since it can be done with less of a strong ABI than the DT.