drivers: health: Add MAXM86161 Optical PPG AFE Driver Support by jcroleda · Pull Request #3270 · analogdevicesinc/no-OS

jcroleda · 2026-08-18T02:21:44Z

Pull Request Description

The MAXM86161 is an ultra-low-power, integrated optical data acquisition device. The MAXM86161 supports 3 programmable high-current LED drivers and an analog front-end with a 19-bit ADC, Ambient-Light Cancellation, and a picket-fence detect and replace algorithm. This device is used for optical sensing applications such as heart rate detection and pulse oximetry.

This PR adds driver support for the MAXM86161 and adds example projects that show example application uses for this device.

Datasheet: MAXM86161

PR Type

Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

I have followed the Coding style guidelines
I have complied with the Submission Checklist
I have performed a self-review of the changes
I have commented my code, at least hard-to-understand parts
I have build all projects affected by the changes in this PR
I have tested in hardware affected projects, at the relevant boards
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe etc), if applies

edelweiseescala

Initial review in this PR #3267 already addressed Thank You!
Added some more comment, also the Maxim Build on the CI fails. Please also check that.

edelweiseescala · 2026-08-18T02:37:04Z

+	target_include_directories(maxm86161 PUBLIC
+				   $ {CMAKE_CURRENT_SOURCE_DIR}/src/examples/basic
+				  )
+	endif()


I think there are unnecessary tabs here as well as the other if below

edelweiseescala · 2026-08-18T02:40:12Z

+ * OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


Following licensing changes, license should be this now

Suggested change

* EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

********************************************************************************

* Copyright 2026(c) Analog Devices, Inc.

*

* SPDX-License-Identifier: BSD-3-Clause

*******************************************************************************/

edelweiseescala · 2026-08-18T02:47:07Z

+ *         -EINVAL if arguments are invalid or IRQ is not enabled,
+ *         -ENOMEM if descriptor allocation fails, or a propagated error
+ *         from the driver/FIFO/IRQ setup.
+ */


doxygen comments are usually in the source file

jcroleda · 2026-08-18T04:29:52Z

Changelog V2:

Fixed formatting issues of CMake files (also causing build errors)
Moved doxygen comments to main code files
Updated licensing to BSD-3-Clause
Removed bus_api abstraction from maxm86161.h (as only I2C is supported)

rbolboac

took a look at the driver for now, nothing too serious, mainly check the file again for all instances in which you use magic numbers and use macros instead

rbolboac · 2026-08-20T13:54:06Z

+{
+	int ret;
+
+	if (!dev || ppg_sr > 0x13)


use macro instead of magic numbers

rbolboac · 2026-08-20T13:56:49Z

+	if (ret)
+		return ret;
+
+	*overflow_count = reg_val & NO_OS_GENMASK(6, 0);


use a mask macro instead of magic number

rbolboac · 2026-08-20T13:59:30Z

+	}
+
+	if (dev->gpio_intb) {
+		r = no_os_gpio_remove(dev->gpio_intb);


why is this r used instead of ret?

RaduSabau1

some comments from my side

RaduSabau1 · 2026-08-25T12:28:53Z

+	if (dev->gpio_intb)
+		no_os_gpio_remove(dev->gpio_intb);
+err_bus:
+	ret = no_os_i2c_remove(dev->i2c_desc);


assigning no_os_i2c_remove return value to ret would overwrite the error code stored through ret at this point and thus report a failure with (most probably, unless i2c_remove fails) a success code.

RaduSabau1 · 2026-08-25T12:31:35Z

+		if (ret)
+			goto err_gpio;
+
+		ret = no_os_irq_register_callback(dev->irq_ctrl, dev->gpio_intb->number,


this code path (irq_init) should also check for gpio_intb, otherwise dev->gpio_intb->number is dereferenced on a NULL gpio_intb reuslting in a NULL-pointer crash.

RaduSabau1 · 2026-08-25T12:36:32Z

+	if (!dev || led_num == 0 || led_num > MAXM86161_LED_NUM_PILOT_GREEN)
+		return -EINVAL;
+
+	reg_addr = MAXM86161_REG_LED1_PA + (led_num - 1);


if led_num is 4 (PILOT_GREEN) the computed register address is 0x26 which seems like a reserved address instead.

RaduSabau1 · 2026-08-25T12:40:42Z

+ * @return 0 on success, negative error code otherwise.
+ */
+int maxm86161_burst_reg_read(struct maxm86161_dev *dev, uint8_t reg_addr,
+			     uint8_t *reg_data, uint16_t count)


have you tested this with a near-full FIFO read? the reads are chunked which is correct. however I see the destination pointer between chunks never advances so a near-full FIFO read could corrupt the buffer. please check on this.

RaduSabau1 · 2026-08-25T12:42:21Z

+	buf[0] = reg_addr;
+	memcpy(&buf[1], reg_data, count);
+
+	ret = no_os_i2c_write(dev->i2c_desc, buf, count + 1, 1);


the truncation of count is present here too, although no current call site exposes this.

RaduSabau1 · 2026-08-25T12:45:18Z

+	if (ret)
+		goto remove_uart;
+
+	no_os_uart_remove(uart_desc);


every successful run seems to free this uart_desc twice. no_os_uart_remove indeed checks for dev so an actual double-free wouldn't occur though the code itself looks wrong this way, this line could perhaps be removed and the if (ret) goto remove_uart; nevertheless since they become redundant in this case.

RaduSabau1 · 2026-08-25T13:10:22Z

+		goto cleanup;
+	}
+
+	ret = maxm86161_set_led_cfg(dev, MAXM86161_LED_SRC_PILOT_GREEN,


this one seems guaranteed to return -EINVAL. LED_SRC_PILOT_GREEN(8) is later used as led_num when calling set_led_range which checks for led_num > LED_NUM_RED(3) and returns -EINVAL if the condition (which is) is met.

RaduSabau1 · 2026-08-25T13:11:54Z

+#define MAXM86161_SELFTEST 1
+
+#if MAXM86161_SELFTEST
+static void maxm86161_selftest(struct maxm86161_iio_desc *desc)


where is this called in example_main()?

This was intended mainly for debugging purposes with the GPIO IRQ during development. I will remove this from the example.

RaduSabau1 · 2026-08-25T13:14:44Z

+	case MAXM86161_IIO_LP_MODE:
+		return maxm86161_set_low_power_mode(dev, (bool)val);
+
+	case MAXM86161_IIO_BURST_EN:


seems like writing burst_en forcefully rewrites the rate to 8Hz and modifying the rate also forcefully enables the burst. a way of avoiding this (and making the two attributes independent) would be to have separate helper function for each e.g. set_burst_mode getting split in set_burst_enable and set_burst_rate or something simillar.

RaduSabau1 · 2026-08-25T13:15:40Z

+		goto err_remove_mutex;
+	}
+
+	return 0;


not only that this is line is duplicated, but *iio_desc =desc; which has a check itself is happening after the iio_desc assignment

The MAXM86161 is an ultra-low-power, integrated optical data acquisition device. The MAXM86161 supports 3 programmable high-current LED drivers and an analog front-end with a 19-bit ADC, Ambient-Light Cancellation, and a picket-fence detect and replace algorithm. This device is used for optical sensing applications such as heart rate detection and pulse oximetry. This commit adds no-OS driver support for the MAXM86161 by exposing device attributes and data. Assisted-by: Claude Code - Opus 4.8 (1M Context) Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

Adds MAXM86161 Sphinx Documentation, and adds a new Health subsystem. Assisted-by: Claude Code - Opus 4.8 (1M Context) Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

Adds sample application for MAXM86161 to sample driver API and device connection. - Basic Non-streaming example using base driver APIs - IIO example initializing a device server instance - Attribute test app to validate Driver access to device attributes Assisted-by: Claude Code - Opus 4.8 (1M Context) Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

Adds project documentation for MAXM86161 project applications to build and flash drivers for the MAX32655FTHR board, and connecting IIO on receiver to access device attributes and buffers. Assisted-by: Claude Code - Opus 4.8 (1M Context) Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda · 2026-09-01T01:30:45Z

Changelog V3:

Updated magic numbers with defined values
Updated mask macros with no-OS utils where applicable
removed seperate return value storage for gpio_remove
added a seperate return for i2c_remove error code
removed redundant removes and returns (i2c, uart)
Updated burst read/write to match buffer address after each chunk
updated set_led_cfg to avoid setting led_range when PILOT_GREEN and helpers point to the proper PILOT_GREEN address (0x29)
removed MAXM86161_SELFTEST from the iio_example.c
added burst_en and burst_rate helpers to seperate the two operations in iio_attrs
moved *iio_desc = desc; to end of init, after shutdown = true

jcroleda requested review from CiprianRegus, RaduSabau1, amiclaus, buha, cristina-suteu, danmois, dbogdan, raezt, ralucabozdog, ranechita, rbolboac, rbudai98 and stefpopa as code owners August 18, 2026 02:21

github-actions Bot added drivers projects documentation labels Aug 18, 2026

jcroleda mentioned this pull request Aug 18, 2026

drivers: health: Add MAXM86161 Optical PPG AFE Driver Support #3267

Closed

11 tasks

edelweiseescala reviewed Aug 18, 2026

View reviewed changes

jcroleda force-pushed the dev/maxm86161 branch 2 times, most recently from 573295d to f809daa Compare August 18, 2026 03:56

rbolboac reviewed Aug 20, 2026

View reviewed changes

RaduSabau1 reviewed Aug 25, 2026

View reviewed changes

jcroleda force-pushed the dev/maxm86161 branch from f809daa to 66452fe Compare September 1, 2026 01:04

jcroleda requested a review from MirceaVlasin as a code owner September 1, 2026 01:04

jcroleda force-pushed the dev/maxm86161 branch from 66452fe to 64b9e60 Compare September 1, 2026 01:20

jcroleda added 4 commits September 1, 2026 09:23

doc: sphinx: Add Health Subsystem and MAXM86161 Documentation

8c9eecf

Adds MAXM86161 Sphinx Documentation, and adds a new Health subsystem. Assisted-by: Claude Code - Opus 4.8 (1M Context) Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda force-pushed the dev/maxm86161 branch from 64b9e60 to a63bd07 Compare September 1, 2026 01:23

- * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+********************************************************************************
+ * Copyright 2026(c) Analog Devices, Inc.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+*******************************************************************************/

Uh oh!

Conversation

jcroleda commented Aug 18, 2026

Pull Request Description

PR Type

PR Checklist

Uh oh!

edelweiseescala left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcroleda commented Aug 18, 2026

Uh oh!

rbolboac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaduSabau1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcroleda commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edelweiseescala left a comment •

edited

Loading