[RESEND] mtd: nand: omap2: Remove omap_nand_platform_data

Message ID 20171006103543.gnb2hxq7msallyzl@lenoch
State Superseded
Headers show
Series
  • [RESEND] mtd: nand: omap2: Remove omap_nand_platform_data
Related show

Commit Message

Ladislav Michl Oct. 6, 2017, 10:35 a.m.
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(-)

Comments

Boris Brezillon Oct. 10, 2017, 9:40 a.m. | #1
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;
>  
> -	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.

>  	info->ops = gpmc_omap_get_nand_ops(&info->reg, info->gpmc_cs);
>  	if (!info->ops) {
>  		dev_err(&pdev->dev, "Failed to get GPMC->NAND interface\n");
> @@ -2002,7 +1983,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		goto return_error;
>  	}
>  
> -	if (!omap2_nand_ecc_check(info, pdata)) {
> +	if (!omap2_nand_ecc_check(info)) {
>  		err = -EINVAL;
>  		goto return_error;
>  	}
> @@ -2167,10 +2148,9 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	if (err)
>  		goto return_error;
>  
> -	if (dev->of_node)
> -		mtd_device_register(mtd, NULL, 0);
> -	else
> -		mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
> +	err = mtd_device_register(mtd, NULL, 0);
> +	if (err)
> +		goto return_error;
>  
>  	platform_set_drvdata(pdev, mtd);
>  
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index 25e267f1970c..619df2431e75 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -64,21 +64,4 @@ struct gpmc_nand_regs {
>  	void __iomem	*gpmc_bch_result5[GPMC_BCH_NUM_REMAINDER];
>  	void __iomem	*gpmc_bch_result6[GPMC_BCH_NUM_REMAINDER];
>  };
> -
> -struct omap_nand_platform_data {
> -	int			cs;
> -	struct mtd_partition	*parts;
> -	int			nr_parts;
> -	bool			flash_bbt;
> -	enum nand_io		xfer_type;
> -	int			devsize;
> -	enum omap_ecc           ecc_opt;
> -
> -	struct device_node	*elm_of_node;
> -
> -	/* deprecated */
> -	struct gpmc_nand_regs	reg;
> -	struct device_node	*of_node;
> -	bool			dev_ready;
> -};
>  #endif
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Roger Quadros Oct. 12, 2017, 11:28 a.m. | #2
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;

>>
>> -    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().

We need to test if the patch survives a suspend/resume.

>
>>      info->ops = gpmc_omap_get_nand_ops(&info->reg, info->gpmc_cs);
>>      if (!info->ops) {
>>              dev_err(&pdev->dev, "Failed to get GPMC->NAND interface\n");
>> @@ -2002,7 +1983,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>>              goto return_error;
>>      }
>>
>> -    if (!omap2_nand_ecc_check(info, pdata)) {
>> +    if (!omap2_nand_ecc_check(info)) {
>>              err = -EINVAL;
>>              goto return_error;
>>      }
>> @@ -2167,10 +2148,9 @@ static int omap_nand_probe(struct platform_device *pdev)
>>      if (err)
>>              goto return_error;
>>
>> -    if (dev->of_node)
>> -            mtd_device_register(mtd, NULL, 0);
>> -    else
>> -            mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
>> +    err = mtd_device_register(mtd, NULL, 0);
>> +    if (err)
>> +            goto return_error;
>>
>>      platform_set_drvdata(pdev, mtd);
>>
>> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
>> index 25e267f1970c..619df2431e75 100644
>> --- a/include/linux/platform_data/mtd-nand-omap2.h
>> +++ b/include/linux/platform_data/mtd-nand-omap2.h
>> @@ -64,21 +64,4 @@ struct gpmc_nand_regs {
>>      void __iomem    *gpmc_bch_result5[GPMC_BCH_NUM_REMAINDER];
>>      void __iomem    *gpmc_bch_result6[GPMC_BCH_NUM_REMAINDER];
>>  };
>> -
>> -struct omap_nand_platform_data {
>> -    int                     cs;
>> -    struct mtd_partition    *parts;
>> -    int                     nr_parts;
>> -    bool                    flash_bbt;
>> -    enum nand_io            xfer_type;
>> -    int                     devsize;
>> -    enum omap_ecc           ecc_opt;
>> -
>> -    struct device_node      *elm_of_node;
>> -
>> -    /* deprecated */
>> -    struct gpmc_nand_regs   reg;
>> -    struct device_node      *of_node;
>> -    bool                    dev_ready;
>> -};
>>  #endif
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
> --
> 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
>

--
cheers,
-roger
Boris Brezillon Oct. 12, 2017, 11:38 a.m. | #3
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])

[1]http://patchwork.ozlabs.org/patch/823820/
[2]http://patchwork.ozlabs.org/patch/823839/
Roger Quadros Oct. 12, 2017, 2:09 p.m. | #4

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/
> --
> 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
>
Boris Brezillon Oct. 12, 2017, 2:15 p.m. | #5
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
> >   
>

Patch

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;
+
 	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;
 
-	platform_set_drvdata(pdev, info);
 	info->ops = gpmc_omap_get_nand_ops(&info->reg, info->gpmc_cs);
 	if (!info->ops) {
 		dev_err(&pdev->dev, "Failed to get GPMC->NAND interface\n");
@@ -2002,7 +1983,7 @@  static int omap_nand_probe(struct platform_device *pdev)
 		goto return_error;
 	}
 
-	if (!omap2_nand_ecc_check(info, pdata)) {
+	if (!omap2_nand_ecc_check(info)) {
 		err = -EINVAL;
 		goto return_error;
 	}
@@ -2167,10 +2148,9 @@  static int omap_nand_probe(struct platform_device *pdev)
 	if (err)
 		goto return_error;
 
-	if (dev->of_node)
-		mtd_device_register(mtd, NULL, 0);
-	else
-		mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
+	err = mtd_device_register(mtd, NULL, 0);
+	if (err)
+		goto return_error;
 
 	platform_set_drvdata(pdev, mtd);
 
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 25e267f1970c..619df2431e75 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -64,21 +64,4 @@  struct gpmc_nand_regs {
 	void __iomem	*gpmc_bch_result5[GPMC_BCH_NUM_REMAINDER];
 	void __iomem	*gpmc_bch_result6[GPMC_BCH_NUM_REMAINDER];
 };
-
-struct omap_nand_platform_data {
-	int			cs;
-	struct mtd_partition	*parts;
-	int			nr_parts;
-	bool			flash_bbt;
-	enum nand_io		xfer_type;
-	int			devsize;
-	enum omap_ecc           ecc_opt;
-
-	struct device_node	*elm_of_node;
-
-	/* deprecated */
-	struct gpmc_nand_regs	reg;
-	struct device_node	*of_node;
-	bool			dev_ready;
-};
 #endif