From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-20.4 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A7FFC433DB for ; Tue, 19 Jan 2021 00:27:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E04F7230FF for ; Tue, 19 Jan 2021 00:27:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731813AbhASA1K (ORCPT ); Mon, 18 Jan 2021 19:27:10 -0500 Received: from m-r1.th.seeweb.it ([5.144.164.170]:50459 "EHLO m-r1.th.seeweb.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726514AbhASA06 (ORCPT ); Mon, 18 Jan 2021 19:26:58 -0500 Received: from IcarusMOD.eternityproject.eu (unknown [2.237.20.237]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by m-r1.th.seeweb.it (Postfix) with ESMTPSA id 42F361F914; Tue, 19 Jan 2021 01:26:15 +0100 (CET) Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: i2c: Add IMX300 CMOS sensor binding To: Sakari Ailus Cc: mchehab@kernel.org, robh+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org, konrad.dybcio@somainline.org, marijn.suijten@somainline.org, martin.botka@somainline.org References: <20210113182934.444727-1-angelogioacchino.delregno@somainline.org> <20210113182934.444727-3-angelogioacchino.delregno@somainline.org> <20210116234404.GX850@valkosipuli.retiisi.org.uk> <20210118223636.GA3@valkosipuli.retiisi.org.uk> From: AngeloGioacchino Del Regno Message-ID: Date: Tue, 19 Jan 2021 01:26:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <20210118223636.GA3@valkosipuli.retiisi.org.uk> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Il 18/01/21 23:40, Sakari Ailus ha scritto: > On Sun, Jan 17, 2021 at 06:51:04PM +0100, AngeloGioacchino Del Regno wrote: >> Il 17/01/21 00:44, Sakari Ailus ha scritto: >>> Hi AngeloGioacchino, >>> >>> On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote: >>>> Add YAML device tree binding for IMX300 CMOS image sensor, and >>>> the relevant MAINTAINERS entries. >>>> >>>> Signed-off-by: AngeloGioacchino Del Regno >>>> --- >>>> .../bindings/media/i2c/sony,imx300.yaml | 112 ++++++++++++++++++ >>>> MAINTAINERS | 7 ++ >>>> 2 files changed, 119 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml >>>> new file mode 100644 >>>> index 000000000000..4fa767feea80 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml >>>> @@ -0,0 +1,112 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor >>>> + >>>> +maintainers: >>>> + - AngeloGioacchino Del Regno >>>> + >>>> +description: |- >>>> + The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image >>>> + sensor with a pixel size of 1.08um and an active array size of >>>> + 5948H x 4140V. It is programmable through I2C interface at address 0x10. >>>> + Image data is sent through MIPI CSI-2, which is configured as either 2 or >>>> + 4 data lanes. >>>> + >>>> +properties: >>>> + compatible: >>>> + const: sony,imx300 >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + maxItems: 1 >>> >>> Please add assigned clock related properties; see >>> Documentation/driver-api/media/camera-sensor.rst . >>> >> Will do! >> >>>> + >>>> + vdig-supply: >>>> + description: >>>> + Digital I/O voltage supply, 1.15-1.20 volts >>>> + >>>> + vana-supply: >>>> + description: >>>> + Analog voltage supply, 2.2 volts >>>> + >>>> + vddl-supply: >>>> + description: >>>> + Digital core voltage supply, 1.8 volts >>>> + >>>> + reset-gpios: >>>> + description: |- >>>> + Reference to the GPIO connected to the xclr pin, if any. >>>> + Must be released (set high) after all supplies are applied. >>>> + >>>> + # See ../video-interfaces.txt for more details >>>> + port: >>>> + type: object >>>> + properties: >>>> + endpoint: >>>> + type: object >>>> + >>>> + properties: >>>> + data-lanes: >>>> + description: |- >>>> + The driver only supports four-lane operation. >>> >>> This can be removed as bindings describe hardware, not driver operation. >>> >> Ack. >> >>>> + items: >>>> + - const: 0 >>>> + - const: 1 >>>> + - const: 2 >>>> + - const: 3 >>> >>> Two lanes here, too? >>> >> >> The driver only supports four-lane operation. >> I am 100% sure that this sensor can also work with two lanes, but it needs >> special configuration which I'm not able to produce, nor test. >> >> As you may imagine (and as you can read in the driver itself), all of this >> was reverse-engineering work, as Sony has never released any datasheet for >> this sensor - and I have a hunch - they never will (but that's another >> story). > > That's all fine. The bindings describe the hardware, not the driver's > capabilities. > Ok, will add the dual-lane configuration! >> >>>> + >>>> + clock-noncontinuous: true >>>> + >>>> + link-frequencies: >>>> + $ref: /schemas/types.yaml#/definitions/uint64-array >>>> + description: >>>> + Allowed data bus frequencies. The driver currently needs >>>> + to switch between 780000000 and 480000000 Hz in order to >>>> + guarantee functionality of all modes. >>> >>> You can omit this description, too. >>> >> >> The intention here was to be clear and provide as much information as I >> could gather during the very time-consuming reverse engineering process that >> took place in the making of this driver. >> >> But okay, I will remove this. > > Again, this is about the hardware, not the driver. That information is also > part of the driver. > Sure! Removing for the next version! >> >>>> + >>>> + required: >>>> + - data-lanes >>>> + - link-frequencies >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - clocks >>>> + - vana-supply >>>> + - vdig-supply >>>> + - vddl-supply >>> >>> Are the regulators really required? I'm not quite sure about the >>> established practices; still the common case is that one or two of these >>> are hard-wired. >>> >> >> On all the Sony phones that I have (....many), with MSM8956, MSM8996, >> SDM630, equipped with the IMX300 camera assy, none of these three are >> hard-wired: sometimes they're wired to the LDOs of the PMIC, sometimes >> they're wired to fixed LDOs, enabled through GPIOs (fixed-regulator binding >> in this case). >> >> So.. yeah, they're really required. > > As noted, that depends on the board. You just happen to have some where > they are not hard-wired. > Sure, but then the supplies are required properties, since we are describing the hardware: it can't work without power. Besides that, when a board supplies power through fixed always-on rails, DT users will specify a fixed-regulator binding with the right voltages: this is a good habit for describing the board in DT. >> >>>> + - port >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + i2c0 { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + imx300: sensor@10 { >>>> + compatible = "sony,imx300"; >>>> + reg = <0x10>; >>>> + clocks = <&imx300_xclk>; >>>> + vana-supply = <&imx300_vana>; /* 2.2v */ >>>> + vdig-supply = <&imx300_vdig>; /* 1.2v */ >>>> + vddl-supply = <&imx300_vddl>; /* 1.8v */ >>>> + >>>> + port { >>>> + imx300_0: endpoint { >>>> + remote-endpoint = <&csi1_ep>; >>>> + data-lanes = <0 1 2 3>; >>>> + clock-noncontinuous; >>>> + link-frequencies = /bits/ 64 <780000000 480000000>; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + >>>> +... >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index ad9abb42f852..5e0f08f48d48 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -16633,6 +16633,13 @@ T: git git://linuxtv.org/media_tree.git >>>> F: Documentation/devicetree/bindings/media/i2c/imx290.txt >>>> F: drivers/media/i2c/imx290.c >>>> +SONY IMX300 SENSOR DRIVER >>>> +M: AngeloGioacchino Del Regno >>>> +L: linux-media@vger.kernel.org >>> >>> Please also add the git tree. >>> >>> Ideally also the MAINTAINERS change comes with the first patch adding the >>> files, which should be the DT bindings. I.e. just reverse the order of the >>> patches. >>> >> >> I haven't added it because last time I did that I got reviews saying that if >> I'm not the owner of the git tree I shall not put it in. >> Though, if that's a requirement for media, then I didn't know that... > > The documentation in MAINTAINERS doesn't say that at least. > > I think it'd be useful to have it. > >> >>>> +S: Maintained >>>> +F: Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml >>>> +F: drivers/media/i2c/imx300.c >>>> + >>>> SONY IMX319 SENSOR DRIVER >>>> M: Bingbu Cao >>>> L: linux-media@vger.kernel.org >>> >> >> Thank you for your review! > > You're welcome! >