Hi,
I have a couple minor comments.
> [...]
> +// Get a System76 ACPI device value by name with index
Shouldn't the comments be the original C-style?
> +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;
Minor thing, but isn't this initialization unnecessary?
> +
> + 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;
I'd personally return -EIO or something similar here. And possibly use
acpi_handle_err() + acpi_format_exception().
> +}
> +
> +// 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;
Casting from 'void*' is redundant.
> + return 0;
> + } else {
> + return -1;
Same here: I'd return -EIO or something similar.
> + }
> +}
> +
> [...]
> +
> +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;
It's a minor thing, but this function returns EINVAL even if the ACPI call failed.
> +}
> +
> [...]
> +
> +// 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),
Any reason why the last two channels are indented +1 tabs?
> + 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);
Shouldn't the return values be checked? (At least using WARN_ON[_ONCE]?)
> + 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;
> }
> [...]
Barnabás Pőcze
