Re: [PATCH v2 1/4] dt-bindings: media: Add Sony IMX585 CMOS image sensor

[Will Whang <will@willwhang.com>]

Hi Krzysztof,
Reply inline.
Thanks,
Will Whang

On Mon, Aug 11, 2025 at 1:01 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sun, Aug 10, 2025 at 11:09:18PM +0100, Will Whang wrote:
> > +description:
> > +  IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sony,imx585
> > +      - sony,imx585-mono
>
> I don't understand this second compatible. Is this different hardware?
> Can you point me to "mono" datasheet?
>
> Your description should explain this. Commit msg as well, instead of
> speaking about driver (in fact drop all driver related comments).
>
Mono version of this sensor is basically just removing the bayer
filter, so the sensor itself actually doesn't know if it is color or
mono and from my knowledge there are no registers programmed in the
factory that will show the variant and model number. (That is why when
the driver probing it only test blacklevel register because there are
no ID registers)
Originally in V1 patch I've made the switch between color and mono in
dtoverlay config but reviewer comments is to move it to compatible
string and not property.(https://lore.kernel.org/linux-media/20250703175121.GA17709@pendragon.ideasonboard.com/)

In this case, what would you recommend?

compatible:
  enum:
    - sony,imx585
    - sony,imx585-mono
  description: IMX585 has two variants, color and mono which the
driver supports both.

Something like this?


> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
>
> Drop all three.
>
> > +
> > +  clocks:
> > +    description: Clock frequency 74.25MHz, 37.125MHz, 72MHz, 27MHz, 24MHz
> > +    maxItems: 1
> > +
> > +  vana-supply:
> > +    description: Analog power supply (3.3V)
> > +
> > +  vddl-supply:
> > +    description: Interface power supply (1.8V)
> > +
> > +  vdig-supply:
> > +    description: Digital power supply (1.1V)
> > +
> > +  reset-gpios:
> > +    description: Sensor reset (XCLR) GPIO
> > +    maxItems: 1
> > +
> > +  sony,sync-mode:
> > +    description: |
> > +      Select the synchronisation mode of the sensor
> > +        0 – internal sync, leader (default)
> > +        1 – internal sync, follower
> > +        2 – external sync
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    enum: [ 0, 1, 2 ]
>
> Previous comments not applied.
>
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          data-lanes:
> > +            oneOf:
> > +              - items:
> > +                  - const: 1
> > +                  - const: 2
> > +              - items:
> > +                  - const: 1
> > +                  - const: 2
> > +                  - const: 3
> > +                  - const: 4
> > +
> > +          link-frequencies: true
>
> Drop
Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml and
sony,283.yaml doesn't seem to drop this?
Just to double check if my understanding is correct, you want me to
remove the line link-frequencies: true?

>
> > +
> > +        required:
> > +          - data-lanes
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - port
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        imx585@1a {
>
> Nothing improved.
>
> You replied that you applied comment, but send the same.
>
>
> Best regards,
> Krzysztof
>