Re: [PATCH 1/2] platform/x86: system76_acpi: Add hwmon driver

Re: [PATCH 1/2] platform/x86: system76_acpi: Add hwmon driver

[Hans de Goede]

Hi Jeremy,

On 9/3/20 10:47 PM, Jeremy Soller wrote:
> The hwmon driver provides a kernel interface for accessing fan duty
> cycle, fan speed, and temperature data as defined by firmware. Up to
> eight fans and eight temperature sensors are supported. The actual
> number is determined by querying the firmware.
> 
> Signed-off-by: Jeremy Soller <jeremy@system76.com>
> Cc: platform-driver-x86@vger.kernel.org

Quick self intro: I have take over drivers/platform/x86
maintainership from Andy; and I'm working my way through
the backlog of old patches in patchwork:
https://patchwork.kernel.org/project/platform-driver-x86/list/

This patch has some initial review remarks from Barnabás Pőcze
which need addressing. Can you please send a v2 of this series
with Barnabás' remarks to patch 1/2 fixed ?

As I mentioned before switching to a sparse-keymap for 2/2
is not really necessary IMHO, but if you want to do it that
is fine.

Regards,

Hans





> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..d2f9c3dcf4b9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1158,6 +1158,7 @@ config SONYPI_COMPAT
>  config SYSTEM76_ACPI
>  	tristate "System76 ACPI Driver"
>  	depends on ACPI
> +	depends on HWMON
>  	select NEW_LEDS
>  	select LEDS_CLASS
>  	select LEDS_TRIGGERS
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index c14fd22ba196..aef0ea34829d 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -10,6 +10,8 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
> @@ -24,6 +26,9 @@ struct system76_data {
>  	enum led_brightness kb_brightness;
>  	enum led_brightness kb_toggle_brightness;
>  	int kb_color;
> +	struct device *therm;
> +	union acpi_object *nfan;
> +	union acpi_object *ntmp;
>  };
>  
>  static const struct acpi_device_id device_ids[] = {
> @@ -68,6 +73,54 @@ static int system76_get(struct system76_data *data, char *method)
>  		return -1;
>  }
>  
> +// Get a System76 ACPI device value by name with index
> +static int system76_get_index(struct system76_data *data, char *method, int index)
> +{
> +	union acpi_object obj;
> +	struct acpi_object_list obj_list;
> +	acpi_handle handle;
> +	acpi_status status;
> +	unsigned long long ret = 0;
> +
> +	obj.type = ACPI_TYPE_INTEGER;
> +	obj.integer.value = index;
> +	obj_list.count = 1;
> +	obj_list.pointer = &obj;
> +	handle = acpi_device_handle(data->acpi_dev);
> +	status = acpi_evaluate_integer(handle, method, &obj_list, &ret);
> +	if (ACPI_SUCCESS(status))
> +		return (int)ret;
> +	else
> +		return -1;
> +}
> +
> +// Get a System76 ACPI device object by name
> +static int system76_get_object(struct system76_data *data, char *method, union acpi_object **obj)
> +{
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	handle = acpi_device_handle(data->acpi_dev);
> +	status = acpi_evaluate_object(handle, method, NULL, &buf);
> +	if (ACPI_SUCCESS(status)) {
> +		*obj = (union acpi_object *)buf.pointer;
> +		return 0;
> +	} else {
> +		return -1;
> +	}
> +}
> +
> +// Get a name from a System76 ACPI device object
> +static char *system76_name(union acpi_object *obj, int index)
> +{
> +	if (obj && obj->type == ACPI_TYPE_PACKAGE && index <= obj->package.count) {
> +		if (obj->package.elements[index].type == ACPI_TYPE_STRING)
> +			return obj->package.elements[index].string.pointer;
> +	}
> +	return NULL;
> +}
> +
>  // Set a System76 ACPI device value by name
>  static int system76_set(struct system76_data *data, char *method, int value)
>  {
> @@ -270,6 +323,120 @@ static void kb_led_hotkey_color(struct system76_data *data)
>  	kb_led_notify(data);
>  }
>  
> +static umode_t thermal_is_visible(
> +	const void *drvdata,
> +	enum hwmon_sensor_types type,
> +	u32 attr,
> +	int channel)
> +{
> +	const struct system76_data *data = drvdata;
> +
> +	if (type == hwmon_fan || type == hwmon_pwm) {
> +		if (system76_name(data->nfan, channel))
> +			return S_IRUGO;
> +	} else if (type == hwmon_temp) {
> +		if (system76_name(data->ntmp, channel))
> +			return S_IRUGO;
> +	}
> +	return 0;
> +}
> +
> +static int thermal_read(
> +	struct device *dev,
> +	enum hwmon_sensor_types type,
> +	u32 attr,
> +	int channel,
> +	long *val)
> +{
> +	struct system76_data *data = dev_get_drvdata(dev);
> +	int raw;
> +
> +	if (type == hwmon_fan && attr == hwmon_fan_input) {
> +		raw = system76_get_index(data, "GFAN", channel);
> +		if (raw >= 0) {
> +			*val = (long)((raw >> 8) & 0xFFFF);
> +			return 0;
> +		}
> +	} else if (type == hwmon_pwm && attr == hwmon_pwm_input) {
> +		raw = system76_get_index(data, "GFAN", channel);
> +		if (raw >= 0) {
> +			*val = (long)(raw & 0xFF);
> +			return 0;
> +		}
> +	} else if (type == hwmon_temp && attr == hwmon_temp_input) {
> +		raw = system76_get_index(data, "GTMP", channel);
> +		if (raw >= 0) {
> +			*val = (long)(raw * 1000);
> +			return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int thermal_read_string(
> +	struct device *dev,
> +	enum hwmon_sensor_types type,
> +	u32 attr,
> +	int channel,
> +	const char **str)
> +{
> +	struct system76_data *data = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_fan && attr == hwmon_fan_label) {
> +		*str = system76_name(data->nfan, channel);
> +		if (*str)
> +			return 0;
> +	} else if (type == hwmon_temp && attr == hwmon_temp_label) {
> +		*str = system76_name(data->ntmp, channel);
> +		if (*str)
> +			return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct hwmon_ops thermal_ops = {
> +	.is_visible = thermal_is_visible,
> +	.read = thermal_read,
> +	.read_string = thermal_read_string,
> +};
> +
> +// Allocate up to 8 fans and temperatures
> +static const struct hwmon_channel_info *thermal_channel_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL,
> +		HWMON_F_INPUT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(pwm,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT,
> +			HWMON_PWM_INPUT),
> +	HWMON_CHANNEL_INFO(temp,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL,
> +			HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info thermal_chip_info = {
> +	.ops = &thermal_ops,
> +	.info = thermal_channel_info,
> +};
> +
>  // Handle ACPI notification
>  static void system76_notify(struct acpi_device *acpi_dev, u32 event)
>  {
> @@ -346,6 +513,17 @@ static int system76_add(struct acpi_device *acpi_dev)
>  			return err;
>  	}
>  
> +	system76_get_object(data, "NFAN", &data->nfan);
> +	system76_get_object(data, "NTMP", &data->ntmp);
> +	data->therm = devm_hwmon_device_register_with_info(
> +		&acpi_dev->dev,
> +		"system76_acpi",
> +		data,
> +		&thermal_chip_info,
> +		NULL);
> +	if (IS_ERR(data->therm))
> +		return PTR_ERR(data->therm);
> +
>  	return 0;
>  }
>  
> @@ -362,6 +540,9 @@ static int system76_remove(struct acpi_device *acpi_dev)
>  
>  	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
>  
> +	kfree(data->nfan);
> +	kfree(data->ntmp);
> +
>  	system76_get(data, "FINI");
>  
>  	return 0;
>