[1/5] dt-bindings: media: i2c: Add bindings for OX05B1S sensor driver

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

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