Re: [RFC PATCH] mtd: devices: add AT24 eeprom support

[Bartosz Golaszewski]

On Mon, Nov 27, 2023 at 5:46 PM Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
>

[snip]

>
> I dropped the backward compatibility since this is a new driver not
> having to deal with it. The old and the new driver can not be used by
> the same kernel config. So it is either using the MTD eeprom driver
> supporting partitioning and NVMEM or the older one which does not
> support partitioning but keeps the backward compatibility.
>
> Comments and suggestions are very welcome :)

I skimmed through the code. Nothing obviously wrong. What I would
suggest - if we're going to have two at24 drivers - is a lot more code
reuse. I dislike the idea of having basically the same code in two
places in the kernel and having to fix bugs in both.

Though if I'm being honest - I would prefer a single driver with
backwards compatibility. Have you estimated the effort it would take
to abstract both nvmem and mtd?

[snip]

> +
> +static const struct of_device_id at24_of_match[] = {
> +       { .compatible = "atmel,24c00",          .data = &at24_data_24c00 },
> +       { .compatible = "atmel,24c01",          .data = &at24_data_24c01 },
> +       { .compatible = "atmel,24cs01",         .data = &at24_data_24cs01 },
> +       { .compatible = "atmel,24c02",          .data = &at24_data_24c02 },
> +       { .compatible = "atmel,24cs02",         .data = &at24_data_24cs02 },
> +       { .compatible = "atmel,24mac402",       .data = &at24_data_24mac402 },
> +       { .compatible = "atmel,24mac602",       .data = &at24_data_24mac602 },
> +       { .compatible = "atmel,spd",            .data = &at24_data_spd },
> +       { .compatible = "atmel,24c04",          .data = &at24_data_24c04 },
> +       { .compatible = "atmel,24cs04",         .data = &at24_data_24cs04 },
> +       { .compatible = "atmel,24c08",          .data = &at24_data_24c08 },
> +       { .compatible = "atmel,24cs08",         .data = &at24_data_24cs08 },
> +       { .compatible = "atmel,24c16",          .data = &at24_data_24c16 },
> +       { .compatible = "atmel,24cs16",         .data = &at24_data_24cs16 },
> +       { .compatible = "atmel,24c32",          .data = &at24_data_24c32 },
> +       { .compatible = "atmel,24cs32",         .data = &at24_data_24cs32 },
> +       { .compatible = "atmel,24c64",          .data = &at24_data_24c64 },
> +       { .compatible = "atmel,24cs64",         .data = &at24_data_24cs64 },
> +       { .compatible = "atmel,24c128",         .data = &at24_data_24c128 },
> +       { .compatible = "atmel,24c256",         .data = &at24_data_24c256 },
> +       { .compatible = "atmel,24c512",         .data = &at24_data_24c512 },
> +       { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
> +       { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
> +       { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
> +       { /* END OF LIST */ },
> +};
> +MODULE_DEVICE_TABLE(of, at24_of_match);

This is one of examples: I have a patch queued for the nvmem version
where we use of_match_ptr() and add __maybe_unused to this struct.
There's no reason really to have that struct duplicated.

[snip]

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 6d83e72a24d2..1a850b19515d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -148,6 +148,9 @@ static ssize_t mtd_type_show(struct device *dev,
>         case MTD_ROM:
>                 type = "rom";
>                 break;
> +       case MTD_EEPROM:
> +               type = "eeprom";
> +               break;
>         case MTD_NORFLASH:
>                 type = "nor";
>                 break;
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 714d55b49d2a..59bf43d58ddb 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -146,6 +146,7 @@ struct mtd_read_req {
>  #define MTD_DATAFLASH          6
>  #define MTD_UBIVOLUME          7
>  #define MTD_MLCNANDFLASH       8       /* MLC NAND (including TLC) */
> +#define MTD_EEPROM             9
>
>  #define MTD_WRITEABLE          0x400   /* Device is writeable */
>  #define MTD_BIT_WRITEABLE      0x800   /* Single bits can be flipped */
> @@ -159,6 +160,7 @@ struct mtd_read_req {
>  #define MTD_CAP_NORFLASH       (MTD_WRITEABLE | MTD_BIT_WRITEABLE)
>  #define MTD_CAP_NANDFLASH      (MTD_WRITEABLE)
>  #define MTD_CAP_NVRAM          (MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_NO_ERASE)
> +#define MTD_CAP_EEPROM         (MTD_WRITEABLE | MTD_BIT_WRITEABLE | MTD_NO_ERASE)
>
>  /* Obsolete ECC byte placement modes (used with obsolete MEMGETOOBSEL) */
>  #define MTD_NANDECC_OFF                0       /* Switch off ECC (Not recommended) */
> --
> 2.39.2
>

The infrastructure for supporting EEPROM should be sent as a separate patch IMO.

Bart