Re: [PATCH v3 09/20] soc: mediatek: mtk-svs: Move t-calibration-data retrieval to svs_probe()

[Eugen Hristev]

On 11/22/23 14:41, AngeloGioacchino Del Regno wrote:
> Il 22/11/23 12:23, Eugen Hristev ha scritto:
> > On 11/21/23 14:50, AngeloGioacchino Del Regno wrote:
> > > The t-calibration-data (SVS-Thermal calibration data) shall exist for
> > > all SoCs or SVS won't work anyway: move it to the common svs_probe()
> > > function and remove it from all of the per-SoC efuse_parsing() probe
> > > callbacks.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno 
> > > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/soc/mediatek/mtk-svs.c | 32 ++++++--------------------------
> > >   1 file changed, 6 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/soc/mediatek/mtk-svs.c 
> > > b/drivers/soc/mediatek/mtk-svs.c
> > > index ab564d48092b..1042af2aee3f 100644
> > > --- a/drivers/soc/mediatek/mtk-svs.c
> > > +++ b/drivers/soc/mediatek/mtk-svs.c
> > > @@ -1884,11 +1884,6 @@ static bool svs_mt8195_efuse_parsing(struct 
> > > svs_platform *svsp)
> > >           svsb->vmax += svsb->dvt_fixed;
> > >       }
> > > -    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > -                 &svsp->tefuse, &svsp->tefuse_max);
> > > -    if (ret)
> > > -        return false;
> > > -
> >
> > Hello Angelo,
> >
> > if you removed the code using `ret` in this patch, it makes sense to 
> > also remove the variable here instead of doing it in patch 18.
> > It will avoid unused variable warnings for this patch.
>
> Yes, though the comment is not for this function, but rather for 8183. 
> Anyway, that
> makes sense, but if it's the only change of this v3, it's something that 
> I can fix
> while applying instead of sending another 20 patches round. Thanks.
>
> > >       for (i = 0; i < svsp->tefuse_max; i++)
> > >           if (svsp->tefuse[i] != 0)
> > >               break;
> > > @@ -1949,11 +1944,6 @@ static bool svs_mt8192_efuse_parsing(struct 
> > > svs_platform *svsp)
> > >           svsb->vmax += svsb->dvt_fixed;
> > >       }
> > > -    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > -                 &svsp->tefuse, &svsp->tefuse_max);
> > > -    if (ret)
> > > -        return false;
> > > -
> > >       for (i = 0; i < svsp->tefuse_max; i++)
> > >           if (svsp->tefuse[i] != 0)
> > >               break;
> > > @@ -2009,11 +1999,6 @@ static bool svs_mt8188_efuse_parsing(struct 
> > > svs_platform *svsp)
> > >           svsb->vmax += svsb->dvt_fixed;
> > >       }
> > > -    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > -                 &svsp->tefuse, &svsp->tefuse_max);
> > > -    if (ret)
> > > -        return false;
> > > -
> > >       for (i = 0; i < svsp->tefuse_max; i++)
> > >           if (svsp->tefuse[i] != 0)
> > >               break;
> > > @@ -2097,11 +2082,6 @@ static bool svs_mt8186_efuse_parsing(struct 
> > > svs_platform *svsp)
> > >           svsb->vmax += svsb->dvt_fixed;
> > >       }
> > > -    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > -                 &svsp->tefuse, &svsp->tefuse_max);
> > > -    if (ret)
> > > -        return false;
> > > -
> > >       golden_temp = (svsp->tefuse[0] >> 24) & GENMASK(7, 0);
> > >       if (!golden_temp)
> > >           golden_temp = 50;
> > > @@ -2198,11 +2178,6 @@ static bool svs_mt8183_efuse_parsing(struct 
> > > svs_platform *svsp)
> > >           }
> > >       }
> > > -    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > -                 &svsp->tefuse, &svsp->tefuse_max);
> > > -    if (ret)
> > > -        return false;
> > > -
> > >       /* Thermal efuse parsing */
> > >       adc_ge_t = (svsp->tefuse[1] >> 22) & GENMASK(9, 0);
> > >       adc_oe_t = (svsp->tefuse[1] >> 12) & GENMASK(9, 0);
> > > @@ -3040,8 +3015,13 @@ static int svs_probe(struct platform_device 
> > > *pdev)
> > >       ret = svs_get_efuse_data(svsp, "svs-calibration-data",
> > >                    &svsp->efuse, &svsp->efuse_max);
> > > +    if (ret)
> > > +        return dev_err_probe(&pdev->dev, ret, "Cannot read SVS 
> > > calibration\n");
> >
> > With the previous code, if svs-calibration-data could not be read, the 
> > code would go to svs_probe_free_efuse. In your case, it returns directly.
> > I believe that svs_get_efuse_data using nvmem_cell_read does not 
> > allocate the buffer for the efuse , hence no more need to free it ? 
> > The exit code is checking if it's ERR or NULL, but still, if the 
> > buffer was not allocated, it doesn't make sense to jump there indeed.
> > In that case, you are also changing the behavior here , and your 
> > commit appears to do more than a simple move.
>
> I'm not changing the behavior: the previous behavior was to fail and 
> free the efuse
> variable if previously allocated, the current behavior is to fail and 
> free the
> efuse variable if previously allocated, and the tefuse variable if 
> previously
> allocated, which is a result of the actual move of the retrieval of the 
> thermal
> fuse calibration data.
>
> I really don't see anything implicit here.

Previous behavior was


ret = svs_get_efuse_data (efuse)

if (ret) goto svs_probe_free_efuse


Now, you have it as

ret = svs_get_efuse_data (efuse)

if (ret) return dev_err_probe...


> > > +
> > > +    ret = svs_get_efuse_data(svsp, "t-calibration-data",
> > > +                 &svsp->tefuse, &svsp->tefuse_max);
> > >       if (ret) {
> > > -        ret = -EPERM;
> > > +        dev_err_probe(&pdev->dev, ret, "Cannot read SVS-Thermal 
> > > calibration\n");
> > >           goto svs_probe_free_efuse;
> >
> > again in this case the tefuse has not been allocated I assume.
> >
> > So previous code was a bit excessive in trying to free the efuse/tefuse ?
>
> The previous code was performing an useless error check on something 
> that was not
> supposed to be allocated *yet*. Yes, it was wrong before.
>
> Cheers,
> Angelo
> _______________________________________________
> Kernel mailing list -- kernel@xxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to kernel-leave@xxxxxxxxxxxxxxxxxxxxx