[PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Tudor Ambarus]

We use the size as an indicator whether to parse SFDP or not. We don't
introduce a dedicated member for SFDP parsing because we'd like to keep
the struct size at a minimum, as it's used for every flash declaration.
Ideally we won't have flash entries at all, but there are still flash
parameters that aren't defined by SFDP, thus we need to statically
specify them.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
Describe member to avoid confusion.
---
 drivers/mtd/spi-nor/core.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 5c33740ed7f5..ceff412f7d65 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -448,7 +448,11 @@ struct spi_nor_id {
  * @id:   pointer to struct spi_nor_id or NULL, which means "no ID" (mostly
  *        older chips).
  * @name: (obsolete) the name of the flash. Do not set it for new additions.
- * @size:           the size of the flash in bytes.
+ * @size:           the size of the flash in bytes. The flash size is one
+ *                  property parsed by the SFDP. We use it as an indicator
+ *                  whether we need SFDP parsing for a particular flash.
+ *                  I.e. non-legacy flash entries in flash_info will have
+ *                  a size of zero iff SFDP should be used.
  * @sector_size:    (optional) the size listed here is what works with
  *                  SPINOR_OP_SE, which isn't necessarily called a "sector" by
  *                  the vendor. Defaults to 64k.

---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241219-spi-nor-flash-info-size-desc-2dc3157e593a

Best regards,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>


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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Miquel Raynal]

On 19/12/2024 at 09:20:04 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> We use the size as an indicator whether to parse SFDP or not. We don't
> introduce a dedicated member for SFDP parsing because we'd like to keep
> the struct size at a minimum, as it's used for every flash declaration.
> Ideally we won't have flash entries at all, but there are still flash
> parameters that aren't defined by SFDP, thus we need to statically
> specify them.

TBH I'd be perfectly fine (and prefer) a dedicated member, because
code clarity is IMO more important than memory footprint today.

However for now I am totally agreeing with changes clarifying what we
currently do, so:

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

One minor nit below though:

> + * @size:           the size of the flash in bytes. The flash size is one
> + *                  property parsed by the SFDP. We use it as an indicator
> + *                  whether we need SFDP parsing for a particular flash.
> + *                  I.e. non-legacy flash entries in flash_info will have
> + *                  a size of zero iff SFDP should be used.

typo, 'if'                            ^^^

Cheers,
Miquèl

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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Tudor Ambarus]

On 12/19/24 9:38 AM, Miquel Raynal wrote:
> On 19/12/2024 at 09:20:04 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>> We use the size as an indicator whether to parse SFDP or not. We don't
>> introduce a dedicated member for SFDP parsing because we'd like to keep
>> the struct size at a minimum, as it's used for every flash declaration.
>> Ideally we won't have flash entries at all, but there are still flash
>> parameters that aren't defined by SFDP, thus we need to statically
>> specify them.
> 
> TBH I'd be perfectly fine (and prefer) a dedicated member, because
> code clarity is IMO more important than memory footprint today.
> 
> However for now I am totally agreeing with changes clarifying what we
> currently do, so:
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> One minor nit below though:
> 
>> + * @size:           the size of the flash in bytes. The flash size is one
>> + *                  property parsed by the SFDP. We use it as an indicator
>> + *                  whether we need SFDP parsing for a particular flash.
>> + *                  I.e. non-legacy flash entries in flash_info will have
>> + *                  a size of zero iff SFDP should be used.
> 
> typo, 'if'                            ^^^
> 

iff is a conjunction: if and only if.

I think Michael used it to emphasize that we don't allow size > 0 when
we want SFDP parsing. Using if allows it.

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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Miquel Raynal]

On 19/12/2024 at 10:01:41 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> On 12/19/24 9:38 AM, Miquel Raynal wrote:
>> On 19/12/2024 at 09:20:04 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>> 
>>> We use the size as an indicator whether to parse SFDP or not. We don't
>>> introduce a dedicated member for SFDP parsing because we'd like to keep
>>> the struct size at a minimum, as it's used for every flash declaration.
>>> Ideally we won't have flash entries at all, but there are still flash
>>> parameters that aren't defined by SFDP, thus we need to statically
>>> specify them.
>> 
>> TBH I'd be perfectly fine (and prefer) a dedicated member, because
>> code clarity is IMO more important than memory footprint today.
>> 
>> However for now I am totally agreeing with changes clarifying what we
>> currently do, so:
>> 
>> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> 
>> One minor nit below though:
>> 
>>> + * @size:           the size of the flash in bytes. The flash size is one
>>> + *                  property parsed by the SFDP. We use it as an indicator
>>> + *                  whether we need SFDP parsing for a particular flash.
>>> + *                  I.e. non-legacy flash entries in flash_info will have
>>> + *                  a size of zero iff SFDP should be used.
>> 
>> typo, 'if'                            ^^^
>> 
>
> iff is a conjunction: if and only if.
>
> I think Michael used it to emphasize that we don't allow size > 0 when
> we want SFDP parsing. Using if allows it.

Ah, ok, I didn't know the abbreviation.

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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Michael Walle]

[-- Attachment #1.1: Type: text/plain, Size: 1969 bytes --]

Hi,

On Thu Dec 19, 2024 at 5:15 PM CET, Miquel Raynal wrote:
> On 19/12/2024 at 10:01:41 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> > On 12/19/24 9:38 AM, Miquel Raynal wrote:
> >> On 19/12/2024 at 09:20:04 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >> 
> >>> We use the size as an indicator whether to parse SFDP or not. We don't
> >>> introduce a dedicated member for SFDP parsing because we'd like to keep
> >>> the struct size at a minimum, as it's used for every flash declaration.
> >>> Ideally we won't have flash entries at all, but there are still flash
> >>> parameters that aren't defined by SFDP, thus we need to statically
> >>> specify them.

Thanks!

Reviewed-by: Michael Walle <mwalle@kernel.org>

> >> 
> >> TBH I'd be perfectly fine (and prefer) a dedicated member, because
> >> code clarity is IMO more important than memory footprint today.

But that would have meant more code churn, because the default
should be the new method. Thus we'd have to change all the old
entries.

> >> However for now I am totally agreeing with changes clarifying what we
> >> currently do, so:
> >> 
> >> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> 
> >> One minor nit below though:
> >> 
> >>> + * @size:           the size of the flash in bytes. The flash size is one
> >>> + *                  property parsed by the SFDP. We use it as an indicator
> >>> + *                  whether we need SFDP parsing for a particular flash.
> >>> + *                  I.e. non-legacy flash entries in flash_info will have
> >>> + *                  a size of zero iff SFDP should be used.
> >> 
> >> typo, 'if'                            ^^^
> >> 
> >
> > iff is a conjunction: if and only if.
> >
> > I think Michael used it to emphasize that we don't allow size > 0 when
> > we want SFDP parsing. Using if allows it.

Yep :)

> Ah, ok, I didn't know the abbreviation.

-michael

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Miquel Raynal]

>> >> TBH I'd be perfectly fine (and prefer) a dedicated member, because
>> >> code clarity is IMO more important than memory footprint today.
>
> But that would have meant more code churn, because the default
> should be the new method. Thus we'd have to change all the old
> entries.

Oh yeah, I feel it would be a patchset on its own. I'm just mentioning
it if someone has the willingness some day. As for now, it is important
to properly document how we use the fields.

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

Re: [PATCH] mtd: spi-nor: extend description of size member of struct flash_info

[Pratyush Yadav]

On Thu, Dec 19 2024, Tudor Ambarus wrote:

> We use the size as an indicator whether to parse SFDP or not. We don't
> introduce a dedicated member for SFDP parsing because we'd like to keep
> the struct size at a minimum, as it's used for every flash declaration.
> Ideally we won't have flash entries at all, but there are still flash
> parameters that aren't defined by SFDP, thus we need to statically
> specify them.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Applied to spi-nor/next. Thanks!

-- 
Regards,
Pratyush Yadav

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