linux-kernel - Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200921112635.GA1233@kozik-lap>
Date:   Mon, 21 Sep 2020 13:26:35 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for
 IMX258 sensor

On Mon, Sep 14, 2020 at 02:13:10PM -0600, Rob Herring wrote:
> On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote:
> > Add bindings for the IMX258 camera sensor.  The bindings, just like the
> > driver, are quite limited, e.g. do not support regulator supplies.
> 
> Bindings should be complete, not what a driver happens to currently 
> support.

I'll add then more complete picture.

> 
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@...nel.org>
> > 
> > ---
> > 
> > Changes since v1:
> > 1. None
> > ---
> >  .../devicetree/bindings/media/i2c/imx258.yaml      | 92 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > new file mode 100644
> > index 000000000000..ef789ad31143
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > @@ -0,0 +1,92 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
> > +
> > +maintainers:
> > +  - Krzysztof Kozlowski <krzk@...nel.org>
> > +
> > +description: |-
> > +  IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active pixel
> > +  type stacked image sensor with a square pixel array of size 4208 x 3120. It
> > +  is programmable through I2C interface.  Image data is sent through MIPI
> > +  CSI-2.
> > +
> > +properties:
> > +  compatible:
> > +    const: sony,imx258
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: Frequency of input clock if clock is not provided
> > +    deprecated: true
> 
> Why are we adding something deprecated on a new binding?

My intention was also to document it but indeed easier to skip it.

> 
> > +    const: 19200000
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        properties:
> > +          data-lanes:
> > +            items:
> > +              - const: 1
> > +              - const: 2
> > +              - const: 3
> > +              - const: 4
> 
> If this is the only config, why does it need to be in DT?

The sensor is capable of two settings: two lanes (1 and 2) and four
lanes described above.  However Linux driver requires the latter (four
lanes, 1+2+3+4).

If I were to describe the bindings for HW, someone would really be
confused and try to use two lanes setup, which won't work. Driver won't
allow it.

I understand that bindings document the HW and describe its interface
but do we really want to put "theoretical" bindings which cannot be used
in practice with Linux kernel?

If yes, how to nicely document this that only one setting is currently
working?

> 
> > +
> > +          link-frequencies:
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/uint64-array
> > +            description:
> > +              Allowed data bus frequencies.
> > +
> > +        required:
> > +          - data-lanes
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - port
> > +
> > +
> > +if:
> > +  not:
> > +    required:
> > +      - clocks
> > +then:
> > +  required:
> > +    - clock-frequency
> > +
> > +unevaluatedProperties: false
> 
> additionalProperties

Right.

Best regards,
Krzysztof