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

[Ronald Claveau <linux-kernel-dev@aliel.fr>]

On 4/23/26 5:17 PM, Daniel Lezcano wrote:
> 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()
> 

Thanks for your answer, that was my second option, I will do that.

-- 
Best regards,
Ronald