Re: [spi-devel-general] [PATCH] Adapt spi_mpc83xx SPI driver for 832x

[David Brownell <david-b@pacbell.net>]

On Wednesday 13 December 2006 7:36 am, Joakim Tjernlund wrote:

> > One problem I have with this patch is the fact that it assumes the  
> > current driver for (mpc834x) and your mods to support mpc832x/QE are  
> > mutually exclusive.
> 
> I don't see that as a problem ATM. If that is added it should be optional.

If PPC supports multi-CPU/board kernels, PPC drivers should too.  I know that
some ARMs don't support them (PXA) while some do (OMAP); so PPC could go either
way.  (Overall it's a win, since it's easier to uncover build breakage if one
kernel can support lots of configurations and drivers.)

The problem I have with this patch is that it has too much #ifdeffery.  If
the PPC code expects that, it should be easy to put infrastructure in place
so that it can be eliminated.  That is, if the PPC powers-that-be allow it!


> > We need to handle the case of having driver support for both the QE  
> > and MPC834x style in the same kernel.
> 
> Adding that will double the number RX_BUF/TX_BUF functions from 6 to 12

Your patch already does this, just they're #ifdeffed so that only one of
the sets will build at a time.


> (possibly this can be reduced by adding more logic to the tx_buf/rx_buf functions)
> not to mention what will happen when support for reversed bit order is added.

Sure enough, sounds ugly.  But cpu_is_xxx() macros, combined with GCC dead
code elimination will strip out functions that are unused, so that e.g.

	if (cpu_is_mpc834x())
		fn = mpc834x_spi_tx_buf_u16;
	else if (cpu_is_mpc832x())
		fn = mpc832x_spi_tx_buf_u16;

would only link one of them unless the kernel supports both SOCs.  And
without in-driver #ifdeffery.

 
> I would argue that the kernel lacks the possibility to remove complexity
> I don't need. Example in this driver is that there is no way to remove
> support for 16 and 32 bit SPI character sizes. The same goes for a lot of
> the probing code in fsl-soc.c.

You could certainly add Kconfig options to help shift some complexity
from runtime to compile time.  But if you do that, remember that it's
not always a win in terms of the overall system.  Every configuration
needs to be tested for correctness, after all.


> It would be nice if a board port could add a custom header file that
> gets included by all .c automatically. Then one could add knobs
> (read #defines) there to futher tune such things as SPI char size.
> That way one don't have add Kconfig entries for all those small
> tuning knobs.

Sounds error prone to me.  What's the point ... removing maybe 100
bytes of code in this one driver?  You could save more than that
just by switching over to platform_device_probe() instead of using
platform_device_register().  And that'd be without adding failure
modes like "oops, this board stack really _does_ use 12 bit words
for the ADCs".

- Dave