On Sat, Oct 4, 2008 at 2:37 PM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Sat, Oct 04, 2008 at 05:35:06PM +0200, Stefan Reinauer wrote:
Uwe Hermann wrote:
Please correct me if I'm wrong, but I think pnp_ops will never be used anyway (every Super I/O defines its own 'ops' struct)...
Stupid question, but could we instead drop all the superio chips' own ops instead?
Or are they really special? Do they have to be?
I'm also not entirely sure, but I guess they have to be special, yes.
This is what the generic (statically defined) 'pnp_ops' looks like:
struct device_operations pnp_ops = { .read_resources = pnp_read_resources, .set_resources = pnp_set_resources, .enable_resources = pnp_enable_resources, .enable = pnp_enable, };
So all operations use the standard PNP read/set/enable functions, and there is no '.init' set.
Pretty much all Super I/Os, e.g. winbond/w83627ehg, use something like this:
static struct device_operations ops = { .read_resources = pnp_read_resources, .set_resources = w83627ehg_pnp_set_resources, .enable_resources = w83627ehg_pnp_enable_resources, .enable = w83627ehg_pnp_enable, .init = w83627ehg_init, };
So, while '.read_resources' still uses the generic pnp_read_resources(), all the other ones are specific for this Super I/O (especially so the '.init' which is _always_ different for each Super I/O).
There is some level of code reuse here already (we can specify some generic pnp_*() functions if this Superio I/O doesn't need special treatment, or override the entries of the struct with specific ones if required (e.g. w83627ehg_pnp_set_resources()).
But (AFAICT) we will never ever need the 'pnp_ops' in its above form, at the very least the '.init' will always be a special one, so the 'pnp_ops' itself is useless. At least that's my current analysis, but please correct me if I'm wrong.
I'm CC'ing Eric and YHLU, maybe they know the rationale behind pnp_ops?
my old local tree
static struct pnp_info pnp_dev_info[] = { { &ops, W83627HF_FDC, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, { &ops, W83627HF_PP, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, { &ops, W83627HF_SP1, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, { &ops, W83627HF_SP2, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, // No 4 { 0,}, { &ops, W83627HF_KBC, PNP_IO0 | PNP_IO1 | PNP_IRQ0 | PNP_IRQ1, { 0x7ff, 0 }, { 0x7ff, 0x4}, }, { &ops, W83627HF_CIR, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, { &ops, W83627HF_GAME_MIDI_GPIO1, PNP_IO0 | PNP_IO1 | PNP_IRQ0, { 0x7ff, 0 }, {0x7fe, 0x4}, }, { &ops, W83627HF_GPIO2, }, { &ops, W83627HF_GPIO3, }, { &ops, W83627HF_ACPI, }, { &ops, W83627HF_HWM, PNP_IO0 | PNP_IRQ0, { 0xff8, 0 }, }, };
static void enable_dev(struct device *dev) { pnp_enable_devices(dev, &ops, sizeof(pnp_dev_info)/sizeof(pnp_dev_info[0]), pnp_dev_info); }
also
void pnp_enable_devices(device_t base_dev, struct device_operations *ops, unsigned functions, struct pnp_info *info) { struct device_path path; device_t dev; int i;
path.type = DEVICE_PATH_PNP; path.u.pnp.port = base_dev->path.u.pnp.port;
/* Setup the ops and resources on the newly allocated devices */ for(i = 0; i < functions; i++) { path.u.pnp.device = info[i].function; dev = alloc_find_dev(base_dev->bus, &path);
/* Don't initialize a device multiple times */ if (dev->ops) continue;
if (info[i].ops == 0) { dev->ops = ops; } else { dev->ops = info[i].ops; } get_resources(dev, &info[i]); }
so it means you can have
static struct pnp_info pnp_dev_info[] = { { &ops, W83627HF_FDC, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, { &ops, W83627HF_PP, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, { &ops, W83627HF_SP1, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, { &ops, W83627HF_SP2, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, // No 4 { 0,}, { &ops, W83627HF_KBC, PNP_IO0 | PNP_IO1 | PNP_IRQ0 | PNP_IRQ1, { 0x7ff, 0 }, { 0x7ff, 0x4}, }, { &ops, W83627HF_CIR, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, }, { 0, W83627HF_GAME_MIDI_GPIO1, PNP_IO0 | PNP_IO1 | PNP_IRQ0, { 0x7ff, 0 }, {0x7fe, 0x4}, }, { 0, W83627HF_GPIO2, }, { 0, W83627HF_GPIO3, }, { 0, W83627HF_ACPI, }, { &ops, W83627HF_HWM, PNP_IO0 | PNP_IRQ0, { 0xff8, 0 }, },
YH