| Message ID | 20221014183459.181567-2-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | media: i2c: ov5645 driver enhancements | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 104 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Fri, 14 Oct 2022 19:34:55 +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Convert the simple OV5645 Device Tree binding to json-schema. > > The previous binding marked the below properties as required which was a > driver requirement and not the device requirement so just drop them from > the required list during the conversion. > - clock-frequency > - enable-gpios > - reset-gpios > > Also drop the "clock-names" property as we have a single clock source for > the sensor and the driver has been updated to drop the clk referencing by > name. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Resend v3: > * No change > > v2 -> v3 > * Dropped clock-names property > * Marked power supplies as mandatory > * Dropped the comment for voltage power supplies > * Included RB tag from Laurent > * Driver change to drop clock-names [0] > > [0] https://lore.kernel.org/linux-media/Yyh%2F3uzOJOu3drEB@pendragon.ideasonboard.com/T/#t > > v1 -> v2 > * Dropped ref to video-interface-devices.yaml# > * Dropped driver specific required items from the list > * Updated commit message > * Dropped clock-lanes and bus-type from the port and example node > * Marked data-lanes as required in port node > --- > .../devicetree/bindings/media/i2c/ov5645.txt | 54 --------- > .../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++++++++ > 2 files changed, 104 insertions(+), 54 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml > Running 'make dtbs_check' with the schema in this patch gives the following warnings. Consider if they are expected or the schema is incorrect. These may not be new warnings. Note that it is not yet a requirement to have 0 warnings for dtbs_check. This will change in the future. Full log is available here: https://patchwork.ozlabs.org/patch/ camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' arch/arm/boot/dts/imx6dl-pico-dwarf.dtb arch/arm/boot/dts/imx6dl-pico-hobbit.dtb arch/arm/boot/dts/imx6dl-pico-nymph.dtb arch/arm/boot/dts/imx6dl-pico-pi.dtb arch/arm/boot/dts/imx6dl-wandboard.dtb arch/arm/boot/dts/imx6dl-wandboard-revb1.dtb arch/arm/boot/dts/imx6dl-wandboard-revd1.dtb arch/arm/boot/dts/imx6q-pico-dwarf.dtb arch/arm/boot/dts/imx6q-pico-hobbit.dtb arch/arm/boot/dts/imx6q-pico-nymph.dtb arch/arm/boot/dts/imx6q-pico-pi.dtb arch/arm/boot/dts/imx6qp-wandboard-revd1.dtb arch/arm/boot/dts/imx6q-wandboard.dtb arch/arm/boot/dts/imx6q-wandboard-revb1.dtb arch/arm/boot/dts/imx6q-wandboard-revd1.dtb ov5645@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' arch/arm64/boot/dts/renesas/r8a774a1-hihope-rzg2m-ex-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774b1-hihope-rzg2n-ex-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774e1-hihope-rzg2h-ex-mipi-2.1.dtb
On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Convert the simple OV5645 Device Tree binding to json-schema. > > The previous binding marked the below properties as required which was a > driver requirement and not the device requirement so just drop them from > the required list during the conversion. > - clock-frequency > - enable-gpios > - reset-gpios > > Also drop the "clock-names" property as we have a single clock source for > the sensor and the driver has been updated to drop the clk referencing by > name. Driver requirements are the ABI! This breaks a kernel without the driver change and a DTB that has dropped the properties. Also, with 'clock-names' dropped, you've just introduced a bunch of warnings on other people's platforms. Are you going to 'fix' all of them? Rob
Hi Rob, Thank you for the review. On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > The previous binding marked the below properties as required which was a > > driver requirement and not the device requirement so just drop them from > > the required list during the conversion. > > - clock-frequency > > - enable-gpios > > - reset-gpios > > > > Also drop the "clock-names" property as we have a single clock source for > > the sensor and the driver has been updated to drop the clk referencing by > > name. > > Driver requirements are the ABI! > > This breaks a kernel without the driver change and a DTB that has > dropped the properties. > I already have a patch for the driver [0] which I missed to include along with the series. > Also, with 'clock-names' dropped, you've just introduced a bunch of > warnings on other people's platforms. Are you going to 'fix' all of > them? > Yes I will fix them, once the patch driver patch [0] is merged in. [0] https://patchwork.kernel.org/project/linux-media/patch/20220919143350.176746-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar
On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > Hi Rob, > > Thank you for the review. > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > The previous binding marked the below properties as required which was a > > > driver requirement and not the device requirement so just drop them from > > > the required list during the conversion. > > > - clock-frequency > > > - enable-gpios > > > - reset-gpios > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > the sensor and the driver has been updated to drop the clk referencing by > > > name. > > > > Driver requirements are the ABI! > > > > This breaks a kernel without the driver change and a DTB that has > > dropped the properties. > > > I already have a patch for the driver [0] which I missed to include > along with the series. You completely miss the point. Read the first sentence again. Changing driver requirements changes the ABI. This breaks the ABI. The driver patch does not help that. > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > warnings on other people's platforms. Are you going to 'fix' all of > > them? > > > Yes I will fix them, once the patch driver patch [0] is merged in. Why? You are just making extra work. We have enough warnings as-is to fix. Rob
Hi Rob, On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: > On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > > > The previous binding marked the below properties as required which was a > > > > driver requirement and not the device requirement so just drop them from > > > > the required list during the conversion. > > > > - clock-frequency > > > > - enable-gpios > > > > - reset-gpios > > > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > > the sensor and the driver has been updated to drop the clk referencing by > > > > name. > > > > > > Driver requirements are the ABI! > > > > > > This breaks a kernel without the driver change and a DTB that has > > > dropped the properties. > > > > > I already have a patch for the driver [0] which I missed to include > > along with the series. > > You completely miss the point. Read the first sentence again. Changing > driver requirements changes the ABI. > > This breaks the ABI. The driver patch does not help that. I'm not following you here. If the DT binding makes a mandatory property optional, it doesn't break any existing platform. The only thing that would not work is a new DT that doesn't contain the now optional property combined with an older driver that makes it required. That's not a regression, as it would be a *new* DT. > > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > > warnings on other people's platforms. Are you going to 'fix' all of > > > them? > > > > > Yes I will fix them, once the patch driver patch [0] is merged in. > > Why? You are just making extra work. We have enough warnings as-is to > fix. I agree that a DT binding change should patch all in-tree DTS to avoid introducing new warnings.
On 15/10/2022 01:54, Laurent Pinchart wrote: > Hi Rob, > > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: >> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: >>> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: >>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: >>>>> >>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> >>>>> Convert the simple OV5645 Device Tree binding to json-schema. >>>>> >>>>> The previous binding marked the below properties as required which was a >>>>> driver requirement and not the device requirement so just drop them from >>>>> the required list during the conversion. >>>>> - clock-frequency >>>>> - enable-gpios >>>>> - reset-gpios >>>>> >>>>> Also drop the "clock-names" property as we have a single clock source for >>>>> the sensor and the driver has been updated to drop the clk referencing by >>>>> name. >>>> >>>> Driver requirements are the ABI! >>>> >>>> This breaks a kernel without the driver change and a DTB that has >>>> dropped the properties. >>>> >>> I already have a patch for the driver [0] which I missed to include >>> along with the series. >> >> You completely miss the point. Read the first sentence again. Changing >> driver requirements changes the ABI. >> >> This breaks the ABI. The driver patch does not help that. > > I'm not following you here. If the DT binding makes a mandatory property > optional, it doesn't break any existing platform. The only thing that > would not work is a new DT that doesn't contain the now optional > property combined with an older driver that makes it required. That's > not a regression, as it would be a *new* DT. You're right although in-tree DTS are now not compatible with older kernels. So it is not only about new DTS, it is about our kernel DTS which requires new kernel to work. DTS are exported and used by other systems, thus if someone blindly takes this new DTS without clock-names, his kernel/OS/bootloader might stop working. That is however a more relaxed requirement than kernel ABI against old DTS. > >>>> Also, with 'clock-names' dropped, you've just introduced a bunch of >>>> warnings on other people's platforms. Are you going to 'fix' all of >>>> them? >>>> >>> Yes I will fix them, once the patch driver patch [0] is merged in. >> >> Why? You are just making extra work. We have enough warnings as-is to >> fix. > > I agree that a DT binding change should patch all in-tree DTS to avoid > introducing new warnings. Yes. Best regards, Krzysztof
On Sat, Oct 15, 2022 at 2:17 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 15/10/2022 01:54, Laurent Pinchart wrote: > > Hi Rob, > > > > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: > >> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > >>> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > >>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > >>>>> > >>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>> > >>>>> Convert the simple OV5645 Device Tree binding to json-schema. > >>>>> > >>>>> The previous binding marked the below properties as required which was a > >>>>> driver requirement and not the device requirement so just drop them from > >>>>> the required list during the conversion. > >>>>> - clock-frequency > >>>>> - enable-gpios > >>>>> - reset-gpios > >>>>> > >>>>> Also drop the "clock-names" property as we have a single clock source for > >>>>> the sensor and the driver has been updated to drop the clk referencing by > >>>>> name. > >>>> > >>>> Driver requirements are the ABI! > >>>> > >>>> This breaks a kernel without the driver change and a DTB that has > >>>> dropped the properties. > >>>> > >>> I already have a patch for the driver [0] which I missed to include > >>> along with the series. > >> > >> You completely miss the point. Read the first sentence again. Changing > >> driver requirements changes the ABI. > >> > >> This breaks the ABI. The driver patch does not help that. > > > > I'm not following you here. If the DT binding makes a mandatory property > > optional, it doesn't break any existing platform. The only thing that > > would not work is a new DT that doesn't contain the now optional > > property combined with an older driver that makes it required. That's > > not a regression, as it would be a *new* DT. > > You're right although in-tree DTS are now not compatible with older > kernels. So it is not only about new DTS, it is about our kernel DTS > which requires new kernel to work. > To confirm, we are ok dropping the clock-names property here right? > DTS are exported and used by other systems, thus if someone blindly > takes this new DTS without clock-names, his kernel/OS/bootloader might > stop working. > > That is however a more relaxed requirement than kernel ABI against old DTS. > > > > >>>> Also, with 'clock-names' dropped, you've just introduced a bunch of > >>>> warnings on other people's platforms. Are you going to 'fix' all of > >>>> them? > >>>> > >>> Yes I will fix them, once the patch driver patch [0] is merged in. > >> > >> Why? You are just making extra work. We have enough warnings as-is to > >> fix. > > > > I agree that a DT binding change should patch all in-tree DTS to avoid > > introducing new warnings. > > Yes. > OK, I'll send dts changes along with this patch series. Cheers, Prabhakar
On Sat, Oct 15, 2022 at 12:54 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Rob, > > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: > > On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > > > > > The previous binding marked the below properties as required which was a > > > > > driver requirement and not the device requirement so just drop them from > > > > > the required list during the conversion. > > > > > - clock-frequency > > > > > - enable-gpios > > > > > - reset-gpios > > > > > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > > > the sensor and the driver has been updated to drop the clk referencing by > > > > > name. > > > > > > > > Driver requirements are the ABI! > > > > > > > > This breaks a kernel without the driver change and a DTB that has > > > > dropped the properties. > > > > > > > I already have a patch for the driver [0] which I missed to include > > > along with the series. > > > > You completely miss the point. Read the first sentence again. Changing > > driver requirements changes the ABI. > > > > This breaks the ABI. The driver patch does not help that. > > I'm not following you here. If the DT binding makes a mandatory property > optional, it doesn't break any existing platform. The only thing that > would not work is a new DT that doesn't contain the now optional > property combined with an older driver that makes it required. That's > not a regression, as it would be a *new* DT. > > > > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > > > warnings on other people's platforms. Are you going to 'fix' all of > > > > them? > > > > > > > Yes I will fix them, once the patch driver patch [0] is merged in. > > > > Why? You are just making extra work. We have enough warnings as-is to > > fix. > > I agree that a DT binding change should patch all in-tree DTS to avoid > introducing new warnings. That is not what I was saying. Why not just keep 'clock-names' and go spend the DTS fixing time fixing some other warnings that we already have. Also, there is no requirement that converting bindings also fix DTS files. The only wish is that any warnings we do see are ones deemed needing to be fixed in the DTS file. Anyways, there's patches now for the new warnings, so nevermind on this one. Rob
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt deleted file mode 100644 index 72ad992f77be..000000000000 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ /dev/null @@ -1,54 +0,0 @@ -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor - -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with -an active array size of 2592H x 1944V. It is programmable through a serial I2C -interface. - -Required Properties: -- compatible: Value should be "ovti,ov5645". -- clocks: Reference to the xclk clock. -- clock-names: Should be "xclk". -- clock-frequency: Frequency of the xclk clock. -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds - to the hardware pin PWDNB which is physically active low. -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to - the hardware pin RESETB. -- vdddo-supply: Chip digital IO regulator. -- vdda-supply: Chip analog regulator. -- vddd-supply: Chip digital core regulator. - -The device node must contain one 'port' child node for its digital output -video port, in accordance with the video interface bindings defined in -Documentation/devicetree/bindings/media/video-interfaces.txt. - -Example: - - &i2c1 { - ... - - ov5645: ov5645@3c { - compatible = "ovti,ov5645"; - reg = <0x3c>; - - enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>; - reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>; - pinctrl-names = "default"; - pinctrl-0 = <&camera_rear_default>; - - clocks = <&clks 200>; - clock-names = "xclk"; - clock-frequency = <24000000>; - - vdddo-supply = <&camera_dovdd_1v8>; - vdda-supply = <&camera_avdd_2v8>; - vddd-supply = <&camera_dvdd_1v2>; - - port { - ov5645_ep: endpoint { - clock-lanes = <1>; - data-lanes = <0 2>; - remote-endpoint = <&csi0_ep>; - }; - }; - }; - }; diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml new file mode 100644 index 000000000000..0b10483cd267 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml @@ -0,0 +1,104 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5645.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: OmniVision OV5645 Image Sensor Device Tree Bindings + +maintainers: + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> + +properties: + compatible: + const: ovti,ov5645 + + reg: + maxItems: 1 + + clocks: + description: XCLK Input Clock + + clock-frequency: + description: Frequency of the xclk clock in Hz. + + vdda-supply: + description: Analog voltage supply, 2.8 volts + + vddd-supply: + description: Digital core voltage supply, 1.5 volts + + vdddo-supply: + description: Digital I/O voltage supply, 1.8 volts + + enable-gpios: + maxItems: 1 + description: + Reference to the GPIO connected to the PWDNB pin, if any. + + reset-gpios: + maxItems: 1 + description: + Reference to the GPIO connected to the RESETB pin, if any. + + port: + description: Digital Output Port + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + minItems: 1 + maxItems: 2 + items: + enum: [1, 2] + + required: + - data-lanes + +required: + - compatible + - reg + - clocks + - vdddo-supply + - vdda-supply + - vddd-supply + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera@3c { + compatible = "ovti,ov5645"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_ov5645>; + reg = <0x3c>; + clocks = <&clks 1>; + clock-frequency = <24000000>; + vdddo-supply = <&ov5645_vdddo_1v8>; + vdda-supply = <&ov5645_vdda_2v8>; + vddd-supply = <&ov5645_vddd_1v5>; + enable-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; + + port { + ov5645_ep: endpoint { + remote-endpoint = <&csi0_ep>; + data-lanes = <1 2>; + }; + }; + }; + }; +...