Re: [PATCH v1] hwmon: drivetemp: support to be a platform driver for thermal_of

[Phinex Hung]

On 3/15/23 11:36, Guenter Roeck" <groeck7@xxxxxxxxx <mailto:groeck7@xxxxxxxxx> wrote:

>This is conceptually wrong. It returns the maximum temperature from all drives,
>not the temperature from a single drive.

>This is not much different from collecting all temperatures from all sensors
>in the system and declaring the maximum of those as single thermal zone.

>If anything, each drive would have to reflect a thermal zone. The big question
>is how to determine the associated devicetree property.

My basic idea is to use a single thermal zone for multiple disks.

In most of the systems, there might be only a single fan that used for cooling.

If each disk has its own thermal zone, we need to add almost the same dts entries for each thermal zone,

and do almost the same cooling operations.

That is why I am trying to using a single thermal zone for multiple disks.

In any case, if temperature of any disk goes high, cooling should take effect.

>Also, essentially your patch claims that arch/arm/boot/dts/kirkwood-nsa310s.dts
>doesn't work and no one ever noticed. I would like to see that confirmed.

To be honest, my first attempt to get your drivetemp.c works in our SoC was referring to a similar patch,

https://lore.kernel.org/linux-arm-kernel/CAJN1KkzR7NR8TguS7uDs6peDOpkFn0duVBqvKKzm3xnMs9iJ7A@xxxxxxxxxxxxxx/T/

By adding the similar entries in our dts but failed.

Two hwmon devices actually populated, a thermal zone described by my dts also was created.

But there is no link between the thermal zone described in dts and hwmons from drivetemp driver.

So that I traced the source and finding that the thermal zone registration failed due to the lack of a device node of a platform device.

That is why I am trying to use a platform device for registration again.

The original call to hwmon_device_register_with_info should call hwmon_thermal_register_sensors, 

But int the last call to thermal_zone_of_sensor_register, dev->of_node would be checked.

This would cause the sensor registration to fail while hwmon still worked.

hwmon_device_register_with_info => __hwmon_device_register => hwmon_thermal_register_sensors =>

hwmon_thermal_add_sensor => devm_thermal_zone_of_sensor_register => thermal_zone_of_sensor_register =>

struct thermal_zone_device *
thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
                                const struct thermal_zone_of_device_ops *ops)
{
        struct device_node *np, *child, *sensor_np;
        struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);

        np = of_find_node_by_name(NULL, "thermal-zones");
        if (!np)
                return ERR_PTR(-ENODEV);

        if (!dev || !dev->of_node) {
                of_node_put(np);
                return ERR_PTR(-ENODEV);
        }

I am also curious why Kirkwood SoC works without specific patch.

In any case, if device tree binding is used, how could we associate drivetemp sensors with a specific thermal zone

if no compatible string is described for these drivetemp sensors in the dts file? 

That is why I am adding a generic compatible string to match a platform sensors with a specific thermal zone.

Regards,
Phinex