Thanks for the comments. I was off for a few days, but will work to address them today.
> 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?
True, I will remove it
>
>
> > +
> > + 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().
Yes, I will return a better error value
>
>
> > +}
> > +
> > +// 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.
I will remove the cast
>
>
> > + return 0;
> > + } else {
> > + return -1;
>
> Same here: I'd return -EIO or something similar.
I will return a better error here
>
>
> > + }
> > +}
> > +
> > [...]
> > +
> > +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.
I will return the error from the system76_get_index call in those cases
>
>
> > +}
> > +
> > [...]
> > +
> > +// 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?
No reason, I will fix the indentation
>
>
> > + 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]?)
On these calls, the firmware may or may not have NFAN or NTMP. Not having them
indicates there is no firmware API for reading fan and temperature values, which is
not an error. So, we ignore errors from these calls, and allow nfan and ntmp to be
set to NULL in the case there is an error.
>
>
> > + 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
>
