Re: [PATCH 2 6/7] Uartlite: Add of-platform-bus binding

["Grant Likely" <grant.likely@secretlab.ca>]

On 10/1/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sun, 2007-09-30 at 16:42 -0600, Grant Likely wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > Add of_platform bus binding so this driver can be used with arch/powerpc
>
> Another option is to have a "constructor" in the platform code that
> generates the platform device from the DT. It might even become the
> preferred way one of these days, I'm not too sure at this stage. Anyway,
> do what you prefer.

I've looked at both, and I'm not to fond of the 'constructor'
approach.  Here are my reasons:

- Separate constructor code must be written for each driver anyway
(except perhaps for very simple cases which only require 'regs' and
'interrupts' properties).  The device tree data must then be mapped
onto a pdata structure; which the driver immediately turns around and
unmaps from pdata to populate it's driver state structure.  It ends up
being a lot of indirection to follow.
- Many (but perhaps not all) of these devices will be registered as an
of_device anyway.  By having a constructor to generate a
platform_device from that means each device will get probed twice;
once by the of_platform bus and once by the platform bus.
- Writing device drivers with multiple binding is easy and probable
results in smaller simpler code than the two step process of using a
constructor.

For example, here is my of_device binding for the systemace driver:

static int __devinit
ulite_of_probe(struct of_device *op, const struct of_device_id *match)
{
        struct resource res;
        const unsigned int *id;
        int irq, rc;
        dev_dbg(&op->dev, "%s(%p, %p)\n", __FUNCTION__, op, match);
        rc = of_address_to_resource(op->node, 0, &res);
        if (rc) {
                dev_err(&op->dev, "invalide address\n");
                return rc;
        }
        irq = irq_of_parse_and_map(op->node, 0);
        id = of_get_property(op->node, "port-number", NULL);
        return ulite_assign(&op->dev, id ? *id : -1, res.start, irq);
}

That's 3 calls to get data out of the device tree and one call to
setup the device.  The final 'ulite_assign()' call is called by both
the of_device and platform_device bindings.

To do the same thing with a constructor requires an additional
allocation and registration of a platform device structure and a pdata
structure.  Plus it requires the platform code to explicitly call a
set of helper functions to find and create platform devices for nodes
in the device tree.  (Unless you're thinking about leveraging the
of_platform probe code and have the .probe hook to the 'constructor'
bit)

What advantages do you see with the constructor approach?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195