[PATCH] Kill 'pnp_ops', have all Super I/Os use their own 'ops'

[Uwe Hermann]

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)...

Uwe.

[PATCH] Kill 'pnp_ops', have all Super I/Os use their own 'ops'

[Stefan Reinauer]

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?

Stefan

[ron minnich]

On Sat, Oct 4, 2008 at 8:35 AM, 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?

I had the same question. Can't we use the generic ops?

ron

[PATCH] Kill 'pnp_ops', have all Super I/Os use their own 'ops'

[Uwe Hermann]

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?

Thanks, Uwe.

[yhlu]

On Sat, Oct 4, 2008 at 2:37 PM, Uwe Hermann  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