On Mon, Jul 1, 2024 at 12:20 PM <Andrei.Simion@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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
