[riot-notifications] [RIOT-OS/RIOT] cpu/atxmega/periph: Add ebi driver (#16288)

Marian Buschsieweke notifications at github.com
Mon May 17 07:20:35 CEST 2021


@maribu commented on this pull request.

Two more comments inline.

I'm now also wondering regarding the naming of `periph_ebi`. My worry is that so far all `periph_<foo>` features have been a vendor neutral hardware abstraction layers with the API being in `drivers/include/periph/<foo>.h`. I think that it makes sense to do one of the following:

1. Rename it to something that highlights the vendor-specific nature, e.g. `xmega_ebi` or so
2. Make the API vendor-neutral and place it in `drivers/include/periph/`. (E.g. the ATmega2560 has an XMEM feature that is conceptually the same as the EBI feature of the ATxmega MCUs - and unless I overlooked something the `hugemem_read8()` and friends functions would work for the ATmega2560 as well.)

I think that other non-AVR 8bit and 16bit MCUs could profit from an EBI / XMEM like feature just as well. Hence, I wouldn't be surprised if other vendors also have similar features. For that reason, I would like option 2. very much. Maybe something like `periph_hugemem` would be a good vendor-neutral name? And API could consist only of the `hugemem_ptr_t` and the corresponding access functions.

> @@ -1,3 +1,8 @@
 CPU_MODEL = atxmega128a1u
 
+FEATURES_PROVIDED += periph_cpuid

I don't quite get why this has become a board level feature. Are there ATxmega MCUs not having a CPU-ID, or does this rely on board-level configuration values to function?

In any case: It would ease review if the restructuring of features that are not related to EBI could be split out into a separate PR.

> +FEATURES_OPTIONAL += periph_cpuid
+FEATURES_OPTIONAL += periph_i2c
+FEATURES_REQUIRED += periph_nvm
+FEATURES_OPTIONAL += periph_spi
+FEATURES_OPTIONAL += periph_uart

I think you might have misunderstood my most recent comment. Using `FEATURES_OPTIONAL` here will result in all boards having these features to *always* use this feature. Hence, this only makes sense for features that one really always wants to use, if available. Only the EBI feature is something that I would consider to be useful regardless of application requirements. E.g. when an application doesn't use the SPI bus at all, it is a waste of ROM to still pull the driver in.

```suggestion
FEATURES_OPTIONAL += periph_ebi
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16288#pullrequestreview-660544356
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210516/a80eae40/attachment.htm>


More information about the notifications mailing list