Re: [PATCHv2 1/3] EDAC/fsl_ddr: use devm_platform_ioremap_resource()

[Rosen Penev]

On Fri, Jul 31, 2026 at 8:05 AM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> On Thu, Jul 30, 2026 at 06:21:28PM -0700, Rosen Penev wrote:
> > Replace the open-coded of_address_to_resource() plus devm_request_mem_region()
> > and devm_ioremap() sequence with devm_platform_ioremap_resource(), which folds
> > the resource lookup, region reservation and mapping into one step and returns
> > an ERR_PTR checked with IS_ERR()/PTR_ERR().
> >
> > Behaviorally equivalent with respect to region reservation: the driver
> > already reserved the region, so the non-overlapping reg requirement was
> > already satisfied. Drop the now-unused linux/of_address.h include.
> >
> > Remove devres stuff. Not needed. devm is already used.
> >
> > Fix a mistaken return code of -ENOMEM when PTR_ERR should be used.
> >
> > Assisted-by: opencode:hy3-free
> > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > ---
>
> Need change log here.
>
> You missed my review-by and tested-by tags. If you delete it by some reason
> like big changes, you should said here explicit.
I changed some stuff yeah. I had them added but removed just in case.
>
> Frank
>
> >  drivers/edac/fsl_ddr_edac.c | 39 +++++++------------------------------
> >  1 file changed, 7 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 2a545e5812e7..3745eec67335 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -22,7 +22,6 @@
> >  #include <linux/gfp.h>
> >
> >  #include <linux/of.h>
> > -#include <linux/of_address.h>
> >  #include "edac_module.h"
> >  #include "fsl_ddr_edac.h"
> >
> > @@ -495,13 +494,14 @@ int fsl_mc_err_probe(struct platform_device *op)
> >       struct mem_ctl_info *mci;
> >       struct edac_mc_layer layers[2];
> >       struct fsl_mc_pdata *pdata;
> > -     struct resource r;
> > +     void __iomem *mc_vbase;
> >       u32 ecc_en_mask;
> >       u32 sdram_ctl;
> >       int res;
> >
> > -     if (!devres_open_group(&op->dev, fsl_mc_err_probe, GFP_KERNEL))
> > -             return -ENOMEM;
> > +     mc_vbase = devm_platform_ioremap_resource(op, 0);
> > +     if (IS_ERR(mc_vbase))
> > +             return PTR_ERR(mc_vbase);
> >
> >       layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> >       layers[0].size = 4;
> > @@ -511,10 +511,8 @@ int fsl_mc_err_probe(struct platform_device *op)
> >       layers[1].is_virt_csrow = false;
> >       mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
> >                           sizeof(*pdata));
> > -     if (!mci) {
> > -             devres_release_group(&op->dev, fsl_mc_err_probe);
> > +     if (!mci)
> >               return -ENOMEM;
> > -     }
> >
> >       pdata = mci->pvt_info;
> >       pdata->name = "fsl_mc_err";
> > @@ -531,33 +529,12 @@ int fsl_mc_err_probe(struct platform_device *op)
> >        * Default is big endian.
> >        */
> >       pdata->little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
> > -
> > -     res = of_address_to_resource(op->dev.of_node, 0, &r);
> > -     if (res) {
> > -             pr_err("%s: Unable to get resource for MC err regs\n",
> > -                    __func__);
> > -             goto err;
> > -     }
> > -
> > -     if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
> > -                                  pdata->name)) {
> > -             pr_err("%s: Error while requesting mem region\n",
> > -                    __func__);
> > -             res = -EBUSY;
> > -             goto err;
> > -     }
> > -
> > -     pdata->mc_vbase = devm_ioremap(&op->dev, r.start, resource_size(&r));
> > -     if (!pdata->mc_vbase) {
> > -             pr_err("%s: Unable to setup MC err regs\n", __func__);
> > -             res = -ENOMEM;
> > -             goto err;
> > -     }
> > +     pdata->mc_vbase = mc_vbase;
> >
> >       if (pdata->flag == TYPE_IMX9) {
> >               pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
> >               if (IS_ERR(pdata->inject_vbase)) {
> > -                     res = -ENOMEM;
> > +                     res = PTR_ERR(pdata->inject_vbase);
> >                       goto err;
> >               }
> >       }
> > @@ -637,7 +614,6 @@ int fsl_mc_err_probe(struct platform_device *op)
> >                      pdata->irq);
> >       }
> >
> > -     devres_remove_group(&op->dev, fsl_mc_err_probe);
> >       edac_dbg(3, "success\n");
> >       pr_info(EDAC_MOD_STR " MC err registered\n");
> >
> > @@ -646,7 +622,6 @@ int fsl_mc_err_probe(struct platform_device *op)
> >  err2:
> >       edac_mc_del_mc(&op->dev);
> >  err:
> > -     devres_release_group(&op->dev, fsl_mc_err_probe);
> >       edac_mc_free(mci);
> >       return res;
> >  }
> > --
> > 2.55.0
> >
> >