[PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

The AppleSMC device is described in ACPI, including a list of its resources.
We should use those rather than hardcoding the ports. A side-effect is that
we can then remove the DMI matching, since there's a unique identifier to
indicate that the machine has one of these devices.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |  185 +++++++++++++++++++++++-----------------------
 1 files changed, 91 insertions(+), 94 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index ee91d69..dbe3fa6 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -30,7 +30,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/delay.h>
-#include <linux/platform_device.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -43,11 +42,13 @@
 #include <linux/leds.h>
 #include <linux/hwmon.h>
 #include <linux/workqueue.h>
+#include <linux/slab.h>
+#include <linux/pnp.h>
 
 /* data port used by Apple SMC */
-#define APPLESMC_DATA_PORT	0x300
+#define APPLESMC_DATA_PORT	0x0
 /* command/status port used by Apple SMC */
-#define APPLESMC_CMD_PORT	0x304
+#define APPLESMC_CMD_PORT	0x4
 
 #define APPLESMC_NR_PORTS	32 /* 0x300-0x31f */
 
@@ -76,6 +77,8 @@
 #define MOTION_SENSOR_Z_KEY	"MO_Z" /* r-o sp78 (2 bytes) */
 #define MOTION_SENSOR_KEY	"MOCN" /* r/w ui16 */
 
+#define NOTIFICATION_KEY	"NTOK"
+
 #define FANS_COUNT		"FNum" /* r-o ui8 */
 #define FANS_MANUAL		"FS! " /* r-w ui16 */
 #define FAN_ID_FMT		"F%dID" /* r-o char[16] */
@@ -103,6 +106,14 @@ static const char *const fan_speed_fmt[] = {
 #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
 #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
 
+struct applesmc_pnp_device {
+	int iobase;
+	int iolen;
+};
+
+struct pnp_dev *pdev;
+struct applesmc_pnp_device *pnp_device;
+
 /* Dynamic device node attributes */
 struct applesmc_dev_attr {
 	struct sensor_device_attribute sda;	/* hwmon attributes */
@@ -143,7 +154,6 @@ static struct applesmc_registers {
 } smcreg;
 
 static const int debug;
-static struct platform_device *pdev;
 static s16 rest_x;
 static s16 rest_y;
 static u8 backlight_state[2];
@@ -159,6 +169,16 @@ static unsigned int key_at_index;
 
 static struct workqueue_struct *applesmc_led_wq;
 
+static u8 applesmc_read_reg(u8 reg)
+{
+	return inb(pnp_device->iobase + reg);
+}
+
+static void applesmc_write_reg(u8 val, u8 reg)
+{
+	outb(val, pnp_device->iobase + reg);
+}
+
 /*
  * __wait_status - Wait up to 32ms for the status port to get a certain value
  * (masked with 0x0f), returning zero if the value is obtained.  Callers must
@@ -172,7 +192,8 @@ static int __wait_status(u8 val)
 
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == val) {
 			return 0;
 		}
 	}
@@ -189,9 +210,10 @@ static int send_command(u8 cmd)
 {
 	int us;
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		outb(cmd, APPLESMC_CMD_PORT);
+		applesmc_write_reg(cmd, APPLESMC_CMD_PORT);
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == 0x0c)
 			return 0;
 	}
 	return -EIO;
@@ -202,7 +224,7 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++) {
-		outb(key[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(key[i], APPLESMC_DATA_PORT);
 		if (__wait_status(0x04))
 			return -EIO;
 	}
@@ -218,14 +240,14 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x05)) {
 			pr_warn("%s: read data fail\n", key);
 			return -EIO;
 		}
-		buffer[i] = inb(APPLESMC_DATA_PORT);
+		buffer[i] = applesmc_read_reg(APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -240,14 +262,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x04)) {
 			pr_warn("%s: write data fail\n", key);
 			return -EIO;
 		}
-		outb(buffer[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(buffer[i], APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -566,20 +588,6 @@ static void applesmc_destroy_smcreg(void)
 	memset(&smcreg, 0, sizeof(smcreg));
 }
 
-/* Device model stuff */
-static int applesmc_probe(struct platform_device *dev)
-{
-	int ret;
-
-	ret = applesmc_init_smcreg();
-	if (ret)
-		return ret;
-
-	applesmc_device_init();
-
-	return 0;
-}
-
 /* Synchronize device with memorized backlight state */
 static int applesmc_pm_resume(struct device *dev)
 {
@@ -600,15 +608,6 @@ static const struct dev_pm_ops applesmc_pm_ops = {
 	.restore = applesmc_pm_restore,
 };
 
-static struct platform_driver applesmc_driver = {
-	.probe = applesmc_probe,
-	.driver	= {
-		.name = "applesmc",
-		.owner = THIS_MODULE,
-		.pm = &applesmc_pm_ops,
-	},
-};
-
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1174,72 +1173,48 @@ static void applesmc_release_key_backlight(void)
 	destroy_workqueue(applesmc_led_wq);
 }
 
-static int applesmc_dmi_match(const struct dmi_system_id *id)
+static int __devinit applesmc_pnp_probe(struct pnp_dev *dev,
+					const struct pnp_device_id *dev_id)
 {
-	return 1;
-}
+	int ret;
+	struct resource *res;
+	struct applesmc_pnp_device *applesmc_pnp_device;
 
-/* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
- * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
-static __initdata struct dmi_system_id applesmc_whitelist[] = {
-	{ applesmc_dmi_match, "Apple MacBook Air", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook Pro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
-	},
-	{ applesmc_dmi_match, "Apple Macmini", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
-	},
-	{ applesmc_dmi_match, "Apple MacPro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
-	},
-	{ applesmc_dmi_match, "Apple iMac", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
-	},
-	{ .ident = NULL }
-};
+	pdev = dev;
 
-static int __init applesmc_init(void)
-{
-	int ret;
+	applesmc_pnp_device = kzalloc(sizeof(struct applesmc_pnp_device),
+				      GFP_KERNEL);
 
-	if (!dmi_check_system(applesmc_whitelist)) {
-		pr_warn("supported laptop not found!\n");
-		ret = -ENODEV;
+	if (!applesmc_pnp_device) {
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	if (!request_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS,
-								"applesmc")) {
+	pnp_device = applesmc_pnp_device;
+
+	pnp_set_drvdata(dev, applesmc_pnp_device);
+
+	res = pnp_get_resource(dev, IORESOURCE_IO, 0);
+
+	if (!res) {
 		ret = -ENXIO;
 		goto out;
 	}
 
-	ret = platform_driver_register(&applesmc_driver);
-	if (ret)
-		goto out_region;
+	applesmc_pnp_device->iobase = res->start;
+	applesmc_pnp_device->iolen = res->end - res->start + 1;
 
-	pdev = platform_device_register_simple("applesmc", APPLESMC_DATA_PORT,
-					       NULL, 0);
-	if (IS_ERR(pdev)) {
-		ret = PTR_ERR(pdev);
-		goto out_driver;
+	if (!request_region(pnp_device->iobase, pnp_device->iolen, "applesmc")) {
+		ret = -ENXIO;
+		goto out;
 	}
 
 	/* create register cache */
 	ret = applesmc_init_smcreg();
 	if (ret)
-		goto out_device;
+		goto out_region;
+
+	applesmc_device_init();
 
 	ret = applesmc_create_nodes(info_group, 1);
 	if (ret)
@@ -1287,19 +1262,17 @@ out_info:
 	applesmc_destroy_nodes(info_group);
 out_smcreg:
 	applesmc_destroy_smcreg();
-out_device:
-	platform_device_unregister(pdev);
-out_driver:
-	platform_driver_unregister(&applesmc_driver);
 out_region:
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
+	release_region(pnp_device->iobase, pnp_device->iolen);
 out:
 	pr_warn("driver init failed (ret=%d)!\n", ret);
 	return ret;
 }
 
-static void __exit applesmc_exit(void)
+static void __devexit applesmc_pnp_remove(struct pnp_dev *dev)
 {
+	struct applesmc_pnp_device *applesmc_pnp_device = pnp_get_drvdata(dev);
+
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
 	applesmc_release_light_sensor();
@@ -1308,9 +1281,33 @@ static void __exit applesmc_exit(void)
 	applesmc_destroy_nodes(fan_group);
 	applesmc_destroy_nodes(info_group);
 	applesmc_destroy_smcreg();
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&applesmc_driver);
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
+	release_region(pnp_device->iobase, pnp_device->iolen);
+	kfree(applesmc_pnp_device);
+}
+
+static const struct pnp_device_id applesmc_dev_table[] = {
+	{"APP0001", 0},
+	{"", 0},
+};
+
+static struct pnp_driver applesmc_pnp_driver = {
+	.name           = "Apple SMC",
+	.probe          = applesmc_pnp_probe,
+	.remove         = applesmc_pnp_remove,
+	.id_table       = applesmc_dev_table,
+	.driver = {
+		.pm	= &applesmc_pm_ops,
+	},
+};
+
+static int __init applesmc_init(void)
+{
+	return pnp_register_driver(&applesmc_pnp_driver);
+}
+
+static void __exit applesmc_exit(void)
+{
+	pnp_unregister_driver(&applesmc_pnp_driver);
 }
 
 module_init(applesmc_init);
@@ -1319,4 +1316,4 @@ module_exit(applesmc_exit);
 MODULE_AUTHOR("Nicolas Boichat");
 MODULE_DESCRIPTION("Apple SMC");
 MODULE_LICENSE("GPL v2");
-MODULE_DEVICE_TABLE(dmi, applesmc_whitelist);
+MODULE_DEVICE_TABLE(pnp, applesmc_dev_table);
-- 
1.7.3.3

[PATCH 2/2] applesmc: Perform some more sanity checking on temperatures

[Matthew Garrett]

It seems that the two-byte temperature format is intended to be signed
("sp78" appears to mean "signed, decimal point, 7 bits before, 8 bits
after") and it doesn't seem terribly plausible that we'll get any of these
machines below freezing. It's probably more reasonable to assume that
negative values indicate errors and drop them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index dbe3fa6..d7ec678 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -739,6 +739,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 		return ret;
 
 	if (entry->len == 2) {
+		if (buffer[0] >= 0x80) {
+			/* The two byte format is signed - ignore negative */
+			return -EINVAL;
+		}
 		temp = buffer[0] * 1000;
 		temp += (buffer[1] >> 6) * 250;
 	} else {
-- 
1.7.3.3

Re: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

Hi Matthew,

> The AppleSMC device is described in ACPI, including a list of its resources.

> We should use those rather than hardcoding the ports. A side-effect is that
> we can then remove the DMI matching, since there's a unique identifier to
> indicate that the machine has one of these devices.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>


Thank you very much for these patches. Unfortunately, they do not quite apply in
this end. If you could please rebase to linux-next [1], that would be great.

Thanks,
Henrik

[1] You find the same version in Guenther's tree or in
git://git.kernel.org/pub/scm/linux/kernel/git/rydberg/hwmon.git, if that is more
convenient.

Re: [lm-sensors] [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Guenter Roeck]

On Thu, Dec 16, 2010 at 10:33:08AM -0500, Matthew Garrett wrote:
> The AppleSMC device is described in ACPI, including a list of its resources.
> We should use those rather than hardcoding the ports. A side-effect is that
> we can then remove the DMI matching, since there's a unique identifier to
> indicate that the machine has one of these devices.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Matthew,

I am having trouble applying this patch to my -next tree. The driver there
(and in the official -next tree) has subtle differences to your version.
What tree is your patch based on ? Can you rebase it to the -next tree and resubmit ?

Couple of comments below; not necessarily complete, since I can not apply the patch.
I hope Henrik can comment on the merits of the patch itself, ie if it is known to work
with all systems.

Thanks,
Guenter

> ---
>  drivers/hwmon/applesmc.c |  185 +++++++++++++++++++++++-----------------------
>  1 files changed, 91 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index ee91d69..dbe3fa6 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -30,7 +30,6 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
>  #include <linux/delay.h>
> -#include <linux/platform_device.h>
>  #include <linux/input-polldev.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> @@ -43,11 +42,13 @@
>  #include <linux/leds.h>
>  #include <linux/hwmon.h>
>  #include <linux/workqueue.h>
> +#include <linux/slab.h>
> +#include <linux/pnp.h>
> 
>  /* data port used by Apple SMC */
> -#define APPLESMC_DATA_PORT     0x300
> +#define APPLESMC_DATA_PORT     0x0
>  /* command/status port used by Apple SMC */
> -#define APPLESMC_CMD_PORT      0x304
> +#define APPLESMC_CMD_PORT      0x4
> 
>  #define APPLESMC_NR_PORTS      32 /* 0x300-0x31f */
> 
> @@ -76,6 +77,8 @@
>  #define MOTION_SENSOR_Z_KEY    "MO_Z" /* r-o sp78 (2 bytes) */
>  #define MOTION_SENSOR_KEY      "MOCN" /* r/w ui16 */
> 
> +#define NOTIFICATION_KEY       "NTOK"
> +
>  #define FANS_COUNT             "FNum" /* r-o ui8 */
>  #define FANS_MANUAL            "FS! " /* r-w ui16 */
>  #define FAN_ID_FMT             "F%dID" /* r-o char[16] */
> @@ -103,6 +106,14 @@ static const char *const fan_speed_fmt[] = {
>  #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
>  #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
> 
> +struct applesmc_pnp_device {
> +       int iobase;
> +       int iolen;
> +};
> +
> +struct pnp_dev *pdev;
> +struct applesmc_pnp_device *pnp_device;
> +
Please make those variables static.

>  /* Dynamic device node attributes */
>  struct applesmc_dev_attr {
>         struct sensor_device_attribute sda;     /* hwmon attributes */
> @@ -143,7 +154,6 @@ static struct applesmc_registers {
>  } smcreg;
> 
>  static const int debug;
> -static struct platform_device *pdev;
>  static s16 rest_x;
>  static s16 rest_y;
>  static u8 backlight_state[2];
> @@ -159,6 +169,16 @@ static unsigned int key_at_index;
> 
>  static struct workqueue_struct *applesmc_led_wq;
> 
> +static u8 applesmc_read_reg(u8 reg)
> +{
> +       return inb(pnp_device->iobase + reg);
> +}
> +
> +static void applesmc_write_reg(u8 val, u8 reg)
> +{
> +       outb(val, pnp_device->iobase + reg);
> +}
> +
>  /*
>   * __wait_status - Wait up to 32ms for the status port to get a certain value
>   * (masked with 0x0f), returning zero if the value is obtained.  Callers must
> @@ -172,7 +192,8 @@ static int __wait_status(u8 val)
> 
>         for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
>                 udelay(us);
> -               if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
> +               if ((applesmc_read_reg(APPLESMC_CMD_PORT)
> +                    & APPLESMC_STATUS_MASK) == val) {
>                         return 0;
>                 }
>         }
> @@ -189,9 +210,10 @@ static int send_command(u8 cmd)
>  {
>         int us;
>         for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> -               outb(cmd, APPLESMC_CMD_PORT);
> +               applesmc_write_reg(cmd, APPLESMC_CMD_PORT);
>                 udelay(us);
> -               if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
> +               if ((applesmc_read_reg(APPLESMC_CMD_PORT)
> +                    & APPLESMC_STATUS_MASK) == 0x0c)
>                         return 0;
>         }
>         return -EIO;
> @@ -202,7 +224,7 @@ static int send_argument(const char *key)
>         int i;
> 
>         for (i = 0; i < 4; i++) {
> -               outb(key[i], APPLESMC_DATA_PORT);
> +               applesmc_write_reg(key[i], APPLESMC_DATA_PORT);
>                 if (__wait_status(0x04))
>                         return -EIO;
>         }
> @@ -218,14 +240,14 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
>                 return -EIO;
>         }
> 
> -       outb(len, APPLESMC_DATA_PORT);
> +       applesmc_write_reg(len, APPLESMC_DATA_PORT);
> 
>         for (i = 0; i < len; i++) {
>                 if (__wait_status(0x05)) {
>                         pr_warn("%s: read data fail\n", key);
>                         return -EIO;
>                 }
> -               buffer[i] = inb(APPLESMC_DATA_PORT);
> +               buffer[i] = applesmc_read_reg(APPLESMC_DATA_PORT);
>         }
> 
>         return 0;
> @@ -240,14 +262,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
>                 return -EIO;
>         }
> 
> -       outb(len, APPLESMC_DATA_PORT);
> +       applesmc_write_reg(len, APPLESMC_DATA_PORT);
> 
>         for (i = 0; i < len; i++) {
>                 if (__wait_status(0x04)) {
>                         pr_warn("%s: write data fail\n", key);
>                         return -EIO;
>                 }
> -               outb(buffer[i], APPLESMC_DATA_PORT);
> +               applesmc_write_reg(buffer[i], APPLESMC_DATA_PORT);
>         }
> 
>         return 0;
> @@ -566,20 +588,6 @@ static void applesmc_destroy_smcreg(void)
>         memset(&smcreg, 0, sizeof(smcreg));
>  }
> 
> -/* Device model stuff */
> -static int applesmc_probe(struct platform_device *dev)
> -{
> -       int ret;
> -
> -       ret = applesmc_init_smcreg();
> -       if (ret)
> -               return ret;
> -
> -       applesmc_device_init();
> -
> -       return 0;
> -}
> -
>  /* Synchronize device with memorized backlight state */
>  static int applesmc_pm_resume(struct device *dev)
>  {
> @@ -600,15 +608,6 @@ static const struct dev_pm_ops applesmc_pm_ops = {
>         .restore = applesmc_pm_restore,
>  };
> 
> -static struct platform_driver applesmc_driver = {
> -       .probe = applesmc_probe,
> -       .driver = {
> -               .name = "applesmc",
> -               .owner = THIS_MODULE,
> -               .pm = &applesmc_pm_ops,
> -       },
> -};
> -
>  /*
>   * applesmc_calibrate - Set our "resting" values.  Callers must
>   * hold applesmc_lock.
> @@ -1174,72 +1173,48 @@ static void applesmc_release_key_backlight(void)
>         destroy_workqueue(applesmc_led_wq);
>  }
> 
> -static int applesmc_dmi_match(const struct dmi_system_id *id)
> +static int __devinit applesmc_pnp_probe(struct pnp_dev *dev,
> +                                       const struct pnp_device_id *dev_id)
>  {
> -       return 1;
> -}
> +       int ret;
> +       struct resource *res;
> +       struct applesmc_pnp_device *applesmc_pnp_device;
> 
> -/* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
> - * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
> -static __initdata struct dmi_system_id applesmc_whitelist[] = {
> -       { applesmc_dmi_match, "Apple MacBook Air", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
> -       },
> -       { applesmc_dmi_match, "Apple MacBook Pro", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
> -       },
> -       { applesmc_dmi_match, "Apple MacBook", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
> -       },
> -       { applesmc_dmi_match, "Apple Macmini", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
> -       },
> -       { applesmc_dmi_match, "Apple MacPro", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
> -       },
> -       { applesmc_dmi_match, "Apple iMac", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
> -       },
> -       { .ident = NULL }
> -};
> +       pdev = dev;
> 
> -static int __init applesmc_init(void)
> -{
> -       int ret;
> +       applesmc_pnp_device = kzalloc(sizeof(struct applesmc_pnp_device),
> +                                     GFP_KERNEL);
> 
> -       if (!dmi_check_system(applesmc_whitelist)) {
> -               pr_warn("supported laptop not found!\n");
> -               ret = -ENODEV;
> +       if (!applesmc_pnp_device) {
> +               ret = -ENOMEM;
>                 goto out;
>         }
> 
> -       if (!request_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS,
> -                                                               "applesmc")) {
> +       pnp_device = applesmc_pnp_device;
> +
Just wondering ...  applesmc_pnp_device doesn't seem to be necessary. 
Why not just use the global variable directly if you have it anyway ?

> +       pnp_set_drvdata(dev, applesmc_pnp_device);
> +

... but then since you assign it to drvdata, can you get rid of the global variable 
and use pnp_get_drvdata() whereever it is needed instead ?

> +       res = pnp_get_resource(dev, IORESOURCE_IO, 0);
> +
> +       if (!res) {
>                 ret = -ENXIO;
>                 goto out;
>         }
> 
> -       ret = platform_driver_register(&applesmc_driver);
> -       if (ret)
> -               goto out_region;
> +       applesmc_pnp_device->iobase = res->start;
> +       applesmc_pnp_device->iolen = res->end - res->start + 1;
> 
> -       pdev = platform_device_register_simple("applesmc", APPLESMC_DATA_PORT,
> -                                              NULL, 0);
> -       if (IS_ERR(pdev)) {
> -               ret = PTR_ERR(pdev);
> -               goto out_driver;
> +       if (!request_region(pnp_device->iobase, pnp_device->iolen, "applesmc")) {

This line has more than 80 columns.

> +               ret = -ENXIO;
> +               goto out;
>         }
> 
>         /* create register cache */
>         ret = applesmc_init_smcreg();
>         if (ret)
> -               goto out_device;
> +               goto out_region;
> +
> +       applesmc_device_init();
> 
>         ret = applesmc_create_nodes(info_group, 1);
>         if (ret)
> @@ -1287,19 +1262,17 @@ out_info:
>         applesmc_destroy_nodes(info_group);
>  out_smcreg:
>         applesmc_destroy_smcreg();
> -out_device:
> -       platform_device_unregister(pdev);
> -out_driver:
> -       platform_driver_unregister(&applesmc_driver);
>  out_region:
> -       release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
> +       release_region(pnp_device->iobase, pnp_device->iolen);

No kfree() of applesmc_pnp_device, so you are leaking memory here.

>  out:
>         pr_warn("driver init failed (ret=%d)!\n", ret);
>         return ret;
>  }
> 
> -static void __exit applesmc_exit(void)
> +static void __devexit applesmc_pnp_remove(struct pnp_dev *dev)
>  {
> +       struct applesmc_pnp_device *applesmc_pnp_device = pnp_get_drvdata(dev);
> +

Why bother with this, as it is assigned to pnp_device ?

>         hwmon_device_unregister(hwmon_dev);
>         applesmc_release_key_backlight();
>         applesmc_release_light_sensor();
> @@ -1308,9 +1281,33 @@ static void __exit applesmc_exit(void)
>         applesmc_destroy_nodes(fan_group);
>         applesmc_destroy_nodes(info_group);
>         applesmc_destroy_smcreg();
> -       platform_device_unregister(pdev);
> -       platform_driver_unregister(&applesmc_driver);
> -       release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
> +       release_region(pnp_device->iobase, pnp_device->iolen);
> +       kfree(applesmc_pnp_device);
> +}
> +
> +static const struct pnp_device_id applesmc_dev_table[] = {
> +       {"APP0001", 0},
> +       {"", 0},
> +};
> +
> +static struct pnp_driver applesmc_pnp_driver = {
> +       .name           = "Apple SMC",
> +       .probe          = applesmc_pnp_probe,
> +       .remove         = applesmc_pnp_remove,
> +       .id_table       = applesmc_dev_table,
> +       .driver = {
> +               .pm     = &applesmc_pm_ops,
> +       },
> +};
> +
> +static int __init applesmc_init(void)
> +{
> +       return pnp_register_driver(&applesmc_pnp_driver);
> +}
> +
> +static void __exit applesmc_exit(void)
> +{
> +       pnp_unregister_driver(&applesmc_pnp_driver);
>  }
> 
>  module_init(applesmc_init);
> @@ -1319,4 +1316,4 @@ module_exit(applesmc_exit);
>  MODULE_AUTHOR("Nicolas Boichat");
>  MODULE_DESCRIPTION("Apple SMC");
>  MODULE_LICENSE("GPL v2");
> -MODULE_DEVICE_TABLE(dmi, applesmc_whitelist);
> +MODULE_DEVICE_TABLE(pnp, applesmc_dev_table);
> --
> 1.7.3.3
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Thu, Dec 16, 2010 at 05:48:14PM +0100, Henrik Rydberg wrote:
> > The AppleSMC device is described in ACPI, including a list of its resources.
> 
> > We should use those rather than hardcoding the ports. A side-effect is that
> > we can then remove the DMI matching, since there's a unique identifier to
> > indicate that the machine has one of these devices.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> 
> Thank you very much for these patches. Unfortunately, they do not quite apply in
> this end. If you could please rebase to linux-next [1], that would be great.

Ah, sorry, I thought I'd pulled the same set of patches. I'll rebase.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Thu, Dec 16, 2010 at 09:00:18AM -0800, Guenter Roeck wrote:

> I am having trouble applying this patch to my -next tree. The driver there
> (and in the official -next tree) has subtle differences to your version.
> What tree is your patch based on ? Can you rebase it to the -next tree and resubmit ?

My mistake - I'd pulled the patches from LKML, but it looks like there 
was a later version of one or two.

> Couple of comments below; not necessarily complete, since I can not apply the patch.
> I hope Henrik can comment on the merits of the patch itself, ie if it is known to work
> with all systems.

> > +struct pnp_dev *pdev;
> > +struct applesmc_pnp_device *pnp_device;
> > +
> Please make those variables static.

Oops! Yup.

> Just wondering ...  applesmc_pnp_device doesn't seem to be necessary. 
> Why not just use the global variable directly if you have it anyway ?

This ended up left over as part of an attempt to get rid of the 
globals...

> > +       pnp_set_drvdata(dev, applesmc_pnp_device);
> > +
> 
> ... but then since you assign it to drvdata, can you get rid of the global variable 
> and use pnp_get_drvdata() whereever it is needed instead ?

But then I ran into some awkward issue and decided to leave it. And then 
failed to clean everything up. I'll repost without that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

The AppleSMC device is described in ACPI, including a list of its resources.
We should use those rather than hardcoding the ports. A side-effect is that
we can then remove the DMI matching, since there's a unique identifier to
indicate that the machine has one of these devices.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |  182 ++++++++++++++++++++++------------------------
 1 files changed, 86 insertions(+), 96 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index ce0372f..6c98b60 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -30,7 +30,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/delay.h>
-#include <linux/platform_device.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -43,11 +42,13 @@
 #include <linux/leds.h>
 #include <linux/hwmon.h>
 #include <linux/workqueue.h>
+#include <linux/slab.h>
+#include <linux/pnp.h>
 
 /* data port used by Apple SMC */
-#define APPLESMC_DATA_PORT	0x300
+#define APPLESMC_DATA_PORT	0x0
 /* command/status port used by Apple SMC */
-#define APPLESMC_CMD_PORT	0x304
+#define APPLESMC_CMD_PORT	0x4
 
 #define APPLESMC_NR_PORTS	32 /* 0x300-0x31f */
 
@@ -76,6 +77,8 @@
 #define MOTION_SENSOR_Z_KEY	"MO_Z" /* r-o sp78 (2 bytes) */
 #define MOTION_SENSOR_KEY	"MOCN" /* r/w ui16 */
 
+#define NOTIFICATION_KEY	"NTOK"
+
 #define FANS_COUNT		"FNum" /* r-o ui8 */
 #define FANS_MANUAL		"FS! " /* r-w ui16 */
 #define FAN_ID_FMT		"F%dID" /* r-o char[16] */
@@ -103,6 +106,14 @@ static const char *const fan_speed_fmt[] = {
 #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
 #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
 
+struct applesmc_pnp_device {
+	int iobase;
+	int iolen;
+};
+
+struct pnp_dev *pdev;
+struct applesmc_pnp_device *apple_pnp_device;
+
 /* Dynamic device node attributes */
 struct applesmc_dev_attr {
 	struct sensor_device_attribute sda;	/* hwmon attributes */
@@ -145,7 +156,6 @@ static struct applesmc_registers {
 };
 
 static const int debug;
-static struct platform_device *pdev;
 static s16 rest_x;
 static s16 rest_y;
 static u8 backlight_state[2];
@@ -161,6 +171,16 @@ static unsigned int key_at_index;
 
 static struct workqueue_struct *applesmc_led_wq;
 
+static u8 applesmc_read_reg(u8 reg)
+{
+	return inb(apple_pnp_device->iobase + reg);
+}
+
+static void applesmc_write_reg(u8 val, u8 reg)
+{
+	outb(val, apple_pnp_device->iobase + reg);
+}
+
 /*
  * __wait_status - Wait up to 32ms for the status port to get a certain value
  * (masked with 0x0f), returning zero if the value is obtained.  Callers must
@@ -174,7 +194,8 @@ static int __wait_status(u8 val)
 
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val)
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == val)
 			return 0;
 	}
 
@@ -190,9 +211,10 @@ static int send_command(u8 cmd)
 {
 	int us;
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		outb(cmd, APPLESMC_CMD_PORT);
+		applesmc_write_reg(cmd, APPLESMC_CMD_PORT);
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == 0x0c)
 			return 0;
 	}
 	return -EIO;
@@ -203,7 +225,7 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++) {
-		outb(key[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(key[i], APPLESMC_DATA_PORT);
 		if (__wait_status(0x04))
 			return -EIO;
 	}
@@ -219,14 +241,14 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x05)) {
 			pr_warn("%s: read data fail\n", key);
 			return -EIO;
 		}
-		buffer[i] = inb(APPLESMC_DATA_PORT);
+		buffer[i] = applesmc_read_reg(APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -241,14 +263,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x04)) {
 			pr_warn("%s: write data fail\n", key);
 			return -EIO;
 		}
-		outb(buffer[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(buffer[i], APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -571,20 +593,6 @@ static void applesmc_destroy_smcreg(void)
 	smcreg.init_complete = false;
 }
 
-/* Device model stuff */
-static int applesmc_probe(struct platform_device *dev)
-{
-	int ret;
-
-	ret = applesmc_init_smcreg();
-	if (ret)
-		return ret;
-
-	applesmc_device_init();
-
-	return 0;
-}
-
 /* Synchronize device with memorized backlight state */
 static int applesmc_pm_resume(struct device *dev)
 {
@@ -605,15 +613,6 @@ static const struct dev_pm_ops applesmc_pm_ops = {
 	.restore = applesmc_pm_restore,
 };
 
-static struct platform_driver applesmc_driver = {
-	.probe = applesmc_probe,
-	.driver	= {
-		.name = "applesmc",
-		.owner = THIS_MODULE,
-		.pm = &applesmc_pm_ops,
-	},
-};
-
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1183,72 +1182,41 @@ static void applesmc_release_key_backlight(void)
 	destroy_workqueue(applesmc_led_wq);
 }
 
-static int applesmc_dmi_match(const struct dmi_system_id *id)
-{
-	return 1;
-}
-
-/* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
- * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
-static __initdata struct dmi_system_id applesmc_whitelist[] = {
-	{ applesmc_dmi_match, "Apple MacBook Air", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook Pro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook") },
-	},
-	{ applesmc_dmi_match, "Apple Macmini", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "Macmini") },
-	},
-	{ applesmc_dmi_match, "Apple MacPro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
-	},
-	{ applesmc_dmi_match, "Apple iMac", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac") },
-	},
-	{ .ident = NULL }
-};
-
-static int __init applesmc_init(void)
+static int __devinit applesmc_pnp_probe(struct pnp_dev *dev,
+					const struct pnp_device_id *dev_id)
 {
 	int ret;
+	struct resource *res;
+	apple_pnp_device = kzalloc(sizeof(struct applesmc_pnp_device),
+				   GFP_KERNEL);
 
-	if (!dmi_check_system(applesmc_whitelist)) {
-		pr_warn("supported laptop not found!\n");
-		ret = -ENODEV;
+	if (!apple_pnp_device) {
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	if (!request_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS,
-								"applesmc")) {
+	res = pnp_get_resource(dev, IORESOURCE_IO, 0);
+
+	if (!res) {
 		ret = -ENXIO;
 		goto out;
 	}
 
-	ret = platform_driver_register(&applesmc_driver);
-	if (ret)
-		goto out_region;
+	apple_pnp_device->iobase = res->start;
+	apple_pnp_device->iolen = res->end - res->start + 1;
 
-	pdev = platform_device_register_simple("applesmc", APPLESMC_DATA_PORT,
-					       NULL, 0);
-	if (IS_ERR(pdev)) {
-		ret = PTR_ERR(pdev);
-		goto out_driver;
+	if (!request_region(apple_pnp_device->iobase, apple_pnp_device->iolen,
+			    "applesmc")) {
+		ret = -ENXIO;
+		goto out;
 	}
 
 	/* create register cache */
 	ret = applesmc_init_smcreg();
 	if (ret)
-		goto out_device;
+		goto out_region;
+
+	applesmc_device_init();
 
 	ret = applesmc_create_nodes(info_group, 1);
 	if (ret)
@@ -1296,18 +1264,16 @@ out_info:
 	applesmc_destroy_nodes(info_group);
 out_smcreg:
 	applesmc_destroy_smcreg();
-out_device:
-	platform_device_unregister(pdev);
-out_driver:
-	platform_driver_unregister(&applesmc_driver);
 out_region:
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
-out:
+	release_region(apple_pnp_device->iobase, apple_pnp_device->iolen);
+out:	
 	pr_warn("driver init failed (ret=%d)!\n", ret);
+	if (apple_pnp_device)
+		kfree(apple_pnp_device);
 	return ret;
 }
 
-static void __exit applesmc_exit(void)
+static void __devexit applesmc_pnp_remove(struct pnp_dev *dev)
 {
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
@@ -1317,9 +1283,33 @@ static void __exit applesmc_exit(void)
 	applesmc_destroy_nodes(fan_group);
 	applesmc_destroy_nodes(info_group);
 	applesmc_destroy_smcreg();
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&applesmc_driver);
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
+	release_region(apple_pnp_device->iobase, apple_pnp_device->iolen);
+	kfree(apple_pnp_device);
+}
+
+static const struct pnp_device_id applesmc_dev_table[] = {
+	{"APP0001", 0},
+	{"", 0},
+};
+
+static struct pnp_driver applesmc_pnp_driver = {
+	.name           = "Apple SMC",
+	.probe          = applesmc_pnp_probe,
+	.remove         = applesmc_pnp_remove,
+	.id_table       = applesmc_dev_table,
+	.driver = {
+		.pm	= &applesmc_pm_ops,
+	},
+};
+
+static int __init applesmc_init(void)
+{
+	return pnp_register_driver(&applesmc_pnp_driver);
+}
+
+static void __exit applesmc_exit(void)
+{
+	pnp_unregister_driver(&applesmc_pnp_driver);
 }
 
 module_init(applesmc_init);
@@ -1328,4 +1318,4 @@ module_exit(applesmc_exit);
 MODULE_AUTHOR("Nicolas Boichat");
 MODULE_DESCRIPTION("Apple SMC");
 MODULE_LICENSE("GPL v2");
-MODULE_DEVICE_TABLE(dmi, applesmc_whitelist);
+MODULE_DEVICE_TABLE(pnp, applesmc_dev_table);
-- 
1.7.3.3

[PATCH 2/2] applesmc: Perform some more sanity checking on temperatures

[Matthew Garrett]

It seems that the two-byte temperature format is intended to be signed
("sp78" appears to mean "signed, decimal point, 7 bits before, 8 bits
after") and it doesn't seem terribly plausible that we'll get any of these
machines below freezing. It's probably more reasonable to assume that
negative values indicate errors and drop them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 6c98b60..c1eead4 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -744,6 +744,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 		return ret;
 
 	if (entry->len == 2) {
+		if (buffer[0] >= 0x80) {
+			/* The two byte format is signed - ignore negative */
+			return -EINVAL;
+		}
 		temp = buffer[0] * 1000;
 		temp += (buffer[1] >> 6) * 250;
 	} else {
-- 
1.7.3.3

Re: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Guenter Roeck]

On Fri, Dec 17, 2010 at 04:58:25PM -0500, Matthew Garrett wrote:
> The AppleSMC device is described in ACPI, including a list of its resources.
> We should use those rather than hardcoding the ports. A side-effect is that
> we can then remove the DMI matching, since there's a unique identifier to
> indicate that the machine has one of these devices.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/hwmon/applesmc.c |  182 ++++++++++++++++++++++------------------------
>  1 files changed, 86 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index ce0372f..6c98b60 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -30,7 +30,6 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/delay.h>
> -#include <linux/platform_device.h>
>  #include <linux/input-polldev.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> @@ -43,11 +42,13 @@
>  #include <linux/leds.h>
>  #include <linux/hwmon.h>
>  #include <linux/workqueue.h>
> +#include <linux/slab.h>
> +#include <linux/pnp.h>
>  
>  /* data port used by Apple SMC */
> -#define APPLESMC_DATA_PORT	0x300
> +#define APPLESMC_DATA_PORT	0x0
>  /* command/status port used by Apple SMC */
> -#define APPLESMC_CMD_PORT	0x304
> +#define APPLESMC_CMD_PORT	0x4
>  
>  #define APPLESMC_NR_PORTS	32 /* 0x300-0x31f */
>  
> @@ -76,6 +77,8 @@
>  #define MOTION_SENSOR_Z_KEY	"MO_Z" /* r-o sp78 (2 bytes) */
>  #define MOTION_SENSOR_KEY	"MOCN" /* r/w ui16 */
>  
> +#define NOTIFICATION_KEY	"NTOK"
> +
>  #define FANS_COUNT		"FNum" /* r-o ui8 */
>  #define FANS_MANUAL		"FS! " /* r-w ui16 */
>  #define FAN_ID_FMT		"F%dID" /* r-o char[16] */
> @@ -103,6 +106,14 @@ static const char *const fan_speed_fmt[] = {
>  #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
>  #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
>  
> +struct applesmc_pnp_device {
> +	int iobase;
> +	int iolen;
> +};
> +
> +struct pnp_dev *pdev;
> +struct applesmc_pnp_device *apple_pnp_device;
> +
Those variables are still global.

Guenter

Re: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Fri, Dec 17, 2010 at 02:16:18PM -0800, Guenter Roeck wrote:
> > +
> Those variables are still global.

I do appear to suck.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

The AppleSMC device is described in ACPI, including a list of its resources.
We should use those rather than hardcoding the ports. A side-effect is that
we can then remove the DMI matching, since there's a unique identifier to
indicate that the machine has one of these devices.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---

Really make things static this time

 drivers/hwmon/applesmc.c |  182 ++++++++++++++++++++++------------------------
 1 files changed, 86 insertions(+), 96 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index ce0372f..851a685 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -30,7 +30,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/delay.h>
-#include <linux/platform_device.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -43,11 +42,13 @@
 #include <linux/leds.h>
 #include <linux/hwmon.h>
 #include <linux/workqueue.h>
+#include <linux/slab.h>
+#include <linux/pnp.h>
 
 /* data port used by Apple SMC */
-#define APPLESMC_DATA_PORT	0x300
+#define APPLESMC_DATA_PORT	0x0
 /* command/status port used by Apple SMC */
-#define APPLESMC_CMD_PORT	0x304
+#define APPLESMC_CMD_PORT	0x4
 
 #define APPLESMC_NR_PORTS	32 /* 0x300-0x31f */
 
@@ -76,6 +77,8 @@
 #define MOTION_SENSOR_Z_KEY	"MO_Z" /* r-o sp78 (2 bytes) */
 #define MOTION_SENSOR_KEY	"MOCN" /* r/w ui16 */
 
+#define NOTIFICATION_KEY	"NTOK"
+
 #define FANS_COUNT		"FNum" /* r-o ui8 */
 #define FANS_MANUAL		"FS! " /* r-w ui16 */
 #define FAN_ID_FMT		"F%dID" /* r-o char[16] */
@@ -103,6 +106,14 @@ static const char *const fan_speed_fmt[] = {
 #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
 #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
 
+struct applesmc_pnp_device {
+	int iobase;
+	int iolen;
+};
+
+static struct pnp_dev *pdev;
+static struct applesmc_pnp_device *apple_pnp_device;
+
 /* Dynamic device node attributes */
 struct applesmc_dev_attr {
 	struct sensor_device_attribute sda;	/* hwmon attributes */
@@ -145,7 +156,6 @@ static struct applesmc_registers {
 };
 
 static const int debug;
-static struct platform_device *pdev;
 static s16 rest_x;
 static s16 rest_y;
 static u8 backlight_state[2];
@@ -161,6 +171,16 @@ static unsigned int key_at_index;
 
 static struct workqueue_struct *applesmc_led_wq;
 
+static u8 applesmc_read_reg(u8 reg)
+{
+	return inb(apple_pnp_device->iobase + reg);
+}
+
+static void applesmc_write_reg(u8 val, u8 reg)
+{
+	outb(val, apple_pnp_device->iobase + reg);
+}
+
 /*
  * __wait_status - Wait up to 32ms for the status port to get a certain value
  * (masked with 0x0f), returning zero if the value is obtained.  Callers must
@@ -174,7 +194,8 @@ static int __wait_status(u8 val)
 
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val)
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == val)
 			return 0;
 	}
 
@@ -190,9 +211,10 @@ static int send_command(u8 cmd)
 {
 	int us;
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
-		outb(cmd, APPLESMC_CMD_PORT);
+		applesmc_write_reg(cmd, APPLESMC_CMD_PORT);
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
+		if ((applesmc_read_reg(APPLESMC_CMD_PORT)
+		     & APPLESMC_STATUS_MASK) == 0x0c)
 			return 0;
 	}
 	return -EIO;
@@ -203,7 +225,7 @@ static int send_argument(const char *key)
 	int i;
 
 	for (i = 0; i < 4; i++) {
-		outb(key[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(key[i], APPLESMC_DATA_PORT);
 		if (__wait_status(0x04))
 			return -EIO;
 	}
@@ -219,14 +241,14 @@ static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x05)) {
 			pr_warn("%s: read data fail\n", key);
 			return -EIO;
 		}
-		buffer[i] = inb(APPLESMC_DATA_PORT);
+		buffer[i] = applesmc_read_reg(APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -241,14 +263,14 @@ static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 		return -EIO;
 	}
 
-	outb(len, APPLESMC_DATA_PORT);
+	applesmc_write_reg(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
 		if (__wait_status(0x04)) {
 			pr_warn("%s: write data fail\n", key);
 			return -EIO;
 		}
-		outb(buffer[i], APPLESMC_DATA_PORT);
+		applesmc_write_reg(buffer[i], APPLESMC_DATA_PORT);
 	}
 
 	return 0;
@@ -571,20 +593,6 @@ static void applesmc_destroy_smcreg(void)
 	smcreg.init_complete = false;
 }
 
-/* Device model stuff */
-static int applesmc_probe(struct platform_device *dev)
-{
-	int ret;
-
-	ret = applesmc_init_smcreg();
-	if (ret)
-		return ret;
-
-	applesmc_device_init();
-
-	return 0;
-}
-
 /* Synchronize device with memorized backlight state */
 static int applesmc_pm_resume(struct device *dev)
 {
@@ -605,15 +613,6 @@ static const struct dev_pm_ops applesmc_pm_ops = {
 	.restore = applesmc_pm_restore,
 };
 
-static struct platform_driver applesmc_driver = {
-	.probe = applesmc_probe,
-	.driver	= {
-		.name = "applesmc",
-		.owner = THIS_MODULE,
-		.pm = &applesmc_pm_ops,
-	},
-};
-
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1183,72 +1182,41 @@ static void applesmc_release_key_backlight(void)
 	destroy_workqueue(applesmc_led_wq);
 }
 
-static int applesmc_dmi_match(const struct dmi_system_id *id)
-{
-	return 1;
-}
-
-/* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
- * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
-static __initdata struct dmi_system_id applesmc_whitelist[] = {
-	{ applesmc_dmi_match, "Apple MacBook Air", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook Pro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro") },
-	},
-	{ applesmc_dmi_match, "Apple MacBook", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook") },
-	},
-	{ applesmc_dmi_match, "Apple Macmini", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "Macmini") },
-	},
-	{ applesmc_dmi_match, "Apple MacPro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
-	},
-	{ applesmc_dmi_match, "Apple iMac", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac") },
-	},
-	{ .ident = NULL }
-};
-
-static int __init applesmc_init(void)
+static int __devinit applesmc_pnp_probe(struct pnp_dev *dev,
+					const struct pnp_device_id *dev_id)
 {
 	int ret;
+	struct resource *res;
+	apple_pnp_device = kzalloc(sizeof(struct applesmc_pnp_device),
+				   GFP_KERNEL);
 
-	if (!dmi_check_system(applesmc_whitelist)) {
-		pr_warn("supported laptop not found!\n");
-		ret = -ENODEV;
+	if (!apple_pnp_device) {
+		ret = -ENOMEM;
 		goto out;
 	}
 
-	if (!request_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS,
-								"applesmc")) {
+	res = pnp_get_resource(dev, IORESOURCE_IO, 0);
+
+	if (!res) {
 		ret = -ENXIO;
 		goto out;
 	}
 
-	ret = platform_driver_register(&applesmc_driver);
-	if (ret)
-		goto out_region;
+	apple_pnp_device->iobase = res->start;
+	apple_pnp_device->iolen = res->end - res->start + 1;
 
-	pdev = platform_device_register_simple("applesmc", APPLESMC_DATA_PORT,
-					       NULL, 0);
-	if (IS_ERR(pdev)) {
-		ret = PTR_ERR(pdev);
-		goto out_driver;
+	if (!request_region(apple_pnp_device->iobase, apple_pnp_device->iolen,
+			    "applesmc")) {
+		ret = -ENXIO;
+		goto out;
 	}
 
 	/* create register cache */
 	ret = applesmc_init_smcreg();
 	if (ret)
-		goto out_device;
+		goto out_region;
+
+	applesmc_device_init();
 
 	ret = applesmc_create_nodes(info_group, 1);
 	if (ret)
@@ -1296,18 +1264,16 @@ out_info:
 	applesmc_destroy_nodes(info_group);
 out_smcreg:
 	applesmc_destroy_smcreg();
-out_device:
-	platform_device_unregister(pdev);
-out_driver:
-	platform_driver_unregister(&applesmc_driver);
 out_region:
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
-out:
+	release_region(apple_pnp_device->iobase, apple_pnp_device->iolen);
+out:	
 	pr_warn("driver init failed (ret=%d)!\n", ret);
+	if (apple_pnp_device)
+		kfree(apple_pnp_device);
 	return ret;
 }
 
-static void __exit applesmc_exit(void)
+static void __devexit applesmc_pnp_remove(struct pnp_dev *dev)
 {
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
@@ -1317,9 +1283,33 @@ static void __exit applesmc_exit(void)
 	applesmc_destroy_nodes(fan_group);
 	applesmc_destroy_nodes(info_group);
 	applesmc_destroy_smcreg();
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&applesmc_driver);
-	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
+	release_region(apple_pnp_device->iobase, apple_pnp_device->iolen);
+	kfree(apple_pnp_device);
+}
+
+static const struct pnp_device_id applesmc_dev_table[] = {
+	{"APP0001", 0},
+	{"", 0},
+};
+
+static struct pnp_driver applesmc_pnp_driver = {
+	.name           = "Apple SMC",
+	.probe          = applesmc_pnp_probe,
+	.remove         = applesmc_pnp_remove,
+	.id_table       = applesmc_dev_table,
+	.driver = {
+		.pm	= &applesmc_pm_ops,
+	},
+};
+
+static int __init applesmc_init(void)
+{
+	return pnp_register_driver(&applesmc_pnp_driver);
+}
+
+static void __exit applesmc_exit(void)
+{
+	pnp_unregister_driver(&applesmc_pnp_driver);
 }
 
 module_init(applesmc_init);
@@ -1328,4 +1318,4 @@ module_exit(applesmc_exit);
 MODULE_AUTHOR("Nicolas Boichat");
 MODULE_DESCRIPTION("Apple SMC");
 MODULE_LICENSE("GPL v2");
-MODULE_DEVICE_TABLE(dmi, applesmc_whitelist);
+MODULE_DEVICE_TABLE(pnp, applesmc_dev_table);
-- 
1.7.3.3

[PATCH 2/2 V3] applesmc: Perform some more sanity checking on temperatures

[Matthew Garrett]

It seems that the two-byte temperature format is intended to be signed
("sp78" appears to mean "signed, decimal point, 7 bits before, 8 bits
after") and it doesn't seem terribly plausible that we'll get any of these
machines below freezing. It's probably more reasonable to assume that
negative values indicate errors and drop them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/hwmon/applesmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 851a685..411a627 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -744,6 +744,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 		return ret;
 
 	if (entry->len == 2) {
+		if (buffer[0] >= 0x80) {
+			/* The two byte format is signed - ignore negative */
+			return -EINVAL;
+		}
 		temp = buffer[0] * 1000;
 		temp += (buffer[1] >> 6) * 250;
 	} else {
-- 
1.7.3.3

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Guenter Roeck]

On Fri, Dec 17, 2010 at 05:24:20PM -0500, Matthew Garrett wrote:
> The AppleSMC device is described in ACPI, including a list of its resources.
> We should use those rather than hardcoding the ports. A side-effect is that
> we can then remove the DMI matching, since there's a unique identifier to
> indicate that the machine has one of these devices.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

You added a whitespace error, and kfree() is safe and doesn't need a check.
I fixed those, so no need to resubmit. If Henrik gives his Ack we are ready to go.

Guenter

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Fri, Dec 17, 2010 at 08:23:21PM -0800, Guenter Roeck wrote:
> On Fri, Dec 17, 2010 at 05:24:20PM -0500, Matthew Garrett wrote:
> > The AppleSMC device is described in ACPI, including a list of its resources.
> > We should use those rather than hardcoding the ports. A side-effect is that
> > we can then remove the DMI matching, since there's a unique identifier to
> > indicate that the machine has one of these devices.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> You added a whitespace error, and kfree() is safe and doesn't need a check.
> I fixed those, so no need to resubmit. If Henrik gives his Ack we are ready to go.

Thanks, I will give it some testing before doing so. Matthew, did you
test this under EFI boot? Also, NOTIFICATION_KEY does not seem to be
used anywhere.

Henrik

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Fri, Dec 17, 2010 at 08:23:21PM -0800, Guenter Roeck wrote:
> On Fri, Dec 17, 2010 at 05:24:20PM -0500, Matthew Garrett wrote:
> > The AppleSMC device is described in ACPI, including a list of its resources.
> > We should use those rather than hardcoding the ports. A side-effect is that
> > we can then remove the DMI matching, since there's a unique identifier to
> > indicate that the machine has one of these devices.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> You added a whitespace error, and kfree() is safe and doesn't need a check.
> I fixed those, so no need to resubmit. If Henrik gives his Ack we are ready to go.

Ok, although the idea is interesting, it seems this patch will need
some reworking and testing. I tested the patches on a recent
MacBookAir3,1, and i get this:

[    1.211182] Pid: 779, comm: modprobe Not tainted 2.6.37-rc5+ #280 Mac-942452F5819B1C1B/MacBookAir3,1
[    1.211282] RIP: 0010:[<ffffffff811259b9>]  [<ffffffff811259b9>] sysfs_create_file+0x9/0x30
[    1.211424] RSP: 0018:ffff88007f1e7d48  EFLAGS: 00010202
[    1.211498] RAX: 0000000000000124 RBX: ffff88006bd58540 RCX: 0000000000000004
[    1.211575] RDX: 0000000000000004 RSI: ffff88006bd58540 RDI: 0000000000000010
[    1.211653] RBP: ffffffffa000b040 R08: 0000000000000000 R09: ffff88006bd58540
[    1.211731] R10: 0000000000000001 R11: 00000000ffffffff R12: 0000000000000001
[    1.211808] R13: 0000000000000000 R14: 0000000000000000 R15: ffff88006bd58568
[    1.211886] FS:  00007fbe4a458700(0000) GS:ffff88006ec00000(0000) knlGS:0000000000000000
[    1.211984] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    1.212007] CR2: 0000000000000040 CR3: 0000000069de9000 CR4: 00000000000406b0
[    1.212007] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    1.212007] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[    1.212007] Process modprobe (pid: 779, threadinfo ffff88007f1e6000, task ffff88006bd38000)
[    1.212007] Stack:
[    1.212007]  0000000000405056 ffffffffa0009ecc 0000000000405056 ffffffff8139b969
[    1.212517]  0000000100000030 0000000000000010 ffffffffa000b040 0000000000000090
[    1.212517]  0000000000000125 0000000000000000 0000000000000000 ffffffffa000ad60
[    1.212517] Call Trace:
[    1.212517]  [<ffffffffa0009ecc>] ? applesmc_create_nodes+0x13c/0x170 [applesmc]
[    1.212517]  [<ffffffff8139b969>] ? printk+0x40/0x45
[    1.212517]  [<ffffffffa000a89f>] ? applesmc_pnp_probe+0x529/0x56a [applesmc]
[    1.212517]  [<ffffffff81222ede>] ? compare_pnp_id+0x1e/0xf0
[    1.212517]  [<ffffffffa000a376>] ? applesmc_pnp_probe+0x0/0x56a [applesmc]
[    1.212517]  [<ffffffff8122307f>] ? pnp_device_probe+0x6f/0xe0
[    1.212517]  [<ffffffff8126a342>] ? driver_sysfs_add+0x72/0xa0
[    1.212517]  [<ffffffff8126a5d7>] ? driver_probe_device+0x87/0x1a0
[    1.212517]  [<ffffffff8126a783>] ? __driver_attach+0x93/0xa0
[    1.212517]  [<ffffffff8126a6f0>] ? __driver_attach+0x0/0xa0
[    1.212517]  [<ffffffff812695cd>] ? bus_for_each_dev+0x4d/0x80
[    1.212517]  [<ffffffff81269ea8>] ? bus_add_driver+0x158/0x2d0
[    1.212517]  [<ffffffffa000e000>] ? applesmc_init+0x0/0x14 [applesmc]
[    1.212517]  [<ffffffff8126a9dc>] ? driver_register+0x6c/0x130
[    1.212517]  [<ffffffffa000e000>] ? applesmc_init+0x0/0x14 [applesmc]
[    1.212517]  [<ffffffff810002ca>] ? do_one_initcall+0x3a/0x170
[    1.212517]  [<ffffffff8106f8a9>] ? sys_init_module+0xb9/0x200
[    1.212517]  [<ffffffff810023bb>] ? system_call_fastpath+0x16/0x1b
[    1.212517] Code: 8b 03 85 c0 74 05 f0 ff 03 eb 9d be b7 00 00 00 48 c7 c7 e7 4e 46 81 e8 06 2b f1 ff eb e8 0f 1f 40 00 48 83 ec 08 48 85 ff 74 1c <48> 8b 7f 30 48 85 f6 74 13 48 85 ff 74 0e ba 02 00 00 00 48 83 
[    1.212517] RIP  [<ffffffff811259b9>] sysfs_create_file+0x9/0x30
[    1.212517]  RSP <ffff88007f1e7d48>
[    1.212517] CR2: 0000000000000040
[    1.218078] ---[ end trace ad091051b87fc759 ]---

So where to attach the sysfs nodes... There is also a userspace issue
here, since a lot of applications are hard-wired to
/sys/devices/platform/applesmc.768/.

Thanks,
Henrik

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Jean Delvare]

On Sat, 18 Dec 2010 10:37:19 +0100, Henrik Rydberg wrote:
> So where to attach the sysfs nodes... There is also a userspace issue
> here, since a lot of applications are hard-wired to
> /sys/devices/platform/applesmc.768/.

Which applications? libsensors-based applications definitely don't
hard-wire anything.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Sat, Dec 18, 2010 at 11:09:54AM +0100, Jean Delvare wrote:
> On Sat, 18 Dec 2010 10:37:19 +0100, Henrik Rydberg wrote:
> > So where to attach the sysfs nodes... There is also a userspace issue
> > here, since a lot of applications are hard-wired to
> > /sys/devices/platform/applesmc.768/.
> 
> Which applications? libsensors-based applications definitely don't
> hard-wire anything.

The ones I am thinking of are pommed and macfanctld, there are
probably others. The sysfs nodes have been around a while, so it is
not really surprising. If there is a policy saying it is ok to break
userspace in this case, that's fine.

Henrik

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Julien BLACHE]

"Henrik Rydberg" <rydberg@euromail.se> wrote:

Hi,

>> Which applications? libsensors-based applications definitely don't
>> hard-wire anything.
>
> The ones I am thinking of are pommed and macfanctld, there are
> probably others. The sysfs nodes have been around a while, so it is
> not really surprising. If there is a policy saying it is ok to break
> userspace in this case, that's fine.

I've just changed pommed to probe for applesmc through /sys/class/hwmon,
so you can go ahead and break it as far as I'm concerned :)

JB.

-- 
Julien BLACHE                                   <http://www.jblache.org> 
<jb@jblache.org>                                  GPG KeyID 0xF5D65169

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Sat, Dec 18, 2010 at 12:29:52PM +0100, Julien BLACHE wrote:
> "Henrik Rydberg" <rydberg@euromail.se> wrote:
> 
> Hi,
> 
> >> Which applications? libsensors-based applications definitely don't
> >> hard-wire anything.
> >
> > The ones I am thinking of are pommed and macfanctld, there are
> > probably others. The sysfs nodes have been around a while, so it is
> > not really surprising. If there is a policy saying it is ok to break
> > userspace in this case, that's fine.
> 
> I've just changed pommed to probe for applesmc through /sys/class/hwmon,
> so you can go ahead and break it as far as I'm concerned :)

Great, thanks Julien. And macfanctld should not be a problem either (ccing the author).

Henrik

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Sat, Dec 18, 2010 at 10:07:14AM +0100, Henrik Rydberg wrote:
> On Fri, Dec 17, 2010 at 08:23:21PM -0800, Guenter Roeck wrote:
> > On Fri, Dec 17, 2010 at 05:24:20PM -0500, Matthew Garrett wrote:
> > > The AppleSMC device is described in ACPI, including a list of its resources.
> > > We should use those rather than hardcoding the ports. A side-effect is that
> > > we can then remove the DMI matching, since there's a unique identifier to
> > > indicate that the machine has one of these devices.
> > > 
> > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > 
> > You added a whitespace error, and kfree() is safe and doesn't need a check.
> > I fixed those, so no need to resubmit. If Henrik gives his Ack we are ready to go.
> 
> Thanks, I will give it some testing before doing so. Matthew, did you
> test this under EFI boot? Also, NOTIFICATION_KEY does not seem to be
> used anywhere.

I tested under EFI and BIOS. NOTIFICATION_KEY is there for adding 
interrupt support - I got that hooked up but it seems that the latest 
Air is lacking the sudden motion sensor (probably because it's an 
SSD-only model) so wasn't able to test it. I'll play some more and add a 
further patch for that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Sat, Dec 18, 2010 at 10:37:19AM +0100, Henrik Rydberg wrote:
> On Fri, Dec 17, 2010 at 08:23:21PM -0800, Guenter Roeck wrote:
> Ok, although the idea is interesting, it seems this patch will need
> some reworking and testing. I tested the patches on a recent
> MacBookAir3,1, and i get this:
> 
> [    1.211182] Pid: 779, comm: modprobe Not tainted 2.6.37-rc5+ #280 Mac-942452F5819B1C1B/MacBookAir3,1
> [    1.211282] RIP: 0010:[<ffffffff811259b9>]  [<ffffffff811259b9>] sysfs_create_file+0x9/0x30
> [    1.211424] RSP: 0018:ffff88007f1e7d48  EFLAGS: 00010202
> [    1.211498] RAX: 0000000000000124 RBX: ffff88006bd58540 RCX: 0000000000000004
> [    1.211575] RDX: 0000000000000004 RSI: ffff88006bd58540 RDI: 0000000000000010
> [    1.211653] RBP: ffffffffa000b040 R08: 0000000000000000 R09: ffff88006bd58540
> [    1.211731] R10: 0000000000000001 R11: 00000000ffffffff R12: 0000000000000001
> [    1.211808] R13: 0000000000000000 R14: 0000000000000000 R15: ffff88006bd58568
> [    1.211886] FS:  00007fbe4a458700(0000) GS:ffff88006ec00000(0000) knlGS:0000000000000000
> [    1.211984] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [    1.212007] CR2: 0000000000000040 CR3: 0000000069de9000 CR4: 00000000000406b0
> [    1.212007] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    1.212007] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [    1.212007] Process modprobe (pid: 779, threadinfo ffff88007f1e6000, task ffff88006bd38000)
> [    1.212007] Stack:
> [    1.212007]  0000000000405056 ffffffffa0009ecc 0000000000405056 ffffffff8139b969
> [    1.212517]  0000000100000030 0000000000000010 ffffffffa000b040 0000000000000090
> [    1.212517]  0000000000000125 0000000000000000 0000000000000000 ffffffffa000ad60
> [    1.212517] Call Trace:
> [    1.212517]  [<ffffffffa0009ecc>] ? applesmc_create_nodes+0x13c/0x170 [applesmc]
> [    1.212517]  [<ffffffff8139b969>] ? printk+0x40/0x45
> [    1.212517]  [<ffffffffa000a89f>] ? applesmc_pnp_probe+0x529/0x56a [applesmc]
> [    1.212517]  [<ffffffff81222ede>] ? compare_pnp_id+0x1e/0xf0
> [    1.212517]  [<ffffffffa000a376>] ? applesmc_pnp_probe+0x0/0x56a [applesmc]
> [    1.212517]  [<ffffffff8122307f>] ? pnp_device_probe+0x6f/0xe0
> [    1.212517]  [<ffffffff8126a342>] ? driver_sysfs_add+0x72/0xa0
> [    1.212517]  [<ffffffff8126a5d7>] ? driver_probe_device+0x87/0x1a0
> [    1.212517]  [<ffffffff8126a783>] ? __driver_attach+0x93/0xa0
> [    1.212517]  [<ffffffff8126a6f0>] ? __driver_attach+0x0/0xa0
> [    1.212517]  [<ffffffff812695cd>] ? bus_for_each_dev+0x4d/0x80
> [    1.212517]  [<ffffffff81269ea8>] ? bus_add_driver+0x158/0x2d0
> [    1.212517]  [<ffffffffa000e000>] ? applesmc_init+0x0/0x14 [applesmc]
> [    1.212517]  [<ffffffff8126a9dc>] ? driver_register+0x6c/0x130
> [    1.212517]  [<ffffffffa000e000>] ? applesmc_init+0x0/0x14 [applesmc]
> [    1.212517]  [<ffffffff810002ca>] ? do_one_initcall+0x3a/0x170
> [    1.212517]  [<ffffffff8106f8a9>] ? sys_init_module+0xb9/0x200
> [    1.212517]  [<ffffffff810023bb>] ? system_call_fastpath+0x16/0x1b
> [    1.212517] Code: 8b 03 85 c0 74 05 f0 ff 03 eb 9d be b7 00 00 00 48 c7 c7 e7 4e 46 81 e8 06 2b f1 ff eb e8 0f 1f 40 00 48 83 ec 08 48 85 ff 74 1c <48> 8b 7f 30 48 85 f6 74 13 48 85 ff 74 0e ba 02 00 00 00 48 83 
> [    1.212517] RIP  [<ffffffff811259b9>] sysfs_create_file+0x9/0x30
> [    1.212517]  RSP <ffff88007f1e7d48>
> [    1.212517] CR2: 0000000000000040
> [    1.218078] ---[ end trace ad091051b87fc759 ]---

Hm. Is this an oops or a lockdep warning?

> So where to attach the sysfs nodes... There is also a userspace issue
> here, since a lot of applications are hard-wired to
> /sys/devices/platform/applesmc.768/.

The correct sysfs ABI is to use the /sys/class interface and the names 
under there rather than assuming a hardwired platform device path.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

> Hm. Is this an oops or a lockdep warning?

It is a null-pointer dereference in sysfs_create_file(). Probably the
kernel is asking the same question as I did - do you actually see the
sysfs nodes when testing the patch?

> > So where to attach the sysfs nodes... There is also a userspace issue
> > here, since a lot of applications are hard-wired to
> > /sys/devices/platform/applesmc.768/.
> 
> The correct sysfs ABI is to use the /sys/class interface and the names 
> under there rather than assuming a hardwired platform device path.

Still breaks user-space, but it seems this is already under control.

Thanks,
Henrik

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

> I tested under EFI and BIOS. NOTIFICATION_KEY is there for adding 
> interrupt support - I got that hooked up but it seems that the latest 
> Air is lacking the sudden motion sensor (probably because it's an 
> SSD-only model) so wasn't able to test it. I'll play some more and add a 
> further patch for that.

Sounds interesting. Please add the define in that patch instead.

Thanks,
Henrik

Re: [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Sat, Dec 18, 2010 at 04:30:12PM +0100, Henrik Rydberg wrote:
> > Hm. Is this an oops or a lockdep warning?
> 
> It is a null-pointer dereference in sysfs_create_file(). Probably the
> kernel is asking the same question as I did - do you actually see the
> sysfs nodes when testing the patch?

Yup. Not sure what's happening there. I'll try to figure that out.

> > > So where to attach the sysfs nodes... There is also a userspace issue
> > > here, since a lot of applications are hard-wired to
> > > /sys/devices/platform/applesmc.768/.
> > 
> > The correct sysfs ABI is to use the /sys/class interface and the names 
> > under there rather than assuming a hardwired platform device path.
> 
> Still breaks user-space, but it seems this is already under control.

If it turns out that there's code that relies on it then we could 
obviously retain the platform device, but it's kind of ugly. sysfs 
paths aren't intended to be hardcoded.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Mikael Ström]

Hi,

I know nothing about the background to the mail below, but if it's of
any help, macfanctld uses the following hardwired paths,

reading:

/sys/devices/platform/applesmc.768/temp<n>_input

and writing:

/sys/devices/platform/applesmc.768/fan1_min
/sys/devices/platform/applesmc.768/fan2_min
/sys/devices/platform/applesmc.768/fan1_manual
/sys/devices/platform/applesmc.768/fan2_manual

If any of those are to be broken, please advice in advance so i can
update the source before you break it, avoiding that the users fry their
MacBooks.

Thanks,
Mike

-- 
Mikael Ström <mikael@sesamiq.com>

On Sat, 2010-12-18 at 12:57 +0100, Henrik Rydberg wrote:
> On Sat, Dec 18, 2010 at 12:29:52PM +0100, Julien BLACHE wrote:
> > "Henrik Rydberg" <rydberg@euromail.se> wrote:
> > 
> > Hi,
> > 
> > >> Which applications? libsensors-based applications definitely don't
> > >> hard-wire anything.
> > >
> > > The ones I am thinking of are pommed and macfanctld, there are
> > > probably others. The sysfs nodes have been around a while, so it is
> > > not really surprising. If there is a policy saying it is ok to break
> > > userspace in this case, that's fine.
> > 
> > I've just changed pommed to probe for applesmc through /sys/class/hwmon,
> > so you can go ahead and break it as far as I'm concerned :)
> 
> Great, thanks Julien. And macfanctld should not be a problem either (ccing the author).
> 
> Henrik
> 

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Jean Delvare]

On Mon, 20 Dec 2010 21:44:34 +0800, Mikael Ström wrote:
> Hi,
> 
> I know nothing about the background to the mail below, but if it's of
> any help, macfanctld uses the following hardwired paths,
> 
> reading:
> 
> /sys/devices/platform/applesmc.768/temp<n>_input
> 
> and writing:
> 
> /sys/devices/platform/applesmc.768/fan1_min
> /sys/devices/platform/applesmc.768/fan2_min
> /sys/devices/platform/applesmc.768/fan1_manual
> /sys/devices/platform/applesmc.768/fan2_manual
> 
> If any of those are to be broken, please advice in advance so i can
> update the source before you break it, avoiding that the users fry their
> MacBooks.

I would expect the kernel driver to behave sanely in the absence of a
user-space application. Isn't it the case? If not, I consider it a
serious bug in the driver, which should be addressed ASAP.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Mon, Dec 20, 2010 at 09:44:34PM +0800, Mikael Ström wrote:
> Hi,
> 
> I know nothing about the background to the mail below, but if it's of
> any help, macfanctld uses the following hardwired paths,
> 
> reading:
> 
> /sys/devices/platform/applesmc.768/temp<n>_input
> 
> and writing:
> 
> /sys/devices/platform/applesmc.768/fan1_min
> /sys/devices/platform/applesmc.768/fan2_min
> /sys/devices/platform/applesmc.768/fan1_manual
> /sys/devices/platform/applesmc.768/fan2_manual
> 
> If any of those are to be broken, please advice in advance so i can
> update the source before you break it, avoiding that the users fry their
> MacBooks.

Yes, it's very broken. Walk /sys/class/hwmon and look for an entry with 
an appropriate name, don't hardcode device paths. But given the 
breakage, it might be easier to just keep the platform device for now. 
I'll redo the patch with that in mind.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Mon, Dec 20, 2010 at 02:57:21PM +0100, Jean Delvare wrote:
> On Mon, 20 Dec 2010 21:44:34 +0800, Mikael Ström wrote:
> > Hi,
> > 
> > I know nothing about the background to the mail below, but if it's of
> > any help, macfanctld uses the following hardwired paths,
> > 
> > reading:
> > 
> > /sys/devices/platform/applesmc.768/temp<n>_input
> > 
> > and writing:
> > 
> > /sys/devices/platform/applesmc.768/fan1_min
> > /sys/devices/platform/applesmc.768/fan2_min
> > /sys/devices/platform/applesmc.768/fan1_manual
> > /sys/devices/platform/applesmc.768/fan2_manual
> > 
> > If any of those are to be broken, please advice in advance so i can
> > update the source before you break it, avoiding that the users fry their
> > MacBooks.
> 
> I would expect the kernel driver to behave sanely in the absence of a
> user-space application. Isn't it the case? If not, I consider it a
> serious bug in the driver, which should be addressed ASAP.

The macbook overheat problems goes way back. All models have automatic
heat protection, but it kicks in at temperatures uncomfortable to most
users. The severity also depends on when the model was made. The
current solution is to use the macfanctld driver, which disables
automatic fan control, replacing it with a control loop yielding more
workable temperatures. A fan control solution in the kernel would
certainly be well received.

Henrik

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Mon, Dec 20, 2010 at 03:23:44PM +0100, Henrik Rydberg wrote:

> The macbook overheat problems goes way back. All models have automatic
> heat protection, but it kicks in at temperatures uncomfortable to most
> users. The severity also depends on when the model was made. The
> current solution is to use the macfanctld driver, which disables
> automatic fan control, replacing it with a control loop yielding more
> workable temperatures. A fan control solution in the kernel would
> certainly be well received.

There is one, you just need to hook into the thermal device interface.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Mon, Dec 20, 2010 at 02:28:15PM +0000, Matthew Garrett wrote:
> On Mon, Dec 20, 2010 at 03:23:44PM +0100, Henrik Rydberg wrote:
> 
> > The macbook overheat problems goes way back. All models have automatic
> > heat protection, but it kicks in at temperatures uncomfortable to most
> > users. The severity also depends on when the model was made. The
> > current solution is to use the macfanctld driver, which disables
> > automatic fan control, replacing it with a control loop yielding more
> > workable temperatures. A fan control solution in the kernel would
> > certainly be well received.
> 
> There is one, you just need to hook into the thermal device interface.

Right. The fans and the right sensors need to be plugged in, which
seems like it should happen at the same time the platform device is
removed. Thanks to macfanctld, there might even be enough information
regarding what sensors to use. Just about.

Henrik

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Matthew Garrett]

On Mon, Dec 20, 2010 at 04:06:48PM +0100, Henrik Rydberg wrote:
> On Mon, Dec 20, 2010 at 02:28:15PM +0000, Matthew Garrett wrote:
> > 
> > There is one, you just need to hook into the thermal device interface.
> 
> Right. The fans and the right sensors need to be plugged in, which
> seems like it should happen at the same time the platform device is
> removed. Thanks to macfanctld, there might even be enough information
> regarding what sensors to use. Just about.

I've been looking into this. The thermal control code under MacOS seems 
to tie into the GPU and CPU power management as well, and I suspect that 
this has more to do with the thermal issues we're seeing than the fan 
control alone.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Henrik Rydberg]

On Mon, Dec 20, 2010 at 03:12:58PM +0000, Matthew Garrett wrote:
> On Mon, Dec 20, 2010 at 04:06:48PM +0100, Henrik Rydberg wrote:
> > On Mon, Dec 20, 2010 at 02:28:15PM +0000, Matthew Garrett wrote:
> > > 
> > > There is one, you just need to hook into the thermal device interface.
> > 
> > Right. The fans and the right sensors need to be plugged in, which
> > seems like it should happen at the same time the platform device is
> > removed. Thanks to macfanctld, there might even be enough information
> > regarding what sensors to use. Just about.
> 
> I've been looking into this. The thermal control code under MacOS seems 
> to tie into the GPU and CPU power management as well, and I suspect that 
> this has more to do with the thermal issues we're seeing than the fan 
> control alone.

Yes, I think you are right. To conclude so far, I think it is great
that there is an effort to elevate apple hardware support in general.
I just do not see it happening overnight. If we tread lightly, we
should be able to start off by removing dmi matching and properly
support efi booting, and take it from there.

Thanks,
Henrik

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Mikael Ström]

On Mon, 2010-12-20 at 14:00 +0000, Matthew Garrett wrote:
> On Mon, Dec 20, 2010 at 09:44:34PM +0800, Mikael Ström wrote:
> > Hi,
> > 
> > I know nothing about the background to the mail below, but if it's of
> > any help, macfanctld uses the following hardwired paths,
> > 
> > reading:
> > 
> > /sys/devices/platform/applesmc.768/temp<n>_input
> > 
> > and writing:
> > 
> > /sys/devices/platform/applesmc.768/fan1_min
> > /sys/devices/platform/applesmc.768/fan2_min
> > /sys/devices/platform/applesmc.768/fan1_manual
> > /sys/devices/platform/applesmc.768/fan2_manual
> > 
> > If any of those are to be broken, please advice in advance so i can
> > update the source before you break it, avoiding that the users fry their
> > MacBooks.
> 
> Yes, it's very broken. Walk /sys/class/hwmon and look for an entry with 
> an appropriate name, don't hardcode device paths. But given the 
> breakage, it might be easier to just keep the platform device for now. 
> I'll redo the patch with that in mind.
> 
>From the other mail conversation, i realize that there is much needed
changes on the way, in particular efi support. A kernel based fan
control would be great as well. Until we get one, i keep macfancld alive
and will update it to use /sys/class/hwmon/, if that is the better way
to do it. 

Before i start to work on that, please confirm that i got this right:

In /sys/class/hwmon/hwmon<0..n>/device, locate fan<1..2>_* for
controlling the fan(s).

In /sys/class/hwmon/hwmon<0..n>/device, locate temp<1..n>_* for
enumerating and reading the temp sensors.

***

Finally, i would like to comment on the issue of fan controls for
MacBooks in general: This issues is not valid only under Linux. In fact,
most Unibody (Aluminum) MacBook users i know, have downloaded some form
of fan control for OSX to keep their laptops reasonable cold. Perhaps
this stems from that Aluminum leads heat much better so the user
"experience" that his laptop is hotter than a comparable plastic dito.
Never the less, the need is there, and users do want a better fan
control. So to make this right, we should probably have a similar fan
control mechanism in the kernel as we have in macfanctld. However,
different versions of MacBooks turn out to behave very differently, so
there is probably a need for some kind of configuration per model and/or
user configurable settings.

If there is anything more i can do to help, just let me know.

Regards,
Mike

Re: [lm-sensors] [PATCH 1/2 V3] applesmc: Use PnP rather than hardcoding resources and devices

[Julien BLACHE]

Mikael Ström <mikael@sesamiq.com> wrote:

Hi,

> Before i start to work on that, please confirm that i got this right:
>
> In /sys/class/hwmon/hwmon<0..n>/device, locate fan<1..2>_* for
> controlling the fan(s).
>
> In /sys/class/hwmon/hwmon<0..n>/device, locate temp<1..n>_* for
> enumerating and reading the temp sensors.

Check that device/name is "applesmc", it's faster, simpler and a lot
safer. See the last commit on pommed if you want to borrow some code.

JB.

-- 
Julien BLACHE                                   <http://www.jblache.org> 
<jb@jblache.org>                                  GPG KeyID 0xF5D65169