| Message ID | 20220124120915.41292-3-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] remoteproc: mtk_scp: Use devm variant of rproc_alloc() | expand |
Hi Angelo, On Mon, Jan 24, 2022 at 01:09:15PM +0100, AngeloGioacchino Del Regno wrote: > Simplify the probe function, where possible, by using dev_err_probe(). > While at it, as to increase human readability, also remove some > unnecessary forced void pointer casts that were previously used in > error checking. I am in favour of all 3 patches (please add a cover letter next time) but weary about testing - do you have access to a Mediatek platform to try this on or is it purely theoretical? I would definitely feel better to see a "Tested-by" tag by someone out there with access to the HW. Thanks, Mathieu > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/remoteproc/mtk_scp.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > index e40706b0e015..dcddb33e9997 100644 > --- a/drivers/remoteproc/mtk_scp.c > +++ b/drivers/remoteproc/mtk_scp.c > @@ -757,10 +757,8 @@ static int scp_probe(struct platform_device *pdev) > int ret, i; > > rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp)); > - if (!rproc) { > - dev_err(dev, "unable to allocate remoteproc\n"); > - return -ENOMEM; > - } > + if (!rproc) > + return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n"); > > scp = (struct mtk_scp *)rproc->priv; > scp->rproc = rproc; > @@ -770,21 +768,20 @@ static int scp_probe(struct platform_device *pdev) > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > scp->sram_base = devm_ioremap_resource(dev, res); > - if (IS_ERR((__force void *)scp->sram_base)) { > - dev_err(dev, "Failed to parse and map sram memory\n"); > - return PTR_ERR((__force void *)scp->sram_base); > - } > + if (IS_ERR(scp->sram_base)) > + return dev_err_probe(dev, PTR_ERR(scp->sram_base), > + "Failed to parse and map sram memory\n"); > + > scp->sram_size = resource_size(res); > scp->sram_phys = res->start; > > /* l1tcm is an optional memory region */ > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > scp->l1tcm_base = devm_ioremap_resource(dev, res); > - if (IS_ERR((__force void *)scp->l1tcm_base)) { > - ret = PTR_ERR((__force void *)scp->l1tcm_base); > + if (IS_ERR(scp->l1tcm_base)) { > + ret = PTR_ERR(scp->l1tcm_base); > if (ret != -EINVAL) { > - dev_err(dev, "Failed to map l1tcm memory\n"); > - return ret; > + return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n"); > } > } else { > scp->l1tcm_size = resource_size(res); > @@ -792,10 +789,9 @@ static int scp_probe(struct platform_device *pdev) > } > > scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); > - if (IS_ERR((__force void *)scp->reg_base)) { > - dev_err(dev, "Failed to parse and map cfg memory\n"); > - return PTR_ERR((__force void *)scp->reg_base); > - } > + if (IS_ERR(scp->reg_base)) > + return dev_err_probe(dev, PTR_ERR(scp->reg_base), > + "Failed to parse and map cfg memory\n"); > > ret = scp->data->scp_clk_get(scp); > if (ret) > -- > 2.33.1 >
Il 01/02/22 19:36, Mathieu Poirier ha scritto: > Hi Angelo, > > On Mon, Jan 24, 2022 at 01:09:15PM +0100, AngeloGioacchino Del Regno wrote: >> Simplify the probe function, where possible, by using dev_err_probe(). >> While at it, as to increase human readability, also remove some >> unnecessary forced void pointer casts that were previously used in >> error checking. > > I am in favour of all 3 patches (please add a cover letter next time) but weary > about testing - do you have access to a Mediatek platform to try this on or > is it purely theoretical? > > I would definitely feel better to see a "Tested-by" tag by someone out there > with access to the HW. > > Thanks, > Mathieu > Hello Mathieu, I have multiple MediaTek platforms and I always test on all of them before pushing such commits upstream, so, even though this kind of patch is trivial, this is *not* purely theoretical and I confirm that this was successfully tested. Regards, Angelo >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> --- >> drivers/remoteproc/mtk_scp.c | 28 ++++++++++++---------------- >> 1 file changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c >> index e40706b0e015..dcddb33e9997 100644 >> --- a/drivers/remoteproc/mtk_scp.c >> +++ b/drivers/remoteproc/mtk_scp.c >> @@ -757,10 +757,8 @@ static int scp_probe(struct platform_device *pdev) >> int ret, i; >> >> rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp)); >> - if (!rproc) { >> - dev_err(dev, "unable to allocate remoteproc\n"); >> - return -ENOMEM; >> - } >> + if (!rproc) >> + return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n"); >> >> scp = (struct mtk_scp *)rproc->priv; >> scp->rproc = rproc; >> @@ -770,21 +768,20 @@ static int scp_probe(struct platform_device *pdev) >> >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); >> scp->sram_base = devm_ioremap_resource(dev, res); >> - if (IS_ERR((__force void *)scp->sram_base)) { >> - dev_err(dev, "Failed to parse and map sram memory\n"); >> - return PTR_ERR((__force void *)scp->sram_base); >> - } >> + if (IS_ERR(scp->sram_base)) >> + return dev_err_probe(dev, PTR_ERR(scp->sram_base), >> + "Failed to parse and map sram memory\n"); >> + >> scp->sram_size = resource_size(res); >> scp->sram_phys = res->start; >> >> /* l1tcm is an optional memory region */ >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); >> scp->l1tcm_base = devm_ioremap_resource(dev, res); >> - if (IS_ERR((__force void *)scp->l1tcm_base)) { >> - ret = PTR_ERR((__force void *)scp->l1tcm_base); >> + if (IS_ERR(scp->l1tcm_base)) { >> + ret = PTR_ERR(scp->l1tcm_base); >> if (ret != -EINVAL) { >> - dev_err(dev, "Failed to map l1tcm memory\n"); >> - return ret; >> + return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n"); >> } >> } else { >> scp->l1tcm_size = resource_size(res); >> @@ -792,10 +789,9 @@ static int scp_probe(struct platform_device *pdev) >> } >> >> scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); >> - if (IS_ERR((__force void *)scp->reg_base)) { >> - dev_err(dev, "Failed to parse and map cfg memory\n"); >> - return PTR_ERR((__force void *)scp->reg_base); >> - } >> + if (IS_ERR(scp->reg_base)) >> + return dev_err_probe(dev, PTR_ERR(scp->reg_base), >> + "Failed to parse and map cfg memory\n"); >> >> ret = scp->data->scp_clk_get(scp); >> if (ret) >> -- >> 2.33.1 >>
On Wed, 2 Feb 2022 at 02:03, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 01/02/22 19:36, Mathieu Poirier ha scritto: > > Hi Angelo, > > > > On Mon, Jan 24, 2022 at 01:09:15PM +0100, AngeloGioacchino Del Regno wrote: > >> Simplify the probe function, where possible, by using dev_err_probe(). > >> While at it, as to increase human readability, also remove some > >> unnecessary forced void pointer casts that were previously used in > >> error checking. > > > > I am in favour of all 3 patches (please add a cover letter next time) but weary > > about testing - do you have access to a Mediatek platform to try this on or > > is it purely theoretical? > > > > I would definitely feel better to see a "Tested-by" tag by someone out there > > with access to the HW. > > > > Thanks, > > Mathieu > > > > Hello Mathieu, > > I have multiple MediaTek platforms and I always test on all of them before > pushing such commits upstream, so, even though this kind of patch is trivial, > this is *not* purely theoretical and I confirm that this was successfully tested. I have applied all 3 patches. > > Regards, > Angelo > > >> > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > >> --- > >> drivers/remoteproc/mtk_scp.c | 28 ++++++++++++---------------- > >> 1 file changed, 12 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c > >> index e40706b0e015..dcddb33e9997 100644 > >> --- a/drivers/remoteproc/mtk_scp.c > >> +++ b/drivers/remoteproc/mtk_scp.c > >> @@ -757,10 +757,8 @@ static int scp_probe(struct platform_device *pdev) > >> int ret, i; > >> > >> rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp)); > >> - if (!rproc) { > >> - dev_err(dev, "unable to allocate remoteproc\n"); > >> - return -ENOMEM; > >> - } > >> + if (!rproc) > >> + return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n"); > >> > >> scp = (struct mtk_scp *)rproc->priv; > >> scp->rproc = rproc; > >> @@ -770,21 +768,20 @@ static int scp_probe(struct platform_device *pdev) > >> > >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > >> scp->sram_base = devm_ioremap_resource(dev, res); > >> - if (IS_ERR((__force void *)scp->sram_base)) { > >> - dev_err(dev, "Failed to parse and map sram memory\n"); > >> - return PTR_ERR((__force void *)scp->sram_base); > >> - } > >> + if (IS_ERR(scp->sram_base)) > >> + return dev_err_probe(dev, PTR_ERR(scp->sram_base), > >> + "Failed to parse and map sram memory\n"); > >> + > >> scp->sram_size = resource_size(res); > >> scp->sram_phys = res->start; > >> > >> /* l1tcm is an optional memory region */ > >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); > >> scp->l1tcm_base = devm_ioremap_resource(dev, res); > >> - if (IS_ERR((__force void *)scp->l1tcm_base)) { > >> - ret = PTR_ERR((__force void *)scp->l1tcm_base); > >> + if (IS_ERR(scp->l1tcm_base)) { > >> + ret = PTR_ERR(scp->l1tcm_base); > >> if (ret != -EINVAL) { > >> - dev_err(dev, "Failed to map l1tcm memory\n"); > >> - return ret; > >> + return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n"); > >> } > >> } else { > >> scp->l1tcm_size = resource_size(res); > >> @@ -792,10 +789,9 @@ static int scp_probe(struct platform_device *pdev) > >> } > >> > >> scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); > >> - if (IS_ERR((__force void *)scp->reg_base)) { > >> - dev_err(dev, "Failed to parse and map cfg memory\n"); > >> - return PTR_ERR((__force void *)scp->reg_base); > >> - } > >> + if (IS_ERR(scp->reg_base)) > >> + return dev_err_probe(dev, PTR_ERR(scp->reg_base), > >> + "Failed to parse and map cfg memory\n"); > >> > >> ret = scp->data->scp_clk_get(scp); > >> if (ret) > >> -- > >> 2.33.1 > >> > > > -- > AngeloGioacchino Del Regno > Software Engineer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c index e40706b0e015..dcddb33e9997 100644 --- a/drivers/remoteproc/mtk_scp.c +++ b/drivers/remoteproc/mtk_scp.c @@ -757,10 +757,8 @@ static int scp_probe(struct platform_device *pdev) int ret, i; rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp)); - if (!rproc) { - dev_err(dev, "unable to allocate remoteproc\n"); - return -ENOMEM; - } + if (!rproc) + return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n"); scp = (struct mtk_scp *)rproc->priv; scp->rproc = rproc; @@ -770,21 +768,20 @@ static int scp_probe(struct platform_device *pdev) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); scp->sram_base = devm_ioremap_resource(dev, res); - if (IS_ERR((__force void *)scp->sram_base)) { - dev_err(dev, "Failed to parse and map sram memory\n"); - return PTR_ERR((__force void *)scp->sram_base); - } + if (IS_ERR(scp->sram_base)) + return dev_err_probe(dev, PTR_ERR(scp->sram_base), + "Failed to parse and map sram memory\n"); + scp->sram_size = resource_size(res); scp->sram_phys = res->start; /* l1tcm is an optional memory region */ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm"); scp->l1tcm_base = devm_ioremap_resource(dev, res); - if (IS_ERR((__force void *)scp->l1tcm_base)) { - ret = PTR_ERR((__force void *)scp->l1tcm_base); + if (IS_ERR(scp->l1tcm_base)) { + ret = PTR_ERR(scp->l1tcm_base); if (ret != -EINVAL) { - dev_err(dev, "Failed to map l1tcm memory\n"); - return ret; + return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n"); } } else { scp->l1tcm_size = resource_size(res); @@ -792,10 +789,9 @@ static int scp_probe(struct platform_device *pdev) } scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg"); - if (IS_ERR((__force void *)scp->reg_base)) { - dev_err(dev, "Failed to parse and map cfg memory\n"); - return PTR_ERR((__force void *)scp->reg_base); - } + if (IS_ERR(scp->reg_base)) + return dev_err_probe(dev, PTR_ERR(scp->reg_base), + "Failed to parse and map cfg memory\n"); ret = scp->data->scp_clk_get(scp); if (ret)
Simplify the probe function, where possible, by using dev_err_probe(). While at it, as to increase human readability, also remove some unnecessary forced void pointer casts that were previously used in error checking. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/remoteproc/mtk_scp.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)