2003-01-13 22:29:20

by Adam Belay

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers

On Sun, Jan 12, 2003 at 08:30:57PM +0100, Jaroslav Kysela wrote:
> Hi,
>
> this patch must be applied after PnP patch v0.94. It contains my
> small cleanups of PnP code and I tried to rewrite almost all ISA PnP
> drivers to new PnP subsystem except sound drivers (ALSA & OSS). Please,
> apply to get away compilation problems.
>
> Jaroslav


Hi Jaroslav,

Next time send pnp related changes to me directly. I would have been happy
to include your work after I carefully reviewed it. I was planning to merge
a very large resource algorithm improvement soon, but now because of these
changes, that I do not even necessarily agree with, I will be unable to
include this major improvement and bug fix for a while. Furthermore many
people of whom are counting on me to merge thier patches will now be
dissappointed to hear that their changes, many of which are critical for
certain pnp hardware configurations will be delayed.

Although I am glad to see the drivers converted, this has been done in a way
that is not desirable. They use compat.c which was intended as a temporary
solution. In fact I may even remove compat.c all together. This has been
clearly stated in both the file compat.c and pnp.txt documentation.
Attached is a copy of Zwane's ide conversion patch against 2.5.56. It can be
used as an example of a correct driver conversion. Notice how it is fully
integrated into the driver model as all drivers should be.

I'm now unhappy with the current pnp code and will most likely revert all pnp
changes between 2.5.56 and 2.5.57 to avoid a merging nightmare. I will then
carefully remerge what I feel is acceptable.

Once again, I'm sorry to those who will be unable to use thier systems due to
this major set back. All pnp changes should be sent to me and me only. I
believe these pnp change conflicts can easily be worked out with better
communication. I intend to make a better effort in the future to explain my
goals and I hope that you will do the same. I appreciate your work with the
pnp layer.

Sincerely,

Adam Belay
Linux Plug and Play Maintainer





===================================================================
PnP IDE Conversion from Zwane Mwaikambo



Index: linux-2.5.56/drivers/ide/ide-pnp.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.56/drivers/ide/ide-pnp.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ide-pnp.c
--- linux-2.5.56/drivers/ide/ide-pnp.c 16 Dec 2002 05:16:23 -0000 1.1.1.1
+++ linux-2.5.56/drivers/ide/ide-pnp.c 16 Dec 2002 08:30:39 -0000
@@ -18,13 +18,11 @@

#include <linux/ide.h>
#include <linux/init.h>
-
-#include <linux/isapnp.h>
+#include <linux/pnp.h>

#define DEV_IO(dev, index) (dev->resource[index].start)
#define DEV_IRQ(dev, index) (dev->irq_resource[index].start)
-
-#define DEV_NAME(dev) (dev->bus->name ? dev->bus->name : "ISA PnP")
+#define DEV_NAME(dev) (dev->protocol->name ? dev->protocol->name : "ISA PnP")

#define GENERIC_HD_DATA 0
#define GENERIC_HD_ERROR 1
@@ -35,125 +33,125 @@
#define GENERIC_HD_SELECT 6
#define GENERIC_HD_STATUS 7

-static int generic_ide_offsets[IDE_NR_PORTS] __initdata = {
+static int generic_ide_offsets[IDE_NR_PORTS] = {
GENERIC_HD_DATA, GENERIC_HD_ERROR, GENERIC_HD_NSECTOR,
GENERIC_HD_SECTOR, GENERIC_HD_LCYL, GENERIC_HD_HCYL,
GENERIC_HD_SELECT, GENERIC_HD_STATUS, -1, -1
};

/* ISA PnP device table entry */
-struct pnp_dev_t {
- unsigned short card_vendor, card_device, vendor, device;
- int (*init_fn)(struct pci_dev *dev, int enable);
+struct idepnp_private {
+ struct pnp_dev *dev;
+ struct pci_dev pci_dev; /* we need this for the upper layers */
+ int (*init_fn)(struct idepnp_private *device, int enable);
};

-/* Generic initialisation function for ISA PnP IDE interface */
+/* Barf bags at the ready! Enough to satisfy IDE core */
+static void pnp_to_pci(struct pnp_dev *pnp_dev, struct pci_dev *pci_dev)
+{
+ pci_dev->dev = pnp_dev->dev;
+ pci_set_drvdata(pci_dev, pnp_get_drvdata(pnp_dev));
+ pci_dev->irq = DEV_IRQ(pnp_dev, 0);
+ pci_set_dma_mask(pci_dev, 0x00ffffff);
+}

-static int __init pnpide_generic_init(struct pci_dev *dev, int enable)
+/* Generic initialisation function for ISA PnP IDE interface */
+static int pnpide_generic_init(struct idepnp_private *device, int enable)
{
hw_regs_t hw;
ide_hwif_t *hwif;
int index;
+ struct pnp_dev *dev = device->dev;

- if (!enable)
+ if (!enable) {
+ /* nothing to do for now */
return 0;
+ }

if (!(DEV_IO(dev, 0) && DEV_IO(dev, 1) && DEV_IRQ(dev, 0)))
- return 1;
+ return -EINVAL;

ide_setup_ports(&hw, (ide_ioreg_t) DEV_IO(dev, 0),
generic_ide_offsets,
(ide_ioreg_t) DEV_IO(dev, 1),
0, NULL,
-// generic_pnp_ide_iops,
+ /* generic_pnp_ide_iops, */
DEV_IRQ(dev, 0));

index = ide_register_hw(&hw, &hwif);

if (index != -1) {
printk(KERN_INFO "ide%d: %s IDE interface\n", index, DEV_NAME(dev));
- hwif->pci_dev = dev;
+ hwif->pci_dev = &device->pci_dev;
return 0;
}

- return 1;
+ return -ENODEV;
}

/* Add your devices here :)) */
-struct pnp_dev_t idepnp_devices[] __initdata = {
- /* Generic ESDI/IDE/ATA compatible hard disk controller */
- { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
- ISAPNP_VENDOR('P', 'N', 'P'), ISAPNP_DEVICE(0x0600),
- pnpide_generic_init },
- { 0 }
+#define IDEPNP_GENERIC_INIT 0
+static const struct pnp_device_id pnp_ide_devs[] = {
+ /* Generic ESDI/IDE/ATA compatible hard disk controller */
+ {"PNP0600", IDEPNP_GENERIC_INIT},
+ {"", 0}
};

#define NR_PNP_DEVICES 8
-struct pnp_dev_inst {
- struct pci_dev *dev;
- struct pnp_dev_t *dev_type;
-};
-static struct pnp_dev_inst devices[NR_PNP_DEVICES];
-static int pnp_ide_dev_idx = 0;
+/* Nb. pnpide_generic_init is indexed as IDEPNP_GENERIC_INIT */
+static int (*init_functions[])(struct idepnp_private *device, int enable) = {pnpide_generic_init};
+static struct idepnp_private devices[NR_PNP_DEVICES];
+static int pnp_ide_dev_idx;

/*
* Probe for ISA PnP IDE interfaces.
*/
-
-void __init pnpide_init(int enable)
+static int pnp_ide_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
{
- struct pci_dev *dev = NULL;
- struct pnp_dev_t *dev_type;
+ int ret;
+ struct idepnp_private *p;

- if (!isapnp_present())
- return;
+ /*
+ * Register device in the array to
+ * deactivate it on a module unload.
+ */
+ if (pnp_ide_dev_idx >= NR_PNP_DEVICES)
+ return -ENOSPC;
+
+ p = &devices[pnp_ide_dev_idx];
+ p->init_fn = init_functions[dev_id->driver_data];
+ p->dev = pdev;
+ pnp_set_drvdata(pdev, p);
+ pnp_to_pci(p->dev, &p->pci_dev);
+ ret = p->init_fn(p, 1);
+ if (!ret)
+ pnp_ide_dev_idx++;
+
+ return ret;
+}

- /* Module unload, deactivate all registered devices. */
- if (!enable) {
- int i;
- for (i = 0; i < pnp_ide_dev_idx; i++) {
- dev = devices[i].dev;
- devices[i].dev_type->init_fn(dev, 0);
- if (dev->deactivate)
- dev->deactivate(dev);
- }
- return;
- }
+static void pnp_ide_remove(struct pnp_dev *dev)
+{
+ struct idepnp_private *p = pnp_get_drvdata(dev);
+
+ /* if p is null you have a bug elsewhere */
+ p->init_fn(p, 0);
+ pnp_ide_dev_idx--;
+ return;
+}

- for (dev_type = idepnp_devices; dev_type->vendor; dev_type++) {
- while ((dev = isapnp_find_dev(NULL, dev_type->vendor,
- dev_type->device, dev))) {
-
- if (dev->active)
- continue;
-
- if (dev->prepare && dev->prepare(dev) < 0) {
- printk(KERN_ERR"ide-pnp: %s prepare failed\n", DEV_NAME(dev));
- continue;
- }
-
- if (dev->activate && dev->activate(dev) < 0) {
- printk(KERN_ERR"ide: %s activate failed\n", DEV_NAME(dev));
- continue;
- }
-
- /* Call device initialization function */
- if (dev_type->init_fn(dev, 1)) {
- if (dev->deactivate(dev))
- dev->deactivate(dev);
- } else {
-#ifdef MODULE
- /*
- * Register device in the array to
- * deactivate it on a module unload.
- */
- if (pnp_ide_dev_idx >= NR_PNP_DEVICES)
- return;
- devices[pnp_ide_dev_idx].dev = dev;
- devices[pnp_ide_dev_idx].dev_type = dev_type;
- pnp_ide_dev_idx++;
-#endif
- }
- }
- }
+static struct pnp_driver idepnp_driver = {
+ .name = "ide-pnp",
+ .id_table = pnp_ide_devs,
+ .probe = pnp_ide_probe,
+ .remove = pnp_ide_remove
+};
+
+void pnpide_init(int enable)
+{
+ if (enable)
+ pnp_register_driver(&idepnp_driver);
+ else
+ pnp_unregister_driver(&idepnp_driver);
}
+
Index: linux-2.5.56/drivers/ide/ide.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.56/drivers/ide/ide.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ide.c
--- linux-2.5.56/drivers/ide/ide.c 16 Dec 2002 05:16:22 -0000 1.1.1.1
+++ linux-2.5.56/drivers/ide/ide.c 16 Dec 2002 08:52:17 -0000
@@ -2080,7 +2080,7 @@
buddha_init();
}
#endif /* CONFIG_BLK_DEV_BUDDHA */
-#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_ISAPNP)
+#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_PNP)
{
extern void pnpide_init(int enable);
pnpide_init(1);
@@ -2256,7 +2256,7 @@
spin_unlock_irqrestore(&ide_lock, flags);
return 1;
}
-#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_ISAPNP) && defined(MODULE)
+#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_PNP) && defined(MODULE)
pnpide_init(0);
#endif /* CONFIG_BLK_DEV_ISAPNP */
#ifdef CONFIG_PROC_FS


2003-01-13 22:51:40

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers


On Mon, 13 Jan 2003, Adam Belay wrote:
>
> PnP IDE Conversion from Zwane Mwaikambo

This, btw, is _worse_ than the current conversion, as far as I can tell.
In particular:

> -/* Generic initialisation function for ISA PnP IDE interface */
> +/* Barf bags at the ready! Enough to satisfy IDE core */
> +static void pnp_to_pci(struct pnp_dev *pnp_dev, struct pci_dev *pci_dev)
> +{
> + pci_dev->dev = pnp_dev->dev;
> + pci_set_drvdata(pci_dev, pnp_get_drvdata(pnp_dev));
> + pci_dev->irq = DEV_IRQ(pnp_dev, 0);
> + pci_set_dma_mask(pci_dev, 0x00ffffff);
> +}

That "pci_dev->dev = pnp_dev->dev" looks totally bletcherous, and does a
structure copy that potentially copies pointers that simply ARE NOT VALID
after the copy.

Not good.

Linus

2003-01-13 22:41:57

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers


On Mon, 13 Jan 2003, Adam Belay wrote:
>
> I'm now unhappy with the current pnp code and will most likely revert all pnp
> changes between 2.5.56 and 2.5.57 to avoid a merging nightmare. I will then
> carefully remerge what I feel is acceptable.

I will _not_ accept just stupid reverts, if that means that a device
driver is not available for people to test with.

We do not break drivers like that, and the reason you have clashes with
other people is that so many drivers ended up being broken for too long.

Feel free to revert the changes one by one _as_they_get_fixed_. But don't
even try to send me a patch that reverts things to a broken state. The PnP
changes have been painful enough as-is.

Linus

2003-01-13 22:55:11

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers

On Mon, Jan 13, 2003 at 05:39:06PM +0000, Adam Belay wrote:
> On Sun, Jan 12, 2003 at 08:30:57PM +0100, Jaroslav Kysela wrote:
> > this patch must be applied after PnP patch v0.94. It contains my
> > small cleanups of PnP code and I tried to rewrite almost all ISA PnP
> > drivers to new PnP subsystem except sound drivers (ALSA & OSS). Please,
> > apply to get away compilation problems.
>
> Hi Jaroslav,
>
> Next time send pnp related changes to me directly.

And Adam, sorry for taking so long in getting your previous changes you
sent to me to Linus. That's my fault, it was in my queue to review when
Jaroslav sent his patches. I guess now I'll just wait for this merge
mess to get cleaned up :(

greg k-h

2003-01-13 23:31:31

by Adam Belay

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers

On Mon, Jan 13, 2003 at 02:48:45PM -0800, Linus Torvalds wrote:
>
> On Mon, 13 Jan 2003, Adam Belay wrote:
> >
> > I'm now unhappy with the current pnp code and will most likely revert all pnp
> > changes between 2.5.56 and 2.5.57 to avoid a merging nightmare. I will then
> > carefully remerge what I feel is acceptable.
>
> I will _not_ accept just stupid reverts, if that means that a device
> driver is not available for people to test with.
>
> We do not break drivers like that, and the reason you have clashes with
> other people is that so many drivers ended up being broken for too long.
>
> Feel free to revert the changes one by one _as_they_get_fixed_. But don't
> even try to send me a patch that reverts things to a broken state. The PnP
> changes have been painful enough as-is.

Hi Linus,

I agree that it is important not to cause further damage to the existing pnp
drivers. Therefore I will carefully modify the drivers one by one before making
any changes to the pnp layer that would conflict. In this way, all the pnp
drivers will remain functional during development.

Thanks,
Adam

2003-01-14 01:36:09

by Zwane Mwaikambo

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers

On Mon, 13 Jan 2003, Linus Torvalds wrote:

>
> On Mon, 13 Jan 2003, Adam Belay wrote:
> >
> > PnP IDE Conversion from Zwane Mwaikambo
>
> This, btw, is _worse_ than the current conversion, as far as I can tell.
> In particular:

I just had a look (and test booted), the current one definitely is better
and less kludgy.

> > -/* Generic initialisation function for ISA PnP IDE interface */
> > +/* Barf bags at the ready! Enough to satisfy IDE core */
> > +static void pnp_to_pci(struct pnp_dev *pnp_dev, struct pci_dev *pci_dev)
> > +{
> > + pci_dev->dev = pnp_dev->dev;
> > + pci_set_drvdata(pci_dev, pnp_get_drvdata(pnp_dev));
> > + pci_dev->irq = DEV_IRQ(pnp_dev, 0);
> > + pci_set_dma_mask(pci_dev, 0x00ffffff);
> > +}
>
> That "pci_dev->dev = pnp_dev->dev" looks totally bletcherous, and does a
> structure copy that potentially copies pointers that simply ARE NOT VALID
> after the copy.
>
> Not good.

Hey, i did offer barf bags ;) I admit, that part is complete bollocks, due
to my bending over for pci_dev.

Zwane
--
function.linuxpower.ca



2003-01-14 02:19:38

by Shawn Starr

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers


Agreed, Some of those improvements will help us fix old ISA devices that are
misbehaving.

On Monday 13 January 2003 12:39 pm, Adam Belay wrote:
> On Sun, Jan 12, 2003 at 08:30:57PM +0100, Jaroslav Kysela wrote:
> > Hi,
> >
> > this patch must be applied after PnP patch v0.94. It contains my
> > small cleanups of PnP code and I tried to rewrite almost all ISA PnP
> > drivers to new PnP subsystem except sound drivers (ALSA & OSS). Please,
> > apply to get away compilation problems.
> >
> > Jaroslav
>
> Hi Jaroslav,
>
> Next time send pnp related changes to me directly. I would have been happy
> to include your work after I carefully reviewed it. I was planning to
> merge a very large resource algorithm improvement soon, but now because of
> these changes, that I do not even necessarily agree with, I will be unable
> to include this major improvement and bug fix for a while. Furthermore
> many people of whom are counting on me to merge thier patches will now be
> dissappointed to hear that their changes, many of which are critical for
> certain pnp hardware configurations will be delayed.
>
> Although I am glad to see the drivers converted, this has been done in a
> way that is not desirable. They use compat.c which was intended as a
> temporary solution. In fact I may even remove compat.c all together. This
> has been clearly stated in both the file compat.c and pnp.txt
> documentation. Attached is a copy of Zwane's ide conversion patch against
> 2.5.56. It can be used as an example of a correct driver conversion.
> Notice how it is fully integrated into the driver model as all drivers
> should be.
>
> I'm now unhappy with the current pnp code and will most likely revert all
> pnp changes between 2.5.56 and 2.5.57 to avoid a merging nightmare. I will
> then carefully remerge what I feel is acceptable.
>
> Once again, I'm sorry to those who will be unable to use thier systems due
> to this major set back. All pnp changes should be sent to me and me only.
> I believe these pnp change conflicts can easily be worked out with better
> communication. I intend to make a better effort in the future to explain
> my goals and I hope that you will do the same. I appreciate your work with
> the pnp layer.
>
> Sincerely,
>
> Adam Belay
> Linux Plug and Play Maintainer
>
>
>
>
>
> ===================================================================
> PnP IDE Conversion from Zwane Mwaikambo
>
>
>
> Index: linux-2.5.56/drivers/ide/ide-pnp.c
> ===================================================================
> RCS file: /build/cvsroot/linux-2.5.56/drivers/ide/ide-pnp.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 ide-pnp.c
> --- linux-2.5.56/drivers/ide/ide-pnp.c 16 Dec 2002 05:16:23 -0000 1.1.1.1
> +++ linux-2.5.56/drivers/ide/ide-pnp.c 16 Dec 2002 08:30:39 -0000
> @@ -18,13 +18,11 @@
>
> #include <linux/ide.h>
> #include <linux/init.h>
> -
> -#include <linux/isapnp.h>
> +#include <linux/pnp.h>
>
> #define DEV_IO(dev, index) (dev->resource[index].start)
> #define DEV_IRQ(dev, index) (dev->irq_resource[index].start)
> -
> -#define DEV_NAME(dev) (dev->bus->name ? dev->bus->name : "ISA PnP")
> +#define DEV_NAME(dev) (dev->protocol->name ? dev->protocol->name : "ISA
> PnP")
>
> #define GENERIC_HD_DATA 0
> #define GENERIC_HD_ERROR 1
> @@ -35,125 +33,125 @@
> #define GENERIC_HD_SELECT 6
> #define GENERIC_HD_STATUS 7
>
> -static int generic_ide_offsets[IDE_NR_PORTS] __initdata = {
> +static int generic_ide_offsets[IDE_NR_PORTS] = {
> GENERIC_HD_DATA, GENERIC_HD_ERROR, GENERIC_HD_NSECTOR,
> GENERIC_HD_SECTOR, GENERIC_HD_LCYL, GENERIC_HD_HCYL,
> GENERIC_HD_SELECT, GENERIC_HD_STATUS, -1, -1
> };
>
> /* ISA PnP device table entry */
> -struct pnp_dev_t {
> - unsigned short card_vendor, card_device, vendor, device;
> - int (*init_fn)(struct pci_dev *dev, int enable);
> +struct idepnp_private {
> + struct pnp_dev *dev;
> + struct pci_dev pci_dev; /* we need this for the upper layers */
> + int (*init_fn)(struct idepnp_private *device, int enable);
> };
>
> -/* Generic initialisation function for ISA PnP IDE interface */
> +/* Barf bags at the ready! Enough to satisfy IDE core */
> +static void pnp_to_pci(struct pnp_dev *pnp_dev, struct pci_dev *pci_dev)
> +{
> + pci_dev->dev = pnp_dev->dev;
> + pci_set_drvdata(pci_dev, pnp_get_drvdata(pnp_dev));
> + pci_dev->irq = DEV_IRQ(pnp_dev, 0);
> + pci_set_dma_mask(pci_dev, 0x00ffffff);
> +}
>
> -static int __init pnpide_generic_init(struct pci_dev *dev, int enable)
> +/* Generic initialisation function for ISA PnP IDE interface */
> +static int pnpide_generic_init(struct idepnp_private *device, int enable)
> {
> hw_regs_t hw;
> ide_hwif_t *hwif;
> int index;
> + struct pnp_dev *dev = device->dev;
>
> - if (!enable)
> + if (!enable) {
> + /* nothing to do for now */
> return 0;
> + }
>
> if (!(DEV_IO(dev, 0) && DEV_IO(dev, 1) && DEV_IRQ(dev, 0)))
> - return 1;
> + return -EINVAL;
>
> ide_setup_ports(&hw, (ide_ioreg_t) DEV_IO(dev, 0),
> generic_ide_offsets,
> (ide_ioreg_t) DEV_IO(dev, 1),
> 0, NULL,
> -// generic_pnp_ide_iops,
> + /* generic_pnp_ide_iops, */
> DEV_IRQ(dev, 0));
>
> index = ide_register_hw(&hw, &hwif);
>
> if (index != -1) {
> printk(KERN_INFO "ide%d: %s IDE interface\n", index, DEV_NAME(dev));
> - hwif->pci_dev = dev;
> + hwif->pci_dev = &device->pci_dev;
> return 0;
> }
>
> - return 1;
> + return -ENODEV;
> }
>
> /* Add your devices here :)) */
> -struct pnp_dev_t idepnp_devices[] __initdata = {
> - /* Generic ESDI/IDE/ATA compatible hard disk controller */
> - { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
> - ISAPNP_VENDOR('P', 'N', 'P'), ISAPNP_DEVICE(0x0600),
> - pnpide_generic_init },
> - { 0 }
> +#define IDEPNP_GENERIC_INIT 0
> +static const struct pnp_device_id pnp_ide_devs[] = {
> + /* Generic ESDI/IDE/ATA compatible hard disk controller */
> + {"PNP0600", IDEPNP_GENERIC_INIT},
> + {"", 0}
> };
>
> #define NR_PNP_DEVICES 8
> -struct pnp_dev_inst {
> - struct pci_dev *dev;
> - struct pnp_dev_t *dev_type;
> -};
> -static struct pnp_dev_inst devices[NR_PNP_DEVICES];
> -static int pnp_ide_dev_idx = 0;
> +/* Nb. pnpide_generic_init is indexed as IDEPNP_GENERIC_INIT */
> +static int (*init_functions[])(struct idepnp_private *device, int enable)
> = {pnpide_generic_init}; +static struct idepnp_private
> devices[NR_PNP_DEVICES];
> +static int pnp_ide_dev_idx;
>
> /*
> * Probe for ISA PnP IDE interfaces.
> */
> -
> -void __init pnpide_init(int enable)
> +static int pnp_ide_probe(struct pnp_dev *pdev, const struct pnp_device_id
> *dev_id) {
> - struct pci_dev *dev = NULL;
> - struct pnp_dev_t *dev_type;
> + int ret;
> + struct idepnp_private *p;
>
> - if (!isapnp_present())
> - return;
> + /*
> + * Register device in the array to
> + * deactivate it on a module unload.
> + */
> + if (pnp_ide_dev_idx >= NR_PNP_DEVICES)
> + return -ENOSPC;
> +
> + p = &devices[pnp_ide_dev_idx];
> + p->init_fn = init_functions[dev_id->driver_data];
> + p->dev = pdev;
> + pnp_set_drvdata(pdev, p);
> + pnp_to_pci(p->dev, &p->pci_dev);
> + ret = p->init_fn(p, 1);
> + if (!ret)
> + pnp_ide_dev_idx++;
> +
> + return ret;
> +}
>
> - /* Module unload, deactivate all registered devices. */
> - if (!enable) {
> - int i;
> - for (i = 0; i < pnp_ide_dev_idx; i++) {
> - dev = devices[i].dev;
> - devices[i].dev_type->init_fn(dev, 0);
> - if (dev->deactivate)
> - dev->deactivate(dev);
> - }
> - return;
> - }
> +static void pnp_ide_remove(struct pnp_dev *dev)
> +{
> + struct idepnp_private *p = pnp_get_drvdata(dev);
> +
> + /* if p is null you have a bug elsewhere */
> + p->init_fn(p, 0);
> + pnp_ide_dev_idx--;
> + return;
> +}
>
> - for (dev_type = idepnp_devices; dev_type->vendor; dev_type++) {
> - while ((dev = isapnp_find_dev(NULL, dev_type->vendor,
> - dev_type->device, dev))) {
> -
> - if (dev->active)
> - continue;
> -
> - if (dev->prepare && dev->prepare(dev) < 0) {
> - printk(KERN_ERR"ide-pnp: %s prepare failed\n", DEV_NAME(dev));
> - continue;
> - }
> -
> - if (dev->activate && dev->activate(dev) < 0) {
> - printk(KERN_ERR"ide: %s activate failed\n", DEV_NAME(dev));
> - continue;
> - }
> -
> - /* Call device initialization function */
> - if (dev_type->init_fn(dev, 1)) {
> - if (dev->deactivate(dev))
> - dev->deactivate(dev);
> - } else {
> -#ifdef MODULE
> - /*
> - * Register device in the array to
> - * deactivate it on a module unload.
> - */
> - if (pnp_ide_dev_idx >= NR_PNP_DEVICES)
> - return;
> - devices[pnp_ide_dev_idx].dev = dev;
> - devices[pnp_ide_dev_idx].dev_type = dev_type;
> - pnp_ide_dev_idx++;
> -#endif
> - }
> - }
> - }
> +static struct pnp_driver idepnp_driver = {
> + .name = "ide-pnp",
> + .id_table = pnp_ide_devs,
> + .probe = pnp_ide_probe,
> + .remove = pnp_ide_remove
> +};
> +
> +void pnpide_init(int enable)
> +{
> + if (enable)
> + pnp_register_driver(&idepnp_driver);
> + else
> + pnp_unregister_driver(&idepnp_driver);
> }
> +
> Index: linux-2.5.56/drivers/ide/ide.c
> ===================================================================
> RCS file: /build/cvsroot/linux-2.5.56/drivers/ide/ide.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 ide.c
> --- linux-2.5.56/drivers/ide/ide.c 16 Dec 2002 05:16:22 -0000 1.1.1.1
> +++ linux-2.5.56/drivers/ide/ide.c 16 Dec 2002 08:52:17 -0000
> @@ -2080,7 +2080,7 @@
> buddha_init();
> }
> #endif /* CONFIG_BLK_DEV_BUDDHA */
> -#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_ISAPNP)
> +#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_PNP)
> {
> extern void pnpide_init(int enable);
> pnpide_init(1);
> @@ -2256,7 +2256,7 @@
> spin_unlock_irqrestore(&ide_lock, flags);
> return 1;
> }
> -#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_ISAPNP) &&
> defined(MODULE) +#if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_PNP)
> && defined(MODULE) pnpide_init(0);
> #endif /* CONFIG_BLK_DEV_ISAPNP */
> #ifdef CONFIG_PROC_FS

2003-01-14 15:38:48

by Jaroslav Kysela

[permalink] [raw]
Subject: Re: [PATCH] PnP update - drivers

On Mon, 13 Jan 2003, Adam Belay wrote:

> On Sun, Jan 12, 2003 at 08:30:57PM +0100, Jaroslav Kysela wrote:
> > Hi,
> >
> > this patch must be applied after PnP patch v0.94. It contains my
> > small cleanups of PnP code and I tried to rewrite almost all ISA PnP
> > drivers to new PnP subsystem except sound drivers (ALSA & OSS). Please,
> > apply to get away compilation problems.
> >
> > Jaroslav
>
>
> Hi Jaroslav,

Hi,

> Next time send pnp related changes to me directly. I would have been happy
> to include your work after I carefully reviewed it. I was planning to merge
> a very large resource algorithm improvement soon, but now because of these
> changes, that I do not even necessarily agree with, I will be unable to
> include this major improvement and bug fix for a while. Furthermore many
> people of whom are counting on me to merge thier patches will now be
> dissappointed to hear that their changes, many of which are critical for
> certain pnp hardware configurations will be delayed.

I'm sorry Adam, but you are doing this work very slowly. While I'm one of
people who approved your changes, I'm feeling a bit responsible for
situation when most of older driver cannot be compiled. So I tried to do
all changes which I need for ALSA and also revert all PnP drivers to state
when they can be USED!!!

> Although I am glad to see the drivers converted, this has been done in a way
> that is not desirable. They use compat.c which was intended as a temporary
> solution. In fact I may even remove compat.c all together. This has been
> clearly stated in both the file compat.c and pnp.txt documentation.
> Attached is a copy of Zwane's ide conversion patch against 2.5.56. It can be
> used as an example of a correct driver conversion. Notice how it is fully
> integrated into the driver model as all drivers should be.

My work is not indended to do the right conversion. It's up to maintainers
of drivers, but I tried to convert drivers to some USEABLE STATE. Also,
you have still the chance to do it better..

> I'm now unhappy with the current pnp code and will most likely revert all pnp
> changes between 2.5.56 and 2.5.57 to avoid a merging nightmare. I will then
> carefully remerge what I feel is acceptable.

Sorry, but I think that Linus won't accept your changes in this way.

> Once again, I'm sorry to those who will be unable to use thier systems due to
> this major set back. All pnp changes should be sent to me and me only. I
> believe these pnp change conflicts can easily be worked out with better
> communication. I intend to make a better effort in the future to explain my
> goals and I hope that you will do the same. I appreciate your work with the
> pnp layer.

But, please, be faster, much faster.... I'm waiting for stabilizing PnP
over two months to make our ALSA ISA PnP drivers working again with all
features. You simply deleted older ISA PnP API leaving all drivers in
non-functional stage. That's bad, very bad. Also, I have other work to do
than rewrite the PnP drivers.

Jaroslav

BTW: I tested ISA PnP code outside kernel (in ALSA drivers) more than half
of year.

BTW2: I also think that I'm author of the auto-configuration mechanism and
resource descriptions, so it would be nice to have also credit in
the pnp_init() function.

BTW3: I'm happy, that I can start with rewritting of ALSA drivers now.

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs