On Tue, Mar 31, 2009 at 07:19:13PM -0700, David Brownell wrote:
[I don't know what's going on with either the mvista email server
or this mail list but none of these emails showed up until this
morning.]
> On Tuesday 31 March 2009, Mark A. Greer wrote:
> > > > +enum {
> > > > + DA830_PDEV_EDMA,
> > > > + DA830_PDEV_I2C_0,
> > > > + DA830_PDEV_I2C_1,
> > > > + DA830_PDEV_SPI_0,
> > > > + ...
> > > > +};
> > > > +
> > > > +struct da830_pdata {
> > > > + int dev; /* Device to enable */
> > > > + void *pdata; /* platform_data for this device */
> > > > +};
> > > > +
> > > > +void da830_add_devices(struct da830_pdata pdata[DA830_PDEV_NUM],
> > > > + unsigned long num);
> > > > +
> > >
> > > Maybe it's getting too late for me, but I don't see the point of these
> > > enums and the extra array of dev/pdata associations.
>
> They do obfuscate things, yes. And they don't easily
> distinguish between data that the driver should care
> about (platform_data) vs data that's only needed to set
> up the infrastructure (like pinmux options).
I don't see how this obfuscates things although I certainly didn't
explain it.
devices-da830.c has all of the platform_device info that is defined
by the SoC. It has no platform_data except for the platform_data that
is defined by the SoC (i.e., plat_serial8250_port, edma_soc_info, and
emac_platform_data). All the other platform_data comes from the board
file and I tried to make it easy to add/attach with da830_add_devices().
Any of the info in devices-da830.c can be overridden by the board code.
Any SoC info that didn't go in devices-da830.c, went into da830.c
(e.g., clk and pinmux info).
> > > I think it's simpler and clearer for the SoC code to just register all the
> > > devices and platform data all the time.
>
> That is, all the devices that are valid for that board?
> That's the model I'm used to.
I agree with this and I used to have an explicit 'enable' member in
struct da830_pdata but I removed it because I anticipated being told
to add all of the platform_devices no matter what. I think I'll put
it back.
> If it's not wired, don't set it up. Avoid conditional
> logic based on Kconfig. (Except maybe at the level of
> which daughtercards the board stack uses.)
>
>
> > There is a patch from Steve Chen (which you haven't seen) that
> > automatically sets up the pinmux from clk_enable. The patch is
> > really nice for 2 reason:
>
> We finally *removed* pinmux from the clk_enable() paths not
> that long ago. So that's one big reason it's not-nice.
> Pinmux is a distinct problem, and doing it right needs
> information that's just not available to clock code.
>
> > 1) The da830 has sooo many pinmux regs to set up,
>
> Much less than OMAP, which keeps those decoupled...
>
>
> > its really
> > nice to have it done automatically (and not having a hundred
> > davinci_cfg_reg() calls in every board file).
>
> The cleanest way is IMO to have it set up when the device
> is registered. The device registration code can know e.g.
> that when the platform data says MMC has eight pins, it
> had better mux all eight pins. (If there are mux options,
> like DAT4 coming out on one of several pins, it can be told
> which option that board uses.)
Okay.
> > 2) It detects and complains about pinmux contention and there
> > is a lot of potential contention with the da830.
>
> No less so than with OMAP, or lots of other SoCs. Contention
> is rarely a problem given a clearly structured framework for
> board init ... like being able to rely on device registration
> to perform the pinmux.
>
>
> > So, assuming that patch eventually makes it in, we can't just register
> > every device because the drivers can pick unwanted devices, clk_enable
> > them and break some other device because it stole its pins.
>
> If only the devices that exist on the board get registered, that
> type of confusion won't happen.
>
>
> > For example, uart2 (console) and spi1 can't be set up at the same time
> > but we definitely want spi0 enabled. So, with the spi driver enabled
> > for spi0 and the device data for spi1 there, the spi driver will
> > clk_enable spi1 causing it to steal uart2's pins.
>
> So make sure you register spi0 but not spi1 ... example, see
> dm355_init_spi0() in mach-davinci/dm355.c, which handles the
> muxing (but maybe not for all chipselects) and registration.
All of the above just convinces me more to put the enable flag back
into struct da830_pdata.
Mark
--
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
