| Message ID | 20231019080437.94849-1-subhajit.ghosh@tweaklogic.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: iio: light: Squash APDS9300 and APDS9960 schemas | expand |
On Thu, Oct 19, 2023 at 06:34:37PM +1030, Subhajit Ghosh wrote: > Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one > file and removing the other. > Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ "Yes, they look similar. I will combine them all in a single yaml file in the next revision. Thank you Krzysztof." Yet this is a follow-up patch, not a version 2. The original patches seem to not have been applied, so I am not sure why you didn't send a v2? Cheers, Conor. > --- > .../bindings/iio/light/avago,apds9300.yaml | 35 ++++++++++++--- > .../bindings/iio/light/avago,apds9960.yaml | 44 ------------------- > 2 files changed, 29 insertions(+), 50 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml > > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > index 206af44f2c43..7826a1749fcd 100644 > --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > @@ -4,17 +4,26 @@ > $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Avago APDS9300 ambient light sensor > +title: Avago Gesture, RGB, ALS and Proximity sensors > > maintainers: > - Jonathan Cameron <jic23@kernel.org> > + - Matt Ranostay <matt.ranostay@konsulko.com> > > description: | > - Datasheet at https://www.avagotech.com/docs/AV02-1077EN > + Avago (Broadcom) optical and proximity sensors with I2C interfaces. > + Datasheet at https://docs.broadcom.com/doc/AV02-1077EN > + Datasheet at https://docs.broadcom.com/doc/AV02-4191EN > > properties: > compatible: > - const: avago,apds9300 > + oneOf: > + - items: > + - const: avago,apds9300 > + - const: avago,apds9960 > + - enum: > + - avago,apds9300 > + - avago,apds9960 > > reg: > maxItems: 1 > @@ -22,14 +31,28 @@ properties: > interrupts: > maxItems: 1 > > -additionalProperties: false > - > required: > - compatible > - reg > > +allOf: > + - $ref: ../common.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - avago,apds9960 > + then: > + required: > + - interrupts > + > +additionalProperties: false > + > examples: > - | > + #include <dt-bindings/interrupt-controller/irq.h> > + > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -38,7 +61,7 @@ examples: > compatible = "avago,apds9300"; > reg = <0x39>; > interrupt-parent = <&gpio2>; > - interrupts = <29 8>; > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; > }; > }; > ... > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml > deleted file mode 100644 > index f06e0fda5629..000000000000 > --- a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml > +++ /dev/null > @@ -1,44 +0,0 @@ > -# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > -%YAML 1.2 > ---- > -$id: http://devicetree.org/schemas/iio/light/avago,apds9960.yaml# > -$schema: http://devicetree.org/meta-schemas/core.yaml# > - > -title: Avago APDS9960 gesture/RGB/ALS/proximity sensor > - > -maintainers: > - - Matt Ranostay <matt.ranostay@konsulko.com> > - > -description: | > - Datasheet at https://www.avagotech.com/docs/AV02-4191EN > - > -properties: > - compatible: > - const: avago,apds9960 > - > - reg: > - maxItems: 1 > - > - interrupts: > - maxItems: 1 > - > -additionalProperties: false > - > -required: > - - compatible > - - reg > - > -examples: > - - | > - i2c { > - #address-cells = <1>; > - #size-cells = <0>; > - > - light-sensor@39 { > - compatible = "avago,apds9960"; > - reg = <0x39>; > - interrupt-parent = <&gpio1>; > - interrupts = <16 1>; > - }; > - }; > -... > -- > 2.34.1 >
On Thu, Oct 19, 2023 at 09:51:39AM +0100, Conor Dooley wrote: > On Thu, Oct 19, 2023 at 06:34:37PM +1030, Subhajit Ghosh wrote: > > Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one > > file and removing the other. > > > Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ > > "Yes, they look similar. I will combine them all in a single yaml file in > the next revision. Thank you Krzysztof." > > Yet this is a follow-up patch, not a version 2. The original patches > seem to not have been applied, so I am not sure why you didn't send a > v2? > > --- > > .../bindings/iio/light/avago,apds9300.yaml | 35 ++++++++++++--- > > .../bindings/iio/light/avago,apds9960.yaml | 44 ------------------- > > 2 files changed, 29 insertions(+), 50 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml > > > > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > > index 206af44f2c43..7826a1749fcd 100644 > > --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > > +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > > @@ -4,17 +4,26 @@ > > $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Avago APDS9300 ambient light sensor > > +title: Avago Gesture, RGB, ALS and Proximity sensors > > > > maintainers: > > - Jonathan Cameron <jic23@kernel.org> > > + - Matt Ranostay <matt.ranostay@konsulko.com> Also: <matt.ranostay@konsulko.com>: host aspmx.l.google.com said: 550-5.1.1 The email account that you tried to reach does not exist.
On 19/10/2023 10:04, Subhajit Ghosh wrote: > Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one > file and removing the other. Please answer: why? > Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ > > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> > --- > .../bindings/iio/light/avago,apds9300.yaml | 35 ++++++++++++--- > .../bindings/iio/light/avago,apds9960.yaml | 44 ------------------- > 2 files changed, 29 insertions(+), 50 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml > > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > index 206af44f2c43..7826a1749fcd 100644 > --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml > @@ -4,17 +4,26 @@ > $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Avago APDS9300 ambient light sensor > +title: Avago Gesture, RGB, ALS and Proximity sensors > > maintainers: > - Jonathan Cameron <jic23@kernel.org> > + - Matt Ranostay <matt.ranostay@konsulko.com> > > description: | > - Datasheet at https://www.avagotech.com/docs/AV02-1077EN > + Avago (Broadcom) optical and proximity sensors with I2C interfaces. > + Datasheet at https://docs.broadcom.com/doc/AV02-1077EN > + Datasheet at https://docs.broadcom.com/doc/AV02-4191EN > > properties: > compatible: > - const: avago,apds9300 > + oneOf: > + - items: > + - const: avago,apds9300 > + - const: avago,apds9960 Why? Commit msg does not explain this. > + - enum: > + - avago,apds9300 > + - avago,apds9960 > > reg: > maxItems: 1 > @@ -22,14 +31,28 @@ properties: > interrupts: > maxItems: 1 > > -additionalProperties: false > - > required: > - compatible > - reg > > +allOf: > + - $ref: ../common.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - avago,apds9960 > + then: > + required: > + - interrupts Why? This wasn't in original binding. > + > +additionalProperties: false > + > examples: > - | > + #include <dt-bindings/interrupt-controller/irq.h> > + > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -38,7 +61,7 @@ examples: > compatible = "avago,apds9300"; > reg = <0x39>; > interrupt-parent = <&gpio2>; > - interrupts = <29 8>; > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; Separate patch please. You are doing way too many unexpected and not explained changes. Best regards, Krzysztof
On 19/10/23 19:21, Conor Dooley wrote: > On Thu, Oct 19, 2023 at 06:34:37PM +1030, Subhajit Ghosh wrote: >> Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one >> file and removing the other. > >> Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ > > "Yes, they look similar. I will combine them all in a single yaml file in > the next revision. Thank you Krzysztof." > > Yet this is a follow-up patch, not a version 2. The original patches > seem to not have been applied, so I am not sure why you didn't send a > v2? > > Cheers, > Conor. Sorry, I should have put a longer description and a longer commit message. That patch series adds a new driver - apds9306 which is separate to this patch. As per Krzysztof's comments, first operation is to merge the existing apds9300 and apds9960 schemas. This patch is the first operation. Second operation will be to add apds9306 support on top of that. I will explain more on Krzysztof's comments. Thank you for reviewing. Regards, Subhajit Ghosh
> Also: > <matt.ranostay@konsulko.com>: host aspmx.l.google.com said: > 550-5.1.1 The email account that you tried to reach does not exist. Thanks Conor for pointing this out. Can you please help me out with this? get_maintainer.pl suggested me to add this email ID. Regards, Subhajit Ghosh
On 19/10/23 19:50, Krzysztof Kozlowski wrote: > On 19/10/2023 10:04, Subhajit Ghosh wrote: >> Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one >> file and removing the other. > > Please answer: why? Apologies for not providing detailed explanation. Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ As per your comments on the patch series in the above link and as per my understanding, I have to do two operations: 1. Squash existing apds9300 schema and apds9960 schema as they look similar. 2. Add apds9306 (work in progress) support after that (which belongs to my original patch series). This patch is the first operation. >> >> +allOf: >> + - $ref: ../common.yaml# >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - avago,apds9960 >> + then: >> + required: >> + - interrupts > > Why? This wasn't in original binding. I am not sure about this. I went through the driver code and found out that probe() of apds9300 handles both situations whether interrupt bindings are provided or not, whereas, apds9960 requires an interrupt binding for probe() to be successful. I thought it would be appropriate to add that in the schema. > > Separate patch please. > > You are doing way too many unexpected and not explained changes. Sure. Thank you for reviewing. Regards, Subhajit Ghosh.
On Thu, 19 Oct 2023 21:24:09 +1030 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > > Also: > > <matt.ranostay@konsulko.com>: host aspmx.l.google.com said: > > 550-5.1.1 The email account that you tried to reach does not exist. > Thanks Conor for pointing this out. Can you please help me out with this? > get_maintainer.pl suggested me to add this email ID. Matt has moved job, I've messaged him to find out if he has a new preferred email address. J > > Regards, > Subhajit Ghosh >
On Thu, Oct 19, 2023 at 09:20:12PM +1030, Subhajit Ghosh wrote: > On 19/10/23 19:21, Conor Dooley wrote: > > On Thu, Oct 19, 2023 at 06:34:37PM +1030, Subhajit Ghosh wrote: > > > Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one > > > file and removing the other. > > > > > Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ > > > > "Yes, they look similar. I will combine them all in a single yaml file in > > the next revision. Thank you Krzysztof." > > > > Yet this is a follow-up patch, not a version 2. The original patches > > seem to not have been applied, so I am not sure why you didn't send a > > v2? > > > > Cheers, > > Conor. > > Sorry, I should have put a longer description and a longer commit message. > That patch series adds a new driver - apds9306 which is separate to this > patch. As per Krzysztof's comments, first operation is to merge the existing > apds9300 and apds9960 schemas. This patch is the first operation. > > Second operation will be to add apds9306 support on top of that. I will > explain more on Krzysztof's comments. Thank you for reviewing. Ahh apologies then. The best course of action would likely be to include the patch merging the two bindings in your series adding the third user.
>> Sorry, I should have put a longer description and a longer commit message. >> That patch series adds a new driver - apds9306 which is separate to this >> patch. As per Krzysztof's comments, first operation is to merge the existing >> apds9300 and apds9960 schemas. This patch is the first operation. >> >> Second operation will be to add apds9306 support on top of that. I will >> explain more on Krzysztof's comments. Thank you for reviewing. > > Ahh apologies then. The best course of action would likely be to include > the patch merging the two bindings in your series adding the third user. No worries. Sure. You can reject this patch then. I will add my changes in the main apds9306 patch series. Regards, Subhajit Ghosh
On 19/10/2023 13:16, Subhajit Ghosh wrote: > On 19/10/23 19:50, Krzysztof Kozlowski wrote: >> On 19/10/2023 10:04, Subhajit Ghosh wrote: >>> Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one >>> file and removing the other. >> >> Please answer: why? > Apologies for not providing detailed explanation. > Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ > As per your comments on the patch series in the above link and as per my understanding, > I have to do two operations: > 1. Squash existing apds9300 schema and apds9960 schema as they look similar. > 2. Add apds9306 (work in progress) support after that (which belongs to my original patch series). > This patch is the first operation. Answer in the commit. The commits should answer to the question: why are you doing it? >>> >>> +allOf: >>> + - $ref: ../common.yaml# >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - avago,apds9960 >>> + then: >>> + required: >>> + - interrupts >> >> Why? This wasn't in original binding. > I am not sure about this. I went through the driver code and found out that probe() Not explained in commit msg. > of apds9300 handles both situations whether interrupt bindings are provided or not, whereas, > apds9960 requires an interrupt binding for probe() to be successful. I thought it would > be appropriate to add that in the schema. Not explained in commit msg. Best regards, Krzysztof
On Thu, 19 Oct 2023 12:21:20 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Thu, 19 Oct 2023 21:24:09 +1030 > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > > > > Also: > > > <matt.ranostay@konsulko.com>: host aspmx.l.google.com said: > > > 550-5.1.1 The email account that you tried to reach does not exist. > > Thanks Conor for pointing this out. Can you please help me out with this? > > get_maintainer.pl suggested me to add this email ID. > > Matt has moved job, I've messaged him to find out if he has a new preferred > email address. > +CC address Matt suggested using going forwards. > J > > > > Regards, > > Subhajit Ghosh > > >
On Fri, Oct 20, 2023 at 08:28:04AM +0100, Jonathan Cameron wrote: > On Thu, 19 Oct 2023 12:21:20 +0100 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > On Thu, 19 Oct 2023 21:24:09 +1030 > > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > > > > > > Also: > > > > <matt.ranostay@konsulko.com>: host aspmx.l.google.com said: > > > > 550-5.1.1 The email account that you tried to reach does not exist. > > > Thanks Conor for pointing this out. Can you please help me out with this? > > > get_maintainer.pl suggested me to add this email ID. > > > > Matt has moved job, I've messaged him to find out if he has a new preferred > > email address. > > > +CC address Matt suggested using going forwards. Matt, can you please update your contact information in the various places where that is relevant? A mailmap entry would likely be a good idea too. Cheers, Conor.
>> > +CC address Matt suggested using going forwards. > >> J Sure. Thanks Jonathan. Regards, Subhajit Ghosh
>> of apds9300 handles both situations whether interrupt bindings are provided or not, whereas, >> apds9960 requires an interrupt binding for probe() to be successful. I thought it would >> be appropriate to add that in the schema. > > Not explained in commit msg. > > Best regards, > Krzysztof > Yes, I will add all these point in my next commit message in apds9306 patch series rather than a separate patch as suggested by Conor. Regards, Subhajit Ghosh
diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml index 206af44f2c43..7826a1749fcd 100644 --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml @@ -4,17 +4,26 @@ $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Avago APDS9300 ambient light sensor +title: Avago Gesture, RGB, ALS and Proximity sensors maintainers: - Jonathan Cameron <jic23@kernel.org> + - Matt Ranostay <matt.ranostay@konsulko.com> description: | - Datasheet at https://www.avagotech.com/docs/AV02-1077EN + Avago (Broadcom) optical and proximity sensors with I2C interfaces. + Datasheet at https://docs.broadcom.com/doc/AV02-1077EN + Datasheet at https://docs.broadcom.com/doc/AV02-4191EN properties: compatible: - const: avago,apds9300 + oneOf: + - items: + - const: avago,apds9300 + - const: avago,apds9960 + - enum: + - avago,apds9300 + - avago,apds9960 reg: maxItems: 1 @@ -22,14 +31,28 @@ properties: interrupts: maxItems: 1 -additionalProperties: false - required: - compatible - reg +allOf: + - $ref: ../common.yaml# + - if: + properties: + compatible: + contains: + enum: + - avago,apds9960 + then: + required: + - interrupts + +additionalProperties: false + examples: - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c { #address-cells = <1>; #size-cells = <0>; @@ -38,7 +61,7 @@ examples: compatible = "avago,apds9300"; reg = <0x39>; interrupt-parent = <&gpio2>; - interrupts = <29 8>; + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; }; }; ... diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml deleted file mode 100644 index f06e0fda5629..000000000000 --- a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml +++ /dev/null @@ -1,44 +0,0 @@ -# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) -%YAML 1.2 ---- -$id: http://devicetree.org/schemas/iio/light/avago,apds9960.yaml# -$schema: http://devicetree.org/meta-schemas/core.yaml# - -title: Avago APDS9960 gesture/RGB/ALS/proximity sensor - -maintainers: - - Matt Ranostay <matt.ranostay@konsulko.com> - -description: | - Datasheet at https://www.avagotech.com/docs/AV02-4191EN - -properties: - compatible: - const: avago,apds9960 - - reg: - maxItems: 1 - - interrupts: - maxItems: 1 - -additionalProperties: false - -required: - - compatible - - reg - -examples: - - | - i2c { - #address-cells = <1>; - #size-cells = <0>; - - light-sensor@39 { - compatible = "avago,apds9960"; - reg = <0x39>; - interrupt-parent = <&gpio1>; - interrupts = <16 1>; - }; - }; -...
Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one file and removing the other. Link: https://lore.kernel.org/all/4e785d2e-d310-4592-a75a-13549938dcef@linaro.org/ Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> --- .../bindings/iio/light/avago,apds9300.yaml | 35 ++++++++++++--- .../bindings/iio/light/avago,apds9960.yaml | 44 ------------------- 2 files changed, 29 insertions(+), 50 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml