Add official support for the ST M24 I2C EEPROM series

[benediktibk]

It is already with the existing implementation of the AT24 possible to use an EEPROM from the ST M24 series, although it is not very well documented (see #73064 and #69021). To avoid this confusion I have generalized the driver for the AT24 and AT25 so that it can be reused for other compatibles and added specific bindings for the ST M24 I2C automotive series.
With this approach it is rather clear which ICs are supported by which driver, and it can be easily extended for other compatible ICs.

[erwango]

Thanks for digging into this direction, but adequate use of dts should help getting things more simple.

[erwango]

Do we actually need to have all these "non-compatible" compatibles using fixed properties?

What about a more generic st,m24xxx compatible ?
Aim it to document that a class of devices is supported and show how to configure it.

[benediktibk]

I would rather prefer to be specific, as this has multiple advantages:

1. If I only document the proper usage, I still have to add some text which has to be maintained. And usually documentation gets forgotten during updates and changes and it is therefore more likely be incorrect in the future.
2. This adds a build time check, which avoids improper use and therefore frustration and bugs in real applications. And as the hardware won't change anymore I think this is worth the effort.
3. Having a very generic compatible brought us to the current situation. From my experience it is rather handy on the first sight to add a more generic compatible, but in the end hardware vendors tend to do all kind of weird stuff with their hardware identifiers. There might not be something currently in the range of st,m24xxx which is incompatible with this driver, but I wouldn't bet on this to be true in the future as well.
4. It is more clear to a user which compatible should be used for which IC in the hardware design.

[henrikbrixandersen]

NAK. This will become a maintenance nightmare given all the different, compatible EEPROMs out there. I wish you had approached me before embarking on this.

[benediktibk]

NAK. This will become a maintenance nightmare given all the different, compatible EEPROMs out there. I wish you had approached me before embarking on this.

Is the NACK for only the IC specific compatibles, or the complete approach? Because I wouldn't be a big fan of having a more generic st,m24xxx (as explained here #73136 (comment)), but I would consider this still be better than the current status.

[henrikbrixandersen]

Is the NACK for only the IC specific compatibles, or the complete approach? Because I wouldn't be a big fan of having a more generic st,m24xxx (as explained here #73136 (comment)), but I would consider this still be better than the current status.

I really do not see an issue with the current compatible and I would not want to add a bunch of vendor-specific aliases for this.

As mentioned earlier, I believe this can be solved by improving the documentation on how to configure serial EEPROMs in Zephyr. I plan on starting this work prior to v3.7.0.

[benediktibk]

Is the NACK for only the IC specific compatibles, or the complete approach? Because I wouldn't be a big fan of having a more generic st,m24xxx (as explained here #73136 (comment)), but I would consider this still be better than the current status.

I really do not see an issue with the current compatible and I would not want to add a bunch of vendor-specific aliases for this.

As mentioned earlier, I believe this can be solved by improving the documentation on how to configure serial EEPROMs in Zephyr. I plan on starting this work prior to v3.7.0.

Unfortunately I will have to disagree, because as a user it is rather confusing to use the compatible of A for B. I'm not convinced that documentation will solve this issue. On top of that, having build time checks and therefore a mechanism to fail early avoids a lot of issues in the long run.

[pdgendt]

I like the comment of @erwango, put a generic compatible at the end if the list?

[benediktibk]

But anyway, @henrikbrixandersen is strongly against this, he prefers to stick to only the historical binding of atmel,at24 as it preceeds the reimplementation by other vendors and wants to avoid the confusion with additional documentation. Please correct me, if I am (unintentionally) citing you incorrectly.

[henrikbrixandersen]

But anyway, @henrikbrixandersen is strongly against this, he prefers to stick to only the historical binding of atmel,at24 as it preceeds the reimplementation by other vendors and wants to avoid the confusion with additional documentation. Please correct me, if I am (unintentionally) citing you incorrectly.

No, that is correct.

[erwango]

But anyway, @henrikbrixandersen is strongly against this, he prefers to stick to only the historical binding of atmel,at24 as it preceeds the reimplementation by other vendors and wants to avoid the confusion with additional documentation. Please correct me, if I am (unintentionally) citing you incorrectly.

No, that is correct.

@henrikbrixandersen I understand your position, but what would you propose to explicit the fact that atmel,at24 compatible devices are supported?
This is already the second PR we have on the same topic and I guess that the ones pushing PRs are much less than the one that needed it and gave up. Obviously we miss some way to document this clearly.

[pdgendt]

I tend to agree, there are ongoing discussions whether to rename struct net_buf to something generic, which has a much bigger impact than to generalize a common driver for different peripherals.

[henrikbrixandersen]

@henrikbrixandersen I understand your position, but what would you propose to explicit the fact that atmel,at24 compatible devices are supported?
This is already the second PR we have on the same topic and I guess that the ones pushing PRs are much less than the one that needed it and gave up. Obviously we miss some way to document this clearly.

My plan is to add a documentation page under https://docs.zephyrproject.org/latest/hardware/peripherals/eeprom.html detailing how to look up the various properties of a serial EEPROM in the data sheet with examples of how to represent these in a devicetree node.

Furthermore, I plan revisiting all occurrences of compatible = "atmel,at2*" within the tree and add a more specific compatible to the left of it (sort of like what is done here, but with the specific part number instead of xxx: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/shields/x_nucleo_eeprma2/x_nucleo_eeprma2.overlay). This should help people grepping through the tree to realise that these devices are already supported.

We can also extend the help text of the bindings and Kconfig options to list "st,m24xxx" and other families to help people find them.

[henrikbrixandersen]

I tend to agree, there are ongoing discussions whether to rename struct net_buf to something generic, which has a much bigger impact than to generalize a common driver for different peripherals.

I honestly don't believe the two are comparable. Also, as you may recall, it was decided not to rename net_buf for now, but merely move it up to increase visibility.

[erwango]

@henrikbrixandersen I understand your position, but what would you propose to explicit the fact that atmel,at24 compatible devices are supported?
This is already the second PR we have on the same topic and I guess that the ones pushing PRs are much less than the one that needed it and gave up. Obviously we miss some way to document this clearly.

My plan is to add a documentation page under https://docs.zephyrproject.org/latest/hardware/peripherals/eeprom.html detailing how to look up the various properties of a serial EEPROM in the data sheet with examples of how to represent these in a devicetree node.

Furthermore, I plan revisiting all occurrences of compatible = "atmel,at2*" within the tree and add a more specific compatible to the left of it (sort of like what is done here, but with the specific part number instead of xxx: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/shields/x_nucleo_eeprma2/x_nucleo_eeprma2.overlay). This should help people grepping through the tree to realise that these devices are already supported.

We can also extend the help text of the bindings and Kconfig options to list "st,m24xxx" and other families to help people find them.

Ok, then, that's basically quite aligned with the content of my review (here and here) so I can't agree more.

[benediktibk]

Lets see if I have understood it correctly. This PR now contains only additional bindings and build_all tests which show how they can be used. This doesn't impact the implementation of the AT24 at all, but states explicitly which devices are supported, with the added benefit of build time failures for improper usage.

[benediktibk]

The build error seems to be unrelated to these changes:

 Traceback (most recent call last):
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/cmd/build.py", line 337, in build_main
    app.build(args.force_all, args.filenames)
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/application.py", line 351, in build
    self.builder.build_update()
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 293, in build_update
    self.build(to_build,
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 313, in build
    updated_docnames = set(self.read())
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 417, in read
    self._read_parallel(docnames, nproc=self.app.parallel)
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 473, in _read_parallel
    tasks.join()
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/util/parallel.py", line 107, in join
    if not self._join_one():
  File "/home/runner/.local/lib/python3.10/site-packages/sphinx/util/parallel.py", line 125, in _join_one
    exc, logs, result = pipe.recv()
  File "/usr/lib/python3.10/multiprocessing/connection.py", line 250, in recv
    buf = self._recv_bytes()
  File "/usr/lib/python3.10/multiprocessing/connection.py", line 414, in _recv_bytes
    buf = self._recv(4)
  File "/usr/lib/python3.10/multiprocessing/connection.py", line 383, in _recv
    raise EOFError
EOFError

Exception occurred:
  File "/usr/lib/python3.10/multiprocessing/connection.py", line 383, in _recv
    raise EOFError

[henrikbrixandersen]

My plan is to add a documentation page under docs.zephyrproject.org/latest/hardware/peripherals/eeprom.html detailing how to look up the various properties of a serial EEPROM in the data sheet with examples of how to represent these in a devicetree node.

Furthermore, I plan revisiting all occurrences of compatible = "atmel,at2*" within the tree and add a more specific compatible to the left of it (sort of like what is done here, but with the specific part number instead of xxx: main/boards/shields/x_nucleo_eeprma2/x_nucleo_eeprma2.overlay). This should help people grepping through the tree to realise that these devices are already supported.

We can also extend the help text of the bindings and Kconfig options to list "st,m24xxx" and other families to help people find them.

Please see #73512

[benediktibk]

Rebased onto main to resolve conflicts.

at a glance looks better

[benediktibk]

Rebased onto main to resolve merge conflicts.

[pdgendt]

I think this makes sense, but needs maintainer (Brix) approval. Should this still be considered for 3.7?

[benediktibk]

I think this makes sense, but needs maintainer (Brix) approval. Should this still be considered for 3.7?

I don't mind when it is getting merged, I just think it should end up in main at some point. Maybe we can convince @henrikbrixandersen with the next PR which tries to implement an AT24 variant ;-).

[henrikbrixandersen]

I think this makes sense, but needs maintainer (Brix) approval. Should this still be considered for 3.7?

I don't mind when it is getting merged, I just think it should end up in main at some point. Maybe we can convince @henrikbrixandersen with the next PR which tries to implement an AT24 variant ;-).

As I have told you already, this PR will not be merged upstream. We already have what you call "official support for the ST M24 I2C EEPROM series". The proper solution is to use the devicetree "compatible" string as it is intended, and simply add a "atmel,at24" (as this is the common programming model for this type of device) at the end of the list..

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[benediktibk]

Rebased onto main to resolve merge conflicts.