| Message ID | 20240628080146.49545-2-andrei.simion@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | Read MAC address through NVMEM for sama7g5ek | expand |
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? 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
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? 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
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) \ 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); +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");