On 2/5/22 18:20, Andrey Smirnov wrote:
Add a driver exposing various bits and pieces of functionality
provided by Steam Deck specific VLV0100 device presented by EC
firmware. This includes but not limited to:
- CPU/device's fan control
- Read-only access to DDIC registers
- Battery tempreature measurements
- Various display related control knobs
- USB Type-C connector event notification
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: Mark Gross <markgross@xxxxxxxxxx>
Cc: Jean Delvare <jdelvare@xxxxxxxx>
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx (open list)
Cc: platform-driver-x86@xxxxxxxxxxxxxxx
Cc: linux-hwmon@xxxxxxxxxxxxxxx
Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
---
...
> +config STEAMDECK
> + tristate "Valve Steam Deck platform driver"
> + depends on X86_64
> + help
> + Driver exposing various bits and pieces of functionality
> + provided by Steam Deck specific VLV0100 device presented by
> + EC firmware. This includes but not limited to:
There seems to be some indentation issue.
> + - CPU/device's fan control
> + - Read-only access to DDIC registers
> + - Battery tempreature measurements
> + - Various display related control knobs
> + - USB Type-C connector event notification
> +
> + Say N unless you are running on a Steam Deck.
> +
This doesn't depend on hwmon, yet it fails if devm_hwmon_device_register_with_info()
returns an eror. That has a couple of problems: if HWMON=n, it won't compile,
and if STEAMDECK=y and HWMON=m it won't compile either. You'll have to provide
some dependency against HWMON to make this work.
...
+
+static int steamdeck_read_fan_speed(struct steamdeck *jup, long *speed)
+{
+ unsigned long long val;
+
+ if (ACPI_FAILURE(acpi_evaluate_integer(jup->adev->handle,
+ "FANR", NULL, &val)))
+ return -EIO;
+
+ *speed = val;
+ return 0;
+}
+
+static int
+steamdeck_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *out)
+{
+ struct steamdeck *sd = dev_get_drvdata(dev);
+ unsigned long long val;
+
+ switch (type) {
+ case hwmon_temp:
+ if (attr != hwmon_temp_input)
+ return -EOPNOTSUPP;
+
+ if (ACPI_FAILURE(acpi_evaluate_integer(sd->adev->handle,
+ "BATT", NULL, &val)))
+ return -EIO;
+ /*
+ * Assuming BATT returns deg C we need to mutiply it
+ * by 1000 to convert to mC
+ */
+ *out = val * 1000;
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return steamdeck_read_fan_speed(sd, out);
There is a bit of inconsistency here: All other atributes are handled directly,
except for this one, yet there isn't really a difference in the actual operation.
Maybe I am missing something. What is the reason for using a function here
but not for the other attributes ?
+ case hwmon_fan_target:
+ *out = sd->fan_target;
+ break;
+ case hwmon_fan_fault:
+ if (ACPI_FAILURE(acpi_evaluate_integer(
+ sd->adev->handle,
+ "FANC", NULL, &val)))
+ return -EIO;
+ /*
+ * FANC (Fan check):
+ * 0: Abnormal
+ * 1: Normal
+ */
+ *out = !val;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int
+steamdeck_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ switch (type) {
+ case hwmon_temp:
+ *str = "Battery Temp";
+ break;
+ case hwmon_fan:
+ *str = "System Fan";
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int
+steamdeck_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct steamdeck *sd = dev_get_drvdata(dev);
+
+ if (type != hwmon_fan ||
+ attr != hwmon_fan_target)
+ return -EOPNOTSUPP;
+
+ if (val > U16_MAX)
+ return -EINVAL;
This accepts negative values, and it expects the user to find
valid ranges. I suggest to use clamp_val() instead.
Guenter
