Message-ID: <20250624185404.GD20757@pendragon.ideasonboard.com>
Date: Tue, 24 Jun 2025 21:54:04 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Frank Li <Frank.li@....com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Kumar M <anil.mamidala@...inx.com>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	imx@...ts.linux.dev, "Guoniu.zhou" <guoniu.zhou@....com>,
	Stefan Hladnik <stefan.hladnik@...il.com>,
	Florian Rebaudo <frebaudo@...ekio.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add bindings for AP1302
 and AR0144

Hi Frank,

On Tue, Jun 24, 2025 at 02:41:12PM -0400, Frank Li wrote:
> On Tue, Jun 24, 2025 at 01:41:09AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 23, 2025 at 03:17:37PM -0400, Frank Li wrote:
> > > From: Anil Kumar Mamidala <anil.mamidala@...inx.com>
> > >
> > > The AP1302 is a standalone ISP for ON Semiconductor sensors, which can
> > > connect RAW sensors (AR0144).
> > >
> > > Add corresponding DT bindings.
> > >
> > > Signed-off-by: Anil Kumar Mamidala <anil.mamidala@...inx.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > Signed-off-by: Stefan Hladnik <stefan.hladnik@...il.com>
> > > Signed-off-by: Florian Rebaudo <frebaudo@...ekio.com>
> > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > ---
> > > Previous try:
> > > https://lore.kernel.org/linux-media/1631091372-16191-2-git-send-email-anil.mamidala@xilinx.com/
> > >
> > > Change in v3:
> > > - Move sensors under ports
> > > - use compatible string to indentify connected raw sensors
> > > - Add onnn,ar0144.yaml
> > > ---
> > >  .../devicetree/bindings/media/i2c/onnn,ap1302.yaml | 151 +++++++++++++++++++++
> > >  .../devicetree/bindings/media/i2c/onnn,ar0144.yaml |  75 ++++++++++
> > >  MAINTAINERS                                        |   9 ++
> > >  3 files changed, 235 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ap1302.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ap1302.yaml
> > > new file mode 100644
> > > index 0000000000000..6b745dcf3fd3f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,ap1302.yaml
> > > @@ -0,0 +1,151 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/onnn,ap1302.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ON Semiconductor AP1302 Advanced Image Coprocessor
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > +  - Anil Kumar M <anil.mamidala@...inx.com>
> > > +
> > > +description:
> > > +  The AP1302 is a standalone ISP for ON Semiconductor sensors. It interfaces to
> > > +  up to two RAW CMOS sensors over MIPI CSI-2 connections, processes the two
> > > +  video streams and outputs YUV frames to the host over a MIPI CSI-2 interface.
> > > +  Frames are output side by side or on two virtual channels.
> > > +
> > > +  The sensors must be identical. They are connected to the AP1302 on dedicated
> > > +  I2C buses, and are controlled by the AP1302 firmware. They are not accessible
> > > +  from the host.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: onnn,ap1302
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description:
> > > +          Reference to the CLK clock.
> > > +
> > > +  reset-gpios:
> > > +    items:
> > > +      - description:
> > > +          Reference to the GPIO connected to the RST pin (active low).
> > > +
> > > +  standby-gpios:
> > > +    items:
> > > +      - description:
> > > +          Reference to the GPIO connected to the STANDBY pin (active high).
> > > +
> > > +  enable-gpios:
> > > +    items:
> > > +      - description:
> > > +          Reference to the GPIO connected to the EN pin (active high).
> > > +
> > > +  dvdd-supply: true
> > > +
> > > +  hmisc-supply: true
> > > +
> > > +  smisc-supply: true
> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    unevaluatedProperties: false
> > > +
> > > +    patternProperties:
> > > +      "^port@[01]":
> > > +        description:
> > > +          Sensors connected to the first and second input, if no sensor
> > > +          connect, isp generate test pattern. The compatible string under
> > > +          port@0 and port@1 have to be the same.
> > > +
> > > +        allOf:
> > > +          - $ref: /schemas/graph.yaml#/$defs/port-base
> > > +          - $ref: onnn,ar0144.yaml
> >
> > You can't do that, that's plain wrong, sorry. There are issue raised in
> > the review of v2, please try to understand the problem and propose a
> > solution there. This is not what we need.
> 
> After review previous thread, Rob suggest use compatible string instead
> vendor specific onnn,model property. I also think Rob's suggest is good
> because compatible already descript the raw sensor model, needn't involve
> new property for it and we can reuse other property like xxx-supply.
> 
> Your concern is that port0 and port1 have to be the same. Rob suggest check
> at the code to make sure both are the same.
> 
> I think Rob may have method to add restriction at binding doc to make dts
> have the same compatble string.
> 
> Anything what I misssed?

The discussion died out in that thread, I didn't have time to reply
right away, and then the mails got buried in my inbox. My bad.

The sensor node does not describe a device in the traditional DT sense,
as the AP1302 completely hides the sensor from the host. The DT node,
and its properties, need to be interpreted in the context of the AP1302
DT binding. Use a "compatible" property, beside duplicating a value and
introducing room for mistakes, is misleading, as "compatible" implies
that the node is meant to go through the standard matching procedure
with drivers. Sure, we could use a "compatible" property without letting
the operating system match it with drivers, but it would still be
misleading. I don't see any advantage in using "compatible".

Regardless, you should *not* reference the ar0144.yaml. A DT binding for
the AR0144 (see below for a link to proper DT binding for this sensor)
would need to describe the device from the point of view of the host.
Here you need to describe it from the point of view of the AP1302, which
is very different. Even if we end up using the compatible property to
identify the sensor model, that compatible property would *not* match a
onnn,ar0144.yaml. That's yet another reason for not using "compatible".

> > > +
> > > +        unevaluatedProperties: false
> > > +
> 
> ...
> 
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ON Semiconductor AP0144 RAW CMOS sensor
> >
> > AP0144 seems to be a typo.
> >
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > +  - Anil Kumar M <anil.mamidala@...inx.com>
> >
> > Listing people as maintainers when they had nothing to do with
> > development of a file isn't very polite.
> 
> It is continue previous thread and copy from ap1302, If you have concern
> I can put my name here.

It's a separate binding, not part of the previous version.

> > > +
> > > +description:
> > > +  AP0144 RAW CMOS can be use standalone with any SOCs, or work with AP1302
> > > +  ISP.
> >
> > How a sensor is used is not relevant for its DT bindings.
> 
> DT is that descript hardware. Many sensors have SPI and I2C interface, but
> they use the same binding.
> 
> DT just descript hardware feature, such as how many clocks, how many power
> supply. All of property does not related usage at all.
> 
> Some legacy binding related usage, but that is what try to avoid now.

Yes, so you shouldn't mention the AP1302 here.

> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - onnn,ar0144
> > > +      - onnn,ar0330
> > > +      - onnn,ar1335
> >
> > There's also no explanation for this.
> 
> It is just chip's compatible string, what do you want to add? Most compatible
> string have not descrption.
> 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vaa-supply: true
> > > +
> > > +  vdd-supply: true
> > > +
> > > +  vddio-supply: true
> > > +
> > > +  vddpll-supply: true
> > > +
> > > +  port:
> > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        $ref: /schemas/media/video-interfaces.yaml#
> > > +        unevaluatedProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          not:
> > > +            contains:
> > > +              const: onnn,ar0330
> > > +    then:
> > > +      properties:
> > > +        vddpll-supply: false
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        camera@10 {
> > > +            compatible = "onnn,ar0144";
> > > +            reg = <0x10>;
> > > +            vaa-supply = <&vaa>;
> > > +            vddio-supply = <&vddio>;
> > > +            vdd-supply = <&vdd>;
> >
> > No input clock, reset signal, ports ?
> 
> I am not famillar with this sensor, just extract from code. Do you know
> where download spec?

The datasheet is not public, but I have posted a driver, with DT
bindings, to the linux-media mailing list. See
https://lore.kernel.org/linux-media/20240905225308.11267-1-laurent.pinchart@ideasonboard.com

> at least ports is needed.
> 
> > > +        };
> > > +    };
> > > +
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index d6f1670290589..1362d351f2574 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -1766,6 +1766,15 @@ L:	linux-sound@...r.kernel.org
> > >  S:	Maintained
> > >  F:	sound/aoa/
> > >
> > > +AP1302 ON SEMICONDUCTOR ISP DRIVER
> > > +M:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > +R:	Frank Li <Frank.Li@....com>
> > > +L:	linux-media@...r.kernel.org
> > > +S:	Maintained
> > > +T:	git git://linuxtv.org/media.git
> > > +F:	Documentation/devicetree/bindings/media/i2c/onnn,ap1302.yaml
> > > +F:	Documentation/devicetree/bindings/media/i2c/onnn,ar0144.yaml
> > > +
> > >  APEX EMBEDDED SYSTEMS STX104 IIO DRIVER
> > >  M:	William Breathitt Gray <wbg@...nel.org>
> > >  L:	linux-iio@...r.kernel.org

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists