Re: [PATCH 6/6] thermal: versal-thermal: Support thermal management in AI Engine

Krzysztof Kozlowski
2025-09-05T10:48:11Z

On 05/09/2025 10:41, Michal Simek wrote:
> + vti->channel = iio_channel_get(NULL, SYSMON_TEMP_CH_NAME);
> + if (IS_ERR(vti->channel)) {
> + vti->num_aie_channels = 0;
> + versal_thermal_iio_chan_release(vti);
> + return dev_err_probe(&pdev->dev, PTR_ERR(vti->channel),
> + "IIO channel %s not found\n",
> + SYSMON_TEMP_CH_NAME);
> + }
> +
> vti->tzd = devm_thermal_of_zone_register(&pdev->dev, 0, vti, &thermal_zone_ops);
> if (IS_ERR(vti->tzd))
> return dev_err_probe(&pdev->dev, PTR_ERR(vti->tzd),
> "Thermal zone sensor register failed\n");
>
> - return devm_thermal_add_hwmon_sysfs(&pdev->dev, vti->tzd);
> + ret = devm_thermal_add_hwmon_sysfs(&pdev->dev, vti->tzd);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to add hwmon sysfs for sysmon temp\n");
> +
> + sysmon_node = of_find_node_by_name(NULL, "sysmon");

Undocumented ABI. Please don't do that. sysmon is not an approved name
and for sure can be changed anytime to anything.

Phandles express relationships usually.

> + if (sysmon_node) {
> + ret = of_property_read_u32(sysmon_node, "xlnx,numaiechannels",
> + &num_aie_channels);
> + if (ret < 0)
> + num_aie_channels = 0;
> + }
> +
> + if (num_aie_channels > 0) {
> + aie_temp_chan_names = devm_kcalloc(&pdev->dev, num_aie_channels,
> + sizeof(*aie_temp_chan_names),
> + GFP_KERNEL);
> + if (!aie_temp_chan_names)
> + return -ENOMEM;
> +
> + for_each_child_of_node(sysmon_node, child_node) {
> + if (of_property_present(child_node, "xlnx,aie-temp")) {
> + ret = of_property_read_string(child_node, "xlnx,name",
> + &aie_temp_chan_name);
> + if (ret < 0) {
> + of_node_put(child_node);
> + return ret;
> + }
> + aie_temp_chan_names[aie_ch_index] = aie_temp_chan_name;
> + aie_ch_index++;
> + }
> + }
> +
> + /* Allocate memory for the dynamic aie temperature channels */
> + vti->channel_aie = devm_kcalloc(&pdev->dev, num_aie_channels,
> + sizeof(*vti->channel_aie), GFP_KERNEL);
> + if (!vti->channel_aie)
> + return -ENOMEM;
> +
> + for (aie_ch_index = 0; aie_ch_index < num_aie_channels; aie_ch_index++) {
> + vti->channel_aie[aie_ch_index] =
> + iio_channel_get(NULL, aie_temp_chan_names[aie_ch_index]);
> + if (IS_ERR(vti->channel_aie[aie_ch_index])) {
> + vti->num_aie_channels = aie_ch_index + 1;
> + versal_thermal_iio_chan_release(vti);
> + return dev_err_probe(&pdev->dev,
> + PTR_ERR(vti->channel_aie[aie_ch_index]),
> + "IIO AIE TEMP channel %s not found\n",
> + aie_temp_chan_names[aie_ch_index]);
> + }
> + }
> +
> + vti->tzd_aie = devm_thermal_of_zone_register(&pdev->dev, 1, vti,
> + &thermal_zone_ops_aie);
> + if (IS_ERR(vti->tzd_aie))
> + return dev_err_probe(&pdev->dev, PTR_ERR(vti->tzd_aie),
> + "Failed to register thermal zone aie temp\n");
> +
> + ret = devm_thermal_add_hwmon_sysfs(&pdev->dev, vti->tzd_aie);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to add hwmon sysfs for aie temp\n");
> + }
> + vti->num_aie_channels = num_aie_channels;
> + platform_set_drvdata(pdev, vti);
> + return 0;
> +}
> +
> +static void versal_thermal_remove(struct platform_device *pdev)
> +{
> + struct versal_thermal_info *vti = platform_get_drvdata(pdev);
> +
> + versal_thermal_iio_chan_release(vti);

Don't mix non-devm with devm. This should be a proper devm cleanup action.

Best regards,
Krzysztof