Re: SPI redux ... driver model support

Mark Underwood
2005-09-02T07:21:32Z

--- David Brownell <david-b@pacbell.net> wrote:

> > Date: Wed, 31 Aug 2005 08:59:44 +0100 (BST)
> > From: Mark Underwood <basicmark@yahoo.com>
> >
> > --- David Brownell wrote:
> >
> > > The last couple times SPI frameworks came up
> here, some of the feedback
> > > included "make it use the driver model properly;
> don't be like I2C".
> > >
> > > In hopes that it'll be useful, here's a small
> SPI core with driver model
> > > support driven from board-specific tables
> listing devices. I expect the
> > > I/O call(s) could stand to change; but at least
> this one starts out right,
> > > based on async I/O. (There's a synchronous
> call; it's a trivial wrapper.)
> > >
> > > ...
> >
> > Well I guess great minds think alike ;-). After
> > looking though my SPI core layer I released that
> it in
> > no way reflected the new driver model (not
> surprising
> > as it was a copy of i2c-core.c) and I would
> probably
> > get laughed off the kernel mailing list if I sent
> it
> > as was ;-).
>
> That usually doesn't happen. You'd just be told
> "make it use the driver
> model properly; don't be like I2C." Though maybe
> there'd be a fiew
> other criticisms mixed in. :)
>
>
> > I am now writing a new spi-core.c which uses the
> new
> > driver model.
>
> How about just merging the code I sent? It's not
> large, and it solves
> that problem. I don't much care about the I/O model
> issues quite yet,
> though requirements for quick sensor captures
> (RPC-ish) would seem
> different from ones like reading bulk SPI flash
> data.
>
>
> > For registering an adapter:
> > 1) Register an adapter that has a cs table showing
> > where devices sit on the adapter.
>
> But how is the adapter driver itself supposed to
> know that?

It gets passed the cs table as part of its platform
data.

>
> That's what I addressed with my patch: the need for
> the config tables
> to be **independent** of controller (and protocol)
> code. It decouples
> all the board-specific tables from the drivers.
>
> (Example shown below.)
>
> The nightmare to avoid is this: EVERY time someone
> adds a new
> SPI-equipped board, working/debugged/stable drivers
> need to change,
> because the board-specific config data was never
> separated from the
> drivers. (And we know it can be, as shown in the
> patch I posted...)

Now I've fixed my version I'll have a more detailed
look.

>
> Ideally adding a new board means adding a source
> file for just that one
> board, with the relevent implementation parameters.
> Only when hardware
> guys do something funky should any driver need to
> change.
>

That's what happens in my SPI subsystem. The adapter
driver only knows how the driver the adapter. When a
adapter gets probed it has platform data passed to it
which contains a pointer to the cs table, the number
of entry’s in the cs table and the pointer to a
function to control some GPIO(s) as cs for adapters
that don’t have any built in.

>
> > 2) This causes spi-core to enumerate the devices
> on
> > the cs table and register them.
> >
> > For un-registering an adapter:
> > 1) Unregister an adapter
> > 2) This causes spi-core to remove all the children
> of
> > the adapter
>
> Right, that's all exactly as in the patch I posted,
> though I punted
> on the "unregister" path -- an exercise for the
> reader! -- because I
> wanted to focus on (a) the driver model structure,
> like where things
> land in sysfs, and (b) how to keep board-specific
> initialization code
> out of controller and protocol drivers.

OK. If you want I could do the same, that is send the
un/registration and sysfs code before I put the
transfer methods in. I have some dummy devices so you
can see what happens in sysfs.

>
> - Dave
>
>
> --- o26.orig/arch/arm/mach-omap1/board-osk.c
> 2005-08-27 02:11:45.000000000 -0700
> +++ o26/arch/arm/mach-omap1/board-osk.c 2005-08-27
> 18:44:20.000000000 -0700
> @@ -193,6 +193,34 @@ static struct
> omap_board_config_kernel o
>
> #ifdef CONFIG_OMAP_OSK_MISTRAL
>
> +#include <linux/spi.h>
> +
> +struct ads7864_info { /* FIXME put in standard
> header */
> + u16 pen_irq, busy; /* GPIO lines */
> + u16 x_ohms, y_ohms;
> +};
> +
> +static struct ads7864_info mistral_ts_info = {
> + .pen_irq = OMAP_GPIO_IRQ(4),
> + .busy = /* GPIO */ 6,
> + .x_ohms = 419,
> + .y_ohms = 486,
> +};
> +
> +static const struct spi_board_info
> mistral_boardinfo[] = {
> +{
> + /* MicroWire CS0 has an ads7846e with touchscreen
> and
> + * other sensors. It's currently glued into some
> OMAP
> + * touchscreen support that ignores the driver
> model.
> + */
> + .driver_name = "ads7846",
> + .platform_data = &mistral_ts_info,
> + .max_speed_hz = 2000000,
> + .bus_num = 2, /* spi2 == microwire */

I did think about doing this but the problem is how do
you know bus 2 is the bus you think it is? This would
work for SPI adapters that are platform devices, but
what about hot-plug devices like PCI and USB (we are
thinking of actually making a USB to SPI converter so
customers can try out some of our SPI devices on a PC
:).

Mark

> + .chip_select = 0,
> +},
> +};
> +
> #ifdef CONFIG_PM
> static irqreturn_t
> osk_mistral_wake_interrupt(int irq, void *ignored,
> struct pt_regs *regs)
> @@ -211,6 +239,9 @@ static void __init
> osk_mistral_init(void
> * But this is too early for that...
> */
>
> + spi_register_board_info(mistral_boardinfo,
> + ARRAY_SIZE(mistral_boardinfo));
> +
> /* the sideways button (SW1) is for use as a
> "wakeup" button */
> omap_cfg_reg(N15_1610_MPUIO2);
> if (omap_request_gpio(OMAP_MPUIO(2)) == 0) {
> -
> To unsubscribe from this list: send the line
> "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

___________________________________________________________
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com