2024-01-21 15:38:10

by Jonathan Cameron

Subject: Re: [PATCH v5 2/3] dt-bindings: iio: light: Avago APDS9306

On Sun, 21 Jan 2024 15:47:33 +1030
Subhajit Ghosh <[email protected]> wrote:

> Adding device tree support for APDS9306 Ambient Light Sensor.
> Updating datasheet hyperlinks.
> Adding interrupt definition macro and header file.

This is an unrelated change, so should probably be in a separate patch.

> Adding vdd-supply property.

This one is reasonable to have in same patch as the new device addition
as, whilst I assume it's valid for the existing devices, you are adding it
to incorporate something that device also has.
Could also be a separate precursor patch.

>
> Signed-off-by: Subhajit Ghosh <[email protected]>
> ---
> v2 -> v5:
> - Implemented changes as per previous reviews:
> Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/
> Link: https://lore.kernel.org/lkml/[email protected]/
> ---
> .../bindings/iio/light/avago,apds9300.yaml | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> index c610780346e8..bee73a590424 100644
> --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> @@ -4,19 +4,21 @@
> $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Avago Gesture/RGB/ALS/Proximity sensors
> +title: Avago (Broadcom) Gesture/RGB/ALS/Proximity sensors
>
> maintainers:
> - Subhajit Ghosh <[email protected]>
>
> description: |
> - Datasheet: https://www.avagotech.com/docs/AV02-1077EN
> - Datasheet: https://www.avagotech.com/docs/AV02-4191EN
> + Datasheet: https://docs.broadcom.com/doc/AV02-1077EN
> + Datasheet: https://docs.broadcom.com/doc/AV02-4191EN
> + Datasheet: https://docs.broadcom.com/doc/AV02-4755EN

Old links seem to still work, so why the change?

>
> properties:
> compatible:
> enum:
> - avago,apds9300
> + - avago,apds9306
> - avago,apds9960
>
> reg:
> @@ -25,6 +27,8 @@ properties:
> interrupts:
> maxItems: 1
>
> + vdd-supply: true
> +
> additionalProperties: false
>
> required:
> @@ -33,6 +37,8 @@ required:
>
> examples:
> - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> i2c {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -41,7 +47,8 @@ examples:
> compatible = "avago,apds9300";
> reg = <0x39>;
> interrupt-parent = <&gpio2>;
> - interrupts = <29 8>;
> + interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
> + vdd-supply = <&regulator_3v3>;
> };
> };
> ...

2024-01-23 05:30:10

by Subhajit Ghosh

Subject: Re: [PATCH v5 2/3] dt-bindings: iio: light: Avago APDS9306

On 22/1/24 02:06, Jonathan Cameron wrote:
> On Sun, 21 Jan 2024 15:47:33 +1030
> Subhajit Ghosh <[email protected]> wrote:
>
>> Adding device tree support for APDS9306 Ambient Light Sensor.
>> Updating datasheet hyperlinks.
>> Adding interrupt definition macro and header file.
>
> This is an unrelated change, so should probably be in a separate patch.
Understood.
>
>> Adding vdd-supply property.
>
> This one is reasonable to have in same patch as the new device addition
> as, whilst I assume it's valid for the existing devices, you are adding it
> to incorporate something that device also has.
> Could also be a separate precursor patch.
>
>>

>>
>> description: |
>> - Datasheet: https://www.avagotech.com/docs/AV02-1077EN
>> - Datasheet: https://www.avagotech.com/docs/AV02-4191EN
>> + Datasheet: https://docs.broadcom.com/doc/AV02-1077EN
>> + Datasheet: https://docs.broadcom.com/doc/AV02-4191EN
>> + Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>
> Old links seem to still work, so why the change?
My bad. I will revert to keep things simple. No reason, just thought of updating.

Thanks for the review.

Regards,
Subhajit Ghosh