* [lm-sensors] abituguru3 review
@ 2007-03-19 21:28 Juerg Haefliger
2007-03-20 14:29 ` Rick Wright
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Juerg Haefliger @ 2007-03-19 21:28 UTC (permalink / raw)
To: lm-sensors
Hans,
Here's my review:
> This patch adds a new driver for the hardware monitoring features of the third
> revision of the Abit uGuru chip, found on recent Abit motherboards. This is an
> entirely different beast then the first and second revision (its again a
> winbond microcontroller, but the "protocol" to talk to it and the bank
> addresses are very different.
>
> Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>
> --- linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru.uguru3 2007-01-18 13:45:20.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru 2007-01-18 16:15:24.000000000 +0100
> @@ -2,7 +2,7 @@
> ===========>
> Supported chips:
> - * Abit uGuru revision 1-3 (Hardware Monitor part only)
> + * Abit uGuru revision 1 & 2 (Hardware Monitor part only)
> Prefix: 'abituguru'
> Addresses scanned: ISA 0x0E0
> Datasheet: Not available, this driver is based on reverse engineering.
> @@ -20,8 +20,8 @@
> uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
> uGuru 2.2.0.0 ~ 2.2.0.6 (AA8 Fatal1ty)
> uGuru 2.3.0.0 ~ 2.3.0.9 (AN8)
> - uGuru 3.0.0.0 ~ 3.0.1.2 (AW8, AL8, NI8)
> - uGuru 4.xxxxx? (AT8 32X) (2)
> + uGuru 3.0.0.0 ~ 3.0.x.x (AW8, AL8, NI8 SLI, AT8 32X, AN8 32X,
> + AW9D-MAX) (2)
Hmm... I don't get it. You're changing the abituguru driver to only
support rev 1 & 2 but here you add rev 3 to the list of supported
devices?
> 1) For revisions 2 and 3 uGuru's the driver can autodetect the
> sensortype (Volt or Temp) for bank1 sensors, for revision 1 uGuru's
> this doesnot always work. For these uGuru's the autodection can
Same here, there is still mentioning of rev 3 in the doc for abituguru.
> @@ -30,8 +30,9 @@
> bank1_types=1,1,0,0,0,0,0,2,0,0,0,0,2,0,0,1
> You may also need to specify the fan_sensors option for these boards
> fan_sensors=5
> - 2) The current version of the abituguru driver is known to NOT work
> - on these Motherboards
> + 2) There is a seperate abituguru3 driver for these motherboards,
> + the abituguru (without the 3 !) driver will not work on these
> + motherboards (and visa versa)!
>
> Authors:
> Hans de Goede <j.w.r.degoede at hhs.nl>,
> @@ -69,13 +70,15 @@
> Description
> -----------
>
> -This driver supports the hardware monitoring features of the Abit uGuru chip
> -found on Abit uGuru featuring motherboards (most modern Abit motherboards).
> -
> -The uGuru chip in reality is a Winbond W83L950D in disguise (despite Abit
> -claiming it is "a new microprocessor designed by the ABIT Engineers").
> -Unfortunatly this doesn't help since the W83L950D is a generic
> -microcontroller with a custom Abit application running on it.
> +This driver supports the hardware monitoring features of the first and
> +second revision of the Abit uGuru chip found on Abit uGuru featuring
> +motherboards (most modern Abit motherboards).
> +
> +The first and second revision of the uGuru chip in reality is a Winbond
> +W83L950D in disguise (despite Abit claiming it is "a new microprocessor
> +designed by the ABIT Engineers"). Unfortunatly this doesn't help since the
> +W83L950D is a generic microcontroller with a custom Abit application running
> +on it.
>
> Despite Abit not releasing any information regarding the uGuru, Olle
> Sandberg <ollebull at gmail.com> has managed to reverse engineer the sensor part
> --- linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru3.uguru3 2007-01-18 16:01:33.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru3 2007-01-18 16:18:15.000000000 +0100
> @@ -0,0 +1,63 @@
> +Kernel driver abituguru3
> +============
> +
> +Supported chips:
> + * Abit uGuru revision 3 (Hardware Monitor part, reading only)
> + Prefix: 'abituguru3'
> + Addresses scanned: ISA 0x0E0
> + Datasheet: Not available, this driver is based on reverse engineering.
> + Note:
> + The uGuru is a microcontroller with onboard firmware which programs
> + it to behave as a hwmon IC. There are many different revisions of the
> + firmware and thus effectivly many different revisions of the uGuru.
> + Below is an incomplete list with which revisions are used for which
> + Motherboards:
> + uGuru 1.00 ~ 1.24 (AI7, KV8-MAX3, AN7)
> + uGuru 2.0.0.0 ~ 2.0.4.2 (KV8-PRO)
> + uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
> + uGuru 2.3.0.0 ~ 2.3.0.9 (AN8)
> + uGuru 3.0.0.0 ~ 3.0.x.x (AW8, AL8, NI8 SLI, AT8 32X, AN8 32X,
> + AW9D-MAX)
> + The abituguru3 driver is only for revison 3.0.x.x motherboards,
> + this driver will not work on older motherboards for older
Missing full stop after motherboards.
> + motherboards use the abituguru (without the 3 !) driver.
> +
> +Authors:
> + Hans de Goede <j.w.r.degoede at hhs.nl>,
> + (Initial reverse engineering done by Louis Kruger)
> +
> +
> +Module Parameters
> +-----------------
> +
> +* force: bool Force detection. Note this parameter only causes the
> + detection to be skipped, if the uGuru can't be read
> + the module initialization (insmod) will still fail.
> +* verbose: int How verbose should the driver be? (0-3):
> + 0 normal output
> + 1 + verbose error reporting (default)
> + Default: 1 (the driver is still in the testing phase)
> +
> +Description
> +-----------
> +
> +This driver supports the hardware monitoring features of the third revision of
> +the Abit uGuru chip, found on recent Abit uGuru featuring motherboards.
> +
> +The 3th revision of the uGuru chip in reality is a Winbond W83L951G.
3rd.
> +Unfortunatly this doesn't help since the W83L951G is a generic microcontroller
> +with a custom Abit application running on it.
> +
> +Despite Abit not releasing any information regarding the uGuru revision 3,
> +Louis Kruger has managed to reverse engineer the sensor part of the uGuru.
> +Without his work this driver would not have been possible.
> +
> +Known Issues
> +------------
> +
> +The voltage and frequency control parts of the Abit uGuru are not supported,
> +neither is writing any of the sensor settings and writing / reading the
> +fanspeed control registers (FanEQ)
> +
> +If you encounter any problems please mail me <j.w.r.degoede at hhs.nl> and
> +include the output of: "dmesg | grep abituguru"
> --- linux-2.6.20-rc4-mm1/drivers/hwmon/abituguru3.c.uguru3 2007-01-18 16:04:28.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/drivers/hwmon/abituguru3.c 2007-01-18 16:04:23.000000000 +0100
> @@ -0,0 +1,1086 @@
> +/*
> + abituguru3.c Copyright (c) 2006 Hans de Goede <j.w.r.degoede at hhs.nl>
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +/*
> + This driver supports the sensor part of revision 3 of the custom Abit uGuru
> + chip found on newer Abit uGuru motherboards. Note: because of lack of specs
> + only reading the sensors and their settings is supported.
> +*/
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <asm/io.h>
> +
> +/* uGuru3 bank addresses */
> +#define ABIT_UGURU3_SETTINGS_BANK 0x01
> +#define ABIT_UGURU3_SENSORS_BANK 0x08
> +#define ABIT_UGURU3_MISC_BANK 0x09
> +#define ABIT_UGURU3_ALARMS_START 0x1E
> +#define ABIT_UGURU3_SETTINGS_START 0x24
> +#define ABIT_UGURU3_VALUES_START 0x80
> +#define ABIT_UGURU3_BOARD_ID 0x0A
> +/* uGuru3 sensor bank flags */ /* Alarm if: */
> +#define ABIT_UGURU3_TEMP_HIGH_ALARM_ENABLE 0x01 /* temp over warn */
> +#define ABIT_UGURU3_VOLT_HIGH_ALARM_ENABLE 0x02 /* volt over max */
> +#define ABIT_UGURU3_VOLT_LOW_ALARM_ENABLE 0x04 /* volt under min */
> +#define ABIT_UGURU3_TEMP_HIGH_ALARM_FLAG 0x10 /* temp is over warn */
> +#define ABIT_UGURU3_VOLT_HIGH_ALARM_FLAG 0x20 /* volt is over max */
> +#define ABIT_UGURU3_VOLT_LOW_ALARM_FLAG 0x40 /* volt is under min */
> +#define ABIT_UGURU3_FAN_LOW_ALARM_ENABLE 0x01 /* fan under min */
> +#define ABIT_UGURU3_BEEP_ENABLE 0x08 /* beep if alarm */
> +#define ABIT_UGURU3_SHUTDOWN_ENABLE 0x80 /* shutdown if alarm */
> +/* sensor types */
> +#define ABIT_UGURU3_IN_SENSOR 0
> +#define ABIT_UGURU3_TEMP_SENSOR 1
> +#define ABIT_UGURU3_FAN_SENSOR 2
> +
> +/* Timeouts / Retries, if these turn out to need a lot of fiddling we could
> + convert them to params. Determined by trial and error. I assume this is
> + cpu-speed independent, since the ISA-bus and not the CPU should be the
> + bottleneck. */
> +#define ABIT_UGURU3_WAIT_TIMEOUT 250
> +/* Normally the 0xAC at the end of synchronize() is reported after the
> + first read, but sometimes not and we need to poll */
> +#define ABIT_UGURU3_SYNCHRONIZE_TIMEOUT 5
> +/* utility macros */
> +#define ABIT_UGURU3_NAME "abituguru3"
> +#define ABIT_UGURU3_DEBUG(format, arg...) \
> + if (verbose) \
> + printk(KERN_DEBUG ABIT_UGURU3_NAME ": " format , ## arg)
> +
> +/* Macros to help calculate the sysfs_names array lenght */
length.
> +#define ABIT_UGURU3_MAX_NO_SENSORS 26
> +/* sum of strlen of: in??_input\0, in??_{min,max}\0, in??_{min,max}_alarm\0,
> + in??_{min,max}_alarm_enable\0, in??_beep\0, in??_shutdown\0, in??_label\0 */
> +#define ABIT_UGURU3_IN_NAMES_LENGTH (11 + 2 * 9 + 2 * 15 + 2 * 22 + 10 + 14 + 11)
strlen is the string length without the termination character \0. So
your numbers are off by one or the comment is incorrect.
> +/* sum of strlen of: temp??_input\0, temp??_max\0, temp??_crit\0,
> + temp??_alarm\0, temp??_alarm_enable\0, temp??_beep\0, temp??_shutdown\0,
> + temp??_label\0 */
> +#define ABIT_UGURU3_TEMP_NAMES_LENGTH (13 + 11 + 12 + 13 + 20 + 12 + 16 + 13)
Ditto. And there's never 10 or more temp sensors so the number could
be further reduced by one (ok nit-picking now...).
> +/* sum of strlen of: fan??_input\0, fan??_min\0, fan??_alarm\0,
> + fan??_alarm_enable\0, fan??_beep\0, fan??_shutdown\0, fan??_label\0 */
> +#define ABIT_UGURU3_FAN_NAMES_LENGTH (12 + 10 + 12 + 19 + 11 + 15 + 12)
Ditto.
> +/* Worst case scenario 16 in sensors (longest names_lenght) and the rest
> + temp sensors (second longest names_lenght). */
length.
> +#define ABIT_UGURU3_SYSFS_NAMES_LENGTH (16 * ABIT_UGURU3_IN_NAMES_LENGTH + \
> + (ABIT_UGURU3_MAX_NO_SENSORS - 16) * ABIT_UGURU3_TEMP_NAMES_LENGTH)
> +
> +/* All the macros below are named identical to the openguru2 program
> + reverse engineered by Louis Kruger, hence the names might not be 100%
> + logical. I could come up with better names, but I prefer keeping the names
> + identical so that this driver can be compared with his work more easily. */
> +/* Two i/o-ports are used by uGuru */
> +#define ABIT_UGURU3_BASE 0x00E0
> +#define ABIT_UGURU3_CMD 0x00
> +#define ABIT_UGURU3_DATA 0x04
> +#define ABIT_UGURU3_REGION_LENGTH 5
> +/* The wait_xxx functions return this on success and the last contents
> + of the DATA register (0-255) on failure. */
> +#define ABIT_UGURU3_SUCCESS -1
> +/* uGuru status flags */
> +#define ABIT_UGURU3_STATUS_READY_FOR_READ 0x01
> +#define ABIT_UGURU3_STATUS_BUSY 0x02
> +
> +
> +/* Structures */
> +struct abituguru3_sensor_info {
> + const char* name;
> + int port;
> + int type;
> + int multiplier;
> + int divisor;
> + int offset;
> +};
> +
> +struct abituguru3_motherboard_info {
> + u16 id;
> + const char *name;
> + /* + 1 -> end of sensors indicated by a sensor with name = NULL */
> + struct abituguru3_sensor_info sensors[ABIT_UGURU3_MAX_NO_SENSORS + 1];
> +};
> +
> +/* For the Abit uGuru, we need to keep some data in memory.
> + The structure is dynamically allocated, at the same time when a new
> + abituguru3 device is allocated. */
> +struct abituguru3_data {
> + struct class_device *class_dev; /* hwmon registered device */
> + struct mutex update_lock; /* protect access to data and uGuru */
> + unsigned short addr; /* uguru base address */
> + char valid; /* !=0 if following fields are valid */
> + unsigned long last_updated; /* In jiffies */
> +
> + /* For convenience the sysfs attr and their names are generated
> + automatically. We have max 10 entries per sensor (for in sensors) */
Here you claim a max of 10 in sensors but further up you use 16 for
the worst case in ABIT_UGURU3_SYSFS_NAMES_LENGTH.
> + struct sensor_device_attribute_2 sysfs_attr[ABIT_UGURU3_MAX_NO_SENSORS
> + * 10];
Add blank line for better readability.
> + /* Buffer to store the dynamically generated sysfs names */
> + char sysfs_names[ABIT_UGURU3_SYSFS_NAMES_LENGTH];
Add blank line for better readability.
> + /* Pointer to the sensors info for the detected motherboard */
> + const struct abituguru3_sensor_info *sensors;
> +
> + /* Alarms for all 48 sensors (48/8 = 6 bytes) */
> + u8 alarms[48/8];
Why / 8? One bit per sensor?
> +
> + /* Value of all 48 sensors */
> + u8 value[48];
> +
> + /* Settings of all 48 sensors, note in and temp sensors have 3 byte
> + of settings, while fans only have 2 bytes, for convenience we
> + use 3 bytes for all sensors */
> + u8 settings[48][3];
Where does the number 48 come from? I thought it was:
#define ABIT_UGURU3_MAX_NO_SENSORS 26
> +};
> +
> +
> +/* Constants */
> +static const struct abituguru3_motherboard_info abituguru3_motherboards[] = {
> + { 0x000C, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS FAN", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 35, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x000D, "Abit AW8", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM1", 26, 1, 1, 1, 0 },
> + { "PWM2", 27, 1, 1, 1, 0 },
> + { "PWM3", 28, 1, 1, 1, 0 },
> + { "PWM4", 29, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 35, 2, 60, 1, 0 },
> + { "AUX2 Fan", 36, 2, 60, 1, 0 },
> + { "AUX3 Fan", 37, 2, 60, 1, 0 },
> + { "AUX4 Fan", 38, 2, 60, 1, 0 },
> + { "AUX5 Fan", 39, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x000E, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x000F, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0010, "Abit NI8 SLI GR", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "NB 1.4V", 4, 0, 10, 1, 0 },
> + { "SB 1.5V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "SYS", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 35, 2, 60, 1, 0 },
> + { "OTES1 Fan", 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0011, "Abit AT8 32X", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 20, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VDDA 2.5V", 6, 0, 20, 1, 0 },
> + { "NB 1.8V", 4, 0, 10, 1, 0 },
> + { "NB 1.8V Dual", 5, 0, 10, 1, 0 },
> + { "HTV 1.2", 3, 0, 10, 1, 0 },
> + { "PCIE 1.2V", 12, 0, 10, 1, 0 },
> + { "NB 1.2V", 13, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "NB", 25, 1, 1, 1, 0 },
> + { "System", 26, 1, 1, 1, 0 },
> + { "PWM", 27, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 35, 2, 60, 1, 0 },
> + { "AUX2 Fan", 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0012, "Abit AN8 32X", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 20, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "HyperTransport", 3, 0, 10, 1, 0 },
> + { "CPU VDDA 2.5V", 5, 0, 20, 1, 0 },
> + { "NB", 4, 0, 10, 1, 0 },
> + { "SB", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "SYS", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0013, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM1", 26, 1, 1, 1, 0 },
> + { "PWM2", 27, 1, 1, 1, 0 },
> + { "PWM3", 28, 1, 1, 1, 0 },
> + { "PWM4", 29, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 35, 2, 60, 1, 0 },
> + { "AUX2 Fan", 36, 2, 60, 1, 0 },
> + { "AUX3 Fan", 37, 2, 60, 1, 0 },
> + { "AUX4 Fan", 38, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0014, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 10, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V", 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V", 4, 0, 10, 1, 0 },
> + { "MCH 2.5V", 5, 0, 20, 1, 0 },
> + { "ICH 1.05V", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "System ", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0015, "unknown", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR", 1, 0, 20, 1, 0 },
> + { "DDR VTT", 2, 0, 10, 1, 0 },
> + { "HyperTransport", 3, 0, 10, 1, 0 },
> + { "CPU VDDA 2.5V", 5, 0, 20, 1, 0 },
> + { "NB", 4, 0, 10, 1, 0 },
> + { "SB", 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)", 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)", 8, 0, 60, 1, 0 },
> + { "ATX +5V", 9, 0, 30, 1, 0 },
> + { "+3.3V", 10, 0, 20, 1, 0 },
> + { "5VSB", 11, 0, 30, 1, 0 },
> + { "CPU", 24, 1, 1, 1, 0 },
> + { "SYS", 25, 1, 1, 1, 0 },
> + { "PWM", 26, 1, 1, 1, 0 },
> + { "CPU Fan", 32, 2, 60, 1, 0 },
> + { "NB Fan", 33, 2, 60, 1, 0 },
> + { "SYS Fan", 34, 2, 60, 1, 0 },
> + { "AUX1 Fan", 33, 2, 60, 1, 0 },
> + { "AUX2 Fan", 35, 2, 60, 1, 0 },
> + { "AUX3 Fan", 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
> + },
> + { 0x0016, "AW9D-MAX", {
> + { "CPU Core", 0, 0, 10, 1, 0 },
> + { "DDR2" , 1, 0, 20, 1, 0 },
> + { "DDR2 VTT" , 2, 0, 10, 1, 0 },
> + { "CPU VTT 1.2V" , 3, 0, 10, 1, 0 },
> + { "MCH & PCIE 1.5V" , 4, 0, 10, 1, 0 },
> + { "MCH 2.5V" , 5, 0, 20, 1, 0 },
> + { "ICH 1.05V" , 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)" , 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)" , 8, 0, 60, 1, 0 },
> + { "ATX +5V" , 9, 0, 30, 1, 0 },
> + { "+3.3V" , 10, 0, 20, 1, 0 },
> + { "5VSB" , 11, 0, 30, 1, 0 },
> + { "CPU" , 24, 1, 1, 1, 0 },
> + { "System " , 25, 1, 1, 1, 0 },
> + { "PWM1" , 26, 1, 1, 1, 0 },
> + { "PWM2" , 27, 1, 1, 1, 0 },
> + { "PWM3" , 28, 1, 1, 1, 0 },
> + { "PWM4" , 29, 1, 1, 1, 0 },
> + { "CPU Fan" , 32, 2, 60, 1, 0 },
> + { "NB Fan" , 33, 2, 60, 1, 0 },
> + { "SYS Fan" , 34, 2, 60, 1, 0 },
> + { "AUX1 Fan" , 35, 2, 60, 1, 0 },
> + { "AUX2 Fan" , 36, 2, 60, 1, 0 },
> + { "AUX3 Fan" , 37, 2, 60, 1, 0 },
> + { "OTES1 Fan" , 38, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
Remove space between strings and commas.
> + },
> + { 0x0017, "unknown", {
> + { "CPU Core" , 0, 0, 10, 1, 0 },
> + { "DDR2" , 1, 0, 20, 1, 0 },
> + { "DDR2 VTT" , 2, 0, 10, 1, 0 },
> + { "HyperTransport" , 3, 0, 10, 1, 0 },
> + { "CPU VDDA 2.5V" , 6, 0, 20, 1, 0 },
> + { "NB 1.8V" , 4, 0, 10, 1, 0 },
> + { "NB 1.2V " , 13, 0, 10, 1, 0 },
> + { "SB 1.2V" , 5, 0, 10, 1, 0 },
> + { "PCIE 1.2V" , 12, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)" , 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)" , 8, 0, 60, 1, 0 },
> + { "ATX +5V" , 9, 0, 30, 1, 0 },
> + { "ATX +3.3V" , 10, 0, 20, 1, 0 },
> + { "ATX 5VSB" , 11, 0, 30, 1, 0 },
> + { "CPU" , 24, 1, 1, 1, 0 },
> + { "System " , 26, 1, 1, 1, 0 },
> + { "PWM" , 27, 1, 1, 1, 0 },
> + { "CPU FAN" , 32, 2, 60, 1, 0 },
> + { "SYS FAN" , 34, 2, 60, 1, 0 },
> + { "AUX1 FAN" , 35, 2, 60, 1, 0 },
> + { "AUX2 FAN" , 36, 2, 60, 1, 0 },
> + { "AUX3 FAN" , 37, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
Remove space between strings and commas.
> + },
> + { 0x0018, "unknown", {
> + { "CPU Core" , 0, 0, 10, 1, 0 },
> + { "DDR2" , 1, 0, 20, 1, 0 },
> + { "DDR2 VTT" , 2, 0, 10, 1, 0 },
> + { "CPU VTT" , 3, 0, 10, 1, 0 },
> + { "MCH 1.25V" , 4, 0, 10, 1, 0 },
> + { "ICHIO 1.5V" , 5, 0, 10, 1, 0 },
> + { "ICH 1.05V" , 6, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)" , 7, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)" , 8, 0, 60, 1, 0 },
> + { "ATX +5V" , 9, 0, 30, 1, 0 },
> + { "+3.3V" , 10, 0, 20, 1, 0 },
> + { "5VSB" , 11, 0, 30, 1, 0 },
> + { "CPU" , 24, 1, 1, 1, 0 },
> + { "System " , 25, 1, 1, 1, 0 },
> + { "PWM Phase1" , 26, 1, 1, 1, 0 },
> + { "PWM Phase2" , 27, 1, 1, 1, 0 },
> + { "PWM Phase3" , 28, 1, 1, 1, 0 },
> + { "PWM Phase4" , 29, 1, 1, 1, 0 },
> + { "PWM Phase5" , 30, 1, 1, 1, 0 },
> + { "CPU Fan" , 32, 2, 60, 1, 0 },
> + { "SYS Fan" , 34, 2, 60, 1, 0 },
> + { "AUX1 Fan" , 33, 2, 60, 1, 0 },
> + { "AUX2 Fan" , 35, 2, 60, 1, 0 },
> + { "AUX3 Fan" , 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
Remove space between strings and commas.
> + },
> + { 0x0019, "unknown", {
> + { "CPU Core" , 7, 0, 10, 1, 0 },
> + { "DDR2" , 13, 0, 20, 1, 0 },
> + { "DDR2 VTT" , 14, 0, 10, 1, 0 },
> + { "CPU VTT" , 3, 0, 20, 1, 0 },
> + { "NB 1.2V " , 4, 0, 10, 1, 0 },
> + { "SB 1.5V" , 6, 0, 10, 1, 0 },
> + { "HyperTransport" , 5, 0, 10, 1, 0 },
> + { "ATX +12V (24-Pin)" , 12, 0, 60, 1, 0 },
> + { "ATX +12V (4-pin)" , 8, 0, 60, 1, 0 },
> + { "ATX +5V" , 9, 0, 30, 1, 0 },
> + { "ATX +3.3V" , 10, 0, 20, 1, 0 },
> + { "ATX 5VSB" , 11, 0, 30, 1, 0 },
> + { "CPU" , 24, 1, 1, 1, 0 },
> + { "System " , 25, 1, 1, 1, 0 },
> + { "PWM Phase1" , 26, 1, 1, 1, 0 },
> + { "PWM Phase2" , 27, 1, 1, 1, 0 },
> + { "PWM Phase3" , 28, 1, 1, 1, 0 },
> + { "PWM Phase4" , 29, 1, 1, 1, 0 },
> + { "PWM Phase5" , 30, 1, 1, 1, 0 },
> + { "CPU FAN" , 32, 2, 60, 1, 0 },
> + { "SYS FAN" , 34, 2, 60, 1, 0 },
> + { "AUX1 FAN" , 33, 2, 60, 1, 0 },
> + { "AUX2 FAN" , 35, 2, 60, 1, 0 },
> + { "AUX3 FAN" , 36, 2, 60, 1, 0 },
> + { NULL, 0, 0, 0, 0, 0 } }
Remove space between strings and commas.
Aligning the numbers helps with reviewing them. With, that, I noticed
that the last two columns (divisor, offset) contain always the same
values. Why even put them in there?
Also, here you use hard-coded values for the sensor types but further
up you have macros defined for them (ABIT_UGURU3_IN_SENSOR,
ABIT_UGURU3_TEMP_SENSOR, ABIT_UGURU3_FAN_SENSOR). You could get out of
sysnc.
Aahhh... covered a lot of ground with the table :-)
> + },
> + { 0x0000, NULL, { { NULL, 0, 0, 0, 0, 0 } } }
> +};
> +
> +
> +/* Insmod parameters */
> +static int force;
No initialization required? Is gcc smart enough to init to 0?
> +module_param(force, bool, 0);
> +MODULE_PARM_DESC(force, "Set to one to force detection.");
> +/* Default verbose is 1, since this driver is still in the testing phase */
> +static int verbose = 1;
> +module_param(verbose, bool, 0644);
> +MODULE_PARM_DESC(verbose, "Enable/disable verbose error reporting");
Comment is inaccurate since the value could be 0-3 (according to the
driver doc).
> +
> +
> +/* wait while the uguru is busy (usually after a write) */
> +static int abituguru3_wait_while_busy(struct abituguru3_data *data)
> +{
> + u8 x;
> + int timeout = ABIT_UGURU3_WAIT_TIMEOUT;
> +
> + while ((x = inb_p(data->addr + ABIT_UGURU3_DATA)) &
> + ABIT_UGURU3_STATUS_BUSY)
> + {
{ misplaced.
> + timeout--;
> + if (timeout = 0)
> + return x;
> + /* sleep a bit before our last try, to give the uGuru3 one
> + last chance to respond. */
> + if (timeout = 1)
> + msleep(1);
> + }
> + return ABIT_UGURU3_SUCCESS;
> +}
> +
> +/* wait till uguru is ready to be read */
> +static int abituguru3_wait_for_read(struct abituguru3_data *data)
> +{
> + u8 x;
> + int timeout = ABIT_UGURU3_WAIT_TIMEOUT;
> +
> + while (!((x = inb_p(data->addr + ABIT_UGURU3_DATA)) &
> + ABIT_UGURU3_STATUS_READY_FOR_READ))
> + {
{ misplaced.
> + timeout--;
> + if (timeout = 0)
> + return x;
> + /* sleep a bit before our last try, to give the uGuru3 one
> + last chance to respond. */
> + if (timeout = 1)
> + msleep(1);
> + }
> + return ABIT_UGURU3_SUCCESS;
> +}
> +
> +/* This synchronizes us with the uGuru3's protocol state machine, this
> + must be done before each command. */
> +static int abituguru3_synchronize(struct abituguru3_data *data)
> +{
> + int x, timeout = ABIT_UGURU3_SYNCHRONIZE_TIMEOUT;
> +
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("synchronize timeout during initial busy "
> + "wait, status: 0x%02x\n", x);
> + return -EIO;
> + }
> +
> + outb(0x20, data->addr + ABIT_UGURU3_DATA);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("synchronize timeout after sending 0x20, "
> + "status: 0x%02x\n", x);
> + return -EIO;
> + }
> +
> + outb(0x10, data->addr + ABIT_UGURU3_CMD);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("synchronize timeout after sending 0x10, "
> + "status: 0x%02x\n", x);
> + return -EIO;
> + }
> +
> + outb(0x00, data->addr + ABIT_UGURU3_CMD);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("synchronize timeout after sending 0x00, "
> + "status: 0x%02x\n", x);
> + return -EIO;
> + }
> +
> + if ((x = abituguru3_wait_for_read(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("synchronize timeout waiting for read, "
> + "status: 0x%02x\n", x);
> + return -EIO;
> + }
> +
> + while ((x = inb(data->addr + ABIT_UGURU3_CMD)) != 0xAC) {
> + timeout--;
> + if (timeout = 0) {
> + ABIT_UGURU3_DEBUG("synchronize timeout cmd does not "
> + "hold 0xAC after synchronize, cmd: 0x%02x\n",
> + x);
> + return -EIO;
> + }
> + msleep(1);
> + }
> + return 0;
> +}
> +
> +/* Read count bytes from sensor sensor_addr in bank bank_addr and store the
> + result in buf */
> +static int abituguru3_read(struct abituguru3_data *data, u8 bank, u8 offset,
> + u8 count, u8 *buf)
> +{
> + int i, x;
> +
> + if ((x = abituguru3_synchronize(data)))
> + return x;
> +
> + outb(0x1A, data->addr + ABIT_UGURU3_DATA);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("read from 0x%02x:0x%02x timed out after "
> + "sending 0x1A, status: 0x%02x\n", (unsigned int)bank,
> + (unsigned int)offset, x);
> + return -EIO;
> + }
> +
> + outb(bank, data->addr + ABIT_UGURU3_CMD);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("read from 0x%02x:0x%02x timed out after "
> + "sending the bank, status: 0x%02x\n",
> + (unsigned int)bank, (unsigned int)offset, x);
> + return -EIO;
> + }
> +
> + outb(offset, data->addr + ABIT_UGURU3_CMD);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("read from 0x%02x:0x%02x timed out after "
> + "sending the offset, status: 0x%02x\n",
> + (unsigned int)bank, (unsigned int)offset, x);
> + return -EIO;
> + }
> +
> + outb(count, data->addr + ABIT_UGURU3_CMD);
> + if ((x = abituguru3_wait_while_busy(data)) != ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("read from 0x%02x:0x%02x timed out after "
> + "sending the count, status: 0x%02x\n",
> + (unsigned int)bank, (unsigned int)offset, x);
> + return -EIO;
> + }
> +
> + for (i=0; i<count; i++) {
Add spaces around = and <.
> + if ((x = abituguru3_wait_for_read(data)) !> + ABIT_UGURU3_SUCCESS) {
> + ABIT_UGURU3_DEBUG("timeout reading byte %d from "
> + "0x%02x:0x%02x, status: 0x%02x\n", i,
> + (unsigned int)bank, (unsigned int)offset, x);
> + break;
> + }
> + buf[i] = inb(data->addr + ABIT_UGURU3_CMD);
> + }
> + return i;
> +}
> +
> +/* Sensor settings are stored 1 byte per offset with the bytes
> + placed add consecutive offsets. */
at?
> +int abituguru3_read_increment_offset(struct abituguru3_data *data, u8 bank,
> + u8 offset, u8 count, u8 *buf, int offset_count)
> +{
> + int i, x;
> +
> + for (i = 0; i < offset_count; i++)
> + if ((x = abituguru3_read(data, bank, offset + i, count,
> + buf + i * count)) != count)
> + return i * count + (i && (x < 0))? 0:x;
Add spaces around colon and before ?.
Why are you and'ing with i? Why not just:
return i * count + (x < 0) ? 0 : x;
> +
> + return i * count;
> +}
> +
> +/* Following are the sysfs callback functions. These functions expect:
> + sensor_device_attribute_2->index: index into the data->sensors array
> + sensor_device_attribute_2->nr: register offset, bitmask or NA. */
> +static struct abituguru3_data *abituguru3_update_device(struct device *dev);
> +
> +static ssize_t show_value(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int value;
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + struct abituguru3_data *data = abituguru3_update_device(dev);
> + struct abituguru3_sensor_info sensor;
Add a blank line between variable declaration and code.
> + if (!data)
> + return -EIO;
Isn't this condition considered a bug? Don't you want to print some fancy text?
> + sensor = data->sensors[attr->index];
Wouldn't it be more efficient to use a pointer rather than copying the
whole thing? E.g.: struct abituguru3_sensors_info *sensor &data->sensors[attr->index];
> + /* are we reading a setting, or is this a normal read? */
> + if (attr->nr)
> + value = data->settings[sensor.port][attr->nr];
> + else
> + value = data->value[sensor.port];
> + /* convert the value */
> + value = (value * sensor.multiplier) / sensor.divisor + sensor.offset;
> + /* alternatively we could update the sensors settings struct for this,
> + but then its contents would differ from the windows sw ini files */
> + if (sensor.type = ABIT_UGURU3_TEMP_SENSOR)
> + value *= 1000;
> + return sprintf(buf, "%d\n", value);
> +}
Some more blank lines would make it easier to read.
> +
> +static ssize_t show_alarm(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int port;
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + struct abituguru3_data *data = abituguru3_update_device(dev);
Add blank line.
> + if (!data)
> + return -EIO;
> + port = data->sensors[attr->index].port;
> + /* See if the alarm bit for this sensor is set and if a bitmask is
> + given in attr->nr also check if the alarm matches the type of alarm
> + we're looking for (for volt it can be either low or high). The type
> + is stored in a few readonly bits in the settings of the sensor. */
> + if ((data->alarms[port / 8] & (0x01 << (port % 8))) &&
> + (!attr->nr || (data->settings[port][0] & attr->nr)))
> + return sprintf(buf, "1\n");
> + else
> + return sprintf(buf, "0\n");
How about
return sprintf(buf, "%d\n",
(data->alarms[port >> 3] & (0x01 << (port % 8))) &&
(!attr->nr || (data->settings[port][0] & attr->nr));
Again, some blank lines would help.
> +}
> +
> +static ssize_t show_mask(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + struct abituguru3_data *data = dev_get_drvdata(dev);
Blank line.
> + if (data->settings[data->sensors[attr->index].port][0] & attr->nr)
> + return sprintf(buf, "1\n");
> + else
> + return sprintf(buf, "0\n");
return sprintf(buf, "%d\n",
data->settings[data->sensors[attr->index].port][0] & attr->nr ? 1 : 0);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + struct abituguru3_data *data = dev_get_drvdata(dev);
Blank line.
> + return sprintf(buf, "%s\n", data->sensors[attr->index].name);
> +}
> +
> +static ssize_t show_name(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + return sprintf(buf, "%s\n", ABIT_UGURU3_NAME);
> +}
> +
> +/* Sysfs attr templates, the real entries are generated automatically. */
> +static const
> +struct sensor_device_attribute_2 abituguru3_sysfs_templ[3][10] = { {
> + SENSOR_ATTR_2(in%d_input, 0444, show_value, NULL, 0, 0),
> + SENSOR_ATTR_2(in%d_min, 0444, show_value, NULL, 1, 0),
> + SENSOR_ATTR_2(in%d_max, 0444, show_value, NULL, 2, 0),
> + SENSOR_ATTR_2(in%d_min_alarm, 0444, show_alarm, NULL,
> + ABIT_UGURU3_VOLT_LOW_ALARM_FLAG, 0),
> + SENSOR_ATTR_2(in%d_max_alarm, 0444, show_alarm, NULL,
> + ABIT_UGURU3_VOLT_HIGH_ALARM_FLAG, 0),
> + SENSOR_ATTR_2(in%d_beep, 0444, show_mask, NULL,
> + ABIT_UGURU3_BEEP_ENABLE, 0),
> + SENSOR_ATTR_2(in%d_shutdown, 0444, show_mask, NULL,
> + ABIT_UGURU3_SHUTDOWN_ENABLE, 0),
> + SENSOR_ATTR_2(in%d_min_alarm_enable, 0444, show_mask, NULL,
> + ABIT_UGURU3_VOLT_LOW_ALARM_ENABLE, 0),
> + SENSOR_ATTR_2(in%d_max_alarm_enable, 0444, show_mask, NULL,
> + ABIT_UGURU3_VOLT_HIGH_ALARM_ENABLE, 0),
> + SENSOR_ATTR_2(in%d_label, 0444, show_label, NULL, 0, 0)
> + }, {
> + SENSOR_ATTR_2(temp%d_input, 0444, show_value, NULL, 0, 0),
> + SENSOR_ATTR_2(temp%d_max, 0444, show_value, NULL, 1, 0),
> + SENSOR_ATTR_2(temp%d_crit, 0444, show_value, NULL, 2, 0),
> + SENSOR_ATTR_2(temp%d_alarm, 0444, show_alarm, NULL, 0, 0),
> + SENSOR_ATTR_2(temp%d_beep, 0444, show_mask, NULL,
> + ABIT_UGURU3_BEEP_ENABLE, 0),
> + SENSOR_ATTR_2(temp%d_shutdown, 0444, show_mask, NULL,
> + ABIT_UGURU3_SHUTDOWN_ENABLE, 0),
> + SENSOR_ATTR_2(temp%d_alarm_enable, 0444, show_mask, NULL,
> + ABIT_UGURU3_TEMP_HIGH_ALARM_ENABLE, 0),
> + SENSOR_ATTR_2(temp%d_label, 0444, show_label, NULL, 0, 0)
> + }, {
> + SENSOR_ATTR_2(fan%d_input, 0444, show_value, NULL, 0, 0),
> + SENSOR_ATTR_2(fan%d_min, 0444, show_value, NULL, 1, 0),
> + SENSOR_ATTR_2(fan%d_alarm, 0444, show_alarm, NULL, 0, 0),
> + SENSOR_ATTR_2(fan%d_beep, 0444, show_mask, NULL,
> + ABIT_UGURU3_BEEP_ENABLE, 0),
> + SENSOR_ATTR_2(fan%d_shutdown, 0444, show_mask, NULL,
> + ABIT_UGURU3_SHUTDOWN_ENABLE, 0),
> + SENSOR_ATTR_2(fan%d_alarm_enable, 0444, show_mask, NULL,
> + ABIT_UGURU3_FAN_LOW_ALARM_ENABLE, 0),
> + SENSOR_ATTR_2(fan%d_label, 0444, show_label, NULL, 0, 0)
> +} };
> +
> +static struct sensor_device_attribute_2 abituguru3_sysfs_attr[] = {
> + SENSOR_ATTR_2(name, 0444, show_name, NULL, 0, 0),
> +};
> +
> +static int __devinit abituguru3_probe(struct platform_device *pdev)
> +{
> + const int no_sysfs_attr[3] = { 10, 8, 7 };
> + int sensor_index[3] = { 0, 1, 1 };
> + struct abituguru3_data *data;
> + int i, j, type, used, sysfs_names_free, sysfs_attr_i, res = -ENODEV;
> + char *sysfs_filename;
> + u8 buf[2];
> + u16 id;
> +
> + if (!(data = kzalloc(sizeof(struct abituguru3_data), GFP_KERNEL)))
> + return -ENOMEM;
> +
> + data->addr = platform_get_resource(pdev, IORESOURCE_IO, 0)->start;
> + mutex_init(&data->update_lock);
> + platform_set_drvdata(pdev, data);
> +
> + /* Read the motherboard ID */
> + if (( i = abituguru3_read(data, ABIT_UGURU3_MISC_BANK,
No space before i.
> + ABIT_UGURU3_BOARD_ID, 2, buf)) != 2) {
> + goto abituguru3_probe_error;
> + }
> +
> + /* Completely read the uGuru to see if one really is there */
> + if (!abituguru3_update_device(&pdev->dev))
> + goto abituguru3_probe_error;
> +
> + /* lookup the ID in our motherboard table */
> + id = ((u16)buf[0] << 8) | (u16)buf[1];
> + for (i = 0; abituguru3_motherboards[i].id; i++)
> + if (abituguru3_motherboards[i].id = id)
> + break;
I'm not a big fan of omitting curly braces for single statements. I
think it's more confusing than anything but it's up to you...
> + if (!abituguru3_motherboards[i].id) {
> + printk(KERN_ERR ABIT_UGURU3_NAME ": error unknown motherboard "
> + "ID: %04X. Please report this to the abituguru3 "
> + "maintainer (see MAINTAINERS)\n", (unsigned int)id);
> + goto abituguru3_probe_error;
> + }
> + data->sensors = abituguru3_motherboards[i].sensors;
> + printk(KERN_INFO ABIT_UGURU3_NAME ": found Abit uGuru3, motherboard "
> + "ID: %04X (%s)\n", (unsigned int)id,
> + abituguru3_motherboards[i].name);
> +
> + /* Fill the sysfs attr array */
> + sysfs_attr_i = 0;
> + sysfs_filename = data->sysfs_names;
> + sysfs_names_free = ABIT_UGURU3_SYSFS_NAMES_LENGTH;
> + for (i = 0; data->sensors[i].name; i++) {
> + type = data->sensors[i].type;
> + for (j = 0; j < no_sysfs_attr[type]; j++) {
> + used = snprintf(sysfs_filename, sysfs_names_free,
> + abituguru3_sysfs_templ[type][j].dev_attr.attr.
> + name, sensor_index[type]) + 1;
> + data->sysfs_attr[sysfs_attr_i] > + abituguru3_sysfs_templ[type][j];
> + data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name > + sysfs_filename;
If I understand this correctly you're concatenating all the sysfs
attribute names and copy them into the sysfs_filename buffer which is
a member of the device data structure. And then later you overwrite
the attr name (data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name) with
a pointer into this buffer.
You're effectively allocating memory twice for the same thing (once in
your device data struct and in the individual sysfs_attr structs).
Wouldn't it be easier and more efficient to somehow reserve enough
space for the final attribute name in the abituguru3_sysfs_templ
struct and then snprintf the name into
data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name rather then
overwriting the string pointer?
> + data->sysfs_attr[sysfs_attr_i].index = i;
> + sysfs_filename += used;
> + sysfs_names_free -= used;
> + sysfs_attr_i++;
> + }
> + sensor_index[type]++;
> + }
> + /* Fail safe check, this should never happen! */
> + if (sysfs_names_free < 0) {
> + printk(KERN_ERR ABIT_UGURU3_NAME
> + ": Fatal error ran out of space for sysfs attr names. This should never happen please report to the abituguru3 maintainer (see MAINTAINERS)\n");
Line too long.
> + res = -ENAMETOOLONG;
> + goto abituguru3_probe_error;
> + }
> +
> + /* Register sysfs hooks */
> + data->class_dev = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(data->class_dev)) {
> + res = PTR_ERR(data->class_dev);
> + goto abituguru3_probe_error;
> + }
> + for (i = 0; i < sysfs_attr_i; i++)
> + if (device_create_file(&pdev->dev,
> + &data->sysfs_attr[i].dev_attr))
> + goto abituguru3_probe_error;
> + for (i = 0; i < ARRAY_SIZE(abituguru3_sysfs_attr); i++)
> + if (device_create_file(&pdev->dev,
> + &abituguru3_sysfs_attr[i].dev_attr))
> + goto abituguru3_probe_error;
> +
> + data->class_dev = hwmon_device_register(&pdev->dev);
> + if (!IS_ERR(data->class_dev))
> + return 0; /* success */
> +
> + res = PTR_ERR(data->class_dev);
You're registering the device twice! and the check for success is
backwards from all the previous checks (i.e., check for error and
handle it vs. check for success and handle it). Drop the first
occurence (before creating the devices) and I'd prefer (but it's your
call - I'm nit-picking again :-):
if (IS_ERR(data->class_dev)) {
res = PTR_ERR(data->class_dev);
goto abituguru3_probe_error;
}
return 0;
> +abituguru3_probe_error:
> + for (i = 0; data->sysfs_attr[i].dev_attr.attr.name; i++)
> + device_remove_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
> + for (i = 0; i < ARRAY_SIZE(abituguru3_sysfs_attr); i++)
> + device_remove_file(&pdev->dev,
> + &abituguru3_sysfs_attr[i].dev_attr);
> + kfree(data);
> + return res;
> +}
> +
> +static int __devexit abituguru3_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct abituguru3_data *data = platform_get_drvdata(pdev);
> +
> + platform_set_drvdata(pdev, NULL);
Not sure what this does (is it freeing any memory?) but is it safe to
do it before unregistering and deleting devices?
> + hwmon_device_unregister(data->class_dev);
> + for (i = 0; data->sysfs_attr[i].dev_attr.attr.name; i++)
> + device_remove_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
> + for (i = 0; i < ARRAY_SIZE(abituguru3_sysfs_attr); i++)
> + device_remove_file(&pdev->dev,
> + &abituguru3_sysfs_attr[i].dev_attr);
> + kfree(data);
> +
> + return 0;
> +}
> +
> +static struct abituguru3_data *abituguru3_update_device(struct device *dev)
> +{
> + int i;
> + struct abituguru3_data *data = dev_get_drvdata(dev);
> +
> + mutex_lock(&data->update_lock);
> + if (!data->valid || time_after(jiffies, data->last_updated + HZ)) {
> + /* Clear data->valid while updating */
> + data->valid = 0;
> + /* Read alarms */
> + if (abituguru3_read_increment_offset(data,
> + ABIT_UGURU3_SETTINGS_BANK,
> + ABIT_UGURU3_ALARMS_START,
> + 1, data->alarms, 48/8) != (48/8))
> + goto LEAVE_UPDATE;
> + /* Read in and temp sensors (3 byte settings / sensor) */
> + for (i = 0; i < 32; i++) {
> + if (abituguru3_read(data, ABIT_UGURU3_SENSORS_BANK,
> + ABIT_UGURU3_VALUES_START + i,
> + 1, &data->value[i]) != 1)
> + goto LEAVE_UPDATE;
> + if (abituguru3_read_increment_offset(data,
> + ABIT_UGURU3_SETTINGS_BANK,
> + ABIT_UGURU3_SETTINGS_START + i * 3,
> + 1,
> + data->settings[i], 3) != 3)
> + goto LEAVE_UPDATE;
> + }
> + /* Read temp sensors (2 byte settings / sensor) */
> + for (i = 0; i < 16; i++) {
> + if (abituguru3_read(data, ABIT_UGURU3_SENSORS_BANK,
> + ABIT_UGURU3_VALUES_START + 32 + i,
> + 1, &data->value[32 + i]) != 1)
> + goto LEAVE_UPDATE;
> + if (abituguru3_read_increment_offset(data,
> + ABIT_UGURU3_SETTINGS_BANK,
> + ABIT_UGURU3_SETTINGS_START + 32 * 3 +
> + i * 2, 1,
> + data->settings[32 + i], 2) != 2)
> + goto LEAVE_UPDATE;
> + }
> + data->last_updated = jiffies;
> + data->valid = 1;
> + }
> +LEAVE_UPDATE:
> + mutex_unlock(&data->update_lock);
> + if (data->valid)
> + return data;
> + else
> + return NULL;
How about:
return data->valid ? data : NULL;
> +}
> +
> +#ifdef CONFIG_PM
> +static int abituguru3_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct abituguru3_data *data = platform_get_drvdata(pdev);
> + /* make sure all communications with the uguru3 are done and no new
> + ones are started */
> + mutex_lock(&data->update_lock);
> + return 0;
> +}
> +
> +static int abituguru3_resume(struct platform_device *pdev)
> +{
> + struct abituguru3_data *data = platform_get_drvdata(pdev);
> + mutex_unlock(&data->update_lock);
> + return 0;
> +}
> +#else
> +#define abituguru3_suspend NULL
> +#define abituguru3_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct platform_driver abituguru3_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = ABIT_UGURU3_NAME,
> + },
> + .probe = abituguru3_probe,
> + .remove = __devexit_p(abituguru3_remove),
> + .suspend = abituguru3_suspend,
> + .resume = abituguru3_resume
> +};
> +
> +static int __init abituguru3_detect(void)
> +{
> + /* See if there is an uguru3 there. An idle uGuru3 will hold 0x00
> + at DATA and 0xAC at CMD. Sometimes the uGuru3 will hold 0x05 at
> + CMD instead, why is unknown. So we test for 0x05 too. */
> + u8 data_val = inb_p(ABIT_UGURU3_BASE + ABIT_UGURU3_DATA);
> + u8 cmd_val = inb_p(ABIT_UGURU3_BASE + ABIT_UGURU3_CMD);
> + if ((data_val = 0x00) && ((cmd_val = 0xAC) || (cmd_val = 0x05)))
> + return ABIT_UGURU3_BASE;
> +
> + ABIT_UGURU3_DEBUG("no Abit uGuru3 found, data = 0x%02X, cmd = "
> + "0x%02X\n", (unsigned int)data_val, (unsigned int)cmd_val);
> +
> + if (force) {
> + printk(KERN_INFO ABIT_UGURU3_NAME ": Assuming Abit uGuru3 is "
> + "present because of \"force\" parameter\n");
> + return ABIT_UGURU3_BASE;
> + }
> +
> + /* No uGuru3 found */
> + return -ENODEV;
> +}
> +
> +static struct platform_device *abituguru3_pdev;
> +
> +static int __init abituguru3_init(void)
> +{
> + int address, err;
> + struct resource res = { .flags = IORESOURCE_IO };
> +
> + address = abituguru3_detect();
> + if (address < 0)
> + return address;
> +
> + err = platform_driver_register(&abituguru3_driver);
> + if (err)
> + goto exit;
> +
> + abituguru3_pdev = platform_device_alloc(ABIT_UGURU3_NAME, address);
> + if (!abituguru3_pdev) {
> + printk(KERN_ERR ABIT_UGURU3_NAME
> + ": Device allocation failed\n");
> + err = -ENOMEM;
> + goto exit_driver_unregister;
> + }
> +
> + res.start = address;
> + res.end = address + ABIT_UGURU3_REGION_LENGTH - 1;
> + res.name = ABIT_UGURU3_NAME;
> +
> + err = platform_device_add_resources(abituguru3_pdev, &res, 1);
> + if (err) {
> + printk(KERN_ERR ABIT_UGURU3_NAME
> + ": Device resource addition failed (%d)\n", err);
> + goto exit_device_put;
> + }
> +
> + err = platform_device_add(abituguru3_pdev);
> + if (err) {
> + printk(KERN_ERR ABIT_UGURU3_NAME
> + ": Device addition failed (%d)\n", err);
> + goto exit_device_put;
> + }
> +
> + return 0;
> +
> +exit_device_put:
> + platform_device_put(abituguru3_pdev);
> +exit_driver_unregister:
> + platform_driver_unregister(&abituguru3_driver);
> +exit:
> + return err;
> +}
> +
> +static void __exit abituguru3_exit(void)
> +{
> + platform_device_unregister(abituguru3_pdev);
> + platform_driver_unregister(&abituguru3_driver);
> +}
> +
> +MODULE_AUTHOR("Hans de Goede <j.w.r.degoede at hhs.nl>");
> +MODULE_DESCRIPTION("Abit uGuru3 Sensor device");
> +MODULE_LICENSE("GPL");
> +
> +module_init(abituguru3_init);
> +module_exit(abituguru3_exit);
> --- linux-2.6.20-rc4-mm1/drivers/hwmon/Makefile.uguru3 2007-01-18 16:04:38.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/drivers/hwmon/Makefile 2007-01-18 16:05:28.000000000 +0100
> @@ -14,6 +14,7 @@
> obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
>
> obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
> +obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
> obj-$(CONFIG_SENSORS_ADM1021) += adm1021.o
> obj-$(CONFIG_SENSORS_ADM1025) += adm1025.o
> obj-$(CONFIG_SENSORS_ADM1026) += adm1026.o
> --- linux-2.6.20-rc4-mm1/drivers/hwmon/Kconfig.uguru3 2007-01-18 16:05:40.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/drivers/hwmon/Kconfig 2007-01-18 16:14:21.000000000 +0100
> @@ -28,17 +28,34 @@
> default n
>
> config SENSORS_ABITUGURU
> - tristate "Abit uGuru"
> + tristate "Abit uGuru (rev 1 & 2)"
> depends on HWMON && EXPERIMENTAL
> help
> - If you say yes here you get support for the Abit uGuru chips
> - sensor part. The voltage and frequency control parts of the Abit
> - uGuru are not supported. The Abit uGuru chip can be found on Abit
> - uGuru featuring motherboards (most modern Abit motherboards).
> + If you say yes here you get support for the sensor part of the first
> + and second revision of the Abit uGuru chip. The voltage and frequency
> + control parts of the Abit uGuru are not supported. The Abit uGuru
> + chip can be found on Abit uGuru featuring motherboards (most modern
> + Abit motherboards from before end 2005). For more info and a list
> + of which motherboards have which revision see
> + Documentation/hwmon/abituguru3
>
> This driver can also be built as a module. If so, the module
> will be called abituguru.
>
> +config SENSORS_ABITUGURU3
> + tristate "Abit uGuru (rev 3)"
> + depends on HWMON && EXPERIMENTAL
> + help
> + If you say yes here you get support for the sensor part of the
> + third revision of the Abit uGuru chip. Only reading the sensors
> + and their settings is supported. The third revision of the Abit
> + uGuru chip can be found on recent Abit motherboards (since end
> + 2005). For more info and a list of which motherboards have which
> + revision see Documentation/hwmon/abituguru3
> +
> + This driver can also be built as a module. If so, the module
> + will be called abituguru3.
> +
> config SENSORS_ADM1021
> tristate "Analog Devices ADM1021 and compatibles"
> depends on HWMON && I2C
> --- linux-2.6.20-rc4-mm1/drivers/hwmon/abituguru.c.uguru3 2007-01-18 16:18:56.000000000 +0100
> +++ linux-2.6.20-rc4-mm1/drivers/hwmon/abituguru.c 2007-01-18 16:20:10.000000000 +0100
> @@ -16,9 +16,9 @@
> Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
> /*
> - This driver supports the sensor part of the custom Abit uGuru chip found
> - on Abit uGuru motherboards. Note: because of lack of specs the CPU / RAM /
> - etc voltage & frequency control is not supported!
> + This driver supports the sensor part of the first and second revision of
> + the custom Abit uGuru chip found on Abit uGuru motherboards. Note: because
> + of lack of specs the CPU/RAM voltage & frequency control is not supported!
> */
> #include <linux/module.h>
> #include <linux/sched.h>
Done. Good job. Looks very clean.
Regards
...juerg
^ permalink raw reply [flat|nested] 4+ messages in thread* [lm-sensors] abituguru3 review
2007-03-19 21:28 [lm-sensors] abituguru3 review Juerg Haefliger
@ 2007-03-20 14:29 ` Rick Wright
2007-03-21 22:10 ` Hans de Goede
2007-03-24 20:07 ` Juerg Haefliger
2 siblings, 0 replies; 4+ messages in thread
From: Rick Wright @ 2007-03-20 14:29 UTC (permalink / raw)
To: lm-sensors
Juerg Haefliger wrote:
>Hans,
>
>Here's my review:
>
>
>
>>This patch adds a new driver for the hardware monitoring features of the third
>>revision of the Abit uGuru chip, found on recent Abit motherboards. This is an
>>entirely different beast then the first and second revision (its again a
>>winbond microcontroller, but the "protocol" to talk to it and the bank
>>addresses are very different.
>>
>>Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>
>>--- linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru.uguru3 2007-01-18 13:45:20.000000000 +0100
>>+++ linux-2.6.20-rc4-mm1/Documentation/hwmon/abituguru 2007-01-18 16:15:24.000000000 +0100
>>@@ -2,7 +2,7 @@
>> ===========>>
>> Supported chips:
>>- * Abit uGuru revision 1-3 (Hardware Monitor part only)
>>+ * Abit uGuru revision 1 & 2 (Hardware Monitor part only)
>> Prefix: 'abituguru'
>> Addresses scanned: ISA 0x0E0
>> Datasheet: Not available, this driver is based on reverse engineering.
>>@@ -20,8 +20,8 @@
>> uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
>> uGuru 2.2.0.0 ~ 2.2.0.6 (AA8 Fatal1ty)
>> uGuru 2.3.0.0 ~ 2.3.0.9 (AN8)
>>- uGuru 3.0.0.0 ~ 3.0.1.2 (AW8, AL8, NI8)
>>- uGuru 4.xxxxx? (AT8 32X) (2)
>>+ uGuru 3.0.0.0 ~ 3.0.x.x (AW8, AL8, NI8 SLI, AT8 32X, AN8 32X,
>>+ AW9D-MAX) (2)
>>
>>
Hans, I just want to add that the Abit Motherboard I successfully tested
this driver on was the "AT8 Crossfire Edition" (don't have an actual
part # handy, but it is probably officially "AT8"). This board is
different than the AT8 32X board, so I think my board should also be
listed here as supported.
Thanks both to Hans & Jeurg for the teamwork that hopefully leads to
getting both of these drivers included soon!
Regards,
Rick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070320/1c961d99/attachment.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* [lm-sensors] abituguru3 review
2007-03-19 21:28 [lm-sensors] abituguru3 review Juerg Haefliger
2007-03-20 14:29 ` Rick Wright
@ 2007-03-21 22:10 ` Hans de Goede
2007-03-24 20:07 ` Juerg Haefliger
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2007-03-21 22:10 UTC (permalink / raw)
To: lm-sensors
Juerg,
First of all a huge thanks for the review.
About all the spelling and code-style comments: I'll fix them all.
For the rest I'll get into more detail below. Please let me know how you think
about how I think. (If you catch my drift)
Juerg Haefliger wrote:
> Hans,
>
> Here's my review:
>
>> @@ -20,8 +20,8 @@
>> uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
>> uGuru 2.2.0.0 ~ 2.2.0.6 (AA8 Fatal1ty)
>> uGuru 2.3.0.0 ~ 2.3.0.9 (AN8)
>> - uGuru 3.0.0.0 ~ 3.0.1.2 (AW8, AL8, NI8)
>> - uGuru 4.xxxxx? (AT8 32X) (2)
>> + uGuru 3.0.0.0 ~ 3.0.x.x (AW8, AL8, NI8 SLI, AT8 32X, AN8 32X,
>> + AW9D-MAX) (2)
>
> Hmm... I don't get it. You're changing the abituguru driver to only
> support rev 1 & 2 but here you add rev 3 to the list of supported
> devices?
>
Thats the disadvantage of reviewing a patch, that is not a list of supported
motherboards, but rather a list which Abit uGuru motherboards have which
revision, so that the user can see which driver to use. This chunk starts with:
Note:
The uGuru is a microcontroller with onboard firmware which programs
it to behave as a hwmon IC. There are many different revisions of the
firmware and thus effectivly many different revisions of the uGuru.
Below is an incomplete list with which revisions are used for which
Motherboards:
uGuru 1.00 ~ 1.24 (AI7, KV8-MAX3, AN7) (1)
uGuru 2.0.0.0 ~ 2.0.4.2 (KV8-PRO)
uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
And also notice that the revision 3 entry has a footnote saying:
+ 2) There is a seperate abituguru3 driver for these motherboards,
+ the abituguru (without the 3 !) driver will not work on these
+ motherboards (and visa versa)!
>> 1) For revisions 2 and 3 uGuru's the driver can autodetect the
>> sensortype (Volt or Temp) for bank1 sensors, for revision 1
>> uGuru's
>> this doesnot always work. For these uGuru's the autodection can
>
> Same here, there is still mentioning of rev 3 in the doc for abituguru.
>
Yes, the doc points revision 3 owners to the revision 3 driver and that driver can
autodetect the sensortype, just like the revison 1&2 driver can autodetect it for
rev2 ugurus. I'm open for documentation enhancement, but what it currently says is
correct. I think it is better to have the complete table with all revisions in both
the rev1&2 driver and the rev 3 driver docs and point to the other driver if
the user started reading the wrong doc / is trying to use the wrong driver.
>> +#define ABIT_UGURU3_MAX_NO_SENSORS 26
>> +/* sum of strlen of: in??_input\0, in??_{min,max}\0,
>> in??_{min,max}_alarm\0,
>> + in??_{min,max}_alarm_enable\0, in??_beep\0, in??_shutdown\0,
>> in??_label\0 */
>> +#define ABIT_UGURU3_IN_NAMES_LENGTH (11 + 2 * 9 + 2 * 15 + 2 * 22 +
>> 10 + 14 + 11)
>
> strlen is the string length without the termination character \0. So
> your numbers are off by one or the comment is incorrect.
>
The comment is incorrect I'll fix it.
>> +/* sum of strlen of: temp??_input\0, temp??_max\0, temp??_crit\0,
>> + temp??_alarm\0, temp??_alarm_enable\0, temp??_beep\0,
>> temp??_shutdown\0,
>> + temp??_label\0 */
>> +#define ABIT_UGURU3_TEMP_NAMES_LENGTH (13 + 11 + 12 + 13 + 20 + 12 +
>> 16 + 13)
>
> Ditto. And there's never 10 or more temp sensors so the number could
> be further reduced by one (ok nit-picking now...).
>
Thats deliberate, the table of supported motherboards is ever growing and in
theory (judgeing from the bank-addreses) the uguru2 can handle upto 16 temp
sensors. Thus the wasted byte is to make things future proof (I don't want to
need to check the entire code for assumptions like < 10 temp sensors when I add
a new motherboard).
>> +#define ABIT_UGURU3_SYSFS_NAMES_LENGTH (16 *
>> ABIT_UGURU3_IN_NAMES_LENGTH + \
>> + (ABIT_UGURU3_MAX_NO_SENSORS - 16) * ABIT_UGURU3_TEMP_NAMES_LENGTH)
>> +
>> +/* All the macros below are named identical to the openguru2 program
>> + reverse engineered by Louis Kruger, hence the names might not be 100%
>> + logical. I could come up with better names, but I prefer keeping
>> the names
>> + identical so that this driver can be compared with his work more
>> easily. */
>> +/* Two i/o-ports are used by uGuru */
>> +#define ABIT_UGURU3_BASE 0x00E0
>> +#define ABIT_UGURU3_CMD 0x00
>> +#define ABIT_UGURU3_DATA 0x04
>> +#define ABIT_UGURU3_REGION_LENGTH 5
>> +/* The wait_xxx functions return this on success and the last contents
>> + of the DATA register (0-255) on failure. */
>> +#define ABIT_UGURU3_SUCCESS -1
>> +/* uGuru status flags */
>> +#define ABIT_UGURU3_STATUS_READY_FOR_READ 0x01
>> +#define ABIT_UGURU3_STATUS_BUSY 0x02
>> +
>> +
>> +/* Structures */
>> +struct abituguru3_sensor_info {
>> + const char* name;
>> + int port;
>> + int type;
>> + int multiplier;
>> + int divisor;
>> + int offset;
>> +};
>> +
>> +struct abituguru3_motherboard_info {
>> + u16 id;
>> + const char *name;
>> + /* + 1 -> end of sensors indicated by a sensor with name = NULL */
>> + struct abituguru3_sensor_info sensors[ABIT_UGURU3_MAX_NO_SENSORS
>> + 1];
>> +};
>> +
>> +/* For the Abit uGuru, we need to keep some data in memory.
>> + The structure is dynamically allocated, at the same time when a new
>> + abituguru3 device is allocated. */
>> +struct abituguru3_data {
>> + struct class_device *class_dev; /* hwmon registered device */
>> + struct mutex update_lock; /* protect access to data and uGuru */
>> + unsigned short addr; /* uguru base address */
>> + char valid; /* !=0 if following fields are valid */
>> + unsigned long last_updated; /* In jiffies */
>> +
>> + /* For convenience the sysfs attr and their names are generated
>> + automatically. We have max 10 entries per sensor (for in
>> sensors) */
>
> Here you claim a max of 10 in sensors but further up you use 16 for
> the worst case in ABIT_UGURU3_SYSFS_NAMES_LENGTH.
>
>
>> + struct sensor_device_attribute_2
>> sysfs_attr[ABIT_UGURU3_MAX_NO_SENSORS
>> + * 10];
No not max 10 in sensors, but max 10 sysfs entries / sensor, and that case
happens when the sensor is an in sensor:
inX_input, inX_min, inX_max, inX_min_alarm, inX_max_alarm,
inX_min_alarm_enable, inX_max_alarm_enable, inX_beep, inX_shutdown, inX_label
Also see below when I try to explain why it sin't true that all the strings are
twice in memory as you think. I admit this is somewhat complicated, I took
this from the other uguru driver (which I wrote too). This is done because the
uguru has a humongous number of sysfs entries (160 for the in sensors alone!)
and I didn't want to use a huge static table for this. Hence I have:
-an array of sysfs attr, which gets dynamicly filled with sysfs attr dependend
on the mobo and thus on the number of in / temp / fan sensors. The size of
this array is "ABIT_UGURU3_MAX_NO_SENSORS * 10"
-an array with all the entries (10 for in sensors) which needs to be generated
for 1 sensor, which I call the "template"
>> + /* Alarms for all 48 sensors (48/8 = 6 bytes) */
>> + u8 alarms[48/8];
>
> Why / 8? One bit per sensor?
>
Yes, shall I add a comment to this extend?
>
>> +
>> + /* Value of all 48 sensors */
>> + u8 value[48];
>> +
>> + /* Settings of all 48 sensors, note in and temp sensors have 3 byte
>> + of settings, while fans only have 2 bytes, for convenience we
>> + use 3 bytes for all sensors */
>> + u8 settings[48][3];
>
> Where does the number 48 come from? I thought it was:
> #define ABIT_UGURU3_MAX_NO_SENSORS 26
>
Either I get a very complicated update function, which modifies its behaviour
depenend on the mobo and thus needs to traverse the mobo table to determine
what to read or I always read (no harm there) 16 sensors of each type even
those aren't all there. Also if I don't do it this way I cannot use the bank
addresses in the sysfs-read functions, then I need to also calculate / store
the offset into the compressed value and settings tables you suggest.
IOW, the uguru has 48 consecutive sensors register-sets, the first 16 are
reserved for in the next 16 to temp and then 16 to fan. To make things easy I
always read all 48 register-sets, so other functions have a one on one in
memory copy of these to work with.
>> + },
>> + { 0x0019, "unknown", {
>> + { "CPU Core" , 7, 0, 10, 1, 0 },
>> + { "DDR2" , 13, 0, 20, 1, 0 },
>> + { "DDR2 VTT" , 14, 0, 10, 1, 0 },
>> + { "CPU VTT" , 3, 0, 20, 1, 0 },
>> + { "NB 1.2V " , 4, 0, 10, 1, 0 },
>> + { "SB 1.5V" , 6, 0, 10, 1, 0 },
>> + { "HyperTransport" , 5, 0, 10, 1, 0 },
>> + { "ATX +12V (24-Pin)" , 12, 0, 60, 1, 0 },
>> + { "ATX +12V (4-pin)" , 8, 0, 60, 1, 0 },
>> + { "ATX +5V" , 9, 0, 30, 1, 0 },
>> + { "ATX +3.3V" , 10, 0, 20, 1, 0 },
>> + { "ATX 5VSB" , 11, 0, 30, 1, 0 },
>> + { "CPU" , 24, 1, 1, 1, 0 },
>> + { "System " , 25, 1, 1, 1, 0 },
>> + { "PWM Phase1" , 26, 1, 1, 1, 0 },
>> + { "PWM Phase2" , 27, 1, 1, 1, 0 },
>> + { "PWM Phase3" , 28, 1, 1, 1, 0 },
>> + { "PWM Phase4" , 29, 1, 1, 1, 0 },
>> + { "PWM Phase5" , 30, 1, 1, 1, 0 },
>> + { "CPU FAN" , 32, 2, 60, 1, 0 },
>> + { "SYS FAN" , 34, 2, 60, 1, 0 },
>> + { "AUX1 FAN" , 33, 2, 60, 1, 0 },
>> + { "AUX2 FAN" , 35, 2, 60, 1, 0 },
>> + { "AUX3 FAN" , 36, 2, 60, 1, 0 },
>> + { NULL, 0, 0, 0, 0, 0 } }
>
> Aligning the numbers helps with reviewing them. With, that, I noticed
> that the last two columns (divisor, offset) contain always the same
> values. Why even put them in there?
>
They come from ini files of the windows program supplied for these boards by
Abit, and they are used in the current read functions. I noticed the same thing
but I decided to keep them in case they start getting different values with
newer motherboards.
> Also, here you use hard-coded values for the sensor types but further
> up you have macros defined for them (ABIT_UGURU3_IN_SENSOR,
> ABIT_UGURU3_TEMP_SENSOR, ABIT_UGURU3_FAN_SENSOR). You could get out of
> sysnc.
>
Actually the defines are to sync my code with the values used by abit in the
ini files. I understand what you're trying to say, but I don't think the tables
above will become more readable with those defines in. I will fix all the extra
spaces in the tables and add (a) tab(s) after the labels to lign up the rest of
the tables.
>> +/* Insmod parameters */
>> +static int force;
>
> No initialization required? Is gcc smart enough to init to 0?
>
Yes, actually I agree with you this is a bit dark-magic, but I've been told
(with the previous driver) that this is the way this is always done within the
kernel, if a global var should be initialised to 0 don't initialise it.
>
>> +module_param(force, bool, 0);
>> +MODULE_PARM_DESC(force, "Set to one to force detection.");
>> +/* Default verbose is 1, since this driver is still in the testing
>> phase */
>> +static int verbose = 1;
>> +module_param(verbose, bool, 0644);
>> +MODULE_PARM_DESC(verbose, "Enable/disable verbose error reporting");
>
> Comment is inaccurate since the value could be 0-3 (according to the
> driver doc).
>
Actually that is a copy/paste error from the abituguru rev 1 & 2 doc, that
driver has some retry magic and verbose settings related to that. I'll fix the
doc for the rev 3 driver to reflect that for the rev 3 driver this is a bool.
>> +/* Sensor settings are stored 1 byte per offset with the bytes
>> + placed add consecutive offsets. */
>
> at?
>
Yes.
>> + if (!data)
>> + return -EIO;
>
> Isn't this condition considered a bug? Don't you want to print some
> fancy text?
>
That condition is caused by the update function failing and the update function
will already have complained and tried to explain whats gone wrong (unless
verbose = 0)
>
>> + sensor = data->sensors[attr->index];
>
> Wouldn't it be more efficient to use a pointer rather than copying the
> whole thing? E.g.: struct abituguru3_sensors_info *sensor > &data->sensors[attr->index];
>
Agreed, will fix.
>> + if ((data->alarms[port / 8] & (0x01 << (port % 8))) &&
>> + (!attr->nr || (data->settings[port][0] & attr->nr)))
>> + return sprintf(buf, "1\n");
>> + else
>> + return sprintf(buf, "0\n");
>
> How about
> return sprintf(buf, "%d\n",
> (data->alarms[port >> 3] & (0x01 << (port % 8))) &&
> (!attr->nr || (data->settings[port][0] & attr->nr));
>
I agree thats more efficient, but I find my variant better readable (and that
is how its done in other hwmon drivers).
>> + if (data->settings[data->sensors[attr->index].port][0] & attr->nr)
>> + return sprintf(buf, "1\n");
>> + else
>> + return sprintf(buf, "0\n");
>
> return sprintf(buf, "%d\n",
> data->settings[data->sensors[attr->index].port][0] & attr->nr ? 1
> : 0);
>
Ditto
>> + /* Fill the sysfs attr array */
>> + sysfs_attr_i = 0;
>> + sysfs_filename = data->sysfs_names;
>> + sysfs_names_free = ABIT_UGURU3_SYSFS_NAMES_LENGTH;
>> + for (i = 0; data->sensors[i].name; i++) {
>> + type = data->sensors[i].type;
>> + for (j = 0; j < no_sysfs_attr[type]; j++) {
>> + used = snprintf(sysfs_filename, sysfs_names_free,
>> + abituguru3_sysfs_templ[type][j].dev_attr.attr.
>> + name, sensor_index[type]) + 1;
>> + data->sysfs_attr[sysfs_attr_i] >> + abituguru3_sysfs_templ[type][j];
>> + data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name >> + sysfs_filename;
>
> If I understand this correctly you're concatenating all the sysfs
> attribute names and copy them into the sysfs_filename buffer which is
> a member of the device data structure. And then later you overwrite
> the attr name (data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name) with
> a pointer into this buffer.
> You're effectively allocating memory twice for the same thing (once in
> your device data struct and in the individual sysfs_attr structs).
> Wouldn't it be easier and more efficient to somehow reserve enough
> space for the final attribute name in the abituguru3_sysfs_templ
> struct and then snprintf the name into
> data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name rather then
> overwriting the string pointer?
>
As I already tried to explain above, and as hopefully indicated with the
_templ(ate) name, abituguru3_sysfs_templ holds the template of the attributes
needed for 1 in sensor + those needed for a temp sensor + those needed for a
fan sensor. From these templates a lot of entries (and thus a lot of names) get
generated. The attr for these entries are stored into data->sysfs_attr[] and
the name pointers in these attr are made to point to names generated into the
data->sysfs_names buffer.
I hope thats clear / understandable.
>> + res = -ENAMETOOLONG;
>> + goto abituguru3_probe_error;
>> + }
>> +
>> + /* Register sysfs hooks */
>> + data->class_dev = hwmon_device_register(&pdev->dev);
>> + if (IS_ERR(data->class_dev)) {
>> + res = PTR_ERR(data->class_dev);
>> + goto abituguru3_probe_error;
>> + }
>> + for (i = 0; i < sysfs_attr_i; i++)
>> + if (device_create_file(&pdev->dev,
>> + &data->sysfs_attr[i].dev_attr))
>> + goto abituguru3_probe_error;
>> + for (i = 0; i < ARRAY_SIZE(abituguru3_sysfs_attr); i++)
>> + if (device_create_file(&pdev->dev,
>> + &abituguru3_sysfs_attr[i].dev_attr))
>> + goto abituguru3_probe_error;
>> +
>> + data->class_dev = hwmon_device_register(&pdev->dev);
>> + if (!IS_ERR(data->class_dev))
>> + return 0; /* success */
>> +
>> + res = PTR_ERR(data->class_dev);
>
> You're registering the device twice! and the check for success is
> backwards from all the previous checks (i.e., check for error and
> handle it vs. check for success and handle it). Drop the first
> occurence (before creating the devices) and I'd prefer (but it's your
> call - I'm nit-picking again :-):
>
> if (IS_ERR(data->class_dev)) {
> res = PTR_ERR(data->class_dev);
> goto abituguru3_probe_error;
> }
>
> return 0;
>
Good catch! I meant to move the registering down because Jean asked me todo the
same thing for the abituguru (to fix a race condition), but I guess I ended up
copy-ing it, will fix!
>> +static int __devexit abituguru3_remove(struct platform_device *pdev)
>> +{
>> + int i;
>> + struct abituguru3_data *data = platform_get_drvdata(pdev);
>> +
>> + platform_set_drvdata(pdev, NULL);
>
> Not sure what this does (is it freeing any memory?) but is it safe to
> do it before unregistering and deleting devices?
>
It doesn't free anything it just sets the platformdevice's drvdata pointer to
NULL. Since once the driver is removed (theoretically) the platform device
could stay around, we don't want the platform device to keep a pointer to stuff
we've just freed.
>> + mutex_unlock(&data->update_lock);
>> + if (data->valid)
>> + return data;
>> + else
>> + return NULL;
>
> How about:
> return data->valid ? data : NULL;
>
I'm not such a big fan of conditional expressions, they are ok when you've got
a long function call (lots of arguments) to make one of the arguments have one
of two values dependend on some check. However I don't like them for uses as
you suggest, but thats just my personal preference.
Phew all done, I'm looking forward to your opinion / reply to this. Once that
is all clear I'll update the driver + docs and send a new patch to the list.
Regards,
Hans
^ permalink raw reply [flat|nested] 4+ messages in thread* [lm-sensors] abituguru3 review
2007-03-19 21:28 [lm-sensors] abituguru3 review Juerg Haefliger
2007-03-20 14:29 ` Rick Wright
2007-03-21 22:10 ` Hans de Goede
@ 2007-03-24 20:07 ` Juerg Haefliger
2 siblings, 0 replies; 4+ messages in thread
From: Juerg Haefliger @ 2007-03-24 20:07 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
> First of all a huge thanks for the review.
You're very welcome, just returning the favor.
> > Hmm... I don't get it. You're changing the abituguru driver to only
> > support rev 1 & 2 but here you add rev 3 to the list of supported
> > devices?
> >
>
> Thats the disadvantage of reviewing a patch, that is not a list of supported
> motherboards, but rather a list which Abit uGuru motherboards have which
> revision, so that the user can see which driver to use. This chunk starts with:
> Note:
> The uGuru is a microcontroller with onboard firmware which programs
> it to behave as a hwmon IC. There are many different revisions of the
> firmware and thus effectivly many different revisions of the uGuru.
> Below is an incomplete list with which revisions are used for which
> Motherboards:
> uGuru 1.00 ~ 1.24 (AI7, KV8-MAX3, AN7) (1)
> uGuru 2.0.0.0 ~ 2.0.4.2 (KV8-PRO)
> uGuru 2.1.0.0 ~ 2.1.2.8 (AS8, AV8, AA8, AG8, AA8XE, AX8)
>
> And also notice that the revision 3 entry has a footnote saying:
> + 2) There is a seperate abituguru3 driver for these motherboards,
> + the abituguru (without the 3 !) driver will not work on these
> + motherboards (and visa versa)!
>
> > >> 1) For revisions 2 and 3 uGuru's the driver can autodetect the
> >> sensortype (Volt or Temp) for bank1 sensors, for revision 1
> >> uGuru's
> >> this doesnot always work. For these uGuru's the autodection can
> >
> > Same here, there is still mentioning of rev 3 in the doc for abituguru.
> >
>
> Yes, the doc points revision 3 owners to the revision 3 driver and that driver can
> autodetect the sensortype, just like the revison 1&2 driver can autodetect it for
> rev2 ugurus. I'm open for documentation enhancement, but what it currently says is
> correct. I think it is better to have the complete table with all revisions in both
> the rev1&2 driver and the rev 3 driver docs and point to the other driver if
> the user started reading the wrong doc / is trying to use the wrong driver.
Yes, that certainly makes sense. I suspected that my confusion came
from the fact that the patch just contained out-of-context snippets.
> > Ditto. And there's never 10 or more temp sensors so the number could
> > be further reduced by one (ok nit-picking now...).
> >
>
> Thats deliberate, the table of supported motherboards is ever growing and in
> theory (judgeing from the bank-addreses) the uguru2 can handle upto 16 temp
> sensors. Thus the wasted byte is to make things future proof (I don't want to
> need to check the entire code for assumptions like < 10 temp sensors when I add
> a new motherboard).
OK, makes sense. It wasn't clear to me that there could be as many as
16 sensors (at least not at this point of the review). Maybe a note
mentioning this?
> >> + /* For convenience the sysfs attr and their names are generated
> >> + automatically. We have max 10 entries per sensor (for in
> >> sensors) */
> >
> > Here you claim a max of 10 in sensors but further up you use 16 for
> > the worst case in ABIT_UGURU3_SYSFS_NAMES_LENGTH.
> >
> >
> >> + struct sensor_device_attribute_2
> >> sysfs_attr[ABIT_UGURU3_MAX_NO_SENSORS
> >> + * 10];
>
> No not max 10 in sensors, but max 10 sysfs entries / sensor, and that case
> happens when the sensor is an in sensor:
> inX_input, inX_min, inX_max, inX_min_alarm, inX_max_alarm,
> inX_min_alarm_enable, inX_max_alarm_enable, inX_beep, inX_shutdown, inX_label
Correct, my bad.
> Also see below when I try to explain why it sin't true that all the strings are
> twice in memory as you think. I admit this is somewhat complicated, I took
> this from the other uguru driver (which I wrote too). This is done because the
> uguru has a humongous number of sysfs entries (160 for the in sensors alone!)
> and I didn't want to use a huge static table for this. Hence I have:
> -an array of sysfs attr, which gets dynamicly filled with sysfs attr dependend
> on the mobo and thus on the number of in / temp / fan sensors. The size of
> this array is "ABIT_UGURU3_MAX_NO_SENSORS * 10"
> -an array with all the entries (10 for in sensors) which needs to be generated
> for 1 sensor, which I call the "template"
>
> >> + /* Alarms for all 48 sensors (48/8 = 6 bytes) */
> >> + u8 alarms[48/8];
> >
> > Why / 8? One bit per sensor?
> >
>
> Yes, shall I add a comment to this extend?
Sure, never hurts.
> >
> >> +
> >> + /* Value of all 48 sensors */
> >> + u8 value[48];
> >> +
> >> + /* Settings of all 48 sensors, note in and temp sensors have 3 byte
> >> + of settings, while fans only have 2 bytes, for convenience we
> >> + use 3 bytes for all sensors */
> >> + u8 settings[48][3];
> >
> > Where does the number 48 come from? I thought it was:
> > #define ABIT_UGURU3_MAX_NO_SENSORS 26
> >
>
> Either I get a very complicated update function, which modifies its behaviour
> depenend on the mobo and thus needs to traverse the mobo table to determine
> what to read or I always read (no harm there) 16 sensors of each type even
> those aren't all there. Also if I don't do it this way I cannot use the bank
> addresses in the sysfs-read functions, then I need to also calculate / store
> the offset into the compressed value and settings tables you suggest.
>
> IOW, the uguru has 48 consecutive sensors register-sets, the first 16 are
> reserved for in the next 16 to temp and then 16 to fan. To make things easy I
> always read all 48 register-sets, so other functions have a one on one in
> memory copy of these to work with.
That explains a lot and would be very helpful to include in the driver.
> > Aligning the numbers helps with reviewing them. With, that, I noticed
> > that the last two columns (divisor, offset) contain always the same
> > values. Why even put them in there?
> >
>
> They come from ini files of the windows program supplied for these boards by
> Abit, and they are used in the current read functions. I noticed the same thing
> but I decided to keep them in case they start getting different values with
> newer motherboards.
>
> > Also, here you use hard-coded values for the sensor types but further
> > up you have macros defined for them (ABIT_UGURU3_IN_SENSOR,
> > ABIT_UGURU3_TEMP_SENSOR, ABIT_UGURU3_FAN_SENSOR). You could get out of
> > sysnc.
> >
>
> Actually the defines are to sync my code with the values used by abit in the
> ini files. I understand what you're trying to say, but I don't think the tables
> above will become more readable with those defines in. I will fix all the extra
> spaces in the tables and add (a) tab(s) after the labels to lign up the rest of
> the tables.
Ah-ah, I didn't know that these tables come from Abit. In that case I
think it's OK to leave them the way they are. Except maybe for the
number alignment which really helps.
> >> +module_param(force, bool, 0);
> >> +MODULE_PARM_DESC(force, "Set to one to force detection.");
> >> +/* Default verbose is 1, since this driver is still in the testing
> >> phase */
> >> +static int verbose = 1;
> >> +module_param(verbose, bool, 0644);
> >> +MODULE_PARM_DESC(verbose, "Enable/disable verbose error reporting");
> >
> > Comment is inaccurate since the value could be 0-3 (according to the
> > driver doc).
> >
>
> Actually that is a copy/paste error from the abituguru rev 1 & 2 doc, that
> driver has some retry magic and verbose settings related to that. I'll fix the
> doc for the rev 3 driver to reflect that for the rev 3 driver this is a bool.
OK.
> >> + if (!data)
> >> + return -EIO;
> >
> > Isn't this condition considered a bug? Don't you want to print some
> > fancy text?
> >
>
> That condition is caused by the update function failing and the update function
> will already have complained and tried to explain whats gone wrong (unless
> verbose = 0)
OK.
> > return sprintf(buf, "%d\n",
> > (data->alarms[port >> 3] & (0x01 << (port % 8))) &&
> > (!attr->nr || (data->settings[port][0] & attr->nr));
> >
>
> I agree thats more efficient, but I find my variant better readable (and that
> is how its done in other hwmon drivers).
That depends which driver you're looking at :-) Agreed, it's a style
change and I won't argue the matter.
> >> + /* Fill the sysfs attr array */
> >> + sysfs_attr_i = 0;
> >> + sysfs_filename = data->sysfs_names;
> >> + sysfs_names_free = ABIT_UGURU3_SYSFS_NAMES_LENGTH;
> >> + for (i = 0; data->sensors[i].name; i++) {
> >> + type = data->sensors[i].type;
> >> + for (j = 0; j < no_sysfs_attr[type]; j++) {
> >> + used = snprintf(sysfs_filename, sysfs_names_free,
> >> + abituguru3_sysfs_templ[type][j].dev_attr.attr.
> >> + name, sensor_index[type]) + 1;
> >> + data->sysfs_attr[sysfs_attr_i] > >> + abituguru3_sysfs_templ[type][j];
> >> + data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name > >> + sysfs_filename;
> >
> > If I understand this correctly you're concatenating all the sysfs
> > attribute names and copy them into the sysfs_filename buffer which is
> > a member of the device data structure. And then later you overwrite
> > the attr name (data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name) with
> > a pointer into this buffer.
> > You're effectively allocating memory twice for the same thing (once in
> > your device data struct and in the individual sysfs_attr structs).
> > Wouldn't it be easier and more efficient to somehow reserve enough
> > space for the final attribute name in the abituguru3_sysfs_templ
> > struct and then snprintf the name into
> > data->sysfs_attr[sysfs_attr_i].dev_attr.attr.name rather then
> > overwriting the string pointer?
> >
>
> As I already tried to explain above, and as hopefully indicated with the
> _templ(ate) name, abituguru3_sysfs_templ holds the template of the attributes
> needed for 1 in sensor + those needed for a temp sensor + those needed for a
> fan sensor. From these templates a lot of entries (and thus a lot of names) get
> generated. The attr for these entries are stored into data->sysfs_attr[] and
> the name pointers in these attr are made to point to names generated into the
> data->sysfs_names buffer.
>
> I hope thats clear / understandable.
Yeah I think I understand now. But you're still wasting the space
allocated by the template, correct? I guess that's OK given the
simplification this entails.
> > You're registering the device twice! and the check for success is
> > backwards from all the previous checks (i.e., check for error and
> > handle it vs. check for success and handle it). Drop the first
> > occurence (before creating the devices) and I'd prefer (but it's your
> > call - I'm nit-picking again :-):
> >
> > if (IS_ERR(data->class_dev)) {
> > res = PTR_ERR(data->class_dev);
> > goto abituguru3_probe_error;
> > }
> >
> > return 0;
> >
>
> Good catch! I meant to move the registering down because Jean asked me todo the
> same thing for the abituguru (to fix a race condition), but I guess I ended up
> copy-ing it, will fix!
OK.
> >> +static int __devexit abituguru3_remove(struct platform_device *pdev)
> >> +{
> >> + int i;
> >> + struct abituguru3_data *data = platform_get_drvdata(pdev);
> >> +
> >> + platform_set_drvdata(pdev, NULL);
> >
> > Not sure what this does (is it freeing any memory?) but is it safe to
> > do it before unregistering and deleting devices?
> >
>
> It doesn't free anything it just sets the platformdevice's drvdata pointer to
> NULL. Since once the driver is removed (theoretically) the platform device
> could stay around, we don't want the platform device to keep a pointer to stuff
> we've just freed.
OK.
> >> + mutex_unlock(&data->update_lock);
> >> + if (data->valid)
> >> + return data;
> >> + else
> >> + return NULL;
> >
> > How about:
> > return data->valid ? data : NULL;
> >
>
> I'm not such a big fan of conditional expressions, they are ok when you've got
> a long function call (lots of arguments) to make one of the arguments have one
> of two values dependend on some check. However I don't like them for uses as
> you suggest, but thats just my personal preference.
Yes, it's a matter of personal preferences. I just think given the
simplicity of the code it doesn't deserve 4 lines :-)
> Phew all done, I'm looking forward to your opinion / reply to this. Once that
> is all clear I'll update the driver + docs and send a new patch to the list.
Looks pretty good to me overall. In general I would suggest to add
more details about the inner works of the chip (how many sensors max,
register arrangements and such). That helps reviewers better to
understand what your code is doing.
Try to get it past Jean now. Good luck :-)
...juerg
> Regards,
>
> Hans
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-03-24 20:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-19 21:28 [lm-sensors] abituguru3 review Juerg Haefliger
2007-03-20 14:29 ` Rick Wright
2007-03-21 22:10 ` Hans de Goede
2007-03-24 20:07 ` Juerg Haefliger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.