| 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
| ||
|
Message-ID: <aFrxSOapkQ/QIXT8@lizhi-Precision-Tower-5810>
Date: Tue, 24 Jun 2025 14:41:12 -0400
From: Frank Li <Frank.li@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.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
On Tue, Jun 24, 2025 at 01:41:09AM +0300, Laurent Pinchart wrote:
> Hi Frank,
>
> Thank you for the patch.
>
> 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?
>
> > +
> > + 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.
>
> > +
> > +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.
>
> > +
> > +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?
at least ports is needed.
Frank
>
> > + };
> > + };
> > +
> > 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