| Message ID | 20241028190628.257249-2-mirela.rabulea@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: Add OX05B1S camera sensor driver | expand |
On 28/10/2024 20:06, Mirela Rabulea wrote: > Add bindings for OX05B1S sensor driver > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. </form letter> Binding also looks very different than all other devices, so re-write it starting from EXISTING GOOD bindings. Not some downstream stuff. A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 A nit, subject: drop second/last "driver". Bindings are for hardware, not drivers. Best regards, Krzysztof
On 28/10/2024 19:06, Mirela Rabulea wrote: > Add bindings for OX05B1S sensor driver > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > .../bindings/media/i2c/ovti,ox05b1s.yaml | 109 ++++++++++++++++++ > 1 file changed, 109 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > new file mode 100644 > index 000000000000..d47e1950f24d > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > @@ -0,0 +1,109 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (C) 2024, NXP > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ox05b1s.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OX05B1S Image Sensor > + > +maintainers: > + - Mirela Rabulea <mirela.rabulea@nxp.com> > + > +description: |- > + The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active array size > + of 2592 x 1944. It is programmable through I2C interface. > + The sensor output is available via CSI-2 serial data output. > + You should add +allOf: + - $ref: /schemas/media/video-interface-devices.yaml# > +properties: > + compatible: > + items: > + - enum: > + - ovti,ox05b1s > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + description: Input clock (24 MHz) > + items: > + - const: csi_mclk > + > + assigned-clocks: > + maxItems: 1 > + > + assigned-clock-parents: > + maxItems: 1 > + > + assigned-clock-rates: > + maxItems: 1 > + assigned-clock* should be dropped. https://lore.kernel.org/all/20241025-b4-linux-next-202041004-i2c-media-yaml-fixes-v2-1-1b4535174a5a@linaro.org/ > + > + orientation: true > + rotation: true I think you can drop both of these too. > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + description: MIPI CSI-2 transmitter port > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + anyOf: > + - items: > + - const: 1 > + - const: 2 > + - items: > + - const: 1 > + - const: 2 > + - const: 3 > + - const: 4 > + required: > + - data-lanes > + > + required: > + - endpoint > + > +required: > + - compatible > + - reg > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ox05b1s: ox05b1s@36 { > + compatible = "ovti,ox05b1s"; > + reg = <0x36>; > + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; > + orientation = <2>; > + rotation = <0>; > + status = "okay"; You should include assigned-clock* here in the example. > + > + port { > + ox05b1s_mipi_0_ep: endpoint { > + remote-endpoint = <&mipi_csi0_ep>; > + data-lanes = <1 2 3 4>; > + }; > + }; > + }; > + }; > +... --- bod
On Tue, Oct 29, 2024 at 11:44:33AM +0000, Bryan O'Donoghue wrote: > On 28/10/2024 19:06, Mirela Rabulea wrote: > > Add bindings for OX05B1S sensor driver > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > --- > > .../bindings/media/i2c/ovti,ox05b1s.yaml | 109 ++++++++++++++++++ > > 1 file changed, 109 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > > new file mode 100644 > > index 000000000000..d47e1950f24d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml > > @@ -0,0 +1,109 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright (C) 2024, NXP > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ox05b1s.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision OX05B1S Image Sensor > > + > > +maintainers: > > + - Mirela Rabulea <mirela.rabulea@nxp.com> > > + > > +description: |- > > + The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active array size Reflow to 80 columns. > > + of 2592 x 1944. It is programmable through I2C interface. > > + The sensor output is available via CSI-2 serial data output. > > + > > You should add > > +allOf: > + - $ref: /schemas/media/video-interface-devices.yaml# > > > +properties: > > + compatible: > > + items: > > + - enum: > > + - ovti,ox05b1s > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: Input clock (24 MHz) > > + items: > > + - const: csi_mclk If there's a single clock you can drop the name. How about regulators ? > > + > > + assigned-clocks: > > + maxItems: 1 > > + > > + assigned-clock-parents: > > + maxItems: 1 > > + > > + assigned-clock-rates: > > + maxItems: 1 > > + > > assigned-clock* should be dropped. > > https://lore.kernel.org/all/20241025-b4-linux-next-202041004-i2c-media-yaml-fixes-v2-1-1b4535174a5a@linaro.org/ Agreed. > > + > > + orientation: true > > + rotation: true > > I think you can drop both of these too. Aren't they needed given that the binding ends with additionalProperties: false ? > > + > > + port: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + description: MIPI CSI-2 transmitter port > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: > > + anyOf: > > + - items: > > + - const: 1 > > + - const: 2 > > + - items: > > + - const: 1 > > + - const: 2 > > + - const: 3 > > + - const: 4 > > + required: > > + - data-lanes > > + > > + required: > > + - endpoint > > + > > +required: > > + - compatible > > + - reg The device requires a clock, shouldn't the clocks property be required ? > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ox05b1s: ox05b1s@36 { > > + compatible = "ovti,ox05b1s"; > > + reg = <0x36>; > > + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; This isn't specified in the bindings. Does the example validate ? > > + orientation = <2>; > > + rotation = <0>; > > + status = "okay"; > > You should include assigned-clock* here in the example. Is that mandatory ? I'd rather omit it, I think it only adds noise. > > + > > + port { > > + ox05b1s_mipi_0_ep: endpoint { > > + remote-endpoint = <&mipi_csi0_ep>; > > + data-lanes = <1 2 3 4>; > > + }; > > + }; > > + }; > > + }; > > +...
On 29/10/2024 11:57, Laurent Pinchart wrote: > Aren't they needed given that the binding ends with > > additionalProperties: false Yes. Might be nice to have unevaluatedProperties: false and just rely on the top level $ref: /schemas/media/video-interface-devices.yaml# Seems redundant to me to keep specifying these properties over and over again. --- bod
On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: > On 28/10/2024 20:06, Mirela Rabulea wrote: > > Add bindings for OX05B1S sensor driver > > > > Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > > <form letter> > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. > > Tools like b4 or scripts/get_maintainer.pl provide you proper list of > people, so fix your workflow. Tools might also fail if you work on some > ancient tree (don't, instead use mainline) or work on fork of kernel > (don't, instead use mainline). Just use b4 and everything should be > fine, although remember about `b4 prep --auto-to-cc` if you added new > patches to the patchset. > > You missed at least devicetree list (maybe more), so this won't be > tested by automated tooling. Performing review on untested code might be > a waste of time. > > Please kindly resend and include all necessary To/Cc entries. > </form letter> > > Binding also looks very different than all other devices, so re-write it > starting from EXISTING GOOD bindings. Not some downstream stuff. Krzysztof, please point to a good example when making this kind of comment. > A nit, subject: drop second/last, redundant "bindings". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > A nit, subject: drop second/last "driver". Bindings are for hardware, > not drivers.
On 29/10/2024 13:10, Laurent Pinchart wrote: > On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: >> On 28/10/2024 20:06, Mirela Rabulea wrote: >>> Add bindings for OX05B1S sensor driver >>> >>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> >> >> <form letter> >> Please use scripts/get_maintainers.pl to get a list of necessary people >> and lists to CC. It might happen, that command when run on an older >> kernel, gives you outdated entries. Therefore please be sure you base >> your patches on recent Linux kernel. >> >> Tools like b4 or scripts/get_maintainer.pl provide you proper list of >> people, so fix your workflow. Tools might also fail if you work on some >> ancient tree (don't, instead use mainline) or work on fork of kernel >> (don't, instead use mainline). Just use b4 and everything should be >> fine, although remember about `b4 prep --auto-to-cc` if you added new >> patches to the patchset. >> >> You missed at least devicetree list (maybe more), so this won't be >> tested by automated tooling. Performing review on untested code might be >> a waste of time. >> >> Please kindly resend and include all necessary To/Cc entries. >> </form letter> >> >> Binding also looks very different than all other devices, so re-write it >> starting from EXISTING GOOD bindings. Not some downstream stuff. > > Krzysztof, please point to a good example when making this kind of > comment. Anything recently added. Git log tells which files were recently added. Best regards, Krzysztof
On Tue, Oct 29, 2024 at 01:15:46PM +0100, Krzysztof Kozlowski wrote: > On 29/10/2024 13:10, Laurent Pinchart wrote: > > On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: > >> On 28/10/2024 20:06, Mirela Rabulea wrote: > >>> Add bindings for OX05B1S sensor driver > >>> > >>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > >> > >> <form letter> > >> Please use scripts/get_maintainers.pl to get a list of necessary people > >> and lists to CC. It might happen, that command when run on an older > >> kernel, gives you outdated entries. Therefore please be sure you base > >> your patches on recent Linux kernel. > >> > >> Tools like b4 or scripts/get_maintainer.pl provide you proper list of > >> people, so fix your workflow. Tools might also fail if you work on some > >> ancient tree (don't, instead use mainline) or work on fork of kernel > >> (don't, instead use mainline). Just use b4 and everything should be > >> fine, although remember about `b4 prep --auto-to-cc` if you added new > >> patches to the patchset. > >> > >> You missed at least devicetree list (maybe more), so this won't be > >> tested by automated tooling. Performing review on untested code might be > >> a waste of time. > >> > >> Please kindly resend and include all necessary To/Cc entries. > >> </form letter> > >> > >> Binding also looks very different than all other devices, so re-write it > >> starting from EXISTING GOOD bindings. Not some downstream stuff. > > > > Krzysztof, please point to a good example when making this kind of > > comment. > > Anything recently added. Git log tells which files were recently added. If the review comment is a copy&paste (given that you review lots of bindings and constantly have to repeat the same things, that would make sense), expanding it with that information for future reviews could help patch authors. Thanks for considering it, it would be much appreciated.
On 29/10/2024 13:21, Laurent Pinchart wrote: > On Tue, Oct 29, 2024 at 01:15:46PM +0100, Krzysztof Kozlowski wrote: >> On 29/10/2024 13:10, Laurent Pinchart wrote: >>> On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: >>>> On 28/10/2024 20:06, Mirela Rabulea wrote: >>>>> Add bindings for OX05B1S sensor driver >>>>> >>>>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> >>>> >>>> <form letter> >>>> Please use scripts/get_maintainers.pl to get a list of necessary people >>>> and lists to CC. It might happen, that command when run on an older >>>> kernel, gives you outdated entries. Therefore please be sure you base >>>> your patches on recent Linux kernel. >>>> >>>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of >>>> people, so fix your workflow. Tools might also fail if you work on some >>>> ancient tree (don't, instead use mainline) or work on fork of kernel >>>> (don't, instead use mainline). Just use b4 and everything should be >>>> fine, although remember about `b4 prep --auto-to-cc` if you added new >>>> patches to the patchset. >>>> >>>> You missed at least devicetree list (maybe more), so this won't be >>>> tested by automated tooling. Performing review on untested code might be >>>> a waste of time. >>>> >>>> Please kindly resend and include all necessary To/Cc entries. >>>> </form letter> >>>> >>>> Binding also looks very different than all other devices, so re-write it >>>> starting from EXISTING GOOD bindings. Not some downstream stuff. >>> >>> Krzysztof, please point to a good example when making this kind of >>> comment. >> >> Anything recently added. Git log tells which files were recently added. > > If the review comment is a copy&paste (given that you review lots of > bindings and constantly have to repeat the same things, that would make > sense), expanding it with that information for future reviews could help > patch authors. Thanks for considering it, it would be much appreciated. Sorry, but that's not the point. You do not take 10 yo, unmaintained driver and use it as template for your new one. Instead you rather take something recent or something which you know is correct. Same with bindings. NXP is not a small company which does not know how to use Linux or how to upstream stuff. This is not individual's contribution, where one does not have colleagues or 3 billions USD of revenue behind, to be able to get some internal help prior sending something downstream. They can spend something out of these 3 billions of revenue or 700 millions of net income to hire you guys or any other open-source company, if basics of upstreaming are unknown. That's the comment I was giving about NXP since a year. Some things around SoC improved, some things from this unit of NXP here did not change at all. So again, it's not about me giving them more things. They will keep ignoring it over and over, because that's how big companies sometimes behave. You know, community people work for free, right? Best regards, Krzysztof
(CC'ing the devicetree mailing list) On Tue, Oct 29, 2024 at 01:28:51PM +0100, Krzysztof Kozlowski wrote: > On 29/10/2024 13:21, Laurent Pinchart wrote: > > On Tue, Oct 29, 2024 at 01:15:46PM +0100, Krzysztof Kozlowski wrote: > >> On 29/10/2024 13:10, Laurent Pinchart wrote: > >>> On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: > >>>> On 28/10/2024 20:06, Mirela Rabulea wrote: > >>>>> Add bindings for OX05B1S sensor driver > >>>>> > >>>>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > >>>> > >>>> <form letter> > >>>> Please use scripts/get_maintainers.pl to get a list of necessary people > >>>> and lists to CC. It might happen, that command when run on an older > >>>> kernel, gives you outdated entries. Therefore please be sure you base > >>>> your patches on recent Linux kernel. > >>>> > >>>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of > >>>> people, so fix your workflow. Tools might also fail if you work on some > >>>> ancient tree (don't, instead use mainline) or work on fork of kernel > >>>> (don't, instead use mainline). Just use b4 and everything should be > >>>> fine, although remember about `b4 prep --auto-to-cc` if you added new > >>>> patches to the patchset. > >>>> > >>>> You missed at least devicetree list (maybe more), so this won't be > >>>> tested by automated tooling. Performing review on untested code might be > >>>> a waste of time. > >>>> > >>>> Please kindly resend and include all necessary To/Cc entries. > >>>> </form letter> > >>>> > >>>> Binding also looks very different than all other devices, so re-write it > >>>> starting from EXISTING GOOD bindings. Not some downstream stuff. > >>> > >>> Krzysztof, please point to a good example when making this kind of > >>> comment. > >> > >> Anything recently added. Git log tells which files were recently added. > > > > If the review comment is a copy&paste (given that you review lots of > > bindings and constantly have to repeat the same things, that would make > > sense), expanding it with that information for future reviews could help > > patch authors. Thanks for considering it, it would be much appreciated. > > Sorry, but that's not the point. You do not take 10 yo, unmaintained > driver and use it as template for your new one. Instead you rather take > something recent or something which you know is correct. Same with bindings. I wouldn't know for sure which driver or binding was used as a starting point. My point was unrelated to this particular patch series. I think that including clear information in ready-made answers will help everybody. It will tell the submitters what they need to know, it will avoid this kind of conversation being repeated, and it could even in the end increase the quality of submissions. Even better, it won't cost anything to add it to answer templates. > NXP is not a small company which does not know how to use Linux or how > to upstream stuff. This is not individual's contribution, where one does > not have colleagues or 3 billions USD of revenue behind, to be able to > get some internal help prior sending something downstream. > > They can spend something out of these 3 billions of revenue or 700 > millions of net income to hire you guys or any other open-source > company, if basics of upstreaming are unknown. > > That's the comment I was giving about NXP since a year. Some things > around SoC improved, some things from this unit of NXP here did not > change at all. If I were on the receiving end of this, as an individual developer, I would consider it very patronizing and insulting. Treating the authors of contributions you don't consider as good enough in such a harsh way will not improve the situation, and will drive people away. You may be frustrated by some companies, but this kind of comment will not help. Please soften your tone towards individual developers, they're not punching balls on which to dump frustration and anger. Firm and polite will work better than lashing out. > So again, it's not about me giving them more things. They will keep > ignoring it over and over, because that's how big companies sometimes > behave. You know, community people work for free, right?
On 29/10/2024 13:46, Laurent Pinchart wrote: > (CC'ing the devicetree mailing list) > > On Tue, Oct 29, 2024 at 01:28:51PM +0100, Krzysztof Kozlowski wrote: >> On 29/10/2024 13:21, Laurent Pinchart wrote: >>> On Tue, Oct 29, 2024 at 01:15:46PM +0100, Krzysztof Kozlowski wrote: >>>> On 29/10/2024 13:10, Laurent Pinchart wrote: >>>>> On Tue, Oct 29, 2024 at 07:14:28AM +0100, Krzysztof Kozlowski wrote: >>>>>> On 28/10/2024 20:06, Mirela Rabulea wrote: >>>>>>> Add bindings for OX05B1S sensor driver >>>>>>> >>>>>>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> >>>>>> >>>>>> <form letter> >>>>>> Please use scripts/get_maintainers.pl to get a list of necessary people >>>>>> and lists to CC. It might happen, that command when run on an older >>>>>> kernel, gives you outdated entries. Therefore please be sure you base >>>>>> your patches on recent Linux kernel. >>>>>> >>>>>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of >>>>>> people, so fix your workflow. Tools might also fail if you work on some >>>>>> ancient tree (don't, instead use mainline) or work on fork of kernel >>>>>> (don't, instead use mainline). Just use b4 and everything should be >>>>>> fine, although remember about `b4 prep --auto-to-cc` if you added new >>>>>> patches to the patchset. >>>>>> >>>>>> You missed at least devicetree list (maybe more), so this won't be >>>>>> tested by automated tooling. Performing review on untested code might be >>>>>> a waste of time. >>>>>> >>>>>> Please kindly resend and include all necessary To/Cc entries. >>>>>> </form letter> >>>>>> >>>>>> Binding also looks very different than all other devices, so re-write it >>>>>> starting from EXISTING GOOD bindings. Not some downstream stuff. >>>>> >>>>> Krzysztof, please point to a good example when making this kind of >>>>> comment. >>>> >>>> Anything recently added. Git log tells which files were recently added. >>> >>> If the review comment is a copy&paste (given that you review lots of >>> bindings and constantly have to repeat the same things, that would make >>> sense), expanding it with that information for future reviews could help >>> patch authors. Thanks for considering it, it would be much appreciated. >> >> Sorry, but that's not the point. You do not take 10 yo, unmaintained >> driver and use it as template for your new one. Instead you rather take >> something recent or something which you know is correct. Same with bindings. > > I wouldn't know for sure which driver or binding was used as a starting > point. My point was unrelated to this particular patch series. I think > that including clear information in ready-made answers will help > everybody. It will tell the submitters what they need to know, it will > avoid this kind of conversation being repeated, and it could even in the > end increase the quality of submissions. Even better, it won't cost > anything to add it to answer templates. > >> NXP is not a small company which does not know how to use Linux or how >> to upstream stuff. This is not individual's contribution, where one does >> not have colleagues or 3 billions USD of revenue behind, to be able to >> get some internal help prior sending something downstream. >> >> They can spend something out of these 3 billions of revenue or 700 >> millions of net income to hire you guys or any other open-source >> company, if basics of upstreaming are unknown. >> >> That's the comment I was giving about NXP since a year. Some things >> around SoC improved, some things from this unit of NXP here did not >> change at all. > > If I were on the receiving end of this, as an individual developer, I > would consider it very patronizing and insulting. Treating the authors > of contributions you don't consider as good enough in such a harsh way > will not improve the situation, and will drive people away. You may be > frustrated by some companies, but this kind of comment will not help. > Please soften your tone towards individual developers, they're not > punching balls on which to dump frustration and anger. Firm and polite > will work better than lashing out. I would be very happy to tell it to the managers, decision makers and CEOs, but they avoid me. :/ Best regards, Krzysztof
Hi Krzysztof, thanks for feedback. On 29.10.2024 08:14, Krzysztof Kozlowski wrote: > On 28/10/2024 20:06, Mirela Rabulea wrote: >> Add bindings for OX05B1S sensor driver >> >> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> > <form letter> > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. > > Tools like b4 or scripts/get_maintainer.pl provide you proper list of > people, so fix your workflow. Tools might also fail if you work on some > ancient tree (don't, instead use mainline) or work on fork of kernel > (don't, instead use mainline). Just use b4 and everything should be > fine, although remember about `b4 prep --auto-to-cc` if you added new > patches to the patchset. > > You missed at least devicetree list (maybe more), so this won't be > tested by automated tooling. Performing review on untested code might be > a waste of time. My bad, I ran get_maintainer.pl on the driver, but forgot to do so for the bindings. I will resend to extended audience, once I will also address the feedback from all reviewers. > > Please kindly resend and include all necessary To/Cc entries. > </form letter> > > Binding also looks very different than all other devices, so re-write it > starting from EXISTING GOOD bindings. Not some downstream stuff. Would this be a good example? Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml > > A nit, subject: drop second/last, redundant "bindings". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > A nit, subject: drop second/last "driver". Bindings are for hardware, > not drivers. I will rephrase it to: dt-bindings: media: i2c: Add OX05B1S sensor Add bindings for OX05B1S sensor. Thanks, Mirela > > Best regards, > Krzysztof >
On 29/10/2024 14:36, Mirela Rabulea wrote: >> >> Binding also looks very different than all other devices, so re-write it >> starting from EXISTING GOOD bindings. Not some downstream stuff. > Would this be a good example? > Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml Yes, looks good. >> >> A nit, subject: drop second/last, redundant "bindings". The >> "dt-bindings" prefix is already stating that these are bindings. >> See also: >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 >> >> A nit, subject: drop second/last "driver". Bindings are for hardware, >> not drivers. > Please correct the example so it matches coding style. Just compare how your DTS and imx DTS look like. Best regards, Krzysztof
Hi Laurent, thanks for feedback. On 29.10.2024 13:57, Laurent Pinchart wrote: >>> + >>> + orientation: true >>> + rotation: true >> I think you can drop both of these too. > Aren't they needed given that the binding ends with > > additionalProperties: false > > ? I added orientation & rotation properties in order to support orientation and rotation controls, libcamera warns about those (optional requirements last time I checked). >>> + >>> +required: >>> + - compatible >>> + - reg > The device requires a clock, shouldn't the clocks property be required ? I intentionally left the clock optional, because NXP has a converter board which supports both ox05b1s and os08a20 sensor, and the converter board has an oscillator, and we are using that, not the SOC clock. Here is how the module looks like for os08a20 for imx8mp: https://docs.nxp.com/bundle/AN13712/page/topics/os08a20_sensor_module.html There's a newer revision for the converter board for imx95, sorry but I do not have a link for that. For imx8mp, we used in the past the clock from the SOC, then switched to the external clock (from the converter board). I think Omnivision has their own module. So, I thought leaving the clock as optional allows for more flexibility. > >>> + - port >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/gpio/gpio.h> >>> + >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + ox05b1s: ox05b1s@36 { >>> + compatible = "ovti,ox05b1s"; >>> + reg = <0x36>; >>> + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; > This isn't specified in the bindings. Does the example validate ? Apparently yes, I mean dt_binding_check passed: $ rm Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb $ make dt_binding_check DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=ovti,ox05b1s.yaml DTC [C] Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb I have dtschema-2024.10.dev6+g12c3cd5. The "reset-gpios" is described in this binding, as the GPIO connected to the XSHUTDOWN pin. The <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW> is what works for imx95 ("nxp,pcal6408"), for imx8mp this works: reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>; Thanks, Mirela
Hi Bryan, thanks for feedback. On 29.10.2024 14:00, Bryan O'Donoghue wrote: > > On 29/10/2024 11:57, Laurent Pinchart wrote: >> Aren't they needed given that the binding ends with >> >> additionalProperties: false > > Yes. > > Might be nice to have > > unevaluatedProperties: false and just rely on the top level > > $ref: /schemas/media/video-interface-devices.yaml# > > Seems redundant to me to keep specifying these properties over and over > again. > > --- > bod I'm not sure I understand all the implications of unevaluatedProperties: false. Does this mean that I will have to add false for properties from video-interface-devices.yaml which should, maybe, not be allowed (lens-focus, flash-leds)? Thanks, Mirela
Hello Mirela, On Wed, Oct 30, 2024 at 08:02:44AM +0200, Mirela Rabulea wrote: > On 29.10.2024 13:57, Laurent Pinchart wrote: > >>> + > >>> + orientation: true > >>> + rotation: true > >> > >> I think you can drop both of these too. > > > > Aren't they needed given that the binding ends with > > > > additionalProperties: false > > > > ? > > I added orientation & rotation properties in order to support > orientation and rotation controls, libcamera warns about those (optional > requirements last time I checked). The orientation and rotation properties should certainly be specified in DT sources. They are standardized in video-interface-devices.yaml which Bryan pointed out you should reference. If you're not familiar yet with with how the YAML schemas used for DT bindings reference core schemas, now would be a good time to have a look at it :-) In a nutshell, you'll find that all properties need to be properly defined with appropriate constraints, and properties shared by multiple devices have constraints defined in core schemas. Some are included automatically and are applied based on property names, other need a manual $ref. You can have a look at https://github.com/devicetree-org/dt-schema.git to see core schemas that get automatically selected, they specify "select: true". For example the schemas defining the reg or clocks properties don't have to be manually referenced. Bryan's comment about dropping the orientation and rotation properties was related to the fact that the video-interface-devices.yaml schema defines them already. With "unevaluatedProperties: false", you won't need to specify "orientation: true". With "additionalProperties: false", you will. It's a good idea to learn about the difference between those two and how they really work. > >>> + > >>> +required: > >>> + - compatible > >>> + - reg > > > > The device requires a clock, shouldn't the clocks property be required ? > > I intentionally left the clock optional, because NXP has a converter > board which supports both ox05b1s and os08a20 sensor, and the converter > board has an oscillator, and we are using that, not the SOC clock. That's fine, you can have a fixed clock node in DT to model that. DT bindings describe the intrinsic needs of a particular device. If the sensor requires a clock, I think it should be mandatory. If the sensor itself could operate without an external clock (i.e. if it had an internal oscillator) then the property could be optional. > Here is how the module looks like for os08a20 for imx8mp: > > https://docs.nxp.com/bundle/AN13712/page/topics/os08a20_sensor_module.html > > There's a newer revision for the converter board for imx95, sorry but I > do not have a link for that. > > For imx8mp, we used in the past the clock from the SOC, then switched to > the external clock (from the converter board). > > I think Omnivision has their own module. > > So, I thought leaving the clock as optional allows for more flexibility. > > >>> + - port > >>> + > >>> +additionalProperties: false > >>> + > >>> +examples: > >>> + - | > >>> + #include <dt-bindings/gpio/gpio.h> > >>> + > >>> + i2c { > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + ox05b1s: ox05b1s@36 { > >>> + compatible = "ovti,ox05b1s"; > >>> + reg = <0x36>; > >>> + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; > > > > This isn't specified in the bindings. Does the example validate ? > > Apparently yes, I mean dt_binding_check passed: > > $ rm Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb > > $ make dt_binding_check DT_CHECKER_FLAGS=-m > DT_SCHEMA_FILES=ovti,ox05b1s.yaml > DTC [C] > Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb > > I have dtschema-2024.10.dev6+g12c3cd5. > > > The "reset-gpios" is described in this binding, as the GPIO connected to > the XSHUTDOWN pin. Ah sorry, Bryan dropped that part from his reply, so I didn't notice it. > The <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW> is what works for imx95 > ("nxp,pcal6408"), for imx8mp this works: > > reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
Hi Laurent, On 04.11.2024 16:25, Laurent Pinchart wrote: > Hello Mirela, > > On Wed, Oct 30, 2024 at 08:02:44AM +0200, Mirela Rabulea wrote: >> On 29.10.2024 13:57, Laurent Pinchart wrote: >>>>> + >>>>> + orientation: true >>>>> + rotation: true >>>> I think you can drop both of these too. >>> Aren't they needed given that the binding ends with >>> >>> additionalProperties: false >>> >>> ? >> I added orientation & rotation properties in order to support >> orientation and rotation controls, libcamera warns about those (optional >> requirements last time I checked). > The orientation and rotation properties should certainly be specified in > DT sources. They are standardized in video-interface-devices.yaml which > Bryan pointed out you should reference. If you're not familiar yet with > with how the YAML schemas used for DT bindings reference core schemas, > now would be a good time to have a look at it :-) > > In a nutshell, you'll find that all properties need to be properly > defined with appropriate constraints, and properties shared by multiple > devices have constraints defined in core schemas. Some are included > automatically and are applied based on property names, other need a > manual $ref. You can have a look at > https://github.com/devicetree-org/dt-schema.git to see core schemas that > get automatically selected, they specify "select: true". For example the > schemas defining the reg or clocks properties don't have to be manually > referenced. > > Bryan's comment about dropping the orientation and rotation properties > was related to the fact that the video-interface-devices.yaml schema > defines them already. With "unevaluatedProperties: false", you won't > need to specify "orientation: true". With "additionalProperties: false", > you will. It's a good idea to learn about the difference between those > two and how they really work. Thanks for info :) I just sent the v2, I added video-interface-devices.yaml. > >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>> The device requires a clock, shouldn't the clocks property be required ? >> I intentionally left the clock optional, because NXP has a converter >> board which supports both ox05b1s and os08a20 sensor, and the converter >> board has an oscillator, and we are using that, not the SOC clock. > That's fine, you can have a fixed clock node in DT to model that. DT > bindings describe the intrinsic needs of a particular device. If the > sensor requires a clock, I think it should be mandatory. If the sensor > itself could operate without an external clock (i.e. if it had an > internal oscillator) then the property could be optional. Also, in v2, I added the clock as mandatory. Strictly from sensor's module point of view, it is mandatory (will use a fixed clock for nxp board). Also added regulators. Again, thank you for feedback. Regards, Mirela > >> Here is how the module looks like for os08a20 for imx8mp: >> >> https://docs.nxp.com/bundle/AN13712/page/topics/os08a20_sensor_module.html >> >> There's a newer revision for the converter board for imx95, sorry but I >> do not have a link for that. >> >> For imx8mp, we used in the past the clock from the SOC, then switched to >> the external clock (from the converter board). >> >> I think Omnivision has their own module. >> >> So, I thought leaving the clock as optional allows for more flexibility. >> >>>>> + - port >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/gpio/gpio.h> >>>>> + >>>>> + i2c { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + ox05b1s: ox05b1s@36 { >>>>> + compatible = "ovti,ox05b1s"; >>>>> + reg = <0x36>; >>>>> + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; >>> This isn't specified in the bindings. Does the example validate ? >> Apparently yes, I mean dt_binding_check passed: >> >> $ rm Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb >> >> $ make dt_binding_check DT_CHECKER_FLAGS=-m >> DT_SCHEMA_FILES=ovti,ox05b1s.yaml >> DTC [C] >> Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.example.dtb >> >> I have dtschema-2024.10.dev6+g12c3cd5. >> >> >> The "reset-gpios" is described in this binding, as the GPIO connected to >> the XSHUTDOWN pin. > Ah sorry, Bryan dropped that part from his reply, so I didn't notice it. > >> The <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW> is what works for imx95 >> ("nxp,pcal6408"), for imx8mp this works: >> >> reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>; > -- > Regards, > > Laurent Pinchart
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml new file mode 100644 index 000000000000..d47e1950f24d --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml @@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (C) 2024, NXP +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ox05b1s.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OX05B1S Image Sensor + +maintainers: + - Mirela Rabulea <mirela.rabulea@nxp.com> + +description: |- + The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active array size + of 2592 x 1944. It is programmable through I2C interface. + The sensor output is available via CSI-2 serial data output. + +properties: + compatible: + items: + - enum: + - ovti,ox05b1s + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + description: Input clock (24 MHz) + items: + - const: csi_mclk + + assigned-clocks: + maxItems: 1 + + assigned-clock-parents: + maxItems: 1 + + assigned-clock-rates: + maxItems: 1 + + reset-gpios: + description: Reference to the GPIO connected to the XSHUTDOWN pin. Active low. + maxItems: 1 + + orientation: true + rotation: true + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + description: MIPI CSI-2 transmitter port + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + anyOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + required: + - data-lanes + + required: + - endpoint + +required: + - compatible + - reg + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ox05b1s: ox05b1s@36 { + compatible = "ovti,ox05b1s"; + reg = <0x36>; + reset-gpios = <&i2c3_gpio_expander_20 2 GPIO_ACTIVE_LOW>; + orientation = <2>; + rotation = <0>; + status = "okay"; + + port { + ox05b1s_mipi_0_ep: endpoint { + remote-endpoint = <&mipi_csi0_ep>; + data-lanes = <1 2 3 4>; + }; + }; + }; + }; +...
Add bindings for OX05B1S sensor driver Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com> --- .../bindings/media/i2c/ovti,ox05b1s.yaml | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml