Re: [PATCH] mtd: nand: convert to unified device property interface

[Kefeng Wang]

On 2017/8/18 5:55, Boris Brezillon wrote:
> Le Wed, 16 Aug 2017 15:29:05 +0800,
> Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>
>> - 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_?
OK.
>
>>  {
>> -	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()?
OK.
>>  {
>> -	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).
Oops, sorry, this patch should be a RFC,  will update it based on your suggestion, thanks.
>
>>  	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).
OK.
>>  	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) {
>>  		/*
>
> .
>