Re: [PATCH 0/3] Couple of sysfs patches

Re: [PATCH 0/3] Couple of sysfs patches

[Dmitry Torokhov]

Russell King wrote:
> On Thu, Jun 10, 2004 at 07:55:59AM -0500, Dmitry Torokhov wrote:
> > On Thursday 10 June 2004 05:16 am, Russell King wrote:
> > >
> > > As this currently stands, you have no chance to add resources to the
> > > platform device before it's made available to the driver.  It's likely
> > > that any attached resources will have the same lifetime as the
> > > device itself, so it makes sense to allocate them together with the
> > > platform device.
> > >
> >
> > Are you suggesting adding pointer to resources as a 3rd argument and
> > automotically release it for the user? It probably could be done but users
> > will be tempted to use static module data and bad things would happen.
> 
> Please read my second sentence again.  It implies a copy of the resources
> is kept with the platform device, so both have the same lifetime.
> 

Ok, so function pointer to allocate resources and associate with the
device? You can't just allocate memory for resources structure, you
need to populate it with data if you want it to be used by a driver
immediately after registration... And have actually release all
resources, not only memory? It is getting beyond the "*_simple"
approach though.

Or do I still misunderstand you?

--
Dmitry

Re: [PATCH 0/3] Couple of sysfs patches

[Russell King]

On Thu, Jun 10, 2004 at 07:46:58AM -0700, Dmitry Torokhov wrote:
> Russell King wrote:
> > On Thu, Jun 10, 2004 at 07:55:59AM -0500, Dmitry Torokhov wrote:
> > > On Thursday 10 June 2004 05:16 am, Russell King wrote:
> > > >
> > > > As this currently stands, you have no chance to add resources to the
> > > > platform device before it's made available to the driver.  It's likely
> > > > that any attached resources will have the same lifetime as the
> > > > device itself, so it makes sense to allocate them together with the
> > > > platform device.
> > > >
> > >
> > > Are you suggesting adding pointer to resources as a 3rd argument and
> > > automotically release it for the user? It probably could be done but users
> > > will be tempted to use static module data and bad things would happen.
> > 
> > Please read my second sentence again.  It implies a copy of the resources
> > is kept with the platform device, so both have the same lifetime.
> > 
> 
> Ok, so function pointer to allocate resources and associate with the
> device? You can't just allocate memory for resources structure, you
> need to populate it with data if you want it to be used by a driver
> immediately after registration... And have actually release all
> resources, not only memory? It is getting beyond the "*_simple"
> approach though.
> 
> Or do I still misunderstand you?

Something like:

struct platform_object {
	struct platform_device pdev;
	struct resource resources[0];
};

struct platform_device *platform_device_register_simple(char *name, unsigned int id, struct resource *res, int num)
{
	struct platform_object *pobj;
	int retval;

	pobj = kmalloc(sizeof(struct platform_object) + sizeof(struct resource) * num, GFP_KERNEL);
	if (!pobj) {
		retval = -ENOMEM;
		goto error;
	}

	memset(pobj, 0, sizeof(*pdev));
	pobj->pdev.name = name;
	pobj->pdev.id = id;
	pobj->pdev.dev.release = platform_device_release_simple;

	if (num) {
		memcpy(pobj->resources, res, sizeof(struct resource) * num);
		pobj->pdev.resource = pobj->resources;
		pobj->pdev.num_resources = num;
	}

ARM is going very much down this route, and we're also going to need these
things dynamically allocated as you do, but with the resource stuff.  So
rather than putting in something which only satisfies your problem, and
then adding yet more interfaces to cover the extra resources, we should
be thinking about something sane from the beginning.

Now that I can see the platform device interfaces multipling like rabbits,
(to GregKH) I think that the patch I submitted for platform_add_device
suffers from this problem as well, and I should've thrown that code
into platform_register_device itself.

Greg - comments?  Would you like a new patch which does that, or do you
think that's too risky?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH 0/3] Couple of sysfs patches

[Greg KH]

On Thu, Jun 10, 2004 at 05:06:07PM +0100, Russell King wrote:
> 
> Now that I can see the platform device interfaces multipling like rabbits,
> (to GregKH) I think that the patch I submitted for platform_add_device
> suffers from this problem as well, and I should've thrown that code
> into platform_register_device itself.
> 
> Greg - comments?  Would you like a new patch which does that, or do you
> think that's too risky?

Hm, I don't think it's too risky.  Make up a patch and let's see how it
looks.

I'm just worried that this "simple" interface really isn't so simple, as
it's almost just as much work to manage it as a normal platform device.

thanks,

greg k-h

Re: [PATCH 0/3] Couple of sysfs patches

[Russell King]

On Thu, Jun 10, 2004 at 09:14:42AM -0700, Greg KH wrote:
> On Thu, Jun 10, 2004 at 05:06:07PM +0100, Russell King wrote:
> > 
> > Now that I can see the platform device interfaces multipling like rabbits,
> > (to GregKH) I think that the patch I submitted for platform_add_device
> > suffers from this problem as well, and I should've thrown that code
> > into platform_register_device itself.
> > 
> > Greg - comments?  Would you like a new patch which does that, or do you
> > think that's too risky?
> 
> Hm, I don't think it's too risky.  Make up a patch and let's see how it
> looks.
> 
> I'm just worried that this "simple" interface really isn't so simple, as
> it's almost just as much work to manage it as a normal platform device.

Ok, here's a patch so you can see what I'm suggesting above.  This is
on top of the previous patch I sent.  Merely discards one over-eager
rabbit [1] and moves the code into platform_device_register().

[1]: No animals were harmed in the creation of this patch.

diff -u -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej linux-tolinus/drivers/base/platform.c linux/drivers/base/platform.c
--- linux-tolinus/drivers/base/platform.c	Thu Jun 10 18:52:45 2004
+++ linux/drivers/base/platform.c	Thu Jun 10 19:15:59 2004
@@ -55,51 +55,24 @@
 }
 
 /**
- *	platform_add_device - add one platform device
- *	@dev: platform device
- *
- *	Adds one platform device, claiming the memory resources
- */
-int platform_add_device(struct platform_device *dev)
-{
-	int i;
-
-	for (i = 0; i < dev->num_resources; i++) {
-		struct resource *p, *r = &dev->resource[i];
-
-		r->name = dev->dev.bus_id;
-
-		p = NULL;
-		if (r->flags & IORESOURCE_MEM)
-			p = &iomem_resource;
-		else if (r->flags & IORESOURCE_IO)
-			p = &ioport_resource;
-
-		if (p && request_resource(p, r)) {
-			printk(KERN_ERR
-			       "%s%d: failed to claim resource %d\n",
-			       dev->name, dev->id, i);
-			break;
-		}
-	}
-	if (i == dev->num_resources)
-		platform_device_register(dev);
-	return 0;
-}
-
-/**
  *	platform_add_devices - add a numbers of platform devices
  *	@devs: array of platform devices to add
  *	@num: number of platform devices in array
  */
 int platform_add_devices(struct platform_device **devs, int num)
 {
-	int i;
+	int i, ret = 0;
 
-	for (i = 0; i < num; i++)
-		platform_add_device(devs[i]);
+	for (i = 0; i < num; i++) {
+		ret = platform_device_register(devs[i]);
+		if (ret) {
+			while (--i > 0)
+				platform_device_unregister(devs[i]);
+			break;
+		}
+	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -109,6 +82,8 @@
  */
 int platform_device_register(struct platform_device * pdev)
 {
+	int i, ret = 0;
+
 	if (!pdev)
 		return -EINVAL;
 
@@ -119,9 +94,37 @@
 	
 	snprintf(pdev->dev.bus_id,BUS_ID_SIZE,"%s%u",pdev->name,pdev->id);
 
-	pr_debug("Registering platform device '%s'. Parent at %s\n",
-		 pdev->dev.bus_id,pdev->dev.parent->bus_id);
-	return device_register(&pdev->dev);
+	for (i = 0; i < pdev->num_resources; i++) {
+		struct resource *p, *r = &pdev->resource[i];
+
+		r->name = pdev->dev.bus_id;
+
+		p = NULL;
+		if (r->flags & IORESOURCE_MEM)
+			p = &iomem_resource;
+		else if (r->flags & IORESOURCE_IO)
+			p = &ioport_resource;
+
+		if (p && request_resource(p, r)) {
+			printk(KERN_ERR
+			       "%s%d: failed to claim resource %d\n",
+			       pdev->name, pdev->id, i);
+
+			while (--i > 0)
+				release_resource(&pdev->resource[i]);
+			ret = -EBUSY;
+			break;
+		}
+	}
+
+	if (ret == 0) {
+		pr_debug("Registering platform device '%s'. Parent at %s\n",
+			 pdev->dev.bus_id,pdev->dev.parent->bus_id);
+
+		ret = device_register(&pdev->dev);
+	}
+
+	return ret;
 }
 
 void platform_device_unregister(struct platform_device * pdev)
diff -u -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej linux-tolinus/include/linux/device.h linux/include/linux/device.h
--- linux-tolinus/include/linux/device.h	Thu Jun 10 18:52:46 2004
+++ linux/include/linux/device.h	Thu Jun 10 19:15:03 2004
@@ -392,7 +392,6 @@
 
 extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
 extern int platform_get_irq(struct platform_device *, unsigned int);
-extern int platform_add_device(struct platform_device *);
 extern int platform_add_devices(struct platform_device **, int);
 
 /* drivers/base/power.c */

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH 0/3] Couple of sysfs patches

[Russell King]

On Thu, Jun 10, 2004 at 07:17:40PM +0100, Russell King wrote:
> On Thu, Jun 10, 2004 at 09:14:42AM -0700, Greg KH wrote:
> > On Thu, Jun 10, 2004 at 05:06:07PM +0100, Russell King wrote:
> > > 
> > > Now that I can see the platform device interfaces multipling like rabbits,
> > > (to GregKH) I think that the patch I submitted for platform_add_device
> > > suffers from this problem as well, and I should've thrown that code
> > > into platform_register_device itself.
> > > 
> > > Greg - comments?  Would you like a new patch which does that, or do you
> > > think that's too risky?
> > 
> > Hm, I don't think it's too risky.  Make up a patch and let's see how it
> > looks.
> > 
> > I'm just worried that this "simple" interface really isn't so simple, as
> > it's almost just as much work to manage it as a normal platform device.
> 
> Ok, here's a patch so you can see what I'm suggesting above.  This is
> on top of the previous patch I sent.  Merely discards one over-eager
> rabbit [1] and moves the code into platform_device_register().
> 
> [1]: No animals were harmed in the creation of this patch.

And for added good behaviour, particularly when things go wrong.

diff -u -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej linux-tolinus/drivers/base/platform.c linux/drivers/base/platform.c
--- linux-tolinus/drivers/base/platform.c	Thu Jun 10 18:52:45 2004
+++ linux/drivers/base/platform.c	Thu Jun 10 20:22:50 2004
@@ -55,51 +55,24 @@
 }
 
 /**
- *	platform_add_device - add one platform device
- *	@dev: platform device
- *
- *	Adds one platform device, claiming the memory resources
- */
-int platform_add_device(struct platform_device *dev)
-{
-	int i;
-
-	for (i = 0; i < dev->num_resources; i++) {
-		struct resource *p, *r = &dev->resource[i];
-
-		r->name = dev->dev.bus_id;
-
-		p = NULL;
-		if (r->flags & IORESOURCE_MEM)
-			p = &iomem_resource;
-		else if (r->flags & IORESOURCE_IO)
-			p = &ioport_resource;
-
-		if (p && request_resource(p, r)) {
-			printk(KERN_ERR
-			       "%s%d: failed to claim resource %d\n",
-			       dev->name, dev->id, i);
-			break;
-		}
-	}
-	if (i == dev->num_resources)
-		platform_device_register(dev);
-	return 0;
-}
-
-/**
  *	platform_add_devices - add a numbers of platform devices
  *	@devs: array of platform devices to add
  *	@num: number of platform devices in array
  */
 int platform_add_devices(struct platform_device **devs, int num)
 {
-	int i;
+	int i, ret = 0;
 
-	for (i = 0; i < num; i++)
-		platform_add_device(devs[i]);
+	for (i = 0; i < num; i++) {
+		ret = platform_device_register(devs[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(devs[i]);
+			break;
+		}
+	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -109,6 +82,8 @@
  */
 int platform_device_register(struct platform_device * pdev)
 {
+	int i, ret = 0;
+
 	if (!pdev)
 		return -EINVAL;
 
@@ -119,9 +94,38 @@
 	
 	snprintf(pdev->dev.bus_id,BUS_ID_SIZE,"%s%u",pdev->name,pdev->id);
 
+	for (i = 0; i < pdev->num_resources; i++) {
+		struct resource *p, *r = &pdev->resource[i];
+
+		r->name = pdev->dev.bus_id;
+
+		p = NULL;
+		if (r->flags & IORESOURCE_MEM)
+			p = &iomem_resource;
+		else if (r->flags & IORESOURCE_IO)
+			p = &ioport_resource;
+
+		if (p && request_resource(p, r)) {
+			printk(KERN_ERR
+			       "%s: failed to claim resource %d\n",
+			       pdev->dev.bus_id, i);
+			ret = -EBUSY;
+			goto failed;
+		}
+	}
+
 	pr_debug("Registering platform device '%s'. Parent at %s\n",
 		 pdev->dev.bus_id,pdev->dev.parent->bus_id);
-	return device_register(&pdev->dev);
+
+	ret = device_register(&pdev->dev);
+	if (ret == 0)
+		return ret;
+
+ failed:
+	while (--i >= 0)
+		if (pdev->resource[i].flags & (IORESOURCE_MEM|IORESOURCE_IO))
+			release_resource(&pdev->resource[i]);
+	return ret;
 }
 
 void platform_device_unregister(struct platform_device * pdev)
diff -u -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej linux-tolinus/include/linux/device.h linux/include/linux/device.h
--- linux-tolinus/include/linux/device.h	Thu Jun 10 18:52:46 2004
+++ linux/include/linux/device.h	Thu Jun 10 19:15:03 2004
@@ -392,7 +392,6 @@
 
 extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
 extern int platform_get_irq(struct platform_device *, unsigned int);
-extern int platform_add_device(struct platform_device *);
 extern int platform_add_devices(struct platform_device **, int);
 
 /* drivers/base/power.c */


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH 0/3] Couple of sysfs patches

[Dmitry Torokhov]

On Thursday 10 June 2004 03:25 pm, Russell King wrote:
> On Thu, Jun 10, 2004 at 07:17:40PM +0100, Russell King wrote:
> > On Thu, Jun 10, 2004 at 09:14:42AM -0700, Greg KH wrote:
> > > On Thu, Jun 10, 2004 at 05:06:07PM +0100, Russell King wrote:
> > > > 
> > > > Now that I can see the platform device interfaces multipling like rabbits,
> > > > (to GregKH) I think that the patch I submitted for platform_add_device
> > > > suffers from this problem as well, and I should've thrown that code
> > > > into platform_register_device itself.
> > > > 
> > > > Greg - comments?  Would you like a new patch which does that, or do you
> > > > think that's too risky?
> > > 
> > > Hm, I don't think it's too risky.  Make up a patch and let's see how it
> > > looks.
> > > 
> > > I'm just worried that this "simple" interface really isn't so simple, as
> > > it's almost just as much work to manage it as a normal platform device.
> > 
> > Ok, here's a patch so you can see what I'm suggesting above.  This is
> > on top of the previous patch I sent.  Merely discards one over-eager
> > rabbit [1] and moves the code into platform_device_register().
> > 
> > [1]: No animals were harmed in the creation of this patch.
> 
> And for added good behaviour, particularly when things go wrong.
> 
>  
> +	for (i = 0; i < pdev->num_resources; i++) {
> +		struct resource *p, *r = &pdev->resource[i];
> +
> +		r->name = pdev->dev.bus_id;
> +
> +		p = NULL;
> +		if (r->flags & IORESOURCE_MEM)
> +			p = &iomem_resource;
> +		else if (r->flags & IORESOURCE_IO)
> +			p = &ioport_resource;
> +
> +		if (p && request_resource(p, r)) {
> +			printk(KERN_ERR
> +			       "%s: failed to claim resource %d\n",
> +			       pdev->dev.bus_id, i);
> +			ret = -EBUSY;
> +			goto failed;
> +		}
> +	}
> +

What about freeing the resources? Can it be put in platform_device_unregister
or is it release handler task? I'd put it in unregister because when I call
unregister I expect device be half-dead and release as much resources as it
can.

-- 
Dmitry

Re: [PATCH 0/3] Couple of sysfs patches

[Russell King]

On Wed, Jun 16, 2004 at 05:51:03PM -0500, Dmitry Torokhov wrote:
> What about freeing the resources? Can it be put in platform_device_unregister
> or is it release handler task? I'd put it in unregister because when I call
> unregister I expect device be half-dead and release as much resources as it
> can.

Greg,

Here's the updated patch - to be applied on top of the
platform_get_resource() patch sent previously.

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej orig/drivers/base/platform.c linux/drivers/base/platform.c
--- orig/drivers/base/platform.c	Fri Jun 18 20:28:13 2004
+++ linux/drivers/base/platform.c	Fri Jun 18 20:24:02 2004
@@ -55,51 +55,24 @@ int platform_get_irq(struct platform_dev
 }
 
 /**
- *	platform_add_device - add one platform device
- *	@dev: platform device
- *
- *	Adds one platform device, claiming the memory resources
- */
-int platform_add_device(struct platform_device *dev)
-{
-	int i;
-
-	for (i = 0; i < dev->num_resources; i++) {
-		struct resource *p, *r = &dev->resource[i];
-
-		r->name = dev->dev.bus_id;
-
-		p = NULL;
-		if (r->flags & IORESOURCE_MEM)
-			p = &iomem_resource;
-		else if (r->flags & IORESOURCE_IO)
-			p = &ioport_resource;
-
-		if (p && request_resource(p, r)) {
-			printk(KERN_ERR
-			       "%s%d: failed to claim resource %d\n",
-			       dev->name, dev->id, i);
-			break;
-		}
-	}
-	if (i == dev->num_resources)
-		platform_device_register(dev);
-	return 0;
-}
-
-/**
  *	platform_add_devices - add a numbers of platform devices
  *	@devs: array of platform devices to add
  *	@num: number of platform devices in array
  */
 int platform_add_devices(struct platform_device **devs, int num)
 {
-	int i;
+	int i, ret = 0;
 
-	for (i = 0; i < num; i++)
-		platform_add_device(devs[i]);
+	for (i = 0; i < num; i++) {
+		ret = platform_device_register(devs[i]);
+		if (ret) {
+			while (--i >= 0)
+				platform_device_unregister(devs[i]);
+			break;
+		}
+	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -109,6 +82,8 @@ int platform_add_devices(struct platform
  */
 int platform_device_register(struct platform_device * pdev)
 {
+	int i, ret = 0;
+
 	if (!pdev)
 		return -EINVAL;
 
@@ -119,15 +94,53 @@ int platform_device_register(struct plat
 	
 	snprintf(pdev->dev.bus_id,BUS_ID_SIZE,"%s%u",pdev->name,pdev->id);
 
+	for (i = 0; i < pdev->num_resources; i++) {
+		struct resource *p, *r = &pdev->resource[i];
+
+		r->name = pdev->dev.bus_id;
+
+		p = NULL;
+		if (r->flags & IORESOURCE_MEM)
+			p = &iomem_resource;
+		else if (r->flags & IORESOURCE_IO)
+			p = &ioport_resource;
+
+		if (p && request_resource(p, r)) {
+			printk(KERN_ERR
+			       "%s: failed to claim resource %d\n",
+			       pdev->dev.bus_id, i);
+			ret = -EBUSY;
+			goto failed;
+		}
+	}
+
 	pr_debug("Registering platform device '%s'. Parent at %s\n",
 		 pdev->dev.bus_id,pdev->dev.parent->bus_id);
-	return device_register(&pdev->dev);
+
+	ret = device_register(&pdev->dev);
+	if (ret == 0)
+		return ret;
+
+ failed:
+	while (--i >= 0)
+		if (pdev->resource[i].flags & (IORESOURCE_MEM|IORESOURCE_IO))
+			release_resource(&pdev->resource[i]);
+	return ret;
 }
 
 void platform_device_unregister(struct platform_device * pdev)
 {
-	if (pdev)
+	int i;
+
+	if (pdev) {
 		device_unregister(&pdev->dev);
+
+		for (i = 0; i < pdev->num_resources; i++) {
+			struct resource *r = &pdev->resource[i];
+			if (r->flags & (IORESOURCE_MEM|IORESOURCE_IO))
+				release_resource(r);
+		}
+	}
 }
 
 
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej orig/include/linux/device.h linux/include/linux/device.h
--- orig/include/linux/device.h	Fri Jun 18 20:28:13 2004
+++ linux/include/linux/device.h	Thu Jun 10 19:15:03 2004
@@ -392,7 +392,6 @@ extern struct device platform_bus;
 
 extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int);
 extern int platform_get_irq(struct platform_device *, unsigned int);
-extern int platform_add_device(struct platform_device *);
 extern int platform_add_devices(struct platform_device **, int);
 
 /* drivers/base/power.c */

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH 0/3] Couple of sysfs patches

[Greg KH]

On Fri, Jun 18, 2004 at 08:29:49PM +0100, Russell King wrote:
> On Wed, Jun 16, 2004 at 05:51:03PM -0500, Dmitry Torokhov wrote:
> > What about freeing the resources? Can it be put in platform_device_unregister
> > or is it release handler task? I'd put it in unregister because when I call
> > unregister I expect device be half-dead and release as much resources as it
> > can.
> 
> Greg,
> 
> Here's the updated patch - to be applied on top of the
> platform_get_resource() patch sent previously.

Looks good, thanks I've applied this and will send it on.

greg k-h

Re: [PATCH 0/3] Couple of sysfs patches

[Dmitry Torokhov]

Russell King wrote:
>  void platform_device_unregister(struct platform_device * pdev)
>  {
> -	if (pdev)
> +	int i;
> +
> +	if (pdev) {
>  		device_unregister(&pdev->dev);
> +
> +		for (i = 0; i < pdev->num_resources; i++) {
> +			struct resource *r = &pdev->resource[i];
> +			if (r->flags & (IORESOURCE_MEM|IORESOURCE_IO))
> +				release_resource(r);
> +		}
> +	}
>  }

Ok, now it's possibly just a nitpicking but would not it be "more correct"
if allocated resources were freed in reverse order?

--
Dmitry

[PATCH 0/3] Couple of sysfs patches

[Dmitry Torokhov]

Hi,

I am trying to add sysfs support to the serio subsystem and I would like you
to consider the following changes:
 
- when registering platform device, if device id is set to -1, do not add it
  as a suffix to device's name. It can be used when there is only one device.
  The reason for change - i find that i80420 looks ugly, just i8042 is much
  better ;)

- create platform_device_simple_releasse function that would just free memory
  occupied by platform device. Having such release routine in core allows
  drivers implementing simple platform devices not wait in module unload code
  till last reference to the device is dropped.

And of course whitespace changes ;) in the very first patch.

Please consider applying.

-- 
Dmitry

Re: [PATCH 0/3] Couple of sysfs patches

[Greg KH]

On Wed, Jun 09, 2004 at 02:21:24AM -0500, Dmitry Torokhov wrote:
> Hi,
> 
> I am trying to add sysfs support to the serio subsystem and I would like you
> to consider the following changes:
>  
> - when registering platform device, if device id is set to -1, do not add it
>   as a suffix to device's name. It can be used when there is only one device.
>   The reason for change - i find that i80420 looks ugly, just i8042 is much
>   better ;)

That sounds sane.

> - create platform_device_simple_releasse function that would just free memory
>   occupied by platform device. Having such release routine in core allows
>   drivers implementing simple platform devices not wait in module unload code
>   till last reference to the device is dropped.

I'm _very_ leary of applying this.  It could be abused very badly
(by putting the platform_device as the first object in the structure and
relying on this function to kfree the whole thing.)  So sorry, but no.

Unless you show me a real need for it..

And as for the whitespace cleanup, why?  The lack of spaces seem to be
something that the original author liked doing.  There's nothing in the
kernel coding style guidelines that really mention it that I can see.

So, care to just send me the name change patch against the latest tree?

thanks,

greg k-h

Re: [PATCH 0/3] Couple of sysfs patches

[Greg KH]

On Wed, Jun 09, 2004 at 05:32:28PM -0500, Dmitry Torokhov wrote:
> Actually, I myself want someting else -
> 
> int platform_device_register_simple(struct platform_device **ppdev,
> 				    const char *name, int id)
> 
> It will allocate platform device, set name and id and release function to
> platform_device_simple_release which in turn will be hidden from outside
> world. Since the function does allocation for user is should prevent the
> abuse you were concerned about.

Ok, that sounds good.  I'll take patches for that kind of interface.

But have the function return the pointer, like the class_simple
functions work.  Not the ** like you just specified.

> > Unless you show me a real need for it..
> > 
> > And as for the whitespace cleanup, why?  The lack of spaces seem to be
> > something that the original author liked doing.  There's nothing in the
> > kernel coding style guidelines that really mention it that I can see.
> >
>  
> If you check the files in question there were already both styles present
> (with and without spaces). And trailing whitespace looks bloody red in my
> vi ;) Will you accept just trailing whitespace cleanup?

Hm, ok, you are correct about the duplicate styles, ok, I'll take the
whole cleanup.  But things have changed in these files lately, and your
patch doesn't apply :(  Care to resend this against the latest -mm
release which has all of the changes in it?

thanks,

greg k-h

Re: [PATCH 0/3] Couple of sysfs patches

[Greg KH]

On Wed, Jun 09, 2004 at 05:54:23PM -0500, Dmitry Torokhov wrote:
> On Wednesday 09 June 2004 05:45 pm, Greg KH wrote:
> > On Wed, Jun 09, 2004 at 05:32:28PM -0500, Dmitry Torokhov wrote:
> > > Actually, I myself want someting else -
> > > 
> > > int platform_device_register_simple(struct platform_device **ppdev,
> > > 				    const char *name, int id)
> > > 
> > > It will allocate platform device, set name and id and release function to
> > > platform_device_simple_release which in turn will be hidden from outside
> > > world. Since the function does allocation for user is should prevent the
> > > abuse you were concerned about.
> > 
> > Ok, that sounds good.  I'll take patches for that kind of interface.
> > 
> > But have the function return the pointer, like the class_simple
> > functions work.  Not the ** like you just specified.
> 
> I want to do both allocation + registration in one shot and I knowing
> the error code may be important to users.

That's fine to do.  Again, look at how the class_simple_create()
function works.  If an error happens, convert it to ERR_PTR() and return
that.  The caller can check it with IS_ERR() and friends.

> Why do you oppose having double pointers in interface?

It's messy, and with the ERR_PTR() macros, not needed :)

thanks,

greg k-h

Re: [PATCH 0/3] Couple of sysfs patches

[Dmitry Torokhov]

On Wednesday 09 June 2004 06:19 pm, Greg KH wrote:
> On Wed, Jun 09, 2004 at 05:54:23PM -0500, Dmitry Torokhov wrote:
> > On Wednesday 09 June 2004 05:45 pm, Greg KH wrote:
> > > On Wed, Jun 09, 2004 at 05:32:28PM -0500, Dmitry Torokhov wrote:
> > > > Actually, I myself want someting else -
> > > > 
> > > > int platform_device_register_simple(struct platform_device **ppdev,
> > > > 				    const char *name, int id)
> > > > 
> > > > It will allocate platform device, set name and id and release function to
> > > > platform_device_simple_release which in turn will be hidden from outside
> > > > world. Since the function does allocation for user is should prevent the
> > > > abuse you were concerned about.
> > > 
> > > Ok, that sounds good.  I'll take patches for that kind of interface.
> > > 
> > > But have the function return the pointer, like the class_simple
> > > functions work.  Not the ** like you just specified.
> > 
> > I want to do both allocation + registration in one shot and I knowing
> > the error code may be important to users.
> 
> That's fine to do.  Again, look at how the class_simple_create()
> function works.  If an error happens, convert it to ERR_PTR() and return
> that.  The caller can check it with IS_ERR() and friends.

I apologize, I wrote above hastily, without looking at class_simple_create
implementation.

Please take a look at the new version of patces. I changed the order so
whitespace changes are on 3rd place to ease merging. The patches are against
tonight's bk pull from Linus' tree.

There is also 4th patch that allows a device driver to register/unregister
a new device on the same bus from driver's probe/remove routine. I need this
functionality for implementing pass-through ports, I tried to work around
sysfs limitation but it was too ugly for words.

Please let me know what you think.
 
-- 
Dmitry