Re: [PATCH V4, 1/2] mtd: nand: brcmnand: Change brcmnand_set_wp() prototype

[Marek Vasut <marek.vasut@gmail.com>]

On 02/28/2017 05:55 PM, Kamal Dasu wrote:
> Changing brcmnand_set_wp() prototype in prepration for refactoring the nand
> write protect logic to add flash status byte check for the protection bit.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 42ebd73..c7c4efe 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -765,11 +765,14 @@ enum {
>  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
>  };
>  
> -static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
> +static int brcmnand_set_wp(struct brcmnand_host *host, int en)
>  {
> +	struct brcmnand_controller *ctrl = host->ctrl;
>  	u32 val = en ? CS_SELECT_NAND_WP : 0;
>  
>  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
> +
> +	return 0;
>  }
>  
>  /***********************************************************************
> @@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>  			dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
>  			old_wp = wp;
>  		}
> -		brcmnand_set_wp(ctrl, wp);
> +		brcmnand_set_wp(host, wp);
>  	}
>  }
>  
> @@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  	/* Disable XOR addressing */
>  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0);
>  
> -	if (ctrl->features & BRCMNAND_HAS_WP) {
> -		/* Permanently disable write protection */
> -		if (wp_on == 2)
> -			brcmnand_set_wp(ctrl, false);
> -	} else {
> -		wp_on = 0;
> -	}
> -
>  	/* IRQ */
>  	ctrl->irq = platform_get_irq(pdev, 0);
>  	if ((int)ctrl->irq < 0) {
> @@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  			}
>  
>  			list_add_tail(&host->node, &ctrl->host_list);
> +
> +			if (ctrl->features & BRCMNAND_HAS_WP) {
> +				/* Permanently disable write protection */
> +				if (wp_on == 2) {
> +					ret = brcmnand_set_wp(host, false);
> +					if (ret)
> +						goto err;
> +				}
> +			} else {
> +				wp_on = 0;
> +			}
> +

Would it be worth it to move this into separate function ? Or maybe make
brcmnand_set_wp() check for BRCMNAND_HAS_WP and only set the WP
if the controller is actually capable of that ? I think that might make
the code readability a bit better ...

>  		}
>  	}
>  
> 


-- 
Best regards,
Marek Vasut