Re: [PATCH v3 1/3] eeprom: at24: avoid adjusting offset for 24AA025E{48, 64}

[<Andrei.Simion@microchip.com>]

On 28.06.2024 17:46, Bartosz Golaszewski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jun 28, 2024 at 4:17 PM <Andrei.Simion@microchip.com> wrote:
>>
>> On 28.06.2024 11:30, Bartosz Golaszewski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Jun 28, 2024 at 10:02 AM Andrei Simion
>>> <andrei.simion@microchip.com> wrote:
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>
>>>> The EEPROMs could be used only for MAC storage. In this case the
>>>> EEPROM areas where MACs resides could be modeled as NVMEM cells
>>>> (directly via DT bindings) such that the already available networking
>>>> infrastructure to read properly the MAC addresses (via
>>>> of_get_mac_address()). The previously available compatibles needs the
>>>> offset adjustment probably for compatibility w/ old DT bindings.
>>>> Add "microchip,24aa025e48", "microchip,24aa025e64" compatible for the
>>>> usage w/ 24AA025E{48, 64} type of EEPROMs where "24aa025e48" stands
>>>> for EUI-48 address and "24aa025e64" stands for EUI-64 address.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> [andrei.simion@microchip.com: Add extended macros to initialize the structure
>>>> with explicit value to adjusting offset. Add extra description for the commit
>>>> message.]
>>>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>>>> ---
>>>> v2 -> v3:
>>>> - add specific compatible names according with
>>>> https://ww1.microchip.com/downloads/en/DeviceDoc/24AA02E48-24AA025E48-24AA02E64-24AA025E64-Data-Sheet-20002124H.pdf
>>>> - add extended macros to initialize the structure with explicit value for adjoff
>>>> - drop co-developed-by to maintain the commit history
>>>>  (chronological order of modifications)
>>>>
>>>> v1 -> v2:
>>>> - no change
>>>> ---
>>>>  drivers/misc/eeprom/at24.c | 28 +++++++++++++++++++++++-----
>>>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>> index 4bd4f32bcdab..e2ac08f656cf 100644
>>>> --- a/drivers/misc/eeprom/at24.c
>>>> +++ b/drivers/misc/eeprom/at24.c
>>>> @@ -121,20 +121,29 @@ struct at24_chip_data {
>>>>         u32 byte_len;
>>>>         u8 flags;
>>>>         u8 bank_addr_shift;
>>>> +       u8 adjoff;
>>>>         void (*read_post)(unsigned int off, char *buf, size_t count);
>>>>  };
>>>>
>>>> -#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>>>> +#define AT24_CHIP_DATA_AO(_name, _len, _flags, _ao)                    \
>>>
>>> Please, don't try to save space on a few letters, call it
>>> AT24_CHIP_DATA_ADJOFF() for better readability.
>>>
>>
>> I will change in next the version.
>>
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> +               .adjoff = _ao                                           \
>>>>         }
>>>>
>>>> -#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>>>> +#define AT24_CHIP_DATA(_name, _len, _flags)                            \
>>>> +       AT24_CHIP_DATA_AO(_name, _len, _flags, 0)
>>>> +
>>>> +#define AT24_CHIP_DATA_CB_AO(_name, _len, _flags, _ao, _read_post)     \
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> +               .adjoff = _ao,                                          \
>>>>                 .read_post = _read_post,                                \
>>>>         }
>>>>
>>>> +#define AT24_CHIP_DATA_CB(_name, _len, _flags, _read_post)             \
>>>> +       AT24_CHIP_DATA_CB_AO(_name, _len, _flags, 0, _read_post)
>>>> +
>>>>  #define AT24_CHIP_DATA_BS(_name, _len, _flags, _bank_addr_shift)       \
>>>>         static const struct at24_chip_data _name = {                    \
>>>>                 .byte_len = _len, .flags = _flags,                      \
>>>> @@ -170,9 +179,13 @@ AT24_CHIP_DATA(at24_data_24cs01, 16,
>>>>  AT24_CHIP_DATA(at24_data_24c02, 2048 / 8, 0);
>>>>  AT24_CHIP_DATA(at24_data_24cs02, 16,
>>>>         AT24_FLAG_SERIAL | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24mac402, 48 / 8,
>>>> +AT24_CHIP_DATA_AO(at24_data_24mac402, 48 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>
>>> And this will not break existing users? I guess you refer to these
>>> changes in your commit message but it's not very clear what you're
>>> doing and why.
>>>
>>
>> For those types of eeprom 24AA025E{48, 64} adjusting offset is not required (at24_get_offset_adj()).
>> So, indeed, it is an entanglement in logic.
>> To keep the implementation as it is:
>> adjoff (which is a flag that indicates when to use the adjusting offset) needs to be 1 for old compatibles but for these new ones needs to be 0.
>>
>> I think that is enough not to break the existing users. What are your thoughts?
>>
> 
> Wait... is the adjoff field effectively a boolean? Why u8?
> 

struct at24_data contains offset_adj which will get value calling at24_get_offset_adj()) if adjoff is true (1).
Yes, adjoff needs to be treated as a boolean. I will change it in the next version.

Best Regards,
Andrei

> Bart
> 
>> Best Regards,
>> Andrei Simion
>>
>>>> +AT24_CHIP_DATA_AO(at24_data_24mac602, 64 / 8,
>>>> +       AT24_FLAG_MAC | AT24_FLAG_READONLY, 1);
>>>> +AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8,
>>>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>> -AT24_CHIP_DATA(at24_data_24mac602, 64 / 8,
>>>> +AT24_CHIP_DATA(at24_data_24aa025e64, 64 / 8,
>>>>         AT24_FLAG_MAC | AT24_FLAG_READONLY);
>>>>  /* spd is a 24c02 in memory DIMMs */
>>>>  AT24_CHIP_DATA(at24_data_spd, 2048 / 8,
>>>> @@ -218,6 +231,8 @@ static const struct i2c_device_id at24_ids[] = {
>>>>         { "24cs02",     (kernel_ulong_t)&at24_data_24cs02 },
>>>>         { "24mac402",   (kernel_ulong_t)&at24_data_24mac402 },
>>>>         { "24mac602",   (kernel_ulong_t)&at24_data_24mac602 },
>>>> +       { "24aa025e48", (kernel_ulong_t)&at24_data_24aa025e48 },
>>>> +       { "24aa025e64", (kernel_ulong_t)&at24_data_24aa025e64 },
>>>>         { "spd",        (kernel_ulong_t)&at24_data_spd },
>>>>         { "24c02-vaio", (kernel_ulong_t)&at24_data_24c02_vaio },
>>>>         { "24c04",      (kernel_ulong_t)&at24_data_24c04 },
>>>> @@ -270,6 +285,8 @@ static const struct of_device_id __maybe_unused at24_of_match[] = {
>>>>         { .compatible = "atmel,24c1024",        .data = &at24_data_24c1024 },
>>>>         { .compatible = "atmel,24c1025",        .data = &at24_data_24c1025 },
>>>>         { .compatible = "atmel,24c2048",        .data = &at24_data_24c2048 },
>>>> +       { .compatible = "microchip,24aa025e48", .data = &at24_data_24aa025e48 },
>>>> +       { .compatible = "microchip,24aa025e64", .data = &at24_data_24aa025e64 },
>>>>         { /* END OF LIST */ },
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, at24_of_match);
>>>> @@ -690,7 +707,8 @@ static int at24_probe(struct i2c_client *client)
>>>>         at24->read_post = cdata->read_post;
>>>>         at24->bank_addr_shift = cdata->bank_addr_shift;
>>>>         at24->num_addresses = num_addresses;
>>>> -       at24->offset_adj = at24_get_offset_adj(flags, byte_len);
>>>> +       at24->offset_adj = cdata->adjoff ?
>>>> +                               at24_get_offset_adj(flags, byte_len) : 0;
>>>>         at24->client_regmaps[0] = regmap;
>>>>
>>>>         at24->vcc_reg = devm_regulator_get(dev, "vcc");
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> Bart