linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2
@ 2025-08-01 22:39 Rodrigo Gobbi
  2025-08-02  8:24 ` Krzysztof Kozlowski
  2025-08-02 11:12 ` Jonathan Cameron
  0 siblings, 2 replies; 4+ messages in thread
From: Rodrigo Gobbi @ 2025-08-01 22:39 UTC (permalink / raw)
  To: robh, krzk+dt, jic23, dlechner, nuno.sa, andy, conor+dt,
	mranostay
  Cc: ~lkcamp/patches, linux-iio, linux-kernel, devicetree

Move existing ABI documentation from trivial to
a dedicated binding file since Lidar is not a trivial
device considering power-enable and mode control pin.

Also, add a fallback compatible for v3, which has the
same pinout and is already supported by the driver.

Fixes: b257c1a45e99 ("iio: pulsedlight-lidar-lite-v2: add lidar-lite-v3 property")
Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
---
I was unsure about sending a new v0 patch for this or send a v2. To avoid losing
the lore about this topic, I`m sending a v2. If this is not correct, I can send a
new patch later.

On 7/3/25 18:26, David Lechner wrote:
> On 7/1/25 5:30 PM, Rodrigo Gobbi wrote:
>> The compatible grmn,lidar-lite-v3 is managed by the same
>> driver of pulsedlight,lidar-lite-v2, which is a trivial device.
> 
> As a general rule of thumb, using the driver as justification for
> dt-bindings is never a good reason. The bindings describe the hardware,
> not the driver.
> 
> Assuming I found the correct datasheet [1], I see a power enable pin
> and a mode control pin, so I would say that this isn't a trivial device.
> Therefore this will need it's own new file. We could at least add
> power-gpios and power-supply properties. How to handle the mode pin
> isn't so clear to me though, so might omit that for now.
About the mode control pin and the data being returned within PWM, it`s also
unclear to me how to describe that here. Looking other kind of existing iio
devices, couldn`t find a reference for it so I`ve not described that.

Also, I`m quoting the driver author about this binding due the maintainer ref for it.

Dear @Matt Ranostay, I`ve noticed you were the original driver author.
During the discussion about adding lidar-v3 as trivial [1], we noticed that
this HW is not actually a trivial due other pins like power-enable
and mode control. We are considering moving v2 and v3 (which was not documented)
out of trivial and this is what this patch is trying to do. 
Also, we need a maintainer for the binding file and I`ve quoted you there.
I would appreciate your comments or suggestions over this topic.

Tks and regards to all.

Changelog:
v2: creating an initial binding for lidar v2 and v3 (fallback to v2)
    also, moving v2 out of trivial
v1: https://lore.kernel.org/all/20250701223341.36835-1-rodrigo.gobbi.7@gmail.com/#t
---
 .../proximity/pulsedlight,lidar-lite-v2.yaml  | 54 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 54 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml b/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml
new file mode 100644
index 000000000000..f49a1c365f3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/pulsedlight,lidar-lite-v2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pulsedlight LIDAR-Lite v2 range-finding sensor
+
+maintainers:
+  - Matt Ranostay <mranostay@gmail.com>
+
+description: |
+  Support for LIDAR_Lite v2 and v3 laser rangefinders. These devices
+  can use a simple I2C communication bus or can operate in a PWM mode using a
+  mode control pin to trigger acquisitions and return the measured distance.
+  It also have a power enable pin, which can be used to shut off the device.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - grmn,lidar-lite-v3
+          - const: pulsedlight,lidar-lite-v2
+      - const: pulsedlight,lidar-lite-v2
+
+  reg:
+    maxItems: 1
+
+  power-gpios:
+    description: GPIO that can be driven low to shut off power to the device.
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        proximity@62 {
+            compatible = "grmn,lidar-lite-v3", "pulsedlight,lidar-lite-v2";
+            reg = <0x62>;
+            vdd-supply = <&vdd_5v0>;
+        };
+    };
+...
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 8da408107e55..347897b999c9 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -313,8 +313,6 @@ properties:
           - onnn,adt7462
             # 48-Lane, 12-Port PCI Express Gen 2 (5.0 GT/s) Switch
           - plx,pex8648
-            # Pulsedlight LIDAR range-finding sensor
-          - pulsedlight,lidar-lite-v2
             # Renesas HS3001 Temperature and Relative Humidity Sensors
           - renesas,hs3001
             # Renesas ISL29501 time-of-flight sensor
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2
  2025-08-01 22:39 [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2 Rodrigo Gobbi
@ 2025-08-02  8:24 ` Krzysztof Kozlowski
  2025-08-02 11:12 ` Jonathan Cameron
  1 sibling, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-02  8:24 UTC (permalink / raw)
  To: Rodrigo Gobbi, robh, krzk+dt, jic23, dlechner, nuno.sa, andy,
	conor+dt, mranostay
  Cc: ~lkcamp/patches, linux-iio, linux-kernel, devicetree

On 02/08/2025 00:39, Rodrigo Gobbi wrote:
> Move existing ABI documentation from trivial to
> a dedicated binding file since Lidar is not a trivial
> device considering power-enable and mode control pin.
> 
> Also, add a fallback compatible for v3, which has the
> same pinout and is already supported by the driver.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597


You add completely new compatible grmn,lidar-lite-v3 and nothing in
subject or commit msg explains that. Actually subject is completely
opposite - says v2, not v3.


> 
> Fixes: b257c1a45e99 ("iio: pulsedlight-lidar-lite-v2: add lidar-lite-v3 property")

What is being fixed? I already asked to describe the issue and I do not
see here issue being fixed. You add new stuff mixed with something else.

See submitting patches how to organize your patchset.


> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> I was unsure about sending a new v0 patch for this or send a v2. To avoid losing
> the lore about this topic, I`m sending a v2. If this is not correct, I can send a
> new patch later.
> 


> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - grmn,lidar-lite-v3
> +          - const: pulsedlight,lidar-lite-v2
> +      - const: pulsedlight,lidar-lite-v2
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-gpios:

Please use standard gpio naming, see gpio-consumer-common.yaml.

> +    description: GPIO that can be driven low to shut off power to the device.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        proximity@62 {

lidar@

> +            compatible = "grmn,lidar-lite-v3", "pulsedlight,lidar-lite-v2";
> +            reg = <0x62>;
> +            vdd-supply = <&vdd_5v0>;
> +        };
> +    };


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2
  2025-08-01 22:39 [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2 Rodrigo Gobbi
  2025-08-02  8:24 ` Krzysztof Kozlowski
@ 2025-08-02 11:12 ` Jonathan Cameron
  2025-08-24 10:23   ` William Breathitt Gray
  1 sibling, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2025-08-02 11:12 UTC (permalink / raw)
  To: Rodrigo Gobbi
  Cc: robh, krzk+dt, dlechner, nuno.sa, andy, conor+dt, mranostay,
	~lkcamp/patches, linux-iio, linux-kernel, devicetree,
	William Breathitt Gray

On Fri,  1 Aug 2025 19:39:15 -0300
Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com> wrote:

> Move existing ABI documentation from trivial to
> a dedicated binding file since Lidar is not a trivial
> device considering power-enable and mode control pin.
> 
> Also, add a fallback compatible for v3, which has the
> same pinout and is already supported by the driver.
> 
> Fixes: b257c1a45e99 ("iio: pulsedlight-lidar-lite-v2: add lidar-lite-v3 property")
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> I was unsure about sending a new v0 patch for this or send a v2. To avoid losing
> the lore about this topic, I`m sending a v2. If this is not correct, I can send a
> new patch later.
> 
> On 7/3/25 18:26, David Lechner wrote:
> > On 7/1/25 5:30 PM, Rodrigo Gobbi wrote:  
> >> The compatible grmn,lidar-lite-v3 is managed by the same
> >> driver of pulsedlight,lidar-lite-v2, which is a trivial device.  
> > 
> > As a general rule of thumb, using the driver as justification for
> > dt-bindings is never a good reason. The bindings describe the hardware,
> > not the driver.
> > 
> > Assuming I found the correct datasheet [1], I see a power enable pin
> > and a mode control pin, so I would say that this isn't a trivial device.
> > Therefore this will need it's own new file. We could at least add
> > power-gpios and power-supply properties. How to handle the mode pin
> > isn't so clear to me though, so might omit that for now.  
> About the mode control pin and the data being returned within PWM, it`s also
> unclear to me how to describe that here. Looking other kind of existing iio
> devices, couldn`t find a reference for it so I`ve not described that.

So far we've never supported a sensor with a PWM output.  Needs some capture logic
and whilst there is some supported in the kernel, I don't think we have the
infrastructure to describe the sensor beyond it. It relies on an odd combination
of triggering via a light pull low that the device then drives high.  To make
that work with a standard capture unit is probably a case of wiring multiple pins
or some external components.

+CC counters subsystem maintainer William.

https://static.garmin.com/pumac/LIDAR_Lite_v3_Operation_Manual_and_Technical_Specifications.pdf
for reference

However, I'm also in agreement with others that this is an unusual case where
we are very likely to missdesign a DT-binding without having explored what the
driver stack looks like and so are best just leaving a gap for now.

Even if we did describe the mode stuff it would be optional so not describing it
for now should be fine.


> 
> Also, I`m quoting the driver author about this binding due the maintainer ref for it.
> 
> Dear @Matt Ranostay, I`ve noticed you were the original driver author.
> During the discussion about adding lidar-v3 as trivial [1], we noticed that
> this HW is not actually a trivial due other pins like power-enable
> and mode control. We are considering moving v2 and v3 (which was not documented)
> out of trivial and this is what this patch is trying to do. 
> Also, we need a maintainer for the binding file and I`ve quoted you there.
> I would appreciate your comments or suggestions over this topic.
> 
> Tks and regards to all.
> 
> Changelog:
> v2: creating an initial binding for lidar v2 and v3 (fallback to v2)
>     also, moving v2 out of trivial
> v1: https://lore.kernel.org/all/20250701223341.36835-1-rodrigo.gobbi.7@gmail.com/#t
> ---
>  .../proximity/pulsedlight,lidar-lite-v2.yaml  | 54 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 -
>  2 files changed, 54 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml b/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml
> new file mode 100644
> index 000000000000..f49a1c365f3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/pulsedlight,lidar-lite-v2.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/pulsedlight,lidar-lite-v2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Pulsedlight LIDAR-Lite v2 range-finding sensor
> +
> +maintainers:
> +  - Matt Ranostay <mranostay@gmail.com>
> +
> +description: |
> +  Support for LIDAR_Lite v2 and v3 laser rangefinders. These devices
> +  can use a simple I2C communication bus or can operate in a PWM mode using a
> +  mode control pin to trigger acquisitions and return the measured distance.
> +  It also have a power enable pin, which can be used to shut off the device.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - grmn,lidar-lite-v3
> +          - const: pulsedlight,lidar-lite-v2
> +      - const: pulsedlight,lidar-lite-v2
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-gpios:
> +    description: GPIO that can be driven low to shut off power to the device.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        proximity@62 {
> +            compatible = "grmn,lidar-lite-v3", "pulsedlight,lidar-lite-v2";
> +            reg = <0x62>;
> +            vdd-supply = <&vdd_5v0>;
> +        };
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 8da408107e55..347897b999c9 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -313,8 +313,6 @@ properties:
>            - onnn,adt7462
>              # 48-Lane, 12-Port PCI Express Gen 2 (5.0 GT/s) Switch
>            - plx,pex8648
> -            # Pulsedlight LIDAR range-finding sensor
> -          - pulsedlight,lidar-lite-v2
>              # Renesas HS3001 Temperature and Relative Humidity Sensors
>            - renesas,hs3001
>              # Renesas ISL29501 time-of-flight sensor


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2
  2025-08-02 11:12 ` Jonathan Cameron
@ 2025-08-24 10:23   ` William Breathitt Gray
  0 siblings, 0 replies; 4+ messages in thread
From: William Breathitt Gray @ 2025-08-24 10:23 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: William Breathitt Gray, Rodrigo Gobbi, robh, krzk+dt, dlechner,
	nuno.sa, andy, conor+dt, mranostay, ~lkcamp/patches, linux-iio,
	linux-kernel, devicetree

On Sat, Aug 02, 2025 at 12:12:38PM +0100, Jonathan Cameron wrote:
> > On 7/3/25 18:26, David Lechner wrote:
> > > On 7/1/25 5:30 PM, Rodrigo Gobbi wrote:  
> > >> The compatible grmn,lidar-lite-v3 is managed by the same
> > >> driver of pulsedlight,lidar-lite-v2, which is a trivial device.  
> > > 
> > > As a general rule of thumb, using the driver as justification for
> > > dt-bindings is never a good reason. The bindings describe the hardware,
> > > not the driver.
> > > 
> > > Assuming I found the correct datasheet [1], I see a power enable pin
> > > and a mode control pin, so I would say that this isn't a trivial device.
> > > Therefore this will need it's own new file. We could at least add
> > > power-gpios and power-supply properties. How to handle the mode pin
> > > isn't so clear to me though, so might omit that for now.  
> > About the mode control pin and the data being returned within PWM, it`s also
> > unclear to me how to describe that here. Looking other kind of existing iio
> > devices, couldn`t find a reference for it so I`ve not described that.
> 
> So far we've never supported a sensor with a PWM output.  Needs some capture logic
> and whilst there is some supported in the kernel, I don't think we have the
> infrastructure to describe the sensor beyond it. It relies on an odd combination
> of triggering via a light pull low that the device then drives high.  To make
> that work with a standard capture unit is probably a case of wiring multiple pins
> or some external components.
> 
> +CC counters subsystem maintainer William.
> 
> https://static.garmin.com/pumac/LIDAR_Lite_v3_Operation_Manual_and_Technical_Specifications.pdf
> for reference

So how does this look from the monitor pin side; a low level for some
amount of time followed by a high for a set duration, where distance can
then be derived from the amount of time spanning between falling edge
and rising edge?

In theory this could be represented as a counter device (in the context
of the Generic Counter paradigm the monitor pin would serve as the
"Signal"), then Counter events could be defined for the edge
transitions, and finally distance can be derived by userspace by
capturing the edge transition events and comparing the timestamps.

However, while that approach works in a technical sense, it definitely
feels clunky. On a conceptual level, a remote detection instrument
like a lidar isn't really a counter device: it's purpose isn't to track
the progression of a count.

I think it would be more appropriate for us first to properly define an
abstract representation for these kind of range-finding devices. For
example, these devices are going to have some sort of trigger (whether a
pin polarity or command sequence), some sort
of transmission signal (involving a particular strength, frequency, and
duration), and some sort of reception interpretation (perhaps involving
noise filtration, multiple previous record averaging, and timestamping).

Once the general abstract representation of these types of devices is
adequately defined, the DT-binding design will naturally arise from it
and subsequently a proper driver interface.

> However, I'm also in agreement with others that this is an unusual case where
> we are very likely to missdesign a DT-binding without having explored what the
> driver stack looks like and so are best just leaving a gap for now.
> 
> Even if we did describe the mode stuff it would be optional so not describing it
> for now should be fine.

I second this as well for now. There may be other devices that we will
encounter with similar mode pins and such that would help us figure out
the best way to introduce these to a DT-binding in the future.

William Breathitt Gray

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-08-24 10:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-01 22:39 [PATCH v2] dt-bindings: iio: proximity: Add Lidar-lite-v2 Rodrigo Gobbi
2025-08-02  8:24 ` Krzysztof Kozlowski
2025-08-02 11:12 ` Jonathan Cameron
2025-08-24 10:23   ` William Breathitt Gray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).