Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl <martin@xxxxxxxxx>]

> On 19.11.2015, at 18:15, Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
>> I am in the progress of writing a (minimal) spi-core extension, that would allow 
>> a spi_master to have a prepare_message function that would call some “message
>> translation functions”.
> 
>> The ones I have currently come up with are:
>> int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
>> int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);
> 
> Please don't do things this way, please make this more data driven -
> have the drivers declare their capabilities so the core can just do
> these things based on those flags rather than requiring active code in
> drivers.  This keeps the code central which in turn helps keep things
> maintainable.
> 

Well - I was thinking about starting with this approach for simplicity.

Making it automatic/data-driven by just defining limits that the core
can then handle transparently is something that could come as a next
step.

Also the bus master driver knows what its limitations are, so putting
them inside prepare_message seems reasonable to me - that is unless
you really have spi_device drivers that would handle those as separate
facts and not refuse to work.


So I wonder how much difference it makes:
int driver_prepare_message (struct spi_master *master, 
                            struct spi_message *msg)
{
  int ret;

  if (ret = spi_split_transfer_alignment(msg, 4))
	ret ret;
  if (ret = spi_split_transfer_maxsize(msg, 65534))
	ret ret
  return 0;
}

driver_probe() {
  ...
  master->prepare_message = driver_prepare_message;
  ...
}

or:
driver_probe() {
  ...
  master->transfer_alignment = 4;
  master->max_transfer_size = 65534; 
  ...
}

both work, but parametrizing may become a pain when more
limitations start to get created.

In my example when looking at the situation where we only care for
alignment when the transfer is valid for DMA (PIO does not care).

so you would need to add something like:
  master->transfer_alignment_min_size = 96;

to handle such “extra” requirements for the spi_master.
(Sometimes ordering of “rules” may be important as well)

So the function inside the core we could then look like this:

int spi_core_prepare_message (struct spi_master *master, 
                              struct spi_message *msg)
{
  int ret;

  if (master->transfer_alignment) {
    ret = spi_split_transfer_alignment(msg,
				       master->transfer_alignment,
				       master->transfer_alignment_min_size);
    if (ret)
      ret ret;
  }

  if (master->transfer_alignment) {
    ret = spi_split_transfer_maxsize(msg,
				     master->max_transfer_size);
    if (ret)
      ret ret;
  }
  return 0
}

Something like the above I envision as a second step,
but first start to make it work...

>> I guess this is a more transparent approach that does not require the
>> individual device drivers to query the spi_master about limitations
>> and replicate code in several drivers - also there is no maintenance cost on
>> individual device drivers to track when there is a new limitation introduced.
> 
> You've got this back to front - drivers are responsible for initialising
> the master when they instantiate it.  There's nothing stopping them
> using the data if they have variants to handle but they shouldn't be
> speculatively looking at it on the off chance there's some limit.

I wonder if such a variant-construct does not create more headaches in
the long run as each spi_driver wanting to do something “something”
special would then need to get updated for any additional constraint…

It seems as if we would need then to add 2 kinds of constraints:
* ones that may be of interest to spi_device to avoid unfavourable 
  situations created by the core implementation (max_transfer_sizes)
* ones that are handled by spi-core
  (like the alignment constraints)


Martin




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