2015-11-27 08:05:51

by Anup Limbu

[permalink] [raw]
Subject: [PATCH] mtd: nand: davinci: remove unnecessary if cases

remove unnecessary if cases

Signed-off-by: Anup Limbu <[email protected]>
---
drivers/mtd/nand/davinci_nand.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index c72313d..1f86ee1 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -568,21 +568,16 @@ static struct davinci_nand_pdata
pdev->dev.platform_data = pdata;
if (!pdata)
return ERR_PTR(-ENOMEM);
- if (!of_property_read_u32(pdev->dev.of_node,
- "ti,davinci-chipselect", &prop))
- pdev->id = prop;
- else
+ if (of_property_read_u32(pdev->dev.of_node,
+ "ti,davinci-chipselect", &pdev->id))
return ERR_PTR(-EINVAL);

- if (!of_property_read_u32(pdev->dev.of_node,
- "ti,davinci-mask-ale", &prop))
- pdata->mask_ale = prop;
- if (!of_property_read_u32(pdev->dev.of_node,
- "ti,davinci-mask-cle", &prop))
- pdata->mask_cle = prop;
- if (!of_property_read_u32(pdev->dev.of_node,
- "ti,davinci-mask-chipsel", &prop))
- pdata->mask_chipsel = prop;
+ of_property_read_u32(pdev->dev.of_node,
+ "ti,davinci-mask-ale", &pdata->mask_ale);
+ of_property_read_u32(pdev->dev.of_node,
+ "ti,davinci-mask-cle", &pdata->mask_cle);
+ of_property_read_u32(pdev->dev.of_node,
+ "ti,davinci-mask-chipsel", &pdata->mask_chipsel);
if (!of_property_read_string(pdev->dev.of_node,
"nand-ecc-mode", &mode) ||
!of_property_read_string(pdev->dev.of_node,
@@ -594,9 +589,8 @@ static struct davinci_nand_pdata
if (!strncmp("hw", mode, 2))
pdata->ecc_mode = NAND_ECC_HW;
}
- if (!of_property_read_u32(pdev->dev.of_node,
- "ti,davinci-ecc-bits", &prop))
- pdata->ecc_bits = prop;
+ of_property_read_u32(pdev->dev.of_node,
+ "ti,davinci-ecc-bits", &pdata->ecc_bits);

prop = of_get_nand_bus_width(pdev->dev.of_node);
if (0 < prop || !of_property_read_u32(pdev->dev.of_node,
--
1.9.1


2015-12-01 00:40:13

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH] mtd: nand: davinci: remove unnecessary if cases

On Fri, Nov 27, 2015 at 01:35:34PM +0530, Anup Limbu wrote:
> remove unnecessary if cases
>
> Signed-off-by: Anup Limbu <[email protected]>
> ---

Did you test this?

> drivers/mtd/nand/davinci_nand.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index c72313d..1f86ee1 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -568,21 +568,16 @@ static struct davinci_nand_pdata
> pdev->dev.platform_data = pdata;
> if (!pdata)
> return ERR_PTR(-ENOMEM);
> - if (!of_property_read_u32(pdev->dev.of_node,
> - "ti,davinci-chipselect", &prop))
> - pdev->id = prop;
> - else
> + if (of_property_read_u32(pdev->dev.of_node,
> + "ti,davinci-chipselect", &pdev->id))
> return ERR_PTR(-EINVAL);
>
> - if (!of_property_read_u32(pdev->dev.of_node,
> - "ti,davinci-mask-ale", &prop))
> - pdata->mask_ale = prop;
> - if (!of_property_read_u32(pdev->dev.of_node,
> - "ti,davinci-mask-cle", &prop))
> - pdata->mask_cle = prop;
> - if (!of_property_read_u32(pdev->dev.of_node,
> - "ti,davinci-mask-chipsel", &prop))
> - pdata->mask_chipsel = prop;
> + of_property_read_u32(pdev->dev.of_node,
> + "ti,davinci-mask-ale", &pdata->mask_ale);
> + of_property_read_u32(pdev->dev.of_node,
> + "ti,davinci-mask-cle", &pdata->mask_cle);
> + of_property_read_u32(pdev->dev.of_node,
> + "ti,davinci-mask-chipsel", &pdata->mask_chipsel);
> if (!of_property_read_string(pdev->dev.of_node,
> "nand-ecc-mode", &mode) ||
> !of_property_read_string(pdev->dev.of_node,
> @@ -594,9 +589,8 @@ static struct davinci_nand_pdata
> if (!strncmp("hw", mode, 2))
> pdata->ecc_mode = NAND_ECC_HW;
> }
> - if (!of_property_read_u32(pdev->dev.of_node,
> - "ti,davinci-ecc-bits", &prop))
> - pdata->ecc_bits = prop;
> + of_property_read_u32(pdev->dev.of_node,
> + "ti,davinci-ecc-bits", &pdata->ecc_bits);

NAK! You're treating a u8 as u32. This should give you some subset of
the following warnings, from gcc and sparse:

In file included from include/linux/of_device.h:8:0,
from drivers/mtd/nand/davinci_nand.c:35:
include/linux/of.h:857:19: note: expected ‘u32 *’ but argument is of type ‘u8 *’
drivers/mtd/nand/davinci_nand.c: In function ‘nand_davinci_get_pdata’:
drivers/mtd/nand/davinci_nand.c:593:4: warning: passing argument 3 of ‘of_property_read_u32’ from incompatible pointer type [enabled by default]
drivers/mtd/nand/davinci_nand.c:572:51: warning: incorrect type in argument 3 (different signedness) [sparse]
drivers/mtd/nand/davinci_nand.c:572:51: expected unsigned int [usertype] *out_value [sparse]
drivers/mtd/nand/davinci_nand.c:572:51: got int *<noident> [sparse]
drivers/mtd/nand/davinci_nand.c:593:49: warning: incorrect type in argument 3 (different type sizes) [sparse]
drivers/mtd/nand/davinci_nand.c:593:49: expected unsigned int [usertype] *out_value [sparse]
drivers/mtd/nand/davinci_nand.c:593:49: got unsigned char *<noident> [sparse]


and it leaves you open to memory corruption, as of_property_read_u32()
might try clobbering 3 more bytes of data after the ecc_bits field.

All in all, there's a reason the code is written the way it is, and I
don't see any good reason to do what your patch does.

Brian

>
> prop = of_get_nand_bus_width(pdev->dev.of_node);
> if (0 < prop || !of_property_read_u32(pdev->dev.of_node,
> --
> 1.9.1
>