[PATCH v3 2/2] powerpc/mpc5121: add initial support for PDM360NG board

[Grant Likely]

On Tue, Jul 27, 2010 at 4:36 AM, Anatolij Gustschin <agust at denx.de> wrote:
> Hi Grant,
>
> On Sun, 25 Jul 2010 01:42:23 -0600
> Grant Likely <grant.likely at secretlab.ca> wrote:
> ...
>> Hi Anatolij,
>>
>> Finally got some time tonight to properly dig into this patch.  Comments below.
>
> Thanks for review and comments! My reply below.

Thanks Anatolij, replies below.

>> > +               spi at 11900 {
>> > +                       compatible = "fsl,mpc5121-psc-spi", "fsl,mpc5121-psc";
>> > +                       cell-index = <9>;
>>
>> Try to drop the cell-index properties.  They are almost always misused.
>
> Removing cell-index would require changing the spi driver's probe.
> Currently cell-index is used to set spi bus number. What could be used
> for bus enumeration instead? Is it okay to use part of the spi node
> address? e.g. obtaining the offset 0x11900, masking out the unrelated
> bits and shifting by 8 would deliver unique index 9 for PSC9 in SPI
> mode. This would work for all 12 PSC SPI controllers of mpc5121.

Does the spi bus number really matter?  The device tree context gives
you a firm association between spi masters and devices which doesn't
require assigning a specific bus number.  The core spi code can
dynamically assign a bus number for the bus by setting bus_num to -1.

>> > +
>> > +       if (bus_num < 0 || bus_num > 11)
>> > +               return -ENODEV;
>> > +
>> > +       info.bus_num = bus_num;
>> > +
>> > +       of_dev = of_find_device_by_node(np);
>> > +       of_node_put(np);
>> > +       if (of_dev) {
>> > +               struct fsl_spi_platform_data *pdata;
>> > +
>> > +               pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>> > +               if (pdata) {
>> > +                       pdata->bus_num = bus_num;
>> > +                       pdata->max_chipselect = 1;
>> > +                       of_dev->dev.platform_data = pdata;
>> > +               }
>> > +       }
>> > +
>> > +       if (pdm360ng_penirq_init())
>> > +               return -ENODEV;
>> > +
>> > +       return spi_register_board_info(&info, 1);
>>
>> This ends up being a lot of code simply to attach a pdata structure to
>> an spi device.  I've been thinking about this problem, and I think
>> there is a better way.  Instead, use a bus notifier attached to the
>> SPI bus to wait for the spi_device to get registered, but before it
>> gets bound to a driver.  Then you can attach the pdata structure very
>> simply.  Something like this I think (completely untested, or even
>> compiled.  Details are left as an exercise to the developer):
>>
>> static int pdm360ng_touchscreen_notifier_call(struct notifier_block *nb,
>>                                       unsigned long event, void *__dev)
>> {
>>       struct device *dev = __dev;
>>
>>       if ((event == BUS_NOTIFIY_ADD_DEVICE) && (dev->of_node == [FOO]))
>>               dev->platform_data = [BAR];
>> }
>>
>> static struct notifier_block pdm360ng_touchscreen_nb = {
>>       notifier_call = pdm360ng_touchscreen_notifier_call;
>> };
>>
>> static int __init pdm360ng_touchscreen_init(void)
>> {
>>       bus_register_notifier(&spi_bus_type, pdm360ng_touchscreen_nb);
>> }
>
> Thanks! Bus notifier is now used for setting platform data in v4.

Cool!

> This requires fixing the mpc5121 psc spi driver to create spi child
> nodes of the spi master node. I have already send the appropriate
> patch to spi-devel list, but it is not the right approach to call
> of_register_spi_devices() in each driver.

It's not wrong; but it isn't ideal either.

> Do you plan to fix it in
> core spi code in v2.6.36?

Apparently I no longer have to because you've gone ahead and done it
for me anyway.  :-)  I'll take a look at that patch and send you my
comments.

Cheers,
g.