| Message ID | 20240628080146.49545-2-andrei.simion@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Read MAC address through NVMEM for sama7g5ek | expand |
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. > 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. > +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 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
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
On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote: > > >> > >> 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. > No, wait. Why can't you just do: AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY); and avoid this whole new macro variant entirely? Bart
On 01.07.2024 11:46, Bartosz Golaszewski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote: >> >>>> >>>> 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. >> > > No, wait. Why can't you just do: > > AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY); > > and avoid this whole new macro variant entirely? > just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY): # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 00000000 ff ff ff ff ff ff |......| 00000006 # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 00000000 ff ff ff ff ff ff |......| 00000006 with this patch (adjoff false and new macro) # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 00000000 04 91 62 [the rest bytes] |..b...| 00000006 # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 00000000 04 91 62 [the rest bytes] |..b..m| 00000006 # > Bart
On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@microchip.com> wrote: > > On 01.07.2024 11:46, Bartosz Golaszewski wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote: > >> > >>>> > >>>> 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. > >> > > > > No, wait. Why can't you just do: > > > > AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY); > > > > and avoid this whole new macro variant entirely? > > > > just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY): > # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 > 00000000 ff ff ff ff ff ff |......| > 00000006 > # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 > 00000000 ff ff ff ff ff ff |......| > 00000006 > > with this patch (adjoff false and new macro) > # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 > 00000000 04 91 62 [the rest bytes] |..b...| > 00000006 > # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 > 00000000 04 91 62 [the rest bytes] |..b..m| > 00000006 > # > Ok, but your goal is for at24_get_offset_adj() to return 0, isn't it? This is what line at24->offset_adj = cdata->adjoff ? at24_get_offset_adj(flags, byte_len) : 0; is effectively achieving. What's the difference between this patch and the solution I'm proposing? Isn't the offset_adj field 0 in both cases? Is there any other difference I'm not seeing? Because I still think we can avoid all this churn. Bart
On 01.07.2024 14:16, Bartosz Golaszewski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@microchip.com> wrote: >> >> On 01.07.2024 11:46, Bartosz Golaszewski wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Mon, Jul 1, 2024 at 9:23 AM <Andrei.Simion@microchip.com> wrote: >>>> >>>>>> >>>>>> 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. >>>> >>> >>> No, wait. Why can't you just do: >>> >>> AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY); >>> >>> and avoid this whole new macro variant entirely? >>> >> >> just AT24_CHIP_DATA(at24_data_24aa025e48, 48 / 8, AT24_FLAG_READONLY): >> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 >> 00000000 ff ff ff ff ff ff |......| >> 00000006 >> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 >> 00000000 ff ff ff ff ff ff |......| >> 00000006 >> >> with this patch (adjoff false and new macro) >> # hexdump -C /sys/bus/nvmem/devices/1-00521/cells/eui48@fa\,0 >> 00000000 04 91 62 [the rest bytes] |..b...| >> 00000006 >> # hexdump -C /sys/bus/nvmem/devices/1-00532/cells/eui48@fa\,0 >> 00000000 04 91 62 [the rest bytes] |..b..m| >> 00000006 >> # >> > > Ok, but your goal is for at24_get_offset_adj() to return 0, isn't it? > This is what line > > at24->offset_adj = cdata->adjoff ? at24_get_offset_adj(flags, byte_len) : 0; > > is effectively achieving. What's the difference between this patch and > the solution I'm proposing? Isn't the offset_adj field 0 in both > cases? Is there any other difference I'm not seeing? > > Because I still think we can avoid all this churn. > I've rechecked what you said and see the function implementation at24_get_offset_adj(flags, byte_len) and made a mistake. I didn't see that you said only AT24_FLAG_READONLY. (Sorry for the wrong output) Ok, if I put only AT24_FLAG_READONLY then at24_get_offset_adj(flags, byte_len) returns 0 -> I've got in both 'cells/eui48@fa\,0' the MAC address. Best Regards, Andrei > 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");