[spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions

[Grant Likely]

On Tue, Aug 10, 2010 at 2:29 AM, Joakim Tjernlund
<joakim.tjernlund at transmode.se> wrote:
>> On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b at pacbell.net> wrote:
>> >
>> >
>> > --- On Mon, 8/9/10, Grant Likely <grant.likely at secretlab.ca> wrote:
>> >
>> >
>> >> > +       nr_parts =
>> >> of_mtd_parse_partitions(&spi->dev, np, &parts);
>> >
>> > Let's keep OF-specific logic out of drivers like
>> > this one ...  intended to work without OF.
>> >
>> > NAK on adding dependencies like OF to drivers
>> > and other infrastructure that starts generic.
>
> Agreed.
>
>>
>> The OF hooks compile to no-ops when CONFIG_OF is disabled.  I've been
>> very careful about that.
>
> OF should not be in the drivers, one should be able use some other method
> than OF.

If a device is described in the device tree, then the code has to live
*somewhere* to translate the data from the device tree into a form the
driver can use.  If not the driver, then where should that code live?

If it is in the machine support code, then that requires foreknowledge
about all the device specific data that needs to be translated out of
the device tree at device registration time, which also means that the
translation code cannot be a module and it nullifies some of the
advantages of the device tree.  Not to mention the fact that it is
just plain ugly!  :-)

Some of it can go into the infrastructure code, but only for parsing
common properties like irqs, address ranges, and some common bindings
like registering spi and i2c bus child nodes as devices.  (This
*particular* case may fall into this category if add_mtd_device() was
able to call the OF partition parsing hook if a device node pointer
was present; which does make a certain amount of sense, but I defer to
dwmw2 in this case).  However, device-specific properties cannot be
parsed in the infrastructure code because the infrastructure has no
knowledge of device specific bits.

The suggestion has been raised to use something like bus notifiers to
get a hook onto the device registration before the driver is probed so
that the platform_data can be translated in an separate chuck of code,
but down that path lies insanity.  There are all kinds of ordering
issues (like making sure the translation code is called before the
driver probe code), and it is a lot of complexity for what is really a
simple thing.

I've yet to be convinced that device-specific OF calls belongs
anywhere in the kernel other than in the driver that uses the data.

That being said, I completely agree that OF calls do not belong in the
vast majority of the driver code.  Using OF calls after the driver is
bound to the device is generally wrong.  However, they make perfect
sense in the probe hook who's whole purpose is to obtain data about
the device and register it.  The model is actually very simple:

if (dev->platform_data)
      get_platform_data();
else if (dev->of_node)
      get_of_data();

For the actual implementation in this case, my comments reflect what I
think is the clearest implementation.  However, if you'd prefer a
single function call at the start of the probe() hook to return a
flash_partition_data pointer, then I'm okay with that too.

Cheers,
g.