Re: [PATCH v2 1/2] media: dt-bindings: i2c: add Himax HM1246 image sensor

[Matthias Fend]

Hi Sakari,

Thanks for you feedback!

Am 05.09.2025 um 11:59 schrieb Sakari Ailus:
> Hi Matthias,
>
> Thanks for the set.
>
> On Mon, May 26, 2025 at 08:59:27AM +0200, Matthias Fend wrote:
> > Add YAML device tree binding for Himax HM1246 image sensor.
> >
> > Reviewed-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> > Signed-off-by: Matthias Fend <matthias.fend@xxxxxxxxx>
> > ---
> >   .../bindings/media/i2c/himax,hm1246.yaml           | 111 +++++++++++++++++++++
> >   1 file changed, 111 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml b/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..8a67de7e3ffcaa9f1acfe443b1e36fffb79dbacf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/himax,hm1246.yaml
> > @@ -0,0 +1,111 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2025 Matthias Fend <matthias.fend@xxxxxxxxx>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/himax,hm1246.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Himax HM1246-AWD 1/3.7-Inch megapixel SoC image sensor
> > +
> > +maintainers:
> > +  - Matthias Fend <matthias.fend@xxxxxxxxx>
> > +
> > +description:
> > +  The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active
> > +  array size of 1296 x 976. It is programmable through an I2C interface and
> > +  connected via parallel bus.
> > +
> > +allOf:
> > +  - $ref: /schemas/media/video-interface-devices.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: himax,hm1246
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: Input reference clock (6 - 27 MHz)
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: Active low XSHUTDOWN pin
> > +    maxItems: 1
> > +
> > +  avdd-supply:
> > +    description: Power for analog circuit (3.0 - 3.6 V)
> > +
> > +  iovdd-supply:
> > +    description: Power for I/O circuit (1.7 - 3.6 V)
> > +
> > +  dvdd-supply:
> > +    description: Power for digital circuit (1.5 / 1.8 V)
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +    description: Parallel video output port
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          bus-type:
> > +            const: 5
>
> Does the device also support e.g. Bt.656? If not, you can drop this.

It is always a parallel interface. Okay, will remove it.
> If the rest of the parallel interface parameter properties aren't
> mandatory, what are their default values?

The defaults are:
hsync-active: 1 (high)
vsync-active: 1 (high)
pclk-sample: 0 (falling)

Should I add the default values? The properties section of the endpoint 
would then look like this:

properties:
  hsync-active:
    default: 1

  vsync-active:
    default: 1

  pclk-sample:
    default: 0

> > +
> > +        required:
> > +          - bus-type

Then I should probably remove that too?

> > +
> > +    required:
> > +      - endpoint
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - avdd-supply
> > +  - iovdd-supply
> > +  - dvdd-supply
> > +  - port
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/media/video-interfaces.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        sensor@24 {
> > +            compatible =  "himax,hm1246";
> > +            reg = <0x24>;
> > +
> > +            clocks = <&hm1246_clk>;
> > +
> > +            reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
> > +
> > +            avdd-supply = <&hm1246_avdd>;
> > +            iovdd-supply = <&hm1246_iovdd>;
> > +            dvdd-supply = <&hm1246_dvdd>;
> > +
> > +            orientation = <2>;
>
> It'd be nice to add macros for these in
> include/dt-bindings/media/video-interfaces.h .

True. However, since this is not specific to this sensor, I think it's 
something for a separate series.
Should I omit these properties in this example?

Thanks
 ~Matthias

> > +            rotation = <0>;
> > +
> > +            port {
> > +                endpoint {
> > +                    remote-endpoint = <&isp_par_in>;
> > +                    bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
> > +                    bus-width = <10>;
> > +                    hsync-active = <1>; /* active high */
> > +                    vsync-active = <1>; /* active high */
> > +                    pclk-sample = <1>; /* sample on rising edge */
> > +                };
> > +            };
> > +        };
> > +    };