Re: [PATCH 1/3] mtd: spi-nand framework

[Ezequiel Garcia <ezequiel.garcia@imgtec.com>]

On 01/07/2015 09:48 PM, Peter Pan 潘栋 (peterpandong) wrote:
[..]
> +
> +static void spi_nand_set_defaults(struct spi_nand_chip *chip)
> +{
> +	struct spi_device *spi = chip->spi;
> +
> +	if (spi->mode & SPI_RX_QUAD)
> +		chip->read_cache = spi_nand_read_from_cache_x4;
> +	else if (spi->mode & SPI_RX_DUAL)
> +		chip->read_cache = spi_nand_read_from_cache_x2;
> +	else
> +		chip->read_cache = spi_nand_read_from_cache;
> +
> +	if (!chip->reset)
> +		chip->reset = spi_nand_reset;
> +	if (!chip->erase_block)
> +		chip->erase_block = spi_nand_erase_block;
> +	if (!chip->load_page)
> +		chip->load_page = spi_nand_read_page_to_cache;
> +	if (!chip->store_cache)
> +		chip->store_cache = spi_nand_program_data_to_cache;

IMO, this is a mistake and one of the reasons why this whole file is so big.

Do you have any reason for keeping the SPI commands (read page to cache,
program data to cache, etc.) in the spi-nand-base.c ?

This shouldn't belong to the framework, but to some spi-nand-device.c
implementing the SPI NAND commands for the devices we have seen so far.
In fact, I don't think you should inherit this "default" hook stuff from
NAND, it only makes the code more obscure.

As Brian noted, such a separation would be benefitial to support a
potential SPI-NAND-specific controller.

However, I think the stronger reason is that it results in a much
simpler and clean initial code.
-- 
Ezequiel