linux-kernel - Re: [PATCH 2/2] iio: health: max30100: Add pulse-width configuration via DT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [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;
>