Re: [PATCH] spi: spi-fsl-dspi: Add ACPI support

[Vladimir Oltean <olteanv@xxxxxxxxx>]

On Wed, Aug 26, 2020 at 11:10:49AM +0000, Qiang Zhao wrote:
> On Sat, Aug 22, 2020 at 23:21PM, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> > Yes, definitely bloatware from the old days. I think this driver needs the existing
> > device tree bindings rethought a little bit before mindlessly porting them to
> > ACPI.
>
> Could you give more details?
>
> Best Regards
> Qiang Zhao

Yes.
This driver has some device tree bindings.
Some thought need to be given as to which one of those is necessary for
a functional ACPI setup, and which one isn't.
For example:

- fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay are many times
  necessary. I don't see an attempt to read something equivalent to
  those in this patch, or to do something about those, otherwise, in
  case a peripheral needs special treatment. If we want to do something
  like e.g. deprecate these bindings and just set up a large enough
  CS-to-SCK and SCK-to-CS delay to make every peripheral happy, in order
  to not carry this binding over to ACPI, at least we should establish
  that and do it now, so that the DT code can benefit from that as well.

- The bus-num property was made optional by Sascha Hauer in commit
  29d2daf2c33c ("spi: spi-fsl-dspi: Make bus-num property optional").
  I think this is because he couldn't just remove it completely. But
  that doesn't mean we should carry it over to ACPI. The SPI core should
  know to allocate a bus_num dynamically (using IDR, or by looking at
  aliases) if we just set spi->bus_num = -1.

- The spi-num-chipselects can be deduced from compatible string and bus
  number, and therefore we can avoid carrying it over to ACPI. But
  again, DT should have this logic first, and then ACPI can be added.

- The compatible string plays an integral part in the functionality of
  the spi-fsl-dspi driver. I want to see a solution for ACPI where the
  driver knows on which SoC it's running on. Otherwise it doesn't know
  what are the silicon parameters of the DSPI module (XSPI present or
  not, DMA present or not, FIFO depth). I don't see that now. I just see
  something hardcoded for:
  { "NXP0005", .driver_data = (kernel_ulong_t)&devtype_data[LS2085A], }

Thanks,
-Vladimir