[coreboot] [PATCH] Kill 'pnp_ops', have all Super I/Os use their own 'ops'
yhlu
yinghailu at gmail.com
Sun Oct 5 01:07:16 CEST 2008
On Sat, Oct 4, 2008 at 2:37 PM, Uwe Hermann <uwe at 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
More information about the coreboot
mailing list