I just got your earlier email reply regarding the CC list, and I think that clarifies (2) for me. Indeed, it seems like you were asking me to also include the mailing lists. I didn't want to bother that many people, but it seems like this is the procedure. On Wed, Nov 9, 2022 at 9:48 AM Nicholas Roth <nicholas@xxxxxxxxxxxxx> wrote: > > 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 > >
