[riot-notifications] [RIOT-OS/RIOT] drivers: support for NXP PCA9685 I2C 16-channel, 12-bit PWM controller (#10556)

Gunar Schorcht notifications at github.com
Fri Sep 6 18:38:19 CEST 2019


gschorcht commented on this pull request.



> +    return _read(dev, PCA9685_REG_MODE1, &byte, 1);
+}
+
+static int _init(pca9685_t *dev)
+{
+    /* set Auto-Increment flag */
+    EXEC_RET(_update(dev, PCA9685_REG_MODE1, PCA9685_MODE1_AI, 1));
+
+    /* switch off all channels */
+    EXEC_RET(_write_word(dev, PCA9685_REG_ALL_LED_OFF, PCA9685_ALL_LED_OFF));
+
+    /* set Auto-Increment flag */
+    uint8_t byte = 0;
+    _set_reg_bit(&byte, PCA9685_MODE2_OUTDRV, dev->params.inv);
+    _set_reg_bit(&byte, PCA9685_MODE2_OUTDRV, dev->params.out_drv);
+    _set_reg_bit(&byte, PCA9685_MODE2_OUTNE, dev->params.out_ne);

Hm, this executed only once. The while loop is not a big thing. I would prefer to leave it as it is, for the sake of clarity. We could think about not to define only a bit mask but also a bit shift value for each for register field. Another way would be to define structures for all registers.

But, since `_set_reg_bit` is only used during `_init`, there should be no performance problem.

But, I realized a real real problem.
```diff
-  _set_reg_bit(&byte, PCA9685_MODE2_OUTDRV, dev->params.inv);
+  _set_reg_bit(&byte, PCA9685_MODE2_INVERT, dev->params.inv);
```

-- 
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/10556#discussion_r321817958
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190906/c9aae12c/attachment-0001.htm>


More information about the notifications mailing list