| Message ID | 20230308090219.12710-2-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Update APDS990x ALS to support IIO | expand |
On Wed, 08 Mar 2023 11:02:16 +0200, Svyatoslav Ryhel wrote: > Add dt-binding for apds990x ALS/proximity sensor. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > --- > .../bindings/iio/light/avago,apds990x.yaml | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/light/avago,apds990x.example.dtb: light-sensor@39: 'interrupt' is a required property From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230308090219.12710-2-clamor95@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Wed, 8 Mar 2023 11:02:16 +0200 Svyatoslav Ryhel <clamor95@gmail.com> wrote: > Add dt-binding for apds990x ALS/proximity sensor. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > --- > .../bindings/iio/light/avago,apds990x.yaml | 76 +++++++++++++++++++ I'm not a fan of wild cards. It breaks far too often. Can we name this instead after a particular supported part - same for compatible. I'm not sure what parts are supported by this, but you may want multiple compatibles. > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml > > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml > new file mode 100644 > index 000000000000..9b47e13f88e3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml > @@ -0,0 +1,76 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/light/avago,apds990x.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Avago APDS990x ALS and proximity sensor > + > +maintainers: > + - Samu Onkalo <samu.p.onkalo@nokia.com> > + > +description: | > + APDS990x is a combined ambient light and proximity sensor. ALS and > + proximity functionality are highly connected. ALS measurement path > + must be running while the proximity functionality is enabled. > + > +properties: > + compatible: > + const: avago,apds990x > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + vdd-supply: true > + vled-supply: true > + > + avago,pdrive: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 32 > + description: | > + Drive value used in configuring control register. Is this something where there is a reasonable default? If so I'd prefer it was optional so that the device is easier to use without needing firmware description. > + > + avago,ppcount: > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 32 > + description: | > + Number of pulses used for proximity sensor calibration. Same for this - if there is a reasonable default it would be good to have that specified. > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupt It would nice to relax the need for an interrupt if the device is still useable with timeouts etc. Board folk have a habit of deciding they don't need to wire up interrupts. We can relax that a later date though if you prefer not to do it now. > + - vdd-supply > + - vled-supply Whilst true that the supplies need to be connected, that doesn't mean they need to provided in the device tree binding. If they are always powered up I think we can fallback to stub regulators. > + - avago,pdrive > + - avago,ppcount > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + light-sensor@39 { > + compatible = "avago,apds990x"; > + reg = <0x39>; > + > + interrupt-parent = <&gpio>; > + interrupts = <82 IRQ_TYPE_EDGE_RISING>; > + > + vdd-supply = <&vdd_3v0_proxi>; > + vled-supply = <&vdd_1v8_sen>; > + > + avago,pdrive = <0x00>; > + avago,ppcount = <0x03>; > + }; > + }; > +...
On 11/03/2023 20:34, Jonathan Cameron wrote: >> + >> +additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupt > It would nice to relax the need for an interrupt if the device is still useable > with timeouts etc. Board folk have a habit of deciding they don't need to wire > up interrupts. We can relax that a later date though if you prefer not to do > it now. >> + - vdd-supply >> + - vled-supply > > Whilst true that the supplies need to be connected, that doesn't > mean they need to provided in the device tree binding. If they are > always powered up I think we can fallback to stub regulators. We can, but others might not. The binding should still require them if they are required for device to work. Mark also made it clear recently: https://lore.kernel.org/all/31ca0ede-012c-4849-bf25-d0492b116681@sirena.org.uk/ https://lore.kernel.org/all/5cd6764c-9b04-42ea-932d-9f14aa465605@sirena.org.uk/ https://lore.kernel.org/all/f6f02138-8ef9-4a33-9b51-0b7cd371230f@sirena.org.uk/ Best regards, Krzysztof
On Sun, 12 Mar 2023 11:47:19 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/03/2023 20:34, Jonathan Cameron wrote: > > >> + > >> +additionalProperties: false > >> + > >> +required: > >> + - compatible > >> + - reg > >> + - interrupt > > It would nice to relax the need for an interrupt if the device is still useable > > with timeouts etc. Board folk have a habit of deciding they don't need to wire > > up interrupts. We can relax that a later date though if you prefer not to do > > it now. > >> + - vdd-supply > >> + - vled-supply > > > > Whilst true that the supplies need to be connected, that doesn't > > mean they need to provided in the device tree binding. If they are > > always powered up I think we can fallback to stub regulators. > > We can, but others might not. The binding should still require them if > they are required for device to work. Mark also made it clear recently: > > https://lore.kernel.org/all/31ca0ede-012c-4849-bf25-d0492b116681@sirena.org.uk/ > https://lore.kernel.org/all/5cd6764c-9b04-42ea-932d-9f14aa465605@sirena.org.uk/ > https://lore.kernel.org/all/f6f02138-8ef9-4a33-9b51-0b7cd371230f@sirena.org.uk/ OK. Then there are a lot of bindings to fix. Seems odd to me but meh it's not something I care about. Note this means that we can't have trivial-device.yaml for instance. Ah well, I guess views change or crystallise over time or just differed in the first place. Jonathan > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml new file mode 100644 index 000000000000..9b47e13f88e3 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml @@ -0,0 +1,76 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/light/avago,apds990x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Avago APDS990x ALS and proximity sensor + +maintainers: + - Samu Onkalo <samu.p.onkalo@nokia.com> + +description: | + APDS990x is a combined ambient light and proximity sensor. ALS and + proximity functionality are highly connected. ALS measurement path + must be running while the proximity functionality is enabled. + +properties: + compatible: + const: avago,apds990x + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + vdd-supply: true + vled-supply: true + + avago,pdrive: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 32 + description: | + Drive value used in configuring control register. + + avago,ppcount: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 32 + description: | + Number of pulses used for proximity sensor calibration. + +additionalProperties: false + +required: + - compatible + - reg + - interrupt + - vdd-supply + - vled-supply + - avago,pdrive + - avago,ppcount + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + light-sensor@39 { + compatible = "avago,apds990x"; + reg = <0x39>; + + interrupt-parent = <&gpio>; + interrupts = <82 IRQ_TYPE_EDGE_RISING>; + + vdd-supply = <&vdd_3v0_proxi>; + vled-supply = <&vdd_1v8_sen>; + + avago,pdrive = <0x00>; + avago,ppcount = <0x03>; + }; + }; +...
Add dt-binding for apds990x ALS/proximity sensor. Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> --- .../bindings/iio/light/avago,apds990x.yaml | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds990x.yaml