[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251004141644.7fcb9106@jic23-huawei>
Date: Sat, 4 Oct 2025 14:16:44 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Shrikant Raskar <raskar.shree97@...il.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
matt@...ostay.sg, skhan@...uxfoundation.org, david.hunter.linux@...il.com,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH 2/2] iio: health: max30100: Add pulse-width
configuration via DT
On Sat, 4 Oct 2025 07:26:23 +0530
Shrikant Raskar <raskar.shree97@...il.com> wrote:
> The MAX30100 driver previously hardcoded the SPO2 pulse width to
> 1600us. This patch adds support for reading the pulse width from
> device tree (`maxim,pulse-width`) and programming it into the SPO2
> configuration register.
>
> If no property is provided, the driver falls back to 1600us to
> preserve existing behavior.
>
> Testing:
> Hardware: Raspberry Pi 3B + MAX30100 breakout
> Verified DT property read in probe()
> Confirmed SPO2_CONFIG register written correctly using regmap_read()
A few minor comments inline.
Thanks,
Jonathan
>
> Signed-off-by: Shrikant Raskar <raskar.shree97@...il.com>
> ---
> drivers/iio/health/max30100.c | 39 +++++++++++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 814f521e47ae..2b3348c75beb 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -5,7 +5,6 @@
> * Copyright (C) 2015, 2018
> * Author: Matt Ranostay <matt.ranostay@...sulko.com>
> *
> - * TODO: enable pulse length controls via device tree properties
> */
>
> #include <linux/module.h>
> @@ -54,6 +53,9 @@
> #define MAX30100_REG_SPO2_CONFIG 0x07
> #define MAX30100_REG_SPO2_CONFIG_100HZ BIT(2)
> #define MAX30100_REG_SPO2_CONFIG_HI_RES_EN BIT(6)
> +#define MAX30100_REG_SPO2_CONFIG_200US 0x0
> +#define MAX30100_REG_SPO2_CONFIG_400US 0x1
> +#define MAX30100_REG_SPO2_CONFIG_800US 0x2
> #define MAX30100_REG_SPO2_CONFIG_1600US 0x3
>
> #define MAX30100_REG_LED_CONFIG 0x09
> @@ -306,19 +308,52 @@ static int max30100_led_init(struct max30100_data *data)
> MAX30100_REG_LED_CONFIG_LED_MASK, reg);
> }
>
> +static int max30100_get_pulse_width(unsigned int pwidth_us)
> +{
> + switch (pwidth_us) {
> + case 200:
> + return MAX30100_REG_SPO2_CONFIG_200US;
> + case 400:
> + return MAX30100_REG_SPO2_CONFIG_400US;
> + case 800:
> + return MAX30100_REG_SPO2_CONFIG_800US;
> + case 1600:
> + return MAX30100_REG_SPO2_CONFIG_1600US;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int max30100_chip_init(struct max30100_data *data)
> {
> int ret;
> + unsigned int pulse_us;
> + unsigned int pulse_width;
> + struct device *dev = &data->client->dev;
>
> /* setup LED current settings */
> ret = max30100_led_init(data);
> if (ret)
> return ret;
>
> + /* Get pulse width from DT, default = 1600us */
> + ret = device_property_read_u32(dev, "maxim,pulse-width", &pulse_us);
> + if (ret) {
> + dev_warn(dev, "no pulse-width defined, defaulting to 1600us\n");
> + pulse_width = MAX30100_REG_SPO2_CONFIG_1600US;
Usual trick for these is to set pulse_us to 1600 before calling the
device_property_read_u32(). If that fails then the default value will remain
in the variable and we can just call the code below without needing
to explicitly handle two cases.
> + } else {
> + pulse_width = max30100_get_pulse_width(pulse_us);
> + if (pulse_width < 0) {
> + dev_err(dev, "invalid pulse-width %u\n", pulse_us);
Only happens in probe() so prefer use of return dev_err_probe()
for compactness in this case.
> + return pulse_width;
> + }
> + }
> +
> /* enable hi-res SPO2 readings at 100Hz */
> ret = regmap_write(data->regmap, MAX30100_REG_SPO2_CONFIG,
> MAX30100_REG_SPO2_CONFIG_HI_RES_EN |
> - MAX30100_REG_SPO2_CONFIG_100HZ);
> + MAX30100_REG_SPO2_CONFIG_100HZ |
> + pulse_width);
Even though it's the lowest field in this register. I'd prefer
a mask being defined and FIELD_PREP() used to set it.
That way we don't need to know it is the lowest field when looking at this
code.
> if (ret)
> return ret;
>
Powered by blists - more mailing lists