Re: [PATCH 3/4] spi: Add OF binding support for SPI busses

["Grant Likely" <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Fri, May 16, 2008 at 4:49 PM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, May 16, 2008 at 04:14:23PM -0600, Grant Likely wrote:
>> > Maybe this code could do something like
>> > spi->dev.platform_data = nc->data;
>> > and board code would fill nc->data at early stages? This needs to be a
>> > convention, not just random use though.. Maybe we can expand the struct
>> > device_node to explicitly include .platform_data for such cases?
>>
>> Hmmm, as you say, this could end up being rather messy.  However, by
>> passing the device node pointer, the driver could extract that data on
>> a per case basis.  (ie. it would be decided on a per driver basis
>> where to get the platform data).  I'm not sure; this bears more
>> thought...
>
> Sometimes it's not worth powder and shot adding OF functionality to
> the drivers, I2C and SPI are major examples. Another [not mmc_spi]
> example is drivers/input/touchscreen/ads7846.c, which is SPI driver
> and needs platform data. There is a board that needs this (touchscreen
> controller on a MPC8360E-RDK).

In my mind; platform_data and the device tree are all about the same
thing: representation.  In other words, how to describe the
configuration of the hardware independent of the driver itself.  The
device tree and platform data structures both solve the same problem.
In both cases, the representation data must be
translated/decoded/interpreted and stored in the drivers own private
data structure so it can be of use.

One of the things I find rather interesting is just how frequently
drivers using platform data structures have a big block of code which
simply copy pdata fields into identically named fields in the device
private data... specifically: copied discretely instead of being a
verbatim block that can be memcopied, or instead of maintaining a
pointer and using the pdata itself.  It highlights for me just how
much pdata structures are decoupled from the driver itself (just like
how the device tree data is decoupled from the driver)... but I
digress.

The point is that the translation of data from the device tree (and
from pdata for that matter) to a form usable by the driver has to live
*somewhere*.  Does it belong in the platform code?  Or should it live
with the driver itself?  I argue that it really belongs as much as
feasibly possible with the driver code.  Even if a pdata structure is
chosen to be used as an intermediary representation, the code is only
relevant to the driver and therefore shouldn't appear anywhere else in
the kernel tree.  Putting it with the driver also has the added
advantage that it can be lumped in with the driver module and
therefore will only get compiled into the kernel if the driver is
present.  Putting driver specific (not platform specific) translation
code anywhere other than with the device driver it is intended for is
just wrong.

In addition, I'd really like to avoid a situation where the same block
of translation code (or at least calls to it) is duplicated all over
the platform code directories.  It's that sort of duplication that the
device tree (and similar schemes) is intended to solve.  I agree that
using platform code is often the best solution, especially when
dealing with one-off board ports that won't appear anywhere else.
However, I strongly believe that the platform code approach should be
the exception, not the rule.  If it is a common data property that all
instantiations of the device must have, then encode it in the device
tree and be done with it.  Doing so keeps platform code straight
forward and reduces duplication.

> Also there is no way to pass functions via device tree, we're
> always end up doing board-specific hooks in the generic drivers...
>
> Finally, let's call this platform_data and be done with it. Then we
> can use this for things like drivers/video/fsl-diu-fb.c (see diu_ops,
> which is global struct, filled by
> arch/powerpc/platforms/86xx/mpc8610_hpcd.c).

Yes, I agree, there are always going to be platform/board specific
hooks and I'm not saying that we should try to eliminate them.  But
(as expressed in the argument above) I don't like the idea of making
the platform code fill in all the necessary pdata structures.  How
about this as an alternative:

Instead of allowing platform code to fill in platform_data pointers at
early platform setup, let it register a driver-specific callback hook
instead.  If the hook it populated, the driver will call it at an
appropriate time to allow the platform code to manipulate the driver
configuration.  The signature of the hook can be driver dependent
(just like how the pdata hook is platform dependent).  Doing this
ensure that the translation code stays where it belongs: with the
driver itself, and it defers execution of the code to a point to
driver initialization time instead of earlier in the platform setup
which should improve boot times in certain circumstances if the
drivers are loaded as modules.

As for adding OF support to the drivers "not worth powder and shot"; I
must disagree.  Adding the device tree support really isn't very
complex and the impact on existing drivers quite minimal.  All of the
code can be restricted to a function called by the drivers probe
routine that can be compiled out entirely if CONFIG_OF is not set.
I've already done similar stuff with drivers supporting both platform
and of_platform busses, and this situation I think should be even less
invasive.

Thoughts?

Cheers,
g.

---
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/