On Tue, Jul 27, 2010 at 4:36 AM, Anatolij Gustschin <ag...@denx.de> wrote: > Hi Grant, > > On Sun, 25 Jul 2010 01:42:23 -0600 > Grant Likely <grant.lik...@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. >> > + s...@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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev