Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 14 Apr 2022 11:03:21 +0200
From:   Michael Walle <michael@walle.cc>
To:     Tudor Ambarus <tudor.ambarus@microchip.com>
Cc:     p.yadav@ti.com, miquel.raynal@bootlin.com, richard@nod.at,
        vigneshr@ti.com, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org, nicolas.ferre@microchip.com
Subject: Re: [PATCH v3 1/3] mtd: spi-nor: Parse BFPT to determine the 4-Byte
 Address Mode methods
In-Reply-To: <20220411125346.118274-2-tudor.ambarus@microchip.com>
References: <20220411125346.118274-1-tudor.ambarus@microchip.com>
 <20220411125346.118274-2-tudor.ambarus@microchip.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <63b485a1e48d3116f8f49cc427049f81@walle.cc>
Precedence: bulk

> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..2e40eba3744d 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -401,6 +401,93 @@ static void
> spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>  	}
>  }
> 
> +/**
> + * spansion_set_4byte_addr_mode() - Set 4-byte address mode for 
> Spansion
> + * flashes.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @enable:	true to enter the 4-byte address mode, false to exit the 
> 4-byte
> + *		address mode.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)

Mh, so now some callback functions are in the core like the quad enable
methods and some are in sfdp.c. I think there should be only the parsing
in sfdp.c and all the callback methods should be in core.c; as they are
potentially used by the vendor modules.

> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		break;
>  	}
> 
> +	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) 
> {

I was wondering why this is encoded as single bits and not as an
enumeration. To me it looks like it is intended to support
different methods at the same time. Thus I think there might be
multiple bits set in each entry and exit mask at once. The spec
lists all the entries as x_xxx1, x_xx1x, ..

> +	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
.. then this will only match if exactly these two bits are set.


> +		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;

Shouldn't this be getting a neutral name? The quad_enable methods
were renamed once, too.

-michael