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

[Guenter Roeck]

On 3/15/23 19:21, Phinex Hung wrote:
> 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.

Sure. But your argument is inappropriate: You could as well argue that
this system with a single fan should bundle all its thermal sensors into
a single thermal zone, and that it should do so in the driver(s)
providing the thermal zone sensors to the thermal subsystem. This does
not take into account that there might be systems with dozens (or hundreds,
for that matter) of drives, in a system with multiple disk trays and fans
for each of those.

I don't know if and how the thermal subsystem deals with the situation
of having N thermal zone sensors and M << N cooling devices. This is
a general problem, not limited to disk drives. Just as we won't bundle
multiple thermal sensors on a multi-channel thermal sensor chip into a
single thermal zone, we won't bundle multiple disk drives into a single
thermal zone.

> > 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
In theory it should work just like described in the kirkwood devicetree
files. If that doesn't work, the question is how to find the sata port
nodes from the drivetemp driver. I don't have a system with such nodes,
so I have no means to find or know the answer.

I also don't know how to attach more than one thermal sensor to a
single thermal zone, or if that is even possible. If it isn't, it
is a limitation of the thermal subsystem, and trying to hack around
it in a driver providing thermal sensors would be inappropriate.

Guenter