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

[Daniel Lezcano]

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()