Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <258c224c-c543-1756-4c1e-fe9d81e24e4c@roeck-us.net>
Date:   Sun, 6 Feb 2022 09:51:16 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: en-US
To:     Andrey Smirnov <andrew.smirnov@gmail.com>,
        platform-driver-x86@vger.kernel.org
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Jean Delvare <jdelvare@suse.com>, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org
References: <20220206022023.376142-1-andrew.smirnov@gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH] platform/x86: Add Steam Deck driver
In-Reply-To: <20220206022023.376142-1-andrew.smirnov@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@redhat.com>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-kernel@vger.kernel.org (open list)
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> 
...

 > +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