| 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: <20250706194528.GA1821@pendragon.ideasonboard.com>
Date: Sun, 6 Jul 2025 22:45:28 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Will Whang <will@...lwhang.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"open list:SONY IMX585 SENSOR DRIVER" <linux-media@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v1 1/4] dt-bindings: media: Add Sony IMX585 CMOS image
sensor
Hi will,
On Sat, Jul 05, 2025 at 11:55:04PM -0700, Will Whang wrote:
> Hi Laurent,
>
> Thank you for the feedback, very much appreciated!
> Reply inline.
> (Resend this email for reply all)
Please reply in plain-text only, as HTML e-mails get filtered out by
kernel mailing lists.
> On Wed, Jul 2, 2025 at 2:37 AM Laurent Pinchart wrote:
> > On Wed, Jul 02, 2025 at 07:38:33AM +0100, Will Whang wrote:
> > > Document the devicetree binding for the Sony IMX585. The schema
> > > covers the CSI-2 data-lanes, the optional 'mono-mode' flag,
> > > and the internal-sync properties used by the driver.
> > >
> > > Signed-off-by: Will Whang <will@...lwhang.com>
> > > ---
> > > .../bindings/media/i2c/sony,imx585.yaml | 120 ++++++++++++++++++
> > > MAINTAINERS | 8 ++
> > > 2 files changed, 128 insertions(+)
> > > create mode 100644
> > Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > b/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > > new file mode 100644
> > > index 000000000..d050d1642
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > > @@ -0,0 +1,120 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright (C) 2024 Ideas on Board Oy
> >
> > Unless there's something I'm not aware of, I don't think Ideas on Board
> > wrote this. You can use your own copyright.
>
> Yeah sorry about this, I've updated this one.
>
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/sony,imx585.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sony IMX585 Sensor
> > > +
> > > +maintainers:
> > > + - Will Whang <will@...lwhang.com>
> > > +
> > > +description:
> > > + IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs.
> > > +
> >
> > You should add
> >
> > allOf:
> > - $ref: /schemas/media/video-interface-devices.yaml#
> >
> > here to support generic sensor properties. You will need to replace
> >
> > additionalProperties: false
> >
> > with
> >
> > unevaluatedProperties: false
> >
> > below.
>
> Updated.
You don't need to reply to individual review comments if you agree with
them and update the code. By only replying to the points that require
discussions, the conversation is easier to read.
> > > +properties:
> > > + compatible:
> > > + const: sony,imx585
> > > +
> > > + clocks:
> > > + maxItems: 1
> > > +
> > > + clock-names:
> > > + const: xclk
> >
> > When there's a single clock you can drop clock-names.
> >
> Updated.
>
> > > +
> > > + clock-frequency:
> > > + enum: [ 74250000, 37125000, 72000000, 27000000, 24000000 ]
> >
> > The clock-frequency property is frowned upon for sensors in DT. If the
> > aim is to set the frequency of the clock, it should be done through
> > assigned-clocks and assigned-clock-rates. If the aim is to convey the
> > clock frequency to the driver, it should be done by calling
> > clk_get_rate() in the driver.
>
> The aim is to set the frequency for the driver to handle different clock frequencies,
> currently the driver is using clk_get_rate() but because it only supports
> these frequencies I thought I need to list the valid options here.
There's no need to. DT writers are expected to know about the hardware,
you don't need to document here what frequencies the sensor supports. DT
properties are meant to convey information to drivers, and drivers don't
need the clock-frequency property as they can get the clock rate (set
from assigned-clock-rates) from the system.
> > > +
> > > + reg:
> > > + maxItems: 1
> > > + description: I2C Address for IMX585
> >
> > You can drop the description, it's always the same for I2C devices.
>
> Updated.
>
> > > +
> > > + 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
> > > +
> > > + port:
> > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > + additionalProperties: false
> > > +
> > > + properties:
> > > + endpoint:
> > > + $ref: /schemas/media/video-interfaces.yaml#
> > > + unevaluatedProperties: false
> > > +
> > > + properties:
> > > + data-lanes:
> > > + anyOf:
> > > + - items:
> > > + - const: 1
> > > + - const: 2
> > > + - const: 3
> > > + - const: 4
> >
> > Does that mean that the sensor supports data lane remapping ? I don't
> > see it implemented by the driver. If it's not supported by the hardware,
> > you should use
> >
> > properties:
> > data-lanes:
> > minItems: 1
> > items:
> > - const: 1
> > - const: 2
> > - const: 3
> > - const: 4
> >
> > To guarantee the order.
>
> The driver can only support either 2-lane or 4-lane mode. Updated.
>
> > > +
> > > + 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 ]
> >
> > This seems to be a sensor-level property, not an endpoint property. As
> > it's not standard, it should also have a vendor prefix, i.e.
> > sony,sync-mode. I'm wondering, though, if we shouldn't try to
> > standardize it in video-interface-devices.yaml.
>
> Updated, along with the feedback from Krzysztof, I've make the following
> modifications:
> sony,sync-mode:
> description:
> Select the global synchronisation mode of the sensor.
> $ref: /schemas/types.yaml#/definitions/string
> enum:
> - internal-leader
> - internal-follower
> - external
> default: internal-leader
I'll comment on that in v2, when reading how the driver handles external
triggers and synchronization.
> > +
> > > + link-frequencies:
> > > + description: Select the MIPI-CSI2 link speed in Mhz
> >
> > You can drop the description, it's already described in
> > video-interfaces.yaml.
>
> Updated.
>
> > > + items:
> > > + enum: [ 297000000, 360000000, 445500000, 594000000,
> > > + 720000000, 891000000, 1039500000 ]
> >
> > Are those frequencies the only ones the hardware can support, or do they
> > come from the driver only supporting a fixed set of sensor PLL
> > configurations ? In the latter case I would drop the enumeration.
>
> Yes, these are the ones that it supports.
>
> > > +
> > > + required:
> > > + - data-lanes
> > > + - link-frequencies
> > > +
> > > + required:
> > > + - endpoint
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - clocks
> > > + - clock-frequency
> > > + - port
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + imx585@1a {
> > > + compatible = "sony,imx585";
> > > + reg = <0x1a>;
> > > + clocks = <&imx585_clk>;
> > > + clock-frequency = <24000000>;
> > > +
> > > + VANA-supply = <&camera_vadd_3v3>;
> > > + VDDL-supply = <&camera_vdd1_1v8>;
> > > + VDIG-supply = <&camera_vdd2_1v1>;
> > > +
> > > + port {
> > > + imx585: endpoint {
> > > + remote-endpoint = <&cam>;
> > > + data-lanes = <1 2 3 4>;
> > > + link-frequencies = /bits/ 64 <720000000>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +...
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index da34c7227..9cc404790 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -23150,6 +23150,14 @@ T: git git://linuxtv.org/media.git
> > > F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> > > F: drivers/media/i2c/imx415.c
> > >
> > > +SONY IMX585 SENSOR DRIVER
> > > +M: Will Whang <will@...lwhang.com>
> > > +L: linux-media@...r.kernel.org
> > > +S: Maintained
> > > +T: git git://linuxtv.org/media.git
> > > +F: Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
> > > +F: drivers/media/i2c/imx585.c
> > > +
> > > SONY MEMORYSTICK SUBSYSTEM
> > > M: Maxim Levitsky <maximlevitsky@...il.com>
> > > M: Alex Dubov <oakad@...oo.com>
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists