Hello, I’m doing my best to follow along here. Your feedback didn’t get lost and I tried my best to follow it— I just must not have understood it correctly. > Where is the driver? If it was sent separately, you must resent entire > patchset. ... > 1. There is no driver, no DTS. You received the feedback about it. Driver: I submitted the .c files to linux-media, and as part of the review they asked for me to separately submit device tree bindings (https://patchwork.kernel.org/project/linux-media/patch/20221106171129.166892-2-nicholas@xxxxxxxxxxxxx/). Are you saying that the driver and the bindings should be the same commit after all? Are you saying that I need to change this into a series and include both linux-media@ and devicetree@ mailing lists in a v3 with both patches? Are you saying something else? I’m afraid I still don’t understand what you mean by this, but I want to, and I’m trying to. > 2. Wrong cc list. You were asked to use get_maintainers.pl and still > decide not to. I included the people from get_maintainers.pl, but it seems like you would like for me to include all entries, including the multiple mailing lists. Do I understand correctly? > Subject: drop redundant, second "binding". Is this new subject good, or would you prefer just "dt-bindings: media: Add Omnivision ov8858"? Just want to make sure I follow up. > How can you test bindings with libcamera? I validated the device tree + driver on this setup, though I did not know about the binding linter (and this one should pass). I am happy to drop the comment about validation though and just say this is tested with the binding checker, or say nothing at all if you'd like. > Filename matching the compatible. ... > Filename still does not match compatible. ovti,ov8858.yaml I didn't understand this the first time, but now I think I do-- seems like you're asking me to change the binding filename to "ovti,ov8858.yaml". I was trying to be consistent with ov8856.yaml, but happy to change the file name if that’s the convention. Is there a doc I can read with this information or is it institutional knowledge? > The frequency of clock should go via common clock framework - you have > get_rate and set_rate. Drop entire property. I am trying to be consistent with the ov8856 driver and bindings but would be happy to change if there's a different standard I should be following. I’m not familiar with that framework though. Is there somewhere I could read about this common clock framework, including the driver and device-tree changes I need in order to use it as compared to this or the ov8856 driver? > Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence. Seems like your point here and in the subsequent comments is to avoid implementation specifics. That makes a ton of sense, and I’ll make those changes for v3. All of your other comments make sense explicitly and I’ll make these changes. Please help me understand what you mean by (1) and (2), correct me where I’m wrong or misunderstanding you here, clarify where needed, and I’ll submit the v3. Thanks, -Nicholas On Wed, Nov 9, 2022 at 2:26 AM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 09/11/2022 03:31, Nicholas Roth wrote: > > Add a device tree binding for the Omnivision OV8858 image sensor. > > The OV8858 is an 8 megapixel image sensor which provides images in RAW > > format over MIPI CSI-2 data bus and is controlled through an > > I2C-compatibile SCCB bus. > > > > Tested on PinePhone Pro with libcamera cam and qcam. > > How can you test bindings with libcamera? > > > > Signed-off-by: Nicholas Roth <nicholas@xxxxxxxxxxxxx> > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. > > Thank you. > > 1. There is no driver, no DTS. You received the feedback about it. > 2. Wrong cc list. You were asked to use get_maintainers.pl and still > decide not to. > > > --- > > .../devicetree/bindings/media/i2c/ov8858.yaml | 139 ++++++++++++++++++ > > 1 file changed, 139 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8858.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml > > new file mode 100644 > > index 000000000000..f004e57b05ed > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml > > @@ -0,0 +1,139 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov8858.yaml# > > Filename still does not match compatible. ovti,ov8858.yaml > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > + > > +maintainers: > > + - Nicholas Roth <nicholas@xxxxxxxxxxxxx> > > + > > +description: |- > > + The Omnivision OV8858 is an 8 megapixel image sensor which provides > > + images in RAW format over MIPI CSI-2 data bus with up to 4 lanes > > + and is controlled through an I2C-compatibile SCCB bus. > > + > > +properties: > > + compatible: > > + const: ovti,ov8858 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Input clock for the sensor. > > + items: > > + - const: xvclk > > + > > + clock-frequency: > > + description: > > + Frequency of the xvclk clock in Hertz. > > The frequency of clock should go via common clock framework - you have > get_rate and set_rate. Drop entire property. > > > + minimum: 6000000 > > + default: 24000000 > > + maximum: 27000000 > > + > > + dovdd-supply: > > + description: > > + Definition of the regulator used as interface power supply. > > Drop "Definition of the regulator used as " > > > + > > + avdd-supply: > > + description: > > + Definition of the regulator used as analog power supply. > > + > > + dvdd-supply: > > + description: > > + Definition of the regulator used as digital power supply. > > + > > + reset-gpios: > > + description: > > + The phandle and specifier for the GPIO that controls sensor reset. > > Drop this sentence. > > > + This corresponds to the hardware pin XSHUTDN which is physically > > + active low. > > + > > + powerdown-gpios: > > + description: > > + The phandle and specifier for the GPIO that controls power down. > > Drop this sentence. > > > + This corresponds to the hardware pin PWDNB which is physically > > + active low. > > + > > + port: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: > > + description: |- > > + The driver supports both two- and four-lane operation. > > Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence. > > > + items: > > + - const: 1 > > + - const: 2 > > + - const: 3 > > + - const: 4 > > + > > + link-frequencies: > > + description: Frequencies listed are driver, not h/w limitations. > > If these are driver limitations, then drop it. Link frequencies are > hardware related and you should here describe the minimum and maximum. > Or leave it empty if any is allowed by hardware. > > > > + maxItems: 1 > > + items: > > + enum: [ 360000000 ] > > + > > + required: > > + - link-frequencies > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - clock-frequency > > + - dovdd-supply > > + - avdd-supply > > + - dvdd-supply > > + - reset-gpios > > + - port > > + > > +additionalProperties: false > > Best regards, > Krzysztof >
