From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Thomas Haschka <haschka@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 1/1]: thermal driver therm_adt746.c
Date: Tue, 24 Feb 2015 08:18:38 +1100 [thread overview]
Message-ID: <1424726318.12891.1.camel@kernel.crashing.org> (raw)
In-Reply-To: <CADAoK=UH4q9x-OMyOpqCmGpNwDo3vey41xajBw-PDdupmEHnAA@mail.gmail.com>
Hi ! Please try sending patches inline rather than as attachments, it
makes replying a bit easier... Also don't CC stable, we can shoot to
stable later on if we think it's justified but first we need to get the
patch upstream
A few comments:
On Mon, 2015-02-23 at 12:58 +0100, Thomas Haschka wrote:
> --- linux/drivers/macintosh/therm_adt746x.c.orig 2015-02-23 12:19:03.984000000 +0100
> +++ linux/drivers/macintosh/therm_adt746x.c 2015-02-23 12:22:34.980000000 +0100
> @@ -1,7 +1,8 @@
> /*
> * Device driver for the i2c thermostat found on the iBook G4, Albook G4
> *
> - * Copyright (C) 2003, 2004 Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt
> + * Copyright (C) 2003, 2004, 2015
> + * Colin Leroy, Rasmus Rohde, Benjamin Herrenschmidt, Thomas Haschka
> *
> * Documentation from 115254175ADT7467_pra.pdf and 3686221171167ADT7460_b.pdf
> * http://www.onsemi.com/PowerSolutions/product.do?id=ADT7467
> @@ -45,7 +46,7 @@ static u8 REM_CONTROL[2] = {0x00, 0x40};
> static u8 FAN_SPEED[2] = {0x28, 0x2a};
> static u8 FAN_SPD_SET[2] = {0x30, 0x31};
>
> -static u8 default_limits_local[3] = {70, 50, 70}; /* local, sensor1, sensor2 */
> +static u8 default_limits_local[3] = {45, 50, 70}; /* local, sensor1, sensor2 */
Here you change the limit for the local sensor for existing machines,
care to explain ? I *think* that got adjusted a while back due to
a bunch of bogus error on some machines.
> static u8 default_limits_chip[3] = {80, 65, 80}; /* local, sensor1, sensor2 */
> static const char *sensor_location[3] = { "?", "?", "?" };
>
> @@ -225,59 +226,123 @@ static void display_stats(struct thermos
>
> static void update_fans_speed (struct thermostat *th)
> {
> - int lastvar = 0; /* last variation, for iBook */
> - int i = 0;
> -
> - /* we don't care about local sensor, so we start at sensor 1 */
> - for (i = 1; i < 3; i++) {
> - int started = 0;
> - int fan_number = (th->type == ADT7460 && i == 2);
> - int var = th->temps[i] - th->limits[i];
> -
> - if (var > -1) {
> - int step = (255 - fan_speed) / 7;
> - int new_speed = 0;
> +
> + /* Multfan Laptops */
> + if ( th->type == ADT7460 ) {
> + int lastvar = 0; /* last variation, for iBook */
> + int i = 0;
> + /* we don't care about local sensor, so we start at sensor 1 */
> + for (i = 1; i < 3; i++) {
> + int started = 0;
> + int fan_number = (th->type == ADT7460 && i == 2);
> + int var = th->temps[i] - th->limits[i];
> +
> + if (var > -1) {
> + int step = (255 - fan_speed) / 7;
> + int new_speed = 0;
The function is too big, please break it down into two sub functions,
one for multifan and one for single fan.
It is also unclear due to the indentation changes whether you changed
the behaviour on "other" laptops. makes the review a bit harder.
> /* hysteresis : change fan speed only if variation is
> * more than two degrees */
> - if (abs(var - th->last_var[fan_number]) < 2)
> - continue;
> -
> - started = 1;
> - new_speed = fan_speed + ((var-1)*step);
> + if (abs(var - th->last_var[fan_number]) < 2)
> + continue;
>
> - if (new_speed < fan_speed)
> - new_speed = fan_speed;
> - if (new_speed > 255)
> - new_speed = 255;
> + started = 1;
> + new_speed = fan_speed + ((var-1)*step);
>
> - if (verbose)
> - printk(KERN_DEBUG "adt746x: Setting fans speed to %d "
> - "(limit exceeded by %d on %s)\n",
> - new_speed, var,
> - sensor_location[fan_number+1]);
> - write_both_fan_speed(th, new_speed);
> - th->last_var[fan_number] = var;
> - } else if (var < -2) {
> + if (new_speed < fan_speed)
> + new_speed = fan_speed;
> + if (new_speed > 255)
> + new_speed = 255;
> +
> + if (verbose)
> + printk(KERN_DEBUG "adt746x: Setting fans speed to %d "
> + "(limit exceeded by %d on %s)\n",
> + new_speed, var,
> + sensor_location[fan_number+1]);
> + write_both_fan_speed(th, new_speed);
> + th->last_var[fan_number] = var;
> + } else if (var < -2) {
> /* don't stop fan if sensor2 is cold and sensor1 is not
> * so cold (lastvar >= -1) */
> - if (i == 2 && lastvar < -1) {
> - if (th->last_speed[fan_number] != 0)
> - if (verbose)
> + if (i == 2 && lastvar < -1) {
> + if (th->last_speed[fan_number] != 0)
> + if (verbose)
> + printk(KERN_DEBUG "adt746x: Stopping "
> + "fans.\n");
> + write_both_fan_speed(th, 0);
> + }
> + }
> +
> + lastvar = var;
> +
> + if (started)
> + return; /* we don't want to re-stop the fan
> + * if sensor1 is heating and sensor2 is not */
> + }
> +
> + } else {
> + /*single fan laptops i.e. 12 inch powerbook/ibook*/
> + int lastvar = 0; /* last variation, for iBook */
> + int i = 0;
> + int var = 0;
> + int var_sensor[3];
> + int started = 0;
> + int fan_number = 0;
> + for (i = 0; i < 3; i++) {
> + var_sensor[i] = th->temps[i] - th->limits[i];
> + }
> + var = var_sensor[0];
> + for (i = 1; i < 3; i++) {
> + if (var_sensor[i] > var) {
> + var = var_sensor[i];
> + }
> + }
> + if (var > -1) {
> + int step = (255 - fan_speed) / 7;
> + int new_speed = 0;
> +
> + /* hysteresis : change fan speed only if variation is
> + * more than two degrees */
> + if (abs(var - th->last_var[fan_number]) > 2) {
> +
> + started = 1;
> + new_speed = fan_speed + ((var-1)*step);
> +
> + if (new_speed < fan_speed)
> + new_speed = fan_speed;
> + if (new_speed > 255)
> + new_speed = 255;
> +
> + if (verbose)
> + printk(
> + KERN_DEBUG "adt746x: Setting fans speed to %d "
> + "(limit exceeded by %d on %s)\n",
> + new_speed, var,
> + sensor_location[fan_number+1]);
> + write_both_fan_speed(th, new_speed);
> + th->last_var[fan_number] = var;
> + }
> + } else if (var < -2) {
> + /* don't stop fan if sensor2 is cold and sensor1 is not
> + * so cold (lastvar >= -1) */
> + if (i == 2 && lastvar < -1) {
> + if (th->last_speed[fan_number] != 0)
> + if (verbose)
> printk(KERN_DEBUG "adt746x: Stopping "
> "fans.\n");
> - write_both_fan_speed(th, 0);
> + write_both_fan_speed(th, 0);
> }
> }
>
> - lastvar = var;
> + lastvar = var;
>
> if (started)
> - return; /* we don't want to re-stop the fan
> - * if sensor1 is heating and sensor2 is not */
> + return; /* we don't want to re-stop the fan
> + * if sensor1 is heating and sensor2 is not */
> }
> }
>
> +
> static int monitor_task(void *arg)
> {
> struct thermostat* th = arg;
> @@ -371,10 +436,13 @@ static ssize_t store_##name(struct devic
> return n; \
> }
>
> -BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1])))
> -BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2])))
> -BUILD_SHOW_FUNC_INT(sensor1_limit, th->limits[1])
> -BUILD_SHOW_FUNC_INT(sensor2_limit, th->limits[2])
> +BUILD_SHOW_FUNC_INT(sensor0_temperature, (read_reg(th, TEMP_REG[0]))*1000)
> +BUILD_SHOW_FUNC_INT(sensor1_temperature, (read_reg(th, TEMP_REG[1]))*1000)
> +BUILD_SHOW_FUNC_INT(sensor2_temperature, (read_reg(th, TEMP_REG[2]))*1000)
> +BUILD_SHOW_FUNC_INT(sensor0_limit, (th->limits[0])*1000)
> +BUILD_SHOW_FUNC_INT(sensor1_limit, (th->limits[1])*1000)
> +BUILD_SHOW_FUNC_INT(sensor2_limit, (th->limits[2])*1000)
You changed the units here, that should be documented and why.
> +BUILD_SHOW_FUNC_STR(sensor0_location, sensor_location[0])
> BUILD_SHOW_FUNC_STR(sensor1_location, sensor_location[1])
> BUILD_SHOW_FUNC_STR(sensor2_location, sensor_location[2])
>
> @@ -387,14 +455,20 @@ BUILD_SHOW_FUNC_FAN(sensor2_fan_speed,
> BUILD_SHOW_FUNC_INT_LITE(limit_adjust, limit_adjust)
> BUILD_STORE_FUNC_DEG(limit_adjust, th)
>
> +static DEVICE_ATTR(sensor0_temperature, S_IRUGO,
> + show_sensor0_temperature,NULL);
> static DEVICE_ATTR(sensor1_temperature, S_IRUGO,
> show_sensor1_temperature,NULL);
> static DEVICE_ATTR(sensor2_temperature, S_IRUGO,
> show_sensor2_temperature,NULL);
> +static DEVICE_ATTR(sensor0_limit, S_IRUGO,
> + show_sensor0_limit, NULL);
> static DEVICE_ATTR(sensor1_limit, S_IRUGO,
> show_sensor1_limit, NULL);
> static DEVICE_ATTR(sensor2_limit, S_IRUGO,
> show_sensor2_limit, NULL);
> +static DEVICE_ATTR(sensor0_location, S_IRUGO,
> + show_sensor0_location, NULL);
> static DEVICE_ATTR(sensor1_location, S_IRUGO,
> show_sensor1_location, NULL);
> static DEVICE_ATTR(sensor2_location, S_IRUGO,
> @@ -426,10 +500,13 @@ static void thermostat_create_files(stru
> return;
> dev = &th->pdev->dev;
> dev_set_drvdata(dev, th);
> - err = device_create_file(dev, &dev_attr_sensor1_temperature);
> + err = device_create_file(dev, &dev_attr_sensor0_temperature);
> + err |= device_create_file(dev, &dev_attr_sensor1_temperature);
> err |= device_create_file(dev, &dev_attr_sensor2_temperature);
> + err |= device_create_file(dev, &dev_attr_sensor0_limit);
> err |= device_create_file(dev, &dev_attr_sensor1_limit);
> err |= device_create_file(dev, &dev_attr_sensor2_limit);
> + err |= device_create_file(dev, &dev_attr_sensor0_location);
> err |= device_create_file(dev, &dev_attr_sensor1_location);
> err |= device_create_file(dev, &dev_attr_sensor2_location);
> err |= device_create_file(dev, &dev_attr_limit_adjust);
> @@ -449,10 +526,13 @@ static void thermostat_remove_files(stru
> if (!th->pdev)
> return;
> dev = &th->pdev->dev;
> + device_remove_file(dev, &dev_attr_sensor0_temperature);
> device_remove_file(dev, &dev_attr_sensor1_temperature);
> device_remove_file(dev, &dev_attr_sensor2_temperature);
> + device_remove_file(dev, &dev_attr_sensor0_limit);
> device_remove_file(dev, &dev_attr_sensor1_limit);
> device_remove_file(dev, &dev_attr_sensor2_limit);
> + device_remove_file(dev, &dev_attr_sensor0_location);
> device_remove_file(dev, &dev_attr_sensor1_location);
> device_remove_file(dev, &dev_attr_sensor2_location);
> device_remove_file(dev, &dev_attr_limit_adjust);
Cheers,
Ben.
next prev parent reply other threads:[~2015-02-23 21:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-23 11:58 [PATCH 1/1]: thermal driver therm_adt746.c Thomas Haschka
2015-02-23 15:23 ` Cedric Le Goater
2015-02-23 21:18 ` Benjamin Herrenschmidt [this message]
2015-02-25 11:24 ` Thomas Haschka
2015-02-25 21:12 ` Benjamin Herrenschmidt
2015-02-26 9:48 ` Thomas Haschka
2015-03-25 5:36 ` Benjamin Herrenschmidt
2015-03-26 16:23 ` Thomas Haschka
2015-03-26 21:31 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1424726318.12891.1.camel@kernel.crashing.org \
--to=benh@kernel.crashing.org \
--cc=haschka@gmail.com \
--cc=linuxppc-dev@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).