Dear Geert-san, Wolfram-san
I'm sorry for the delay at the comments of this patch.
And I think it's not hurry!
+ /* Enable hwmon thermal sysfs */
+ tsc->zone->tzp->no_hwmon = false;
+ ret = thermal_add_hwmon_sysfs(tsc->zone);
+ if (ret)
+ dev_err(dev, "Can't register hwmon sysfs\n");
+
dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
}
You forgot to call thermal_remove_hwmon_sysfs() in the .remove()
callback?
I got it, Thanks!
On 2018/11/13 20:57, Wolfram Sang wrote:
Hi Hoan,
+ /* Enable hwmon thermal sysfs */
+ tsc->zone->tzp->no_hwmon = false;
A driver diving so deep into core structures always looks suspicious.
Questions I have:
1)
We have that code also in rcar_thermal.c, but the commit introducing
it[1] has a reason which does not apply for this driver, or?
This is not the reason to apply this patch.
I would say here are the general Linux systems, or as if the systems
that thermal was registered by "thermal_zone_device_register()".
The Linux community has developed tools that use this, such as
"lm-sensors",
my Ubuntu and rcar-Gen2 are still compatible with lm-sensors but Gen3
does not.
Although I do not want to say paradoxical that the kernel drivers must
be compatible
with the user-space applications.
2)
of_parse_thermal_zones() intentionally sets this flag with this
comment:
/* No hwmon because there might be hwmon drivers registering */
Why does it not apply for us?
I think so it should be registered separately.
3)
If it turns out, we really need it, there should be some thermal core
helper or flag for it, I'd think...
Yes, It up to you!
Regards,
Wolfram
[1] 64a411e8042e ("thermal: rcar-thermal: enable hwmon when thermal_zone_of_sensor_register is used")
Thank you very much for your comments :-)!
Hoan.
