Re: [PATCH] platform/x86: Add Steam Deck driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux