linux-kernel - Re: [PATCH v2 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] [day] [month] [year] [list]

Message-ID: <CAHc1_P6w4mywfs38ORsmFyvcr9aBiLLCetquJgfevWqShg9=MA@mail.gmail.com>
Date: Sun, 12 Oct 2025 23:15:33 +0530
From: Shrikant <raskar.shree97@...il.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: jic23@...nel.org, 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 v2 2/2] iio: health: max30100: Add pulse-width
 configuration via DT

On Thu, Oct 9, 2025 at 4:21 PM Nuno Sá <noname.nuno@...il.com> wrote:
>
> 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@...il.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@...log.com>
Hello Nuno Sá,
Thanks for reviewing my patch.
I have removed the changelog from commit message and shared the v3
patch for review.

Thanks and Regards,
Shrikant
>
> > ---
> >  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@...sulko.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;
> >