Re: [RESEND PATCH] mtd: nand: omap2: Remove omap_nand_platform_data

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 12 Oct 2017 17:09:26 +0300
Roger Quadros <rogerq@ti.com> wrote:

> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 12/10/17 14:38, Boris Brezillon wrote:
> > On Thu, 12 Oct 2017 14:28:45 +0300
> > Roger Quadros <rogerq@ti.com> wrote:
> >   
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 10/10/17 12:40, Boris Brezillon wrote:  
> >>> On Fri, 6 Oct 2017 12:35:43 +0200
> >>> Ladislav Michl <ladis@linux-mips.org> wrote:
> >>>    
> >>>> As driver is now configured using DT, omap_nand_platform_data structure
> >>>> is no longer needed.
> >>>>
> >>>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> >>>> ---
> >>>>  Resend to linux-mtd as other part this patch was depending on
> >>>>  was merged months ago via linux-omap tree...
> >>>>
> >>>>  drivers/mtd/nand/omap2.c                     | 40 +++++++---------------------
> >>>>  include/linux/platform_data/mtd-nand-omap2.h | 17 ------------
> >>>>  2 files changed, 10 insertions(+), 47 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> >>>> index 54540c8fa1a2..b1fc070c8279 100644
> >>>> --- a/drivers/mtd/nand/omap2.c
> >>>> +++ b/drivers/mtd/nand/omap2.c
> >>>> @@ -1588,8 +1588,7 @@ static bool is_elm_present(struct omap_nand_info *info,
> >>>>      return true;
> >>>>  }
> >>>>
> >>>> -static bool omap2_nand_ecc_check(struct omap_nand_info *info,
> >>>> -                             struct omap_nand_platform_data *pdata)
> >>>> +static bool omap2_nand_ecc_check(struct omap_nand_info *info)
> >>>>  {
> >>>>      bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm;
> >>>>
> >>>> @@ -1804,7 +1803,6 @@ static const struct mtd_ooblayout_ops omap_sw_ooblayout_ops = {
> >>>>  static int omap_nand_probe(struct platform_device *pdev)
> >>>>  {
> >>>>      struct omap_nand_info           *info;
> >>>> -    struct omap_nand_platform_data  *pdata = NULL;
> >>>>      struct mtd_info                 *mtd;
> >>>>      struct nand_chip                *nand_chip;
> >>>>      int                             err;
> >>>> @@ -1814,6 +1812,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> >>>>      int                             min_oobbytes = BADBLOCK_MARKER_LENGTH;
> >>>>      int                             oobbytes_per_step;
> >>>>
> >>>> +    if (!dev->of_node)
> >>>> +            return -EINVAL;
> >>>> +    
> >>>
> >>> Is this really needed? I expect omap_get_dt_info() to return an error
> >>> when dev->of_node is NULL.
> >>>    
> >>>>      info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
> >>>>                              GFP_KERNEL);
> >>>>      if (!info)
> >>>> @@ -1821,29 +1822,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> >>>>
> >>>>      info->pdev = pdev;
> >>>>
> >>>> -    if (dev->of_node) {
> >>>> -            if (omap_get_dt_info(dev, info))
> >>>> -                    return -EINVAL;
> >>>> -    } else {
> >>>> -            pdata = dev_get_platdata(&pdev->dev);
> >>>> -            if (!pdata) {
> >>>> -                    dev_err(&pdev->dev, "platform data missing\n");
> >>>> -                    return -EINVAL;
> >>>> -            }
> >>>> -
> >>>> -            info->gpmc_cs = pdata->cs;
> >>>> -            info->reg = pdata->reg;
> >>>> -            info->ecc_opt = pdata->ecc_opt;
> >>>> -            if (pdata->dev_ready)
> >>>> -                    dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
> >>>> -
> >>>> -            info->xfer_type = pdata->xfer_type;
> >>>> -            info->devsize = pdata->devsize;
> >>>> -            info->elm_of_node = pdata->elm_of_node;
> >>>> -            info->flash_bbt = pdata->flash_bbt;
> >>>> -    }
> >>>> +    if (omap_get_dt_info(dev, info))
> >>>> +            return -EINVAL;    
> >>
> >> how about
> >>
> >>         err = omap_get_dt_info(dev, info);
> >>         if (err)
> >>                 return err;  
> > 
> > Already done like that in v2 [1]
> >   
> >>  
> >>>>
> >>>> -    platform_set_drvdata(pdev, info);    
> >>>
> >>> This removal seems unrelated to the change you're describing in the
> >>> commit log. I'm not saying we should keep this platform_set_drvdata()
> >>> if it's useless, but it should be done in a separate patch.    
> >>
> >> we do use platform_get_drvdata() in omap_nand_remove() so I suppose
> >> we can't get rid of platorm_set_drvdata()  
> > 
> > There's another platform_set_drvdata() later in the probe function, I
> > think this one is indeed useless (see patch [2])  
> 
> I agree.
> 
> > 
> > [1]http://patchwork.ozlabs.org/patch/823820/
> > [2]http://patchwork.ozlabs.org/patch/823839/

Feel free to add your Acked-by/Tested-by on these patches.

> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >   
>