[riot-notifications] [RIOT-OS/RIOT] cpu/atxmega: Add periph power management (#16212)

Marian Buschsieweke notifications at github.com
Fri Apr 2 09:52:15 CEST 2021


@maribu commented on this pull request.

lgtm. I have one suggestion regarding style, but that is opinionated, so feel free to ignore. If you take the suggestion, squash right away.

Let me know when I should start testing. Sorry for the long stall.

> +enum
+{

```suggestion
enum {
```

> + * @brief     Extract the register id of the given power reduction mask
+ */
+static inline uint8_t _register_id(pwr_reduction_t pwr)
+{
+    return (pwr >> 8) & 0xff;
+}
+
+/**
+ * @brief     Generate the register index of the given power reduction mask
+ */
+static inline uint8_t *_register_addr(pwr_reduction_t pwr)
+{
+    uint8_t id = _register_id(pwr);
+    uint16_t addr = PWR_REG_BASE + (id * PWR_REG_OFFSET);
+
+    return (uint8_t *) addr;

```suggestion
    return (uint8_t *)addr;
```

> +void pm_periph_enable(pwr_reduction_t pwr, bool enable)
+{
+    uint8_t mask = _device_mask(pwr);
+    uint8_t *reg = _register_addr(pwr);
+
+    if (enable) {
+        *reg &= ~mask;      /* Clear to Enable */
+    }
+    else {
+        *reg |= mask;
+    }
+}

I have the feeling that the function call overhead here is greater than the implementation. So if this would be provided as `static inline` instead, I think ROM would be reduced.

If that is the case, would you mind to split this into `pm_periph_enable()` and `pm_periph_disable()`, as this would make reading the code much easier.

If not, maybe you could name the C function to e.g. `pm_periph_set_enabled()` and provide a `static inline pm_periph_enable()` and `static inline pm_periph_disable()` as wrapper? The compiler will optimize the wrappers out, so this would come with no cost.

-- 
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/16212#pullrequestreview-626835959
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210402/c76e42b4/attachment.htm>


More information about the notifications mailing list