[PATCH] Apple SMC driver (hardware monitoring and control)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 19 Mar 2007 13:19:00 +0800 Nicolas Boichat <nicolas at boichat.ch> wrote:

> 
> This driver provides support for the Apple System Management Controller, which
> provides an accelerometer (Apple Sudden Motion Sensor), light sensors,
> temperature sensors, keyboard backlight control and fan control. Only
> Intel-based Apple's computers are supported (MacBook Pro, MacBook, MacMini).
> 

It's trivia time:

> +#define MOTION_SENSOR_X_KEY	"MO_X" //r-o length 2
> +#define MOTION_SENSOR_Y_KEY	"MO_Y" //r-o length 2
> +#define MOTION_SENSOR_Z_KEY	"MO_Z" //r-o length 2
> +#define MOTION_SENSOR_KEY	"MOCN" //r/w length 2
> +
> +#define FANS_COUNT		"FNum" //r-o length 1
> +#define FANS_MANUAL		"FS! " //r-w length 2
> +#define FAN_ACTUAL_SPEED	"F0Ac" //r-o length 2
> +#define FAN_MIN_SPEED		"F0Mn" //r-o length 2
> +#define FAN_MAX_SPEED		"F0Mx" //r-o length 2
> +#define FAN_SAFE_SPEED		"F0Sf" //r-o length 2
> +#define FAN_TARGET_SPEED	"F0Tg" //r-w length 2

Please avoid C++-style comments.

> +/* Temperature sensors keys. First set for Macbook(Pro), second for Macmini */
> +static const char* temperature_sensors_sets[][8] = {
> +	{ "TB0T", "TC0D", "TC0P", "Th0H", "Ts0P", "Th1H", "Ts1P", NULL },
> +	{ "TC0D", "TC0P", NULL }
> +};

The NULLs here are harmless, but unneeded.

> +/* Structure to be passed to DMI_MATCH function */
> +struct dmi_match_data {
> +/* Indicates whether this computer has an accelerometer. */
> +	int accelerometer;
> +/* Indicates whether this computer has light sensors and keyboard backlight. */
> +	int light;
> +/* Indicates which temperature sensors set to use. */
> +	int temperature_set;
> +};
> +
> +static int debug = 0;
> +static struct platform_device *pdev;
> +static s16 rest_x;
> +static s16 rest_y;
> +static struct timer_list applesmc_timer;
> +static struct input_dev *applesmc_idev;
> +
> +/* Indicates whether this computer has an accelerometer. */
> +static unsigned int applesmc_accelerometer = 0;
> +
> +/* Indicates whether this computer has light sensors and keyboard backlight. */
> +static unsigned int applesmc_light = 0;
> +
> +/* Indicates which temperature sensors set to use. */
> +static unsigned int applesmc_temperature_set = 0;

All the "= 0"s above are unneeded and will increase the module or vmlinux
size - they should be removed.

> +static DECLARE_MUTEX(applesmc_sem);

Semaphores should be used only when their counting feature is required.  I
think thsi can be switched to `struct mutex'.

> +/*
> + * applesmc_read_key - reads len bytes from a given key, and put them in buffer.
> + * Returns zero on success or a negative error on failure. Callers must
> + * hold applesmc_sem.
> + */
> +static int applesmc_read_key(const char* key, u8* buffer, u8 len)
> +{
> +	int ret = -EIO;
> +	int i;
> +
> +	outb(APPLESMC_READ_CMD, APPLESMC_CMD_PORT);
> +	if (__wait_status(0x0c))
> +		goto out;
> +	
> +	for (i = 0; i < 4; i++) {
> +		outb(key[i], APPLESMC_DATA_PORT);
> +		if (__wait_status(0x04))
> +			goto out;
> +	}
> +	if (debug) printk(KERN_DEBUG "<%s", key);
> +
> +	outb(len, APPLESMC_DATA_PORT);
> +	if (debug) printk(KERN_DEBUG ">%x", len);

Please convert to standard kernel style:

	if (debug)
		printk(KERN_DEBUG ">%x", len);

There are many instances of this.

> +
> +	for (i = 0; i < len; i++) {
> +		if (__wait_status(0x05))
> +			goto out;
> +		buffer[i] = inb(APPLESMC_DATA_PORT);
> +		if (debug) printk(KERN_DEBUG "<%x", buffer[i]);
> +	}
> +	if (debug) printk(KERN_DEBUG "\n");
> +	ret = 0;
> +
> +out:
> +	return ret;
> +}
> +
>
> ...
>
> +/*
> + * applesmc_device_init - initialize the accelerometer.  Returns zero on success
> + * and negative error code on failure.  Can sleep.
> + */
> +static int applesmc_device_init(void)
> +{
> +	int total, ret = -ENXIO;
> +	u8 buffer[2];
> +
> +	if (!applesmc_accelerometer) return 0;
> +
> +	down(&applesmc_sem);
> +
> +	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
> +		if (debug) printk(KERN_DEBUG "applesmc try %d\n", total);
> +		if (!applesmc_read_key(MOTION_SENSOR_KEY, buffer, 2) &&
> +				(buffer[0] != 0x00 || buffer[1] != 0x00)) {
> +			if (total == INIT_TIMEOUT_MSECS) {
> +				printk(KERN_DEBUG "applesmc: device has" 
> +						" already been initialized"
> +						" (0x%02x, 0x%02x).\n",
> +						buffer[0], buffer[1]);
> +			}
> +			else {

Please use

			} else {

here and wherever else the above appears.

> +				printk(KERN_DEBUG "applesmc: device" 
> +						" successfully initialized"
> +						" (0x%02x, 0x%02x).\n",
> +						buffer[0], buffer[1]);
> +			}
> +			ret = 0;
> +			goto out;
> +		}
> +		buffer[0] = 0xe0;
> +		buffer[1] = 0x00;
> +		applesmc_write_key(MOTION_SENSOR_KEY, buffer, 2);
> +		msleep(INIT_WAIT_MSECS);
> +	}
> +
> +	printk(KERN_WARNING "applesmc: failed to init the device\n");
> +
> +out:
> +	up(&applesmc_sem);
> +	return ret;
> +}
> +
>
> ...
>
> +/*
> + * Macro defining helper functions and DEVICE_ATTR for a fan sysfs entries.
> + *  - show actual speed
> + *  - show/store minimum speed
> + *  - show maximum speed
> + *  - show safe speed
> + *  - show/store target speed
> + *  - show/store manual mode
> + */
> +#define sysfs_fan_speeds_offset(offset) \
> +static ssize_t show_fan_actual_speed_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_speed(dev, buf, FAN_ACTUAL_SPEED, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_actual_speed, S_IRUGO, \
> +					show_fan_actual_speed_##offset, NULL); \
> +\
> +static ssize_t show_fan_minimum_speed_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_speed(dev, buf, FAN_MIN_SPEED, offset); \
> +} \
> +static ssize_t store_fan_minimum_speed_##offset (struct device *dev, \
> +		struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	return applesmc_store_fan_speed(dev, buf, count, FAN_MIN_SPEED, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_minimum_speed, S_IRUGO | S_IWUSR, \
> +	show_fan_minimum_speed_##offset, store_fan_minimum_speed_##offset); \
> +\
> +static ssize_t show_fan_maximum_speed_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_speed(dev, buf, FAN_MAX_SPEED, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_maximum_speed, S_IRUGO, \
> +				show_fan_maximum_speed_##offset, NULL); \
> +\
> +static ssize_t show_fan_safe_speed_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_speed(dev, buf, FAN_SAFE_SPEED, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_safe_speed, S_IRUGO, \
> +					show_fan_safe_speed_##offset, NULL); \
> +\
> +static ssize_t show_fan_target_speed_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_speed(dev, buf, FAN_TARGET_SPEED, offset); \
> +} \
> +static ssize_t store_fan_target_speed_##offset (struct device *dev, \
> +		struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	return applesmc_store_fan_speed(dev, buf, count, FAN_TARGET_SPEED, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_target_speed, S_IRUGO | S_IWUSR, \
> +	show_fan_target_speed_##offset, store_fan_target_speed_##offset); \
> +static ssize_t show_fan_manual_##offset (struct device *dev, \
> +				struct device_attribute *attr, char *buf) \
> +{ \
> +	return applesmc_show_fan_manual(dev, buf, offset); \
> +} \
> +static ssize_t store_fan_manual_##offset (struct device *dev, \
> +		struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	return applesmc_store_fan_manual(dev, buf, count, offset); \
> +} \
> +static DEVICE_ATTR(fan##offset##_manual, S_IRUGO | S_IWUSR, \
> +		   show_fan_manual_##offset, store_fan_manual_##offset);

erk.  Can we use attribute groups here?

> +/*
> + * Create the needed functions for each fan using the macro defined above 
> + * (2 fans are supported)
> + */
> +sysfs_fan_speeds_offset(0);
> +sysfs_fan_speeds_offset(1);
> +
> +/* Macro creating the sysfs entries for a fan */
> +#define device_create_file_fan(ret, client, offset) \
> +do { \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_actual_speed.attr); \
> +if (ret) break; \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_minimum_speed.attr); \
> +if (ret) break; \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_maximum_speed.attr); \
> +if (ret) break; \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_safe_speed.attr); \
> +if (ret) break; \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_target_speed.attr); \
> +if (ret) break; \
> +ret = sysfs_create_file(client, &dev_attr_fan##offset##_manual.attr); \
> +} while (0)

And here?

>
> ...
>
> +/* 
> + * applesmc_dmi_match - found a match.  return one, short-circuiting the hunt.
> + */
> +static int applesmc_dmi_match(struct dmi_system_id *id)
> +{
> +	int i = 0;
> +	struct dmi_match_data* dmi_data =
> +					(struct dmi_match_data*)id->driver_data;

Unneeded and undesirable cast of void*.

> +	printk(KERN_INFO "applesmc: %s detected:\n", id->ident);
> +	applesmc_accelerometer = dmi_data->accelerometer;
> +	printk(KERN_INFO "applesmc:  - Model %s accelerometer\n",
> +				applesmc_accelerometer ? "with" : "without");
> +	applesmc_light = dmi_data->light;
> +	printk(KERN_INFO "applesmc:  - Model %s light sensors and backlight\n",
> +					applesmc_light ? "with" : "without");
> +
> +	applesmc_temperature_set =  dmi_data->temperature_set;
> +	while (temperature_sensors_sets[applesmc_temperature_set][i] != NULL)
> +		i++;
> +	printk(KERN_INFO "applesmc:  - Model with %d temperature sensors\n", i);
> +	return 1;
> +}
> +
> +/* Create accelerometer ressources */
> +static int applesmc_create_accelerometer(void) {

Opening brace goes in column 1

> +	int ret;
> +
> +	ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_position.attr);
> +	if (ret)
> +		goto out;
> +
> +	ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_calibrate.attr);
> +	if (ret)
> +		goto out;
> +
> +	applesmc_idev = input_allocate_device();
> +	if (!applesmc_idev) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* initial calibrate for the input device */
> +	applesmc_calibrate();
> +
> +	/* initialize the input class */
> +	applesmc_idev->name = "applesmc";
> +	applesmc_idev->cdev.dev = &pdev->dev;
> +	applesmc_idev->evbit[0] = BIT(EV_ABS);
> +	input_set_abs_params(applesmc_idev, ABS_X,
> +			-256, 256, APPLESMC_INPUT_FUZZ, APPLESMC_INPUT_FLAT);
> +	input_set_abs_params(applesmc_idev, ABS_Y,
> +			-256, 256, APPLESMC_INPUT_FUZZ, APPLESMC_INPUT_FLAT);
> +
> +	input_register_device(applesmc_idev);
> +
> +	/* start up our timer for the input device */
> +	init_timer(&applesmc_timer);
> +	applesmc_timer.function = applesmc_mousedev_poll;
> +	applesmc_timer.expires = jiffies + APPLESMC_POLL_PERIOD;
> +	add_timer(&applesmc_timer);
> +
> +	return 0;
> +
> +out:
> +	printk(KERN_WARNING "applesmc: driver init failed (ret=%d)!\n", ret);
> +	return ret;
> +}
> +
> +/* Release all ressources used by the accelerometer */
> +static void applesmc_release_accelerometer(void) {
> +	del_timer_sync(&applesmc_timer);
> +	input_unregister_device(applesmc_idev);
> +}

Opening brace in column 1

> +static int __init applesmc_init(void)
> +{
> +	int ret;
> +	int count;
> +
> +	struct dmi_match_data applesmc_dmi_data[] = {
> +	/* MacBook Pro: accelerometer, backlight and temperature set 0 */
> +	  { .accelerometer = 1, .light = 1, .temperature_set = 0 },
> +	/* MacBook: accelerometer and temperature set 0 */
> +	  { .accelerometer = 1, .light = 0, .temperature_set = 0 },
> +	/* MacBook: temperature set 1 */
> +	  { .accelerometer = 0, .light = 0, .temperature_set = 1 }
> +	};
> +
> +	/* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
> +	 * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
> +	struct dmi_system_id applesmc_whitelist[] = {
> +		{ applesmc_dmi_match, "Apple MacBook Pro", {
> +		  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> +		  DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
> +			(void*)&applesmc_dmi_data[0]},
> +		{ applesmc_dmi_match, "Apple MacBook", {
> +		  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> +		  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
> +			(void*)&applesmc_dmi_data[1]},
> +		{ applesmc_dmi_match, "Apple Macmini", {
> +		  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> +		  DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
> +			(void*)&applesmc_dmi_data[2]},
> +		{ .ident = NULL }
> +	};

The compiler will need to build the above arrays on the stack at runtime. 
Is it possible to make these static so they are build at compile-time?  And
to then make them __initdata so they get discarded?

> +	if (!dmi_check_system(applesmc_whitelist)) {
> +		printk(KERN_WARNING "applesmc: supported laptop not found!\n");
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	if (!request_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS,
> +								"applesmc")) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
> +	ret = platform_driver_register(&applesmc_driver);
> +	if (ret)
> +		goto out_region;
> +
> +	pdev = platform_device_register_simple("applesmc", -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		goto out_driver;
> +	}
> +





[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux