Re: [PATCH v7 1/2] hwmon: (asus_wmi_ec_sensors) Support B550 Asus WMI.
From: Guenter Roeck
Date: Fri Oct 15 2021 - 05:07:42 EST
On 10/15/21 1:21 AM, Andy Shevchenko wrote:
On Fri, Oct 15, 2021 at 8:58 AM Denis Pauk <pauk.denis@xxxxxxxxx> wrote:
Linux HWMON sensors driver for ASUS motherboards to read
sensors from the embedded controller.
Many ASUS motherboards do not publish all the available
sensors via the Super I/O chip but the missing ones are
available through the embedded controller (EC) registers.
This driver implements reading those sensor data via the
WMI method BREC, which is known to be present in all ASUS
motherboards based on the AMD 500 series chipsets (and
probably is available in other models too). The driver
needs to know exact register addresses for the sensors and
thus support for each motherboard has to be added explicitly.
The EC registers do not provide critical values for the
sensors and as such they are not published to the HWMON.
Supported motherboards:
* PRIME X570-PRO
* Pro WS X570-ACE
* ROG CROSSHAIR VIII HERO
* ROG CROSSHAIR VIII DARK HERO
* ROG CROSSHAIR VIII FORMULA
* ROG STRIX X570-E GAMING
* ROG STRIX B550-E GAMING
...
Reported-by: kernel test robot <lkp@xxxxxxxxx>
New code can't be reported as regression. Or what did you mean by that?
Some people include that after 0-day reported a bug in an earlier
revision of the patch. No idea why (it's like expecting submitters
to add "Reported-by:" for everyone providing code review feedback).
Guenter
...
+Kernel driver asus-wmi-ec-sensors
+=================================
+
+Supported boards:
+ * PRIME X570-PRO,
+ * Pro WS X570-ACE,
+ * ROG CROSSHAIR VIII DARK HERO,
+ * ROG CROSSHAIR VIII FORMULA,
+ * ROG CROSSHAIR VIII HERO,
+ * ROG STRIX B550-E GAMING,
+ * ROG STRIX X570-E GAMING.
+
+Authors:
+ Eugene Shalygin <eugene.shalygin@xxxxxxxxx>
reST has a special keyword for that.
...
+/*
+ * HWMON driver for ASUS B550/X570 motherboards that publish sensor
+ * values via the embedded controller registers.
+ *
+ * Copyright (C) 2021 Eugene Shalygin <eugene.shalygin@xxxxxxxxx>
+ * Copyright (C) 2018-2019 Ed Brindley <kernel@xxxxxxxxxxxxx>
+ *
+ * EC provides:
+ * Chipset temperature,
+ * CPU temperature,
+ * Motherboard temperature,
+ * T_Sensor temperature,
+ * VRM temperature,
+ * Water In temperature,
+ * Water Out temperature,
+ * CPU Optional Fan RPM,
+ * Chipset Fan RPM,
+ * Water Flow Fan RPM,
+ * CPU current.
+ *
Redundant line.
+ */
+#include <asm/unaligned.h>
The common rule is to go from generic to particular, hence
linux/*
followed by
asm/*
followed by local subsystem / local headers like
linux/hwmon*
with a blank line between each group.
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nls.h>
+#include <linux/units.h>
+#include <linux/wmi.h>
...
+/* BLOCK_READ_EC */
Not sure what this means, but...
+#define ASUSWMI_METHODID_BREC 0x42524543
...above has definitely an ASCII combination in hex format, care to
decode it in the comment?
...
+/* from the ASUS DSDT source */
Keep the same style for all comments, i.e. here "From ..."
...
+#define MAKE_SENSOR_ADDRESS(size_i, bank_i, index_i) \
+ { .size = size_i,\
+ .bank = bank_i,\
+ .index = index_i}
You should choose one style for all macros. Here is one, there is
another... I would recommend to use one of
#define ... { \
... \
}
or
#define ... \
{ \
... \
}
In the second case the indentation of the {} is defined by the
semantics of the macro: in case of function-like macro it starts from
the first column, in case of data structure filler usually with one
TAB.
...
+/*
+ * All the known sensors for ASUS EC controllers
+ */
One line and "All known sensors..."
...
+struct asus_wmi_ec_info {
+ struct ec_sensor sensors[SENSOR_MAX];
+ char read_arg[((ASUSWMI_BREC_REGISTERS_MAX * 4) + 1) * 2];
Too many parentheses.
+ u8 read_buffer[ASUSWMI_BREC_REGISTERS_MAX];
+ unsigned int nr_sensors;
+ unsigned int nr_registers;
+ unsigned long last_updated;
+};
...
+ for (i = 0; i < SENSOR_MAX && bsi[i] != SENSOR_MAX; i++) {
+ s[i].info_index = bsi[i];
+ s[i].cached_value = 0;
Isn't it filled by kzalloc() or alike?
+ ec->nr_sensors++;
+ ec->nr_registers += known_ec_sensors[bsi[i]].addr.size;
+ }
...
+/*
+ * The next four functions converts to/from BRxx string argument format
+ * The format of the string is as follows:
+ * The string consists of two-byte UTF-16 characters
+ * The value of the very first byte int the string is equal to the total length
+ * of the next string in bytes, thus excluding the first two-byte character
+ * The rest of the string encodes pairs of (bank, index) pairs, where both
+ * values are byte-long (0x00 to 0xFF)
+ * Numbers are encoded as UTF-16 hex values
The comment above misses a lot of punctuation. Also it would be nice
to indent the list of items if any and so on.
+ */
+static void asus_wmi_ec_decode_reply_buffer(const u8 *inp, u8 *out, u32 length)
+{
+ char buffer[ASUSWMI_MAX_BUF_LEN * 2];
+ const char *pos = buffer;
+ const u8 *data = inp + 2;
+ unsigned int i;
+ u32 len;
+ len = min3((u32)ASUSWMI_MAX_BUF_LEN, (length - 2) / 4, (u32)inp[0] / 4);
Instead I would prefer to have to min_t() calls without adding castings.
Also might be useful to have a short comment explaining this choice.
+ utf16s_to_utf8s((wchar_t *)data, len * 2, UTF16_LITTLE_ENDIAN, buffer, len * 2);
+ for (i = 0; i < len; i++, pos += 2)
+ out[i] = (hex_to_bin(pos[0]) << 4) + hex_to_bin(pos[1]);
NIH hex2bin().
+}
...
+ for (i = 0; i < len; i++) {
+ byte = registers[i] >> 8;
+ *pos = hex_asc_hi(byte);
+ pos++;
+ *pos = hex_asc_lo(byte);
+ pos++;
+ byte = registers[i];
+ *pos = hex_asc_hi(byte);
+ pos++;
+ *pos = hex_asc_lo(byte);
+ pos++;
+ }
NIH bin2hex()
...
+ for (j = 0; j < si->addr.size;
+ j++, register_idx++) {
One line.
+ registers[register_idx] = (si->addr.bank << 8) + si->addr.index + j;
+ }
...
+ /* the first byte of the BRxx() argument string has to be the string size */
Comment style.
...
+ input.length = (acpi_size)query[0] + 2;
Why is casting needed?
...
+ status = wmi_evaluate_method(ASUSWMI_MONITORING_GUID, 0, method_id, &input,
+ &output);
Logically better to keep &input and &output on the same line, i.e.
second one here.
+ obj = output.pointer;
+ if (!obj || obj->type != ACPI_TYPE_BUFFER) {
+ acpi_os_free(obj);
What's the point of calling acpi_os_free(obj) when you already know it's NULL?
On top of that is the acpi_os_free() high-level enough API for this?
+ return -EIO;
+ }
+ asus_wmi_ec_decode_reply_buffer(obj->buffer.pointer, out, obj->buffer.length);
+ acpi_os_free(obj);
Ditto.
...
+ switch (data_type) {
+ case hwmon_curr:
+ case hwmon_temp:
+ case hwmon_in:
+ return value * KILO;
Thanks for using KILO, but do you think it's the correct one and not MILLI?
MILLI means that we got value in <units> and we would like to convert
it to <milliunits>.
KILO would mean the opposite, like we got in <milliunits> and we would
like to have <units>.
+ default:
+ return value;
+ }
...
+/*
+ * Now follow the functions that implement the hwmon interface
+ */
One line.
...
+ int index;
+ const struct asus_wmi_sensors *sensor_data = drvdata;
Here and elsewhere, perhaps reversed xmas tree order?
...
+ int nr_count[hwmon_max] = { 0 }, nr_types = 0;
{} will work as well.
...
+ /*
+ * if we can get values for all the registers in a single query,
+ * the query will not change from call to call
"If..."
And period at the end since it's a multi-line comment.
+ */
...
+ sensor_data = devm_kzalloc(dev, sizeof(struct asus_wmi_sensors),
+ GFP_KERNEL);
One line?
...
+ /* ec init */
Usefulness of this comment?
+ return asus_wmi_ec_configure_sensor_setup(dev,
+ sensor_data, bsi);
One line?
...
+ .driver = {
+ .name = KBUILD_MODNAME,
It's kinda semi-ABI, please be not dependent on compile-time variables.
+ },