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

[Rob Herring]

On Mon, Sep 21, 2020 at 5:27 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > >
> > > ---
> > >
> > > 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@xxxxxxxxxx>
> > > +
> > > +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.

If someone has h/w with only 2 lanes connected, then they have to go
add support to the driver whether we've documented 2 lanes in the
binding or not.

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

You don't, at least in the binding. That's a driver issue. Bindings
are separate. They are stored in the kernel tree for convenience, not
because they are part of the kernel.

Rob