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

["Nuno Sá" <noname.nuno@gmail.com>]

Hi Shrikant,

Thanks for your patch.

On Wed, 2025-10-08 at 08:47 +0530, Shrikant Raskar 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-us`) 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()
> 
> Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
> 
> Changes since v1:
> Use FIELD_PREP() and define a pulse width bit mask.
> Initialize default pulse_us before property read.
> Use dev_err_probe() for error reporting.
> Make pulse_width signed to handle negative return values.
> 
> Link to v1:
> https://lore.kernel.org/all/20251004015623.7019-3-raskar.shree97@gmail.com/

As mentioned in the bindings patch, this is not place for changelog. With that
fixed:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> ---
>  drivers/iio/health/max30100.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 814f521e47ae..50cd4fd13849 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@konsulko.com>
>   *
> - * TODO: enable pulse length controls via device tree properties
>   */
>  
>  #include <linux/module.h>
> @@ -54,6 +53,10 @@
>  #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_PW_MASK	GENMASK(1, 0)
> +#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 +309,47 @@ 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;
> +	int pulse_width;
> +	/* set default pulse-width-us to 1600us */
> +	unsigned int pulse_us = 1600;
> +	struct device *dev = &data->client->dev;
>  
>  	/* setup LED current settings */
>  	ret = max30100_led_init(data);
>  	if (ret)
>  		return ret;
>  
> +	/* Read pulse-width-us from DT */
> +	device_property_read_u32(dev, "maxim,pulse-width-us", &pulse_us);
> +
> +	pulse_width = max30100_get_pulse_width(pulse_us);
> +	if (pulse_width < 0)
> +		return dev_err_probe(dev, pulse_width, "invalid pulse-width
> %uus\n", pulse_us);
> +
>  	/* 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 |
> +				 FIELD_PREP(MAX30100_REG_SPO2_CONFIG_PW_MASK,
> pulse_width));
>  	if (ret)
>  		return ret;
>