Re: [PATCH v4 1/2] dt-bindings: iio: light: ROHM BH1745

[Jonathan Cameron <jic23@kernel.org>]

On Thu,  6 Jun 2024 17:29:41 +0100
Mudit Sharma <muditsharma.info@gmail.com> wrote:

> Add ROHM BH1745 - 4 channel I2C colour sensor's dt-bindings.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Hi Mudit,

I'd like to see the vcc-supply in here from the start.

Thanks,

Jonathan


> ---
> v3->v4:
> - No changes
> v2->v3:
> - Move 'additionalProperties' after 'required' block
> - Remove block style indicator '|' from description
> v1->v2:
> - Fix yaml issue: Make `maintainers` a list
> 
>  .../bindings/iio/light/rohm,bh1745.yaml       | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> new file mode 100644
> index 000000000000..d5338a0af6b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/rohm,bh1745.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BH1745 colour sensor
> +
> +maintainers:
> +  - Mudit Sharma <muditsharma.info@gmail.com>
> +
> +description:
> +  BH1745 is an I2C colour sensor with red, green, blue and clear
> +  channels. It has a programmable active low interrupt pin.
> +  Interrupt occurs when the signal from the selected interrupt
> +  source channel crosses set interrupt threshold high/low level.
> +
> +properties:
> +  compatible:
> +    const: rohm,bh1745
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
There are some historical bindings that are missing supplies, but
for everything recent we have tried to document them from the first.
So this should have

  vcc-supply: true

> +
> +required:
> +  - compatible
> +  - reg
    - vcc-supply

Even though the regulator framework will provide you with a dummy regulator,
the binding reflects that the device definitely needs power.

Note that there are nice devm_regulator_get_enable() and similar to deal
with turning the power on at driver load (I guess that doesn't do anything on
your board) and off at driver remove.  Ideally the driver should use
that.  There have been many occasions in the past (all a while back)
where we didn't add power supply control from the start and had to add
it soon after when it turned out some board didn't default to power
on for the sensors.

Thanks,

Jonathan

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        colour-sensor@38 {
> +            compatible = "rohm,bh1745";
> +            reg = <0x38>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +
> +...