Re: [PATCH v3 4/8] thermal: amlogic: Add support for secure monitor calibration readout

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

On 4/23/26 17:09, Ronald Claveau wrote:
> Hi Daniel,
> 
> Thanks for your feedback.
> 
> On 4/23/26 12:25 PM, Daniel Lezcano wrote:
>>
>> Hi Ronald,
>>

function1() {


>>> +    if (pdata->data->use_sm) {
>>> +        struct device_node *sm_np;
>>> +        struct of_phandle_args ph_args;
>>> +
>>> +        ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>>> +                               "amlogic,secure-monitor",
>>> +                               1, 0, &ph_args);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        sm_np = ph_args.np;
>>> +        if (!sm_np) {
>>> +            dev_err(dev,
>>> +                "Failed to parse secure monitor phandle\n");
>>> +            return -ENODEV;
>>> +        }
>>> +
>>> +        pdata->sm_fw = meson_sm_get(sm_np);
>>> +        of_node_put(sm_np);
>>> +        if (!pdata->sm_fw) {
>>> +            dev_err(dev, "Failed to get secure monitor firmware\n");
>>> +            return -EPROBE_DEFER;
>>> +        }
>>> +
>>> +        pdata->tsensor_id = ph_args.args[0];
>>> +    } 

}

function2() {

else {
>>> +        pdata->sec_ao_map = syscon_regmap_lookup_by_phandle
>>> +            (pdev->dev.of_node, "amlogic,ao-secure");
>>> +        if (IS_ERR(pdata->sec_ao_map)) {
>>> +            dev_err(dev, "syscon regmap lookup failed.\n");
>>> +            return PTR_ERR(pdata->sec_ao_map);
>>> +        }
>>>        }

}

> Sure, I will do that.
> 
>>>        pdata->tzd = devm_thermal_of_zone_register(&pdev->dev
>>
>> The thermal zone is registered before calling
>> amlogic_thermal_initialize(), thus pdata->trim_info is not initialized.
>> When a thermal zone is registered the thermal framework reads the
>> temperature, so it reads an invalid value because:
>>
>> devm_thermal_of_zone_register()
>>   -> thermal_of_zone_register()
>>     -> thermal_zone_device_register_with_trips()
>>     -> thermal_zone_device_enable()
>>        -> __thermal_zone_device_update()
>>          -> __thermal_zone_get_temp()
>>            -> amlogic_thermal_get_temp()
>>               -> amlogic_thermal_code_to_millicelsius()
>>                   [ Use of uninitialized pdata->trim_info ]
>>
>> Right ?
>>
> 
> Yes, I will move the initialize before the register.
> 
>> IIUC, amlogic_thermal_initialize() can be also split and moved the
>> corresponding blocks to the functions to be created in the comment above.
>>
> 
> The SM and syscon setup will be extracted into two functions.
> amlogic_thermal_initialize() itself is kept as-is but moved before
> devm_thermal_of_zone_register().
> Let me know if you'd prefer a different approach.

In the initialize function the following chunk is added:

+    if (pdata->data->use_sm) {
+        return meson_sm_get_thermal_calib(pdata->sm_fw,
+                          &pdata->trim_info,
+                          pdata->tsensor_id);
+    }

I was suggesting to move it to function1() and the rest of code from 
initialize in function2()

That results in amlogic_thermal_initialize() to be dissolved in 
function1() and function2()