* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-04 1:56 ` [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property Shrikant Raskar
@ 2025-10-04 3:22 ` Bhanu Seshu Kumar Valluri
2025-10-04 6:19 ` Shrikant
2025-10-04 13:12 ` Jonathan Cameron
2025-10-06 14:40 ` Krzysztof Kozlowski
2 siblings, 1 reply; 12+ messages in thread
From: Bhanu Seshu Kumar Valluri @ 2025-10-04 3:22 UTC (permalink / raw)
To: Shrikant Raskar, jic23, robh, krzk+dt, conor+dt
Cc: dlechner, nuno.sa, andy, matt, skhan, david.hunter.linux,
linux-iio, devicetree, linux-kernel, linux-kernel-mentees
On 04/10/25 07:26, Shrikant Raskar wrote:
> The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> 800us, 1600us). These settings affect measurement resolution and power
> consumption. Until now, the driver always defaulted to 1600us.
>
> Introduce a new device tree property `maxim,pulse-width` that allows
> users to select the desired pulse width in microseconds from device
> tree.
>
> Valid values are: 200, 400, 800, 1600.
>
> This prepares for driver changes that read this property and configure
> the SPO2 register accordingly.
>
> Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
The subject prefix [PATCH 1/2] says it's a two part patch series. But I think
you send all changes in a single patch. If single patch use [PATCH] instead
of [PATCH 1/2].
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-04 3:22 ` Bhanu Seshu Kumar Valluri
@ 2025-10-04 6:19 ` Shrikant
0 siblings, 0 replies; 12+ messages in thread
From: Shrikant @ 2025-10-04 6:19 UTC (permalink / raw)
To: Bhanu Seshu Kumar Valluri
Cc: jic23, robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, matt,
skhan, david.hunter.linux, linux-iio, devicetree, linux-kernel,
linux-kernel-mentees
On Sat, Oct 4, 2025 at 8:52 AM Bhanu Seshu Kumar Valluri
<bhanuseshukumar@gmail.com> wrote:
>
> On 04/10/25 07:26, Shrikant Raskar wrote:
> > The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> > 800us, 1600us). These settings affect measurement resolution and power
> > consumption. Until now, the driver always defaulted to 1600us.
> >
> > Introduce a new device tree property `maxim,pulse-width` that allows
> > users to select the desired pulse width in microseconds from device
> > tree.
> >
> > Valid values are: 200, 400, 800, 1600.
> >
> > This prepares for driver changes that read this property and configure
> > the SPO2 register accordingly.
> >
> > Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
>
> The subject prefix [PATCH 1/2] says it's a two part patch series. But I think
> you send all changes in a single patch. If single patch use [PATCH] instead
> of [PATCH 1/2].
>
>
Thanks for your feedback.
I have shared 2 patches, one for device tree property update and
respective driver update in
MAX30100 driver. Can you please check the patch with subject ' iio:
health: max30100: Add pulse-width configuration via DT' ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-04 1:56 ` [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property Shrikant Raskar
2025-10-04 3:22 ` Bhanu Seshu Kumar Valluri
@ 2025-10-04 13:12 ` Jonathan Cameron
2025-10-06 3:04 ` Shrikant
2025-10-06 14:40 ` Krzysztof Kozlowski
2 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2025-10-04 13:12 UTC (permalink / raw)
To: Shrikant Raskar
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, matt, skhan,
david.hunter.linux, linux-iio, devicetree, linux-kernel,
linux-kernel-mentees
On Sat, 4 Oct 2025 07:26:22 +0530
Shrikant Raskar <raskar.shree97@gmail.com> wrote:
> The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> 800us, 1600us). These settings affect measurement resolution and power
> consumption. Until now, the driver always defaulted to 1600us.
>
> Introduce a new device tree property `maxim,pulse-width` that allows
> users to select the desired pulse width in microseconds from device
> tree.
>
> Valid values are: 200, 400, 800, 1600.
>
> This prepares for driver changes that read this property and configure
> the SPO2 register accordingly.
>
> Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
Hi Shrikant,
Explain why this is in some way related to characteristics of how the
system containing this chip is built (wiring, lenses etc). Otherwise
this might instead be something that should be controlled from userspace
not firmware.
Also, give a little more on why we care about controlling it at all.
Is there a usecase where power use of this chip matters? Mostly I'd expect
it to be in measurement equipment with relatively short measuring periods.
Jonathan
> ---
> .../devicetree/bindings/iio/health/maxim,max30100.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml b/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> index 967778fb0ce8..55aaf2ff919b 100644
> --- a/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> +++ b/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> @@ -27,6 +27,11 @@ properties:
> LED current whilst the engine is running. First indexed value is
> the configuration for the RED LED, and second value is for the IR LED.
>
> + maxim,pulse-width:
> + maxItems: 1
> + description: Pulse width in microseconds
> + enum: [200, 400, 800, 1600]
> +
> additionalProperties: false
>
> required:
> @@ -44,6 +49,7 @@ examples:
> compatible = "maxim,max30100";
> reg = <0x57>;
> maxim,led-current-microamp = <24000 50000>;
> + maxim,pulse-width = <1600>;
> interrupt-parent = <&gpio1>;
> interrupts = <16 2>;
> };
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-04 13:12 ` Jonathan Cameron
@ 2025-10-06 3:04 ` Shrikant
2025-10-12 14:11 ` Jonathan Cameron
0 siblings, 1 reply; 12+ messages in thread
From: Shrikant @ 2025-10-06 3:04 UTC (permalink / raw)
To: Jonathan Cameron
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, matt, skhan,
david.hunter.linux, linux-iio, devicetree, linux-kernel,
linux-kernel-mentees
On Sat, Oct 4, 2025 at 6:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 4 Oct 2025 07:26:22 +0530
> Shrikant Raskar <raskar.shree97@gmail.com> wrote:
>
> > The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> > 800us, 1600us). These settings affect measurement resolution and power
> > consumption. Until now, the driver always defaulted to 1600us.
> >
> > Introduce a new device tree property `maxim,pulse-width` that allows
> > users to select the desired pulse width in microseconds from device
> > tree.
> >
> > Valid values are: 200, 400, 800, 1600.
> >
> > This prepares for driver changes that read this property and configure
> > the SPO2 register accordingly.
> >
> > Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
> Hi Shrikant,
>
> Explain why this is in some way related to characteristics of how the
> system containing this chip is built (wiring, lenses etc). Otherwise
> this might instead be something that should be controlled from userspace
> not firmware.
>
> Also, give a little more on why we care about controlling it at all.
> Is there a usecase where power use of this chip matters? Mostly I'd expect
> it to be in measurement equipment with relatively short measuring periods.
>
> Jonathan
Hi Jonathan,
Thanks for the feedback.
The pulse width configuration is indeed dependent on the hardware integration
of the MAX30100. It affects how much optical energy the LEDs emit per sample,
which in turn depends on physical factors such as:
- The type and thickness of the optical window or lens covering the sensor
- The distance between the LED and photodiode
- The reflectivity of the skin/contact surface
For example:
- A smartwatch/wearable ring with a thin glass window can operate
with 200–400 µs pulses to
save power, while
- A medical-grade pulse oximeter or a sensor mounted behind a thicker
protective layer may require 800–1600 µs for reliable signal amplitude.
I believe it would be appropriate to describe these fixed,
board-specific characteristics in the Device Tree,
since they are determined by the hardware design rather than being
runtime or user-controlled parameters.
Would it be okay if I send v2 of the patch with the above explanation
included in the commit message?
Thanks and regards,
Shrikant
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-06 3:04 ` Shrikant
@ 2025-10-12 14:11 ` Jonathan Cameron
2025-10-12 16:52 ` Shrikant
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2025-10-12 14:11 UTC (permalink / raw)
To: Shrikant
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, matt, skhan,
david.hunter.linux, linux-iio, devicetree, linux-kernel,
linux-kernel-mentees
On Mon, 6 Oct 2025 08:34:03 +0530
Shrikant <raskar.shree97@gmail.com> wrote:
> On Sat, Oct 4, 2025 at 6:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 4 Oct 2025 07:26:22 +0530
> > Shrikant Raskar <raskar.shree97@gmail.com> wrote:
> >
> > > The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> > > 800us, 1600us). These settings affect measurement resolution and power
> > > consumption. Until now, the driver always defaulted to 1600us.
> > >
> > > Introduce a new device tree property `maxim,pulse-width` that allows
> > > users to select the desired pulse width in microseconds from device
> > > tree.
> > >
> > > Valid values are: 200, 400, 800, 1600.
> > >
> > > This prepares for driver changes that read this property and configure
> > > the SPO2 register accordingly.
> > >
> > > Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
> > Hi Shrikant,
> >
> > Explain why this is in some way related to characteristics of how the
> > system containing this chip is built (wiring, lenses etc). Otherwise
> > this might instead be something that should be controlled from userspace
> > not firmware.
> >
> > Also, give a little more on why we care about controlling it at all.
> > Is there a usecase where power use of this chip matters? Mostly I'd expect
> > it to be in measurement equipment with relatively short measuring periods.
> >
> > Jonathan
> Hi Jonathan,
>
> Thanks for the feedback.
>
> The pulse width configuration is indeed dependent on the hardware integration
> of the MAX30100. It affects how much optical energy the LEDs emit per sample,
> which in turn depends on physical factors such as:
>
> - The type and thickness of the optical window or lens covering the sensor
> - The distance between the LED and photodiode
> - The reflectivity of the skin/contact surface
>
> For example:
> - A smartwatch/wearable ring with a thin glass window can operate
> with 200–400 µs pulses to
> save power, while
> - A medical-grade pulse oximeter or a sensor mounted behind a thicker
> protective layer may require 800–1600 µs for reliable signal amplitude.
>
> I believe it would be appropriate to describe these fixed,
> board-specific characteristics in the Device Tree,
> since they are determined by the hardware design rather than being
> runtime or user-controlled parameters.
>
> Would it be okay if I send v2 of the patch with the above explanation
> included in the commit message?
Hi Shrikant,
I'd have this excellent detail + examples summarised in the patch description
and also a small amount of description in the actual binding even if that just says
something like
Description:
Pulse width in... . Appropriate pulse width is dependant on factors
such as optical window absorption, distances and expected reflectivity
of skin / contact surface.
That's just a quick mash up of what you have above, feel free to not use this
particular text!
The inclusion of target surface reflectivity in there makes me thing that
for some applications we may also need userspace tweaking or some algorithmic
way to increase or decrease the value according to skin characteristics. However
I guess maybe it isn't that sensitive.
Jonathan
>
> Thanks and regards,
> Shrikant
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-12 14:11 ` Jonathan Cameron
@ 2025-10-12 16:52 ` Shrikant
0 siblings, 0 replies; 12+ messages in thread
From: Shrikant @ 2025-10-12 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, matt, skhan,
david.hunter.linux, linux-iio, devicetree, linux-kernel,
linux-kernel-mentees
On Sun, Oct 12, 2025 at 7:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 6 Oct 2025 08:34:03 +0530
> Shrikant <raskar.shree97@gmail.com> wrote:
>
> > On Sat, Oct 4, 2025 at 6:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Sat, 4 Oct 2025 07:26:22 +0530
> > > Shrikant Raskar <raskar.shree97@gmail.com> wrote:
> > >
> > > > The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> > > > 800us, 1600us). These settings affect measurement resolution and power
> > > > consumption. Until now, the driver always defaulted to 1600us.
> > > >
> > > > Introduce a new device tree property `maxim,pulse-width` that allows
> > > > users to select the desired pulse width in microseconds from device
> > > > tree.
> > > >
> > > > Valid values are: 200, 400, 800, 1600.
> > > >
> > > > This prepares for driver changes that read this property and configure
> > > > the SPO2 register accordingly.
> > > >
> > > > Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
> > > Hi Shrikant,
> > >
> > > Explain why this is in some way related to characteristics of how the
> > > system containing this chip is built (wiring, lenses etc). Otherwise
> > > this might instead be something that should be controlled from userspace
> > > not firmware.
> > >
> > > Also, give a little more on why we care about controlling it at all.
> > > Is there a usecase where power use of this chip matters? Mostly I'd expect
> > > it to be in measurement equipment with relatively short measuring periods.
> > >
> > > Jonathan
> > Hi Jonathan,
> >
> > Thanks for the feedback.
> >
> > The pulse width configuration is indeed dependent on the hardware integration
> > of the MAX30100. It affects how much optical energy the LEDs emit per sample,
> > which in turn depends on physical factors such as:
> >
> > - The type and thickness of the optical window or lens covering the sensor
> > - The distance between the LED and photodiode
> > - The reflectivity of the skin/contact surface
> >
> > For example:
> > - A smartwatch/wearable ring with a thin glass window can operate
> > with 200–400 µs pulses to
> > save power, while
> > - A medical-grade pulse oximeter or a sensor mounted behind a thicker
> > protective layer may require 800–1600 µs for reliable signal amplitude.
> >
> > I believe it would be appropriate to describe these fixed,
> > board-specific characteristics in the Device Tree,
> > since they are determined by the hardware design rather than being
> > runtime or user-controlled parameters.
> >
> > Would it be okay if I send v2 of the patch with the above explanation
> > included in the commit message?
> Hi Shrikant,
>
> I'd have this excellent detail + examples summarised in the patch description
> and also a small amount of description in the actual binding even if that just says
> something like
> Description:
> Pulse width in... . Appropriate pulse width is dependant on factors
> such as optical window absorption, distances and expected reflectivity
> of skin / contact surface.
> That's just a quick mash up of what you have above, feel free to not use this
> particular text!
>
Thanks for your response. Sure, I will update the description in the
binding and will
update the commit message describing the details and examples. I will share the
updated version of the patch shortly.
> The inclusion of target surface reflectivity in there makes me thing that
> for some applications we may also need userspace tweaking or some algorithmic
> way to increase or decrease the value according to skin characteristics. However
> I guess maybe it isn't that sensitive.
Need to check on this point.
Thanks and regards,
Shrikant
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property
2025-10-04 1:56 ` [PATCH 1/2] dt-bindings: iio: max30100: Add pulse-width property Shrikant Raskar
2025-10-04 3:22 ` Bhanu Seshu Kumar Valluri
2025-10-04 13:12 ` Jonathan Cameron
@ 2025-10-06 14:40 ` Krzysztof Kozlowski
2 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2025-10-06 14:40 UTC (permalink / raw)
To: Shrikant Raskar, jic23, robh, krzk+dt, conor+dt
Cc: dlechner, nuno.sa, andy, matt, skhan, david.hunter.linux,
linux-iio, devicetree, linux-kernel, linux-kernel-mentees
On 04/10/2025 10:56, Shrikant Raskar wrote:
> The MAX30100 sensor supports multiple LED pulse widths (200us, 400us,
> 800us, 1600us). These settings affect measurement resolution and power
> consumption. Until now, the driver always defaulted to 1600us.
>
> Introduce a new device tree property `maxim,pulse-width` that allows
> users to select the desired pulse width in microseconds from device
> tree.
>
> Valid values are: 200, 400, 800, 1600.
Don't describe the contents of patch. We see the contents from the diff.
Above two paragraphs are completely redundant, drop.
>
> This prepares for driver changes that read this property and configure
> the SPO2 register accordingly.
Drop, redundant. Not really relevant.
>
> Signed-off-by: Shrikant Raskar <raskar.shree97@gmail.com>
> ---
> .../devicetree/bindings/iio/health/maxim,max30100.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml b/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> index 967778fb0ce8..55aaf2ff919b 100644
> --- a/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> +++ b/Documentation/devicetree/bindings/iio/health/maxim,max30100.yaml
> @@ -27,6 +27,11 @@ properties:
> LED current whilst the engine is running. First indexed value is
> the configuration for the RED LED, and second value is for the IR LED.
>
> + maxim,pulse-width:
You miss unit suffix and testing, most likely.
> + maxItems: 1
> + description: Pulse width in microseconds
> + enum: [200, 400, 800, 1600]x
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread