Message-ID: <20170817235502.6c0e8804@bbrezillon>
Date:   Thu, 17 Aug 2017 23:55:02 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Kefeng Wang <wangkefeng.wang@...wei.com>
Cc:     Richard Weinberger <richard@....at>, linux-mtd@...ts.infradead.org,
        linux-kernel@...r.kernel.org, guohanjun@...wei.com
Subject: Re: [PATCH] mtd: nand: convert to unified device property interface

Le Wed, 16 Aug 2017 15:29:05 +0800,
Kefeng Wang <wangkefeng.wang@...wei.com> a écrit :

> Changing from of_* to device_* interface, then we can also extract
> the properties from ACPI tables as well as from DT.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@...wei.com>
> ---
> 
> - APCI will be supported in hisi504_nand.c, and it will use nand_scan_ident().
> 
>  drivers/mtd/nand/nand_base.c | 54 ++++++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index c6c18b8..27a0947 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -46,7 +46,7 @@
>  #include <linux/bitops.h>
>  #include <linux/io.h>
>  #include <linux/mtd/partitions.h>
> -#include <linux/of.h>
> +#include <linux/property.h>
>  
>  static int nand_get_device(struct mtd_info *mtd, int new_state);
>  
> @@ -4209,12 +4209,12 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  	[NAND_ECC_ON_DIE]	= "on-die",
>  };
>  
> -static int of_get_nand_ecc_mode(struct device_node *np)
> +static int device_get_nand_ecc_mode(struct device *dev)
>  {
>  	const char *pm;
>  	int err, i;
>  
> -	err = of_property_read_string(np, "nand-ecc-mode", &pm);
> +	err = device_property_read_string(dev, "nand-ecc-mode", &pm);
>  	if (err < 0)
>  		return err;
>  
> @@ -4238,12 +4238,12 @@ static int of_get_nand_ecc_mode(struct device_node *np)
>  	[NAND_ECC_BCH]		= "bch",
>  };
>  
> -static int of_get_nand_ecc_algo(struct device_node *np)
> +static int device_get_nand_ecc_algo(struct device *dev)
>  {
>  	const char *pm;
>  	int err, i;
>  
> -	err = of_property_read_string(np, "nand-ecc-algo", &pm);
> +	err = device_property_read_string(dev, "nand-ecc-algo", &pm);
>  	if (!err) {
>  		for (i = NAND_ECC_HAMMING; i < ARRAY_SIZE(nand_ecc_algos); i++)
>  			if (!strcasecmp(pm, nand_ecc_algos[i]))
> @@ -4255,7 +4255,7 @@ static int of_get_nand_ecc_algo(struct device_node *np)
>  	 * For backward compatibility we also read "nand-ecc-mode" checking
>  	 * for some obsoleted values that were specifying ECC algorithm.
>  	 */
> -	err = of_property_read_string(np, "nand-ecc-mode", &pm);
> +	err = device_property_read_string(dev, "nand-ecc-mode", &pm);
>  	if (err < 0)
>  		return err;
>  
> @@ -4267,29 +4267,29 @@ static int of_get_nand_ecc_algo(struct device_node *np)
>  	return -ENODEV;
>  }
>  
> -static int of_get_nand_ecc_step_size(struct device_node *np)
> +static int device_get_nand_ecc_step_size(struct device *dev)
>  {
>  	int ret;
>  	u32 val;
>  
> -	ret = of_property_read_u32(np, "nand-ecc-step-size", &val);
> +	ret = device_property_read_u32(dev, "nand-ecc-step-size", &val);
>  	return ret ? ret : val;
>  }
>  
> -static int of_get_nand_ecc_strength(struct device_node *np)
> +static int device_get_nand_ecc_strength(struct device *dev)
>  {
>  	int ret;
>  	u32 val;
>  
> -	ret = of_property_read_u32(np, "nand-ecc-strength", &val);
> +	ret = device_property_read_u32(dev, "nand-ecc-strength", &val);
>  	return ret ? ret : val;
>  }
>  
> -static int of_get_nand_bus_width(struct device_node *np)
> +static int device_get_nand_bus_width(struct device *dev)
>  {
>  	u32 val;
>  
> -	if (of_property_read_u32(np, "nand-bus-width", &val))
> +	if (device_property_read_u32(dev, "nand-bus-width", &val))
>  		return 8;
>  
>  	switch (val) {
> @@ -4301,29 +4301,28 @@ static int of_get_nand_bus_width(struct device_node *np)
>  	}
>  }
>  
> -static bool of_get_nand_on_flash_bbt(struct device_node *np)
> +static bool device_get_nand_on_flash_bbt(struct device *dev)

Not sure I like the device prefix, for the same reason I don't like
nand_chip_init(). How about fw_ or fwnode_?

>  {
> -	return of_property_read_bool(np, "nand-on-flash-bbt");
> +	return device_property_read_bool(dev, "nand-on-flash-bbt");
>  }
>  
> -static int nand_dt_init(struct nand_chip *chip)
> +static int nand_chip_init(struct nand_chip *chip, struct device *dev)

Hm, nand_chip_init() is confusing. How about nand_fwnode_init() or
nand_get_fw_props()?

>  {
> -	struct device_node *dn = nand_get_flash_node(chip);
>  	int ecc_mode, ecc_algo, ecc_strength, ecc_step;
>  
> -	if (!dn)
> +	if (!dev)
>  		return 0;
>  
> -	if (of_get_nand_bus_width(dn) == 16)
> +	if (device_get_nand_bus_width(dev) == 16)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> -	if (of_get_nand_on_flash_bbt(dn))
> +	if (device_get_nand_on_flash_bbt(dev))
>  		chip->bbt_options |= NAND_BBT_USE_FLASH;
>  
> -	ecc_mode = of_get_nand_ecc_mode(dn);
> -	ecc_algo = of_get_nand_ecc_algo(dn);
> -	ecc_strength = of_get_nand_ecc_strength(dn);
> -	ecc_step = of_get_nand_ecc_step_size(dn);
> +	ecc_mode = device_get_nand_ecc_mode(dev);
> +	ecc_algo = device_get_nand_ecc_algo(dev);
> +	ecc_strength = device_get_nand_ecc_strength(dev);
> +	ecc_step = device_get_nand_ecc_step_size(dev);
>  
>  	if (ecc_mode >= 0)
>  		chip->ecc.mode = ecc_mode;
> @@ -4337,7 +4336,7 @@ static int nand_dt_init(struct nand_chip *chip)
>  	if (ecc_step > 0)
>  		chip->ecc.size = ecc_step;
>  
> -	if (of_property_read_bool(dn, "nand-ecc-maximize"))
> +	if (device_property_read_bool(dev, "nand-ecc-maximize"))
>  		chip->ecc.options |= NAND_ECC_MAXIMIZE;
>  
>  	return 0;
> @@ -4358,14 +4357,15 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  {
>  	int i, nand_maf_id, nand_dev_id;
>  	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct device *dev = mtd->dev.parent;

Sorry but this is wrong. If you do that you break all drivers that have
NAND devices represented as children nodes of the NAND controller.

If you want to support ACPI, you'll have to use &mtd->dev as the
reference dev, and make sure its ->fwnode is correctly set (just patch
nand_get_flash_node() to do the right thing).

>  	int ret;
>  
> -	ret = nand_dt_init(chip);
> +	ret = nand_chip_init(chip, dev);

No need to pass dev as a second argument here (can be extracted inside
the function).

>  	if (ret)
>  		return ret;
>  
> -	if (!mtd->name && mtd->dev.parent)
> -		mtd->name = dev_name(mtd->dev.parent);
> +	if (!mtd->name && dev)
> +		mtd->name = dev_name(dev);
>  
>  	if ((!chip->cmdfunc || !chip->select_chip) && !chip->cmd_ctrl) {
>  		/*

Powered by blists - more mailing lists