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
