From: Lad Prabhakar <[email protected]>
Hi All,
The main aim of this series is to add PM support to the sensor driver.
I had two more patches [0] and [1] which were for ov5645, so instead
sending them separately I have clubbed them as series.
v1-> v2
- patch #1 is infact a v3 [1] no changes
- patch #2 fixed review comments pointed by Sakari
- patch #3 [0] no changes
- patches #4 and #5 are new
[0] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
[1] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
Cheers,
Prabhakar
Lad Prabhakar (5):
media: dt-bindings: ov5645: Convert OV5645 binding to a schema
media: i2c: ov5645: Use runtime PM
media: i2c: ov5645: Drop empty comment
media: i2c: ov5645: Return zero for s_stream(0)
media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering
the subdev
.../devicetree/bindings/media/i2c/ov5645.txt | 54 -------
.../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ov5645.c | 151 +++++++++---------
4 files changed, 178 insertions(+), 133 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
--
2.25.1
From: Lad Prabhakar <[email protected]>
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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%[email protected]/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
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 <[email protected]>
+
+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>;
+ };
+ };
+ };
+ };
+...
--
2.25.1
From: Lad Prabhakar <[email protected]>
Make sure we call ov5645_entity_init_cfg() before registering the subdev
to make sure default formats are set up.
Suggested-by: Sakari Ailus <[email protected]>
Signed-off-by: Lad Prabhakar <[email protected]>
---
v1->v2
* New patch
---
drivers/media/i2c/ov5645.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index b3825294aaf1..14bcc6e42dd2 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1212,6 +1212,8 @@ static int ov5645_probe(struct i2c_client *client)
goto err_pm_runtime;
}
+ ov5645_entity_init_cfg(&ov5645->sd, NULL);
+
ret = v4l2_async_register_subdev(&ov5645->sd);
if (ret < 0) {
dev_err(dev, "could not register v4l2 device\n");
@@ -1224,8 +1226,6 @@ static int ov5645_probe(struct i2c_client *client)
pm_runtime_use_autosuspend(dev);
pm_runtime_put_autosuspend(dev);
- ov5645_entity_init_cfg(&ov5645->sd, NULL);
-
return 0;
err_pm_runtime:
--
2.25.1
Hi Sakari,
On Fri, Oct 14, 2022 at 7:35 PM Prabhakar <[email protected]> wrote:
>
> From: Lad Prabhakar <[email protected]>
>
> Hi All,
>
> The main aim of this series is to add PM support to the sensor driver.
>
> I had two more patches [0] and [1] which were for ov5645, so instead
> sending them separately I have clubbed them as series.
>
> v1-> v2
> - patch #1 is infact a v3 [1] no changes
> - patch #2 fixed review comments pointed by Sakari
> - patch #3 [0] no changes
> - patches #4 and #5 are new
>
> [0] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
>
> Cheers,
> Prabhakar
>
> Lad Prabhakar (5):
> media: dt-bindings: ov5645: Convert OV5645 binding to a schema
> media: i2c: ov5645: Use runtime PM
> media: i2c: ov5645: Drop empty comment
> media: i2c: ov5645: Return zero for s_stream(0)
> media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering
> the subdev
>
After sending this series I realized I had an additional patch [0] for
ov5645 which I should have tagged along with the series. Can you
please pick [0] while reviewing this series.
[0] https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
Cheers,
Prabhakar
On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
>
> From: Lad Prabhakar <[email protected]>
>
> 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
On Fri, 14 Oct 2022 19:34:55 +0100, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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%[email protected]/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
Hi Rob,
Thank you for the review.
On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <[email protected]> wrote:
>
> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
> >
> > From: Lad Prabhakar <[email protected]>
> >
> > 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/[email protected]/
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 <[email protected]> wrote:
> >
> > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
> > >
> > > From: Lad Prabhakar <[email protected]>
> > >
> > > 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 <[email protected]> wrote:
> > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
> > > >
> > > > From: Lad Prabhakar <[email protected]>
> > > >
> > > > 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.
--
Regards,
Laurent Pinchart
Hi Prabhakar,
Thank you for the patch.
On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Make sure we call ov5645_entity_init_cfg() before registering the subdev
> to make sure default formats are set up.
>
> Suggested-by: Sakari Ailus <[email protected]>
> Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
If you have a few spare cycles, it would be even better to convert the
driver to the subdev active state API :-) You could then drop this call
entirely.
> ---
> v1->v2
> * New patch
> ---
> drivers/media/i2c/ov5645.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index b3825294aaf1..14bcc6e42dd2 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1212,6 +1212,8 @@ static int ov5645_probe(struct i2c_client *client)
> goto err_pm_runtime;
> }
>
> + ov5645_entity_init_cfg(&ov5645->sd, NULL);
> +
> ret = v4l2_async_register_subdev(&ov5645->sd);
> if (ret < 0) {
> dev_err(dev, "could not register v4l2 device\n");
> @@ -1224,8 +1226,6 @@ static int ov5645_probe(struct i2c_client *client)
> pm_runtime_use_autosuspend(dev);
> pm_runtime_put_autosuspend(dev);
>
> - ov5645_entity_init_cfg(&ov5645->sd, NULL);
> -
> return 0;
>
> err_pm_runtime:
--
Regards,
Laurent Pinchart
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 <[email protected]> wrote:
>>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
>>>>>
>>>>> From: Lad Prabhakar <[email protected]>
>>>>>
>>>>> 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
<[email protected]> 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 <[email protected]> wrote:
> >>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
> >>>>>
> >>>>> From: Lad Prabhakar <[email protected]>
> >>>>>
> >>>>> 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
Hi Laurent,
Thank you for the review.
On Sat, Oct 15, 2022 at 7:26 AM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Make sure we call ov5645_entity_init_cfg() before registering the subdev
> > to make sure default formats are set up.
> >
> > Suggested-by: Sakari Ailus <[email protected]>
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> If you have a few spare cycles, it would be even better to convert the
> driver to the subdev active state API :-) You could then drop this call
> entirely.
>
For v3 I did think of it, but it looks like I'll need to spend more
time on the subdev state for this driver (as this driver does cropping
which makes use of TRY/ACTIVE). So for v3 I'll keep this patch as and
will work on the subdev state switch in parallel and post when
complete. (Its just I dont want to miss the v6.2 window for RZ/G2L CRU
driver ;-))
Cheers,
Prabhakar
On Sat, Oct 15, 2022 at 12:54 AM Laurent Pinchart
<[email protected]> 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 <[email protected]> wrote:
> > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <[email protected]> wrote:
> > > > >
> > > > > From: Lad Prabhakar <[email protected]>
> > > > >
> > > > > 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