diff mbox series

[v5,2/3] dt-bindings: iio: light: Avago APDS9306

Message ID 20240121051735.32246-3-subhajit.ghosh@tweaklogic.com
State New
Headers
Series Support for Avago APDS9306 Ambient Light Sensor |

Commit Message

Subhajit Ghosh Jan. 21, 2024, 5:17 a.m. UTC 
  Adding device tree support for APDS9306 Ambient Light Sensor.
Updating datasheet hyperlinks.
Adding interrupt definition macro and header file.
Adding vdd-supply property.

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
---
v2 -> v5:
 - Implemented changes as per previous reviews:
   Link: https://lore.kernel.org/lkml/20231028142944.7e210eb6@jic23-huawei/
   Link: https://lore.kernel.org/lkml/22e9e5e9-d26a-46e9-8986-5062bbfd72ec@linaro.org/
---
 .../bindings/iio/light/avago,apds9300.yaml        | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Krzysztof Kozlowski Jan. 22, 2024, 9:51 a.m. UTC | #1 
On 21/01/2024 06:17, Subhajit Ghosh wrote:
> Adding device tree support for APDS9306 Ambient Light Sensor.
> Updating datasheet hyperlinks.
> Adding interrupt definition macro and header file.
> Adding vdd-supply property.

Why? Do other devices have it?


>  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>;

Separate change.

Best regards,
Krzysztof
Subhajit Ghosh Jan. 22, 2024, 10:03 a.m. UTC | #2 
On 22/1/24 02:06, Jonathan Cameron wrote:
> On Sun, 21 Jan 2024 15:47:33 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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
Subhajit Ghosh Jan. 22, 2024, 10:07 a.m. UTC | #3 
On 22/1/24 20:21, Krzysztof Kozlowski wrote:
> On 21/01/2024 06:17, Subhajit Ghosh wrote:
>> Adding device tree support for APDS9306 Ambient Light Sensor.
>> Updating datasheet hyperlinks.
>> Adding interrupt definition macro and header file.
>> Adding vdd-supply property.
> 
> Why? Do other devices have it?
Are you referring to vdd-supply? No, I guess, haven't checked actually.
If other devices don't have, can you suggest the best way to handle that?
> 
> 
>>   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>;
> 
> Separate change.
Understood.
> 
> Best regards,
> Krzysztof
> 
Regards,
Subhajit Ghosh
Krzysztof Kozlowski Jan. 22, 2024, 12:30 p.m. UTC | #4 
On 22/01/2024 11:07, Subhajit Ghosh wrote:
> On 22/1/24 20:21, Krzysztof Kozlowski wrote:
>> On 21/01/2024 06:17, Subhajit Ghosh wrote:
>>> Adding device tree support for APDS9306 Ambient Light Sensor.
>>> Updating datasheet hyperlinks.
>>> Adding interrupt definition macro and header file.
>>> Adding vdd-supply property.
>>
>> Why? Do other devices have it?
> Are you referring to vdd-supply? No, I guess, haven't checked actually.
> If other devices don't have, can you suggest the best way to handle that?

Commit msg should explain that. I assume all devices need it, so just
like Jonathan suggested: this might be separate patch.

Best regards,
Krzysztof
diff mbox series

Patch

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 <subhajit.ghosh@tweaklogic.com>
 
 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
 
 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>;
         };
     };
 ...