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

[Guenter Roeck]

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