[lm-sensors] some remarks over platform_device use in f71805f.c

[lm-sensors] some remarks over platform_device use in f71805f.c

[Hans de Goede]

Hi,

After reading the discussion about the usage of platform_device in 
f71805f.c, I've been busy converting the Abit uGuru driver to a 
platform_driver.

I've taken the f71805f.c file as an example and see some room for 
improvements there:

-f71805f_device_add can effectively be removed by using
  platform_device_register_simple, which does all this in 1 step except
  for filling the resource struct.

-Also you pass in the base address as the id, this will lead to a dir
  name in sysfs of f71805f<baseaddr> where base addr will be decimal.

  Since you clearly plan on supporting only one device for now, you
  should / could pass -1 as id, which will get you a sysfs dir entry of
  just f71805f.

-You've made the resource struct a static global, but it can be
  a normal local variable since the platform_device copy allocs its own
  copy, see the lifetime is not an issue.


Regards,

Hans

[lm-sensors] some remarks over platform_device use in f71805f.c

[Jean Delvare]

Hi Hans,

> After reading the discussion about the usage of platform_device in 
> f71805f.c, I've been busy converting the Abit uGuru driver to a 
> platform_driver.
> 
> I've taken the f71805f.c file as an example and see some room for 
> improvements there:

Thanks for the review and comments, these are always welcome.

> -f71805f_device_add can effectively be removed by using
>   platform_device_register_simple, which does all this in 1 step except
>   for filling the resource struct.

Except that platform_device_register_simple is planned for removal soon:
  http://marc.theaimsgroup.com/?l=linux-kernel&m\x113696287624649&w=2

> -Also you pass in the base address as the id, this will lead to a dir
>   name in sysfs of f71805f<baseaddr> where base addr will be decimal.
> 
>   Since you clearly plan on supporting only one device for now, you
>   should / could pass -1 as id, which will get you a sysfs dir entry of
>   just f71805f.

The problem is that we need the address in userspace (for libsensors).
This was the most simple way to pass it, with the added benefit that it
also guarantees the uniqueness if we ever have two similar ISA hardware
monitoring chips in a system (very unlikely, I admit.)

I could have gone with no ID but then we would have had to add an
address sysfs attribute, and then add code to libsensors to grab its
value.

I'm not particularly happy with the current implementation (the decimal
address is not very elegant) but it works and was very simple to setup.
If anyone wants it to be implemented differently, please speak up now
and provide patches (both to the f71805f drivers and to libsensors) for
the alternative implementation.

> -You've made the resource struct a static global, but it can be
>   a normal local variable since the platform_device copy allocs its own
>   copy, see the lifetime is not an issue.

It is tagged __initdata so it will be freed after initialization anyway
(at least as I understand the __init tags.) However, it now seems to me
that using a global that way is not correct, because it prevents
f71805f_device_add twice in parallel. We don't do it anyway, but the
driver design shouldn't prevent it.

Usually I don't much like structures on the stack, but struct resource
isn't too large, so it should be acceptable. Can you please propose a
patch?

Thanks,
-- 
Jean Delvare

[lm-sensors] some remarks over platform_device use in f71805f.c

[Jean Delvare]

Hi Hans,

> > -You've made the resource struct a static global, but it can be
> >   a normal local variable since the platform_device copy allocs its own
> >   copy, see the lifetime is not an issue.
> 
> It is tagged __initdata so it will be freed after initialization anyway
> (at least as I understand the __init tags.) However, it now seems to me
> that using a global that way is not correct, because it prevents
> f71805f_device_add twice in parallel. We don't do it anyway, but the
> driver design shouldn't prevent it.
> 
> Usually I don't much like structures on the stack, but struct resource
> isn't too large, so it should be acceptable. Can you please propose a
> patch?

Something like this?

The F71805F I/O resource structure needs not be a global variable,
as the platform core allocs its own copy of it anyway.

Signed-off-by: Jean Delvare <khali at linux-fr.org>
---
 drivers/hwmon/f71805f.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

--- linux-2.6.16-rc5.orig/drivers/hwmon/f71805f.c	2006-02-27 21:01:16.000000000 +0100
+++ linux-2.6.16-rc5/drivers/hwmon/f71805f.c	2006-03-02 22:44:15.000000000 +0100
@@ -99,10 +99,6 @@
 #define ADDR_REG_OFFSET		0
 #define DATA_REG_OFFSET		1
 
-static struct resource f71805f_resource __initdata = {
-	.flags	= IORESOURCE_IO,
-};
-
 /*
  * Registers
  */
@@ -782,6 +778,11 @@
 
 static int __init f71805f_device_add(unsigned short address)
 {
+	struct resource res = {
+		.start	= address,
+		.end	= address + REGION_LENGTH - 1,
+		.flags	= IORESOURCE_IO,
+	};
 	int err;
 
 	pdev = platform_device_alloc(DRVNAME, address);
@@ -791,10 +792,8 @@
 		goto exit;
 	}
 
-	f71805f_resource.start = address;
-	f71805f_resource.end = address + REGION_LENGTH - 1;
-	f71805f_resource.name = pdev->name;
-	err = platform_device_add_resources(pdev, &f71805f_resource, 1);
+	res.name = pdev->name;
+	err = platform_device_add_resources(pdev, &res, 1);
 	if (err) {
 		printk(KERN_ERR DRVNAME ": Device resource addition failed "
 		       "(%d)\n", err);

Thanks,
-- 
Jean Delvare