[v1,1/4] dt-bindings: iio: light: add apds990x binding

  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
  

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
>