Re: [PATCH 1/2] dt-bindings: iio: light: add as7343

[Jonathan Cameron]

> Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
> with i2c address of 0x39.
> 
> Signed-off-by: Chang Yu <marcus.yu.56@xxxxxxxxx>

Given you need to send again to include right folk for testing and
review here are a few initial things to fix up and make it a proper
v2

>
> diff --git a/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> new file mode 100644
> index 000000000000..fb996160cf8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/ams,as7343.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS AS7343 14-Channel Multi-Spectral Sensor
> +
> +maintainers:
> +  - Chang Yu <marcus.yu.56@xxxxxxxxx>
> +
> +description: |
> +  The AMS AS7343 is a 14-channel multi-spectral sensor with i2c address of 0x39.
> +  https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ams,as7343
> +
> +  reg:
> +    description:
> +      I2C address of the device (0x39).
> +    maxItems: 1
> +
> +  vdd-supply: true

Power supplies that are needed in practice for a device to function
are always added to the required list (there are some historical
quirks in some bindings).  We do that even though the Linux regulator
framework will hand you a fake supply if you don't provide one via
the dt on basis the power is presumably always turned on.  The reason
for this is otherwise we can't see when we have a truely optional
power supply (common for references where there may be a number of
alternative sources such as internal voltage references).

I'd expect some binding info about whether LDR is connected to
anything or not. (LED Driver - who picked a common term like LDR
which is normally Light Dependent Resistor?!!)  We need the driver
to be able to decide to turn that on or off + set the drive
current.  Look at proximity sensors for examples of ways this might
be done.

There is also an interrupt pin that definitely wants to
be documented.  Bindings should be as full as possible.  The only
time we leave stuff out is when we have no idea how to write
the binding - that happens for sufficiently unusual corners that
aren't true for this.

The gpio is more interesting but at lest one option is to use
it as just that, so I'd expect to see binding for that.  The
sync stuff is perhaps an area where the binding design may be
less obvious and postponing that (mention it in the patch description)
might be the best path forwards.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>

Sashiko:
[Severity: Low]
Is it necessary to include irq.h here? The example does not appear to
define or use any interrupts.
-

Seems like a valid thing to clean up!
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        as73211@74 {

Should be a generic name not the part number.  If there isn't a
suitable one in the dt specification or in general use, make up
a new one.  Maybe multispectral is suitable here - I haven't really
thought about it much!

> +            compatible = "ams,as7343";
> +            reg = <0x39>;

Sashiko:
[Severity: Low]
Does the unit address (@74) mismatch the first address in the reg property
(<0x39>)? This might trigger a unit_address_vs_reg warning during
dt_binding_check and dtc compilation.

Also, should the node name use a generic functional name (such as
sensor@39) instead of referring to a different device (as73211)?
-


> +        };
> +    };

Thanks,

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>