Messages in this thread | | | Date | Wed, 22 Nov 2023 14:51:31 +0200 | Subject | Re: [PATCH v3 09/20] soc: mediatek: mtk-svs: Move t-calibration-data retrieval to svs_probe() | From | 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@collabora.com> >>> --- >>> 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@mailman.collabora.com > To unsubscribe send an email to kernel-leave@mailman.collabora.com
| |