...
+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.
+ },
