Re: [patch/rfc 2.6.20-git] parport reports physical devices

[David Brownell <david-b@pacbell.net>]

On Tuesday 20 February 2007 1:10 pm, Jean Delvare wrote:

> Here is the naive patch I have come up with. It does the job, even
> though it is not clean by any means. But as you said, it's certainly not
> worse than the current state, so I hope we can still apply it.

One glitch I noticed:  on driver rmmod it removes *any* parport platform
device, rather than just ones that it created.  So the minute any system
starts doing the Right Thing, registering platform devices itself, trouble
starts!  Minimally, stick a magic cookie in platform_data and don't remove
devices without that cookie.  (Cookie might be a pointer to something not
exported by that driver.)

Plus, that platform_driver won't work with a real platform_device ... but
that looks like it'd take more time to fix cleanly than I have time for.
(PowerPC and SPARC64 would probably be happier with real platform_device
setup, given what their <asm/parport.h> is doing.)

- Dave


> 
> * * * * *
> 
> Give legacy parallel ports a platform device in the device tree.
> This is a quick and dirty implementation, it doesn't actually convert
> the legacy parport code to the device driver model, but at least
> parallel port device drivers will have a device to work with.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/parport/parport_pc.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> --- linux-2.6.21-pre.orig/drivers/parport/parport_pc.c	2007-02-19 12:03:44.000000000 +0100
> +++ linux-2.6.21-pre/drivers/parport/parport_pc.c	2007-02-19 18:15:41.000000000 +0100
> @@ -53,6 +53,7 @@
>  #include <linux/slab.h>
>  #include <linux/pci.h>
>  #include <linux/pnp.h>
> +#include <linux/platform_device.h>
>  #include <linux/sysctl.h>
>  
>  #include <asm/io.h>
> @@ -2156,6 +2157,17 @@ struct parport *parport_pc_probe_port (u
>  	struct resource *base_res;
>  	struct resource	*ECR_res = NULL;
>  	struct resource	*EPP_res = NULL;
> +	struct platform_device *pdev = NULL;
> +
> +	if (!dev) {
> +		/* We need a physical device to attach to, but none was
> +		   provided. Create our own. */
> +		pdev = platform_device_register_simple("parport_pc",
> +						       base, NULL, 0);
> +		if (IS_ERR(pdev))
> +			return NULL;
> +		dev = &pdev->dev;
> +	}
>  
>  	ops = kmalloc(sizeof (struct parport_operations), GFP_KERNEL);
>  	if (!ops)
> @@ -2359,6 +2371,8 @@ out3:
>  out2:
>  	kfree (ops);
>  out1:
> +	if (pdev)
> +		platform_device_unregister(pdev);
>  	return NULL;
>  }
>  
> @@ -3106,6 +3120,21 @@ static struct pnp_driver parport_pc_pnp_
>  };
>  
>  
> +static int __devinit parport_pc_platform_probe(struct platform_device *pdev)
> +{
> +	/* Always succeed, the actual probing is done in
> +	   parport_pc_probe_port(). */
> +	return 0;
> +}
> +
> +static struct platform_driver parport_pc_platform_driver = {
> +	.driver = {
> +		.owner	= THIS_MODULE,
> +		.name	= "parport_pc",
> +	},
> +	.probe		= parport_pc_platform_probe,
> +};
> +
>  /* This is called by parport_pc_find_nonpci_ports (in asm/parport.h) */
>  static int __devinit __attribute__((unused))
>  parport_pc_find_isa_ports (int autoirq, int autodma)
> @@ -3381,9 +3410,15 @@ __setup("parport_init_mode=",parport_ini
>  
>  static int __init parport_pc_init(void)
>  {
> +	int err;
> +
>  	if (parse_parport_params())
>  		return -EINVAL;
>  
> +	err = platform_driver_register(&parport_pc_platform_driver);
> +	if (err)
> +		return err;
> +
>  	if (io[0]) {
>  		int i;
>  		/* Only probe the ports we were given. */
> @@ -3408,6 +3443,7 @@ static void __exit parport_pc_exit(void)
>  		pci_unregister_driver (&parport_pc_pci_driver);
>  	if (pnp_registered_parport)
>  		pnp_unregister_driver (&parport_pc_pnp_driver);
> +	platform_driver_unregister(&parport_pc_platform_driver);
>  
>  	spin_lock(&ports_lock);
>  	while (!list_empty(&ports_list)) {
> @@ -3416,6 +3452,9 @@ static void __exit parport_pc_exit(void)
>  		priv = list_entry(ports_list.next,
>  				  struct parport_pc_private, list);
>  		port = priv->port;
> +		if (port->dev && port->dev->bus == &platform_bus_type)
> +			platform_device_unregister(
> +				to_platform_device(port->dev));
>  		spin_unlock(&ports_lock);
>  		parport_pc_unregister_port(port);
>  		spin_lock(&ports_lock);
> 
> * * * * *
> 
> > There are probably good reasons (== deep hardware braindamage on older
> > systems that are now hard to find) for the strange init sequencing in
> > that code, but I can't see why they should prevent splitting out
> > 
> > 	(a) device discovery via probing, PNP, or whatever; from
> > 
> > 	(b) the driver itself, getting handed a device node that's
> > 	    pre-configured with the resources reported by discovery.
> > 
> > Maybe the maintainers of the parport stack will have comments.  Though
> > the info in MAINTAINERS seems dated, if not obsolete.
> 
> Phil Blundell and Tim Waugh did not reply to me last time I sent a
> parport cleanup patch to them. I suspect they are indeed no longer
> maintaining parport in practice.
> 
> -- 
> Jean Delvare
> 
-
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/