| Message ID | 20220928155519.31977-5-matthias.bgg@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: mediatek: mtk-svs: refactor and cleanup the driver | expand |
Il 28/09/22 17:55, matthias.bgg@kernel.org ha scritto: > From: Matthias Brugger <matthias.bgg@gmail.com> > > The platform name and efuse parsing function pointer are only used while > probing the device. Use them from the svs_platform_data struct instead. > > Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi Matthias Sir, On Wed, 2022-09-28 at 17:55 +0200, matthias.bgg@kernel.org wrote: > From: Matthias Brugger <matthias.bgg@gmail.com> > > The platform name and efuse parsing function pointer are only used while > probing the device. Use them from the svs_platform_data struct instead. > > Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > > drivers/soc/mediatek/mtk-svs.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c > index 8342627f8dea..482cc8d7e7cf 100644 > --- a/drivers/soc/mediatek/mtk-svs.c > +++ b/drivers/soc/mediatek/mtk-svs.c > @@ -311,14 +311,12 @@ static const u32 svs_regs_v2[] = { > > /** > * struct svs_platform - svs platform control > - * @name: svs platform name > * @base: svs platform register base > * @dev: svs platform device > * @main_clk: main clock for svs bank > * @pbank: svs bank pointer needing to be protected by spin_lock section > * @banks: svs banks that svs platform supports > * @rst: svs platform reset control > - * @efuse_parsing: svs platform efuse parsing function pointer > * @efuse_max: total number of svs efuse > * @tefuse_max: total number of thermal efuse > * @regs: svs platform registers map > @@ -327,14 +325,12 @@ static const u32 svs_regs_v2[] = { > * @tefuse: thermal efuse data received from NVMEM framework > */ > struct svs_platform { > - char *name; > void __iomem *base; > struct device *dev; > struct clk *main_clk; > struct svs_bank *pbank; > struct svs_bank *banks; > struct reset_control *rst; > - bool (*efuse_parsing)(struct svs_platform *svsp); > size_t efuse_max; > size_t tefuse_max; > const u32 *regs; > @@ -2009,7 +2005,7 @@ static bool svs_is_efuse_data_correct(struct > svs_platform *svsp) > svsp->efuse_max /= sizeof(u32); > nvmem_cell_put(cell); > > - return svsp->efuse_parsing(svsp); > + return true; > } Based on the current coding design, I think this function can be removed. We can rename/refactor `svs_thermal_efuse_get_data()` to `svs_get_efuse_data()` as below. Is this acceptable? Thanks. int svs_get_efuse_data(struct svs_platform *svsp, const char *nvmem_cell_name) { struct nvmem_cell *cell; /* Thermal efuse parsing */ cell = nvmem_cell_get(svsp->dev, nvmem_cell_name); if (IS_ERR_OR_NULL(cell)) { dev_err(svsp->dev, "no \"%s\"? %ld\n", nvmem_cell_name, PTR_ERR(cell)); return PTR_ERR(cell); } svsp->tefuse = nvmem_cell_read(cell, &svsp->tefuse_max); if (IS_ERR(svsp->tefuse)) { dev_err(svsp->dev, "cannot read thermal efuse: %ld\n", PTR_ERR(svsp->tefuse)); nvmem_cell_put(cell); return PTR_ERR(svsp->tefuse); } svsp->tefuse_max /= sizeof(u32); nvmem_cell_put(cell); return 0; } > > static struct device *svs_get_subsys_device(struct svs_platform *svsp, > @@ -2338,9 +2334,7 @@ static int svs_probe(struct platform_device *pdev) > return -ENOMEM; > > svsp->dev = &pdev->dev; > - svsp->name = svsp_data->name; > svsp->banks = svsp_data->banks; > - svsp->efuse_parsing = svsp_data->efuse_parsing; > svsp->regs = svsp_data->regs; > svsp->bank_max = svsp_data->bank_max; > > @@ -2351,6 +2345,12 @@ static int svs_probe(struct platform_device *pdev) > if (!svs_is_efuse_data_correct(svsp)) { > dev_notice(svsp->dev, "efuse data isn't correct\n"); > ret = -EPERM; > + goto svs_probe_free_efuse; > + } > + Remove `svs_is_efuse_data_correct()` and add below get-efuse-data function. ret = svs_get_efuse_data(svsp, "svs-calibration-data"); if (ret) goto svs_probe_free_efuse; > + if (!svsp_data->efuse_parsing(svsp)) { > + dev_notice(svsp->dev, "efuse data parsing failed\n"); > + ret = -EPERM; > goto svs_probe_free_resource; > } > > @@ -2367,7 +2367,7 @@ static int svs_probe(struct platform_device *pdev) > } > > ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr, > - IRQF_ONESHOT, svsp->name, svsp); > + IRQF_ONESHOT, svsp_data->name, svsp); > if (ret) { > dev_err(svsp->dev, "register irq(%d) failed: %d\n", > svsp_irq, ret); > @@ -2416,11 +2416,13 @@ static int svs_probe(struct platform_device *pdev) > clk_disable_unprepare(svsp->main_clk); > > svs_probe_free_resource: For current coding design, I suggest we rename the goto-label to `svs_probe_free_tefuse` to identify the resource we free. > - if (!IS_ERR_OR_NULL(svsp->efuse)) > - kfree(svsp->efuse); > if (!IS_ERR_OR_NULL(svsp->tefuse)) > kfree(svsp->tefuse); > > +svs_probe_free_efuse: > + if (!IS_ERR_OR_NULL(svsp->efuse)) > + kfree(svsp->efuse); > + > return ret; > } > Sincerely, Roger Lu.
diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c index 8342627f8dea..482cc8d7e7cf 100644 --- a/drivers/soc/mediatek/mtk-svs.c +++ b/drivers/soc/mediatek/mtk-svs.c @@ -311,14 +311,12 @@ static const u32 svs_regs_v2[] = { /** * struct svs_platform - svs platform control - * @name: svs platform name * @base: svs platform register base * @dev: svs platform device * @main_clk: main clock for svs bank * @pbank: svs bank pointer needing to be protected by spin_lock section * @banks: svs banks that svs platform supports * @rst: svs platform reset control - * @efuse_parsing: svs platform efuse parsing function pointer * @efuse_max: total number of svs efuse * @tefuse_max: total number of thermal efuse * @regs: svs platform registers map @@ -327,14 +325,12 @@ static const u32 svs_regs_v2[] = { * @tefuse: thermal efuse data received from NVMEM framework */ struct svs_platform { - char *name; void __iomem *base; struct device *dev; struct clk *main_clk; struct svs_bank *pbank; struct svs_bank *banks; struct reset_control *rst; - bool (*efuse_parsing)(struct svs_platform *svsp); size_t efuse_max; size_t tefuse_max; const u32 *regs; @@ -2009,7 +2005,7 @@ static bool svs_is_efuse_data_correct(struct svs_platform *svsp) svsp->efuse_max /= sizeof(u32); nvmem_cell_put(cell); - return svsp->efuse_parsing(svsp); + return true; } static struct device *svs_get_subsys_device(struct svs_platform *svsp, @@ -2338,9 +2334,7 @@ static int svs_probe(struct platform_device *pdev) return -ENOMEM; svsp->dev = &pdev->dev; - svsp->name = svsp_data->name; svsp->banks = svsp_data->banks; - svsp->efuse_parsing = svsp_data->efuse_parsing; svsp->regs = svsp_data->regs; svsp->bank_max = svsp_data->bank_max; @@ -2351,6 +2345,12 @@ static int svs_probe(struct platform_device *pdev) if (!svs_is_efuse_data_correct(svsp)) { dev_notice(svsp->dev, "efuse data isn't correct\n"); ret = -EPERM; + goto svs_probe_free_efuse; + } + + if (!svsp_data->efuse_parsing(svsp)) { + dev_notice(svsp->dev, "efuse data parsing failed\n"); + ret = -EPERM; goto svs_probe_free_resource; } @@ -2367,7 +2367,7 @@ static int svs_probe(struct platform_device *pdev) } ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr, - IRQF_ONESHOT, svsp->name, svsp); + IRQF_ONESHOT, svsp_data->name, svsp); if (ret) { dev_err(svsp->dev, "register irq(%d) failed: %d\n", svsp_irq, ret); @@ -2416,11 +2416,13 @@ static int svs_probe(struct platform_device *pdev) clk_disable_unprepare(svsp->main_clk); svs_probe_free_resource: - if (!IS_ERR_OR_NULL(svsp->efuse)) - kfree(svsp->efuse); if (!IS_ERR_OR_NULL(svsp->tefuse)) kfree(svsp->tefuse); +svs_probe_free_efuse: + if (!IS_ERR_OR_NULL(svsp->efuse)) + kfree(svsp->efuse); + return ret; }