[riot-notifications] [RIOT-OS/RIOT] Add PWM-DAC and supporting infrastructure for thingy:52 (#15618)

benpicco notifications at github.com
Wed Aug 25 19:03:07 CEST 2021


@benpicco commented on this pull request.



> +config HAS_BOARD_SPEAKER
+    bool
+    help
+        Indicates that a power-switchable speaker is present.

I don't like the name since this implies all boards that have a speaker would advertise this.
Maybe call it `HAS_BOARD_SPEAKER_PIN`.

But then again, why not use the same approach as with buttons and just check if the pin is defined 

> @@ -336,6 +336,12 @@ int main(void)
     dac_set(DAC_DDS_DAC, 1 << 15);
     _dac_init();
 
+#ifdef MODULE_BOARD_SPEAKER

```suggestion
#ifdef SPK0_PWR_CTRL_PIN
    gpio_write(SPK0_PWR_CTRL_PIN, SPK0_PWR_CTRL_ON);
```

> @@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2020 Christian Amsüss <chrysn at fsfe.org>
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     drivers_pwm_dac

`drivers_pwm_dac` is not defined 

> +
+/**
+ * @brief    Value of initialization parameters for DAC PWMs
+ */
+#ifndef PWM_DAC_PARAMS
+#define PWM_DAC_PARAMS           { .dev  = PWM_DAC_PARAM_DEV, \
+                                   .mode = PWM_DAC_PARAM_MODE, \
+                                   .freq = PWM_DAC_PARAM_FREQ, \
+                                   .resbits = PWM_DAC_PARAM_RESBITS }
+#endif
+
+static const pwm_dac_params_t pwm_dac_params[] = {
+    PWM_DAC_PARAMS
+};
+
+const size_t pwm_dac_numof = ARRAY_SIZE(pwm_dac_params);

```suggestion
#define PWM_DAC_NUMOF ARRAY_SIZE(pwm_dac_params);
```

for consistency 

-- 
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/15618#pullrequestreview-738604218
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210825/9f60098f/attachment.htm>


More information about the notifications mailing list