Re: [PATCH v6 18/30] mtd: rawnand: timings: Make onfi_fill_interface_config() a void helper

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Rickard Andersson <rickaran@axis.com>,
	Michal Simek <monstr@monstr.eu>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Naga Sureshkumar Relli <nagasure@xilinx.com>
Subject: Re: [PATCH v6 18/30] mtd: rawnand: timings: Make onfi_fill_interface_config() a void helper
Date: Fri, 29 May 2020 09:27:35 +0200	[thread overview]
Message-ID: <20200529092735.04c02100@collabora.com> (raw)
In-Reply-To: <20200528231612.8958-19-miquel.raynal@bootlin.com>

On Fri, 29 May 2020 01:16:00 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Warn the user if the parameters are wrong but basically it would mean
> there is a serious issue in the NAND core. So no need to ever check
> its output, let's make this helper return void.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

One comment below.

> ---
>  drivers/mtd/nand/raw/internals.h    |  8 ++++----
>  drivers/mtd/nand/raw/nand_base.c    |  6 ++----
>  drivers/mtd/nand/raw/nand_timings.c | 19 +++++++++----------
>  3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index 93386bdfbad6..11415b054d2e 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -84,10 +84,10 @@ int nand_bbm_get_next_page(struct nand_chip *chip, int page);
>  int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
>  int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>  		    int allowbbt);
> -int onfi_fill_interface_config(struct nand_chip *chip,
> -			       struct nand_interface_config *iface,
> -			       enum nand_interface_type type,
> -			       int timing_mode);
> +void onfi_fill_interface_config(struct nand_chip *chip,
> +				struct nand_interface_config *iface,
> +				enum nand_interface_type type,
> +				int timing_mode);
>  unsigned int
>  onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings);
>  int nand_get_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index de596c06c9c2..07a7d918b462 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1041,10 +1041,8 @@ static int nand_choose_interface(struct nand_chip *chip)
>  	}
>  
>  	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> -		ret = onfi_fill_interface_config(chip, &chip->interface_config,
> -						 NAND_SDR_IFACE, mode);
> -		if (ret)
> -			continue;
> +		onfi_fill_interface_config(chip, &chip->interface_config,
> +					   NAND_SDR_IFACE, mode);
>  
>  		/*
>  		 * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
> diff --git a/drivers/mtd/nand/raw/nand_timings.c b/drivers/mtd/nand/raw/nand_timings.c
> index 997a1dd929de..6c12cd52323d 100644
> --- a/drivers/mtd/nand/raw/nand_timings.c
> +++ b/drivers/mtd/nand/raw/nand_timings.c
> @@ -347,18 +347,19 @@ onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings)
>   * @type: The interface type
>   * @timing_mode: The ONFI timing mode
>   */
> -int onfi_fill_interface_config(struct nand_chip *chip,
> -			       struct nand_interface_config *iface,
> -			       enum nand_interface_type type,
> -			       int timing_mode)
> +void onfi_fill_interface_config(struct nand_chip *chip,
> +				struct nand_interface_config *iface,
> +				enum nand_interface_type type,
> +				int timing_mode)
>  {
>  	struct onfi_params *onfi = chip->parameters.onfi;
>  
> -	if (type != NAND_SDR_IFACE)
> -		return -EINVAL;
> +	if (WARN_ON(type != NAND_SDR_IFACE))
> +		return;
>  
> -	if (timing_mode < 0 || timing_mode >= ARRAY_SIZE(onfi_sdr_timings))
> -		return -EINVAL;
> +	if (WARN_ON(timing_mode < 0 ||

timing_mode should probably be unsigned, that would allow us to get rid
of this test.

> +		    timing_mode >= ARRAY_SIZE(onfi_sdr_timings)))
> +		return;
>  
>  	*iface = onfi_sdr_timings[timing_mode];
>  
> @@ -378,6 +379,4 @@ int onfi_fill_interface_config(struct nand_chip *chip,
>  		/* nanoseconds -> picoseconds */
>  		timings->tCCS_min = 1000UL * onfi->tCCS;
>  	}
> -
> -	return 0;
>  }

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/