* [PATCH v5 0/5] ov5645: Deprecate usage of the clock-frequency @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar Hi All, This patch series does the following: 1: Deprecate usage of the clock-frequency property. 2: Converts bindings to json-schema Thanks, Prabhakar Changes for v5: * Dropped assigned-clocks * Fixed review comments from Laurent for patch 5/5 Changes for v4: * Addressed to comments from Laurent updated patch description for patch 1/5, 4/5. * Included Reviewed-by tags from Laurent. * Increased tolerance level to 5% for external clock frequency. * Patch 5/5 is new patch which converts bindings to json-schema. Changed for v3: * Dropped reading assigned-clock-rates * Increased the maximum clock frequency to 24480000 Changes for v2: * Instead of completely dropping clock-frequency property marked it as deprecated. * Split up imx6qdl-wandboard.dtsi changes as separate patch. Lad Prabhakar (5): media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property media: i2c: ov5645: Drop reading clock-frequency dt-property media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema .../devicetree/bindings/media/i2c/ov5645.txt | 54 -------- .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - drivers/media/i2c/ov5645.c | 21 +-- 4 files changed, 133 insertions(+), 69 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml -- 2.20.1 ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 0/5] ov5645: Deprecate usage of the clock-frequency @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media Hi All, This patch series does the following: 1: Deprecate usage of the clock-frequency property. 2: Converts bindings to json-schema Thanks, Prabhakar Changes for v5: * Dropped assigned-clocks * Fixed review comments from Laurent for patch 5/5 Changes for v4: * Addressed to comments from Laurent updated patch description for patch 1/5, 4/5. * Included Reviewed-by tags from Laurent. * Increased tolerance level to 5% for external clock frequency. * Patch 5/5 is new patch which converts bindings to json-schema. Changed for v3: * Dropped reading assigned-clock-rates * Increased the maximum clock frequency to 24480000 Changes for v2: * Instead of completely dropping clock-frequency property marked it as deprecated. * Split up imx6qdl-wandboard.dtsi changes as separate patch. Lad Prabhakar (5): media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property media: i2c: ov5645: Drop reading clock-frequency dt-property media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema .../devicetree/bindings/media/i2c/ov5645.txt | 54 -------- .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - drivers/media/i2c/ov5645.c | 21 +-- 4 files changed, 133 insertions(+), 69 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:42 ` Lad Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz but the driver strictly expects this to be 24MHz (with tolerance of 1%) with this restrictions let the driver enforce the clock frequency internally to 24MHz rather then being passed as dt-property. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt index 72ad992f77be..a55bb728ea48 100644 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt @@ -8,7 +8,6 @@ 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 @@ -37,7 +36,6 @@ Example: clocks = <&clks 200>; clock-names = "xclk"; - clock-frequency = <24000000>; vdddo-supply = <&camera_dovdd_1v8>; vdda-supply = <&camera_avdd_2v8>; -- 2.20.1 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz but the driver strictly expects this to be 24MHz (with tolerance of 1%) with this restrictions let the driver enforce the clock frequency internally to 24MHz rather then being passed as dt-property. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt index 72ad992f77be..a55bb728ea48 100644 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt @@ -8,7 +8,6 @@ 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 @@ -37,7 +36,6 @@ Example: clocks = <&clks 200>; clock-names = "xclk"; - clock-frequency = <24000000>; vdddo-supply = <&camera_dovdd_1v8>; vdda-supply = <&camera_avdd_2v8>; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 17:30 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:30 UTC (permalink / raw) To: Lad Prabhakar Cc: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:37PM +0100, Lad Prabhakar wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency Maybe s/with/With/ ? > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > index 72ad992f77be..a55bb728ea48 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > @@ -8,7 +8,6 @@ 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 > @@ -37,7 +36,6 @@ Example: > > clocks = <&clks 200>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > > vdddo-supply = <&camera_dovdd_1v8>; > vdda-supply = <&camera_avdd_2v8>; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property @ 2020-04-06 17:30 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:30 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:37PM +0100, Lad Prabhakar wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency Maybe s/with/With/ ? > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > index 72ad992f77be..a55bb728ea48 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > @@ -8,7 +8,6 @@ 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 > @@ -37,7 +36,6 @@ Example: > > clocks = <&clks 200>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > > vdddo-supply = <&camera_dovdd_1v8>; > vdda-supply = <&camera_avdd_2v8>; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property 2020-04-06 17:30 ` Laurent Pinchart @ 2020-04-07 7:35 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:35 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, LAK Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:30 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:37PM +0100, Lad Prabhakar wrote: > > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > > with this restrictions let the driver enforce the clock frequency > > Maybe s/with/With/ ? > OK will fix it next version. Cheers, --Prabhakar > > internally to 24MHz rather then being passed as dt-property. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > index 72ad992f77be..a55bb728ea48 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > @@ -8,7 +8,6 @@ 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 > > @@ -37,7 +36,6 @@ Example: > > > > clocks = <&clks 200>; > > clock-names = "xclk"; > > - clock-frequency = <24000000>; > > > > vdddo-supply = <&camera_dovdd_1v8>; > > vdda-supply = <&camera_avdd_2v8>; > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property @ 2020-04-07 7:35 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:35 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, LKML, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, LAK, linux-media Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:30 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:37PM +0100, Lad Prabhakar wrote: > > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > > with this restrictions let the driver enforce the clock frequency > > Maybe s/with/With/ ? > OK will fix it next version. Cheers, --Prabhakar > > internally to 24MHz rather then being passed as dt-property. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > index 72ad992f77be..a55bb728ea48 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > @@ -8,7 +8,6 @@ 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 > > @@ -37,7 +36,6 @@ Example: > > > > clocks = <&clks 200>; > > clock-names = "xclk"; > > - clock-frequency = <24000000>; > > > > vdddo-supply = <&camera_dovdd_1v8>; > > vdda-supply = <&camera_avdd_2v8>; > > -- > Regards, > > Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-07 7:14 ` Geert Uytterhoeven -1 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:14 UTC (permalink / raw) To: Lad Prabhakar Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM, Lad Prabhakar On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property @ 2020-04-07 7:14 ` Geert Uytterhoeven 0 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:14 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Fabio Estevam, Sascha Hauer, Kieran Bingham, Linux Media Mailing List, Linux Kernel Mailing List, Lad Prabhakar, Rob Herring, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, NXP Linux Team On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-14 13:55 ` Rob Herring -1 siblings, 0 replies; 66+ messages in thread From: Rob Herring @ 2020-04-14 13:55 UTC (permalink / raw) To: Lad Prabhakar Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar On Mon, 6 Apr 2020 17:42:37 +0100, Lad Prabhakar wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > 1 file changed, 2 deletions(-) > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed. ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property @ 2020-04-14 13:55 ` Rob Herring 0 siblings, 0 replies; 66+ messages in thread From: Rob Herring @ 2020-04-14 13:55 UTC (permalink / raw) To: Lad Prabhakar Cc: devicetree, Lad Prabhakar, Sakari Ailus, Geert Uytterhoeven, Sascha Hauer, linux-media, linux-kernel, Lad Prabhakar, Kieran Bingham, Laurent Pinchart, Pengutronix Kernel Team, Mauro Carvalho Chehab, Fabio Estevam, linux-arm-kernel, NXP Linux Team On Mon, 6 Apr 2020 17:42:37 +0100, Lad Prabhakar wrote: > OV5645 sensor supports input clock frequency ranging from 6MHz to 27MHz > but the driver strictly expects this to be 24MHz (with tolerance of 1%) > with this restrictions let the driver enforce the clock frequency > internally to 24MHz rather then being passed as dt-property. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 2 -- > 1 file changed, 2 deletions(-) > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:42 ` Lad Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar Modes in the driver are based on xvclk frequency fixed to 24MHz, but where as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to 24MHz. So instead making clock-frequency as dt-property just let the driver enforce the required clock frequency. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/i2c/ov5645.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index a6c17d15d754..52848fff8a08 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -61,6 +61,8 @@ #define OV5645_SDE_SAT_U 0x5583 #define OV5645_SDE_SAT_V 0x5584 +#define OV5645_XVCLK_FREQ 24000000 + /* regulator supplies */ static const char * const ov5645_supply_name[] = { "vdddo", /* Digital I/O (1.8V) supply */ @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) return PTR_ERR(ov5645->xclk); } - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); if (ret) { - dev_err(dev, "could not get xclk frequency\n"); + dev_err(dev, "could not set xclk frequency\n"); return ret; } - /* external clock must be 24MHz, allow 1% tolerance */ + xclk_freq = clk_get_rate(ov5645->xclk); if (xclk_freq < 23760000 || xclk_freq > 24240000) { dev_err(dev, "external clock frequency %u is not supported\n", xclk_freq); return -EINVAL; } - ret = clk_set_rate(ov5645->xclk, xclk_freq); - if (ret) { - dev_err(dev, "could not set xclk frequency\n"); - return ret; - } - for (i = 0; i < OV5645_NUM_SUPPLIES; i++) ov5645->supplies[i].supply = ov5645_supply_name[i]; -- 2.20.1 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media Modes in the driver are based on xvclk frequency fixed to 24MHz, but where as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to 24MHz. So instead making clock-frequency as dt-property just let the driver enforce the required clock frequency. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/i2c/ov5645.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index a6c17d15d754..52848fff8a08 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -61,6 +61,8 @@ #define OV5645_SDE_SAT_U 0x5583 #define OV5645_SDE_SAT_V 0x5584 +#define OV5645_XVCLK_FREQ 24000000 + /* regulator supplies */ static const char * const ov5645_supply_name[] = { "vdddo", /* Digital I/O (1.8V) supply */ @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) return PTR_ERR(ov5645->xclk); } - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); if (ret) { - dev_err(dev, "could not get xclk frequency\n"); + dev_err(dev, "could not set xclk frequency\n"); return ret; } - /* external clock must be 24MHz, allow 1% tolerance */ + xclk_freq = clk_get_rate(ov5645->xclk); if (xclk_freq < 23760000 || xclk_freq > 24240000) { dev_err(dev, "external clock frequency %u is not supported\n", xclk_freq); return -EINVAL; } - ret = clk_set_rate(ov5645->xclk, xclk_freq); - if (ret) { - dev_err(dev, "could not set xclk frequency\n"); - return ret; - } - for (i = 0; i < OV5645_NUM_SUPPLIES; i++) ov5645->supplies[i].supply = ov5645_supply_name[i]; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:51 ` Sakari Ailus -1 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-06 16:51 UTC (permalink / raw) To: Lad Prabhakar Cc: Laurent Pinchart, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. Even if some current systems where the driver is used are using 24 MHz clock, that doesn't mean there wouldn't be systems using another frequency that the driver does not support right now. The driver really should not set the frequency unless it gets it from DT, but I think the preferred means is to use assigned-clock-rates instead, and not to involve the driver with setting the frequency. Otherwise we'll make it impossible to support other frequencies, at least without more or less random defaults. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-06 16:51 ` Sakari Ailus 0 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-06 16:51 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-media, linux-kernel, Lad Prabhakar, Rob Herring, Laurent Pinchart, Pengutronix Kernel Team, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, NXP Linux Team Hi Prabhakar, On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. Even if some current systems where the driver is used are using 24 MHz clock, that doesn't mean there wouldn't be systems using another frequency that the driver does not support right now. The driver really should not set the frequency unless it gets it from DT, but I think the preferred means is to use assigned-clock-rates instead, and not to involve the driver with setting the frequency. Otherwise we'll make it impossible to support other frequencies, at least without more or less random defaults. -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:51 ` Sakari Ailus @ 2020-04-06 17:11 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-06 17:11 UTC (permalink / raw) To: Sakari Ailus Cc: Maxime Ripard, Lad Prabhakar, Laurent Pinchart, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, LAK Hi Sakari, On Mon, Apr 6, 2020 at 5:51 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Prabhakar, > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > Even if some current systems where the driver is used are using 24 MHz > clock, that doesn't mean there wouldn't be systems using another frequency > that the driver does not support right now. > > The driver really should not set the frequency unless it gets it from DT, > but I think the preferred means is to use assigned-clock-rates instead, and > not to involve the driver with setting the frequency. > > Otherwise we'll make it impossible to support other frequencies, at least > without more or less random defaults. > Ouch! my previous version of patches switched the driver for using assigned-clock-rates but I was asked to not do so [1]. [1] https://patchwork.linuxtv.org/patch/62185/ Cheers, --Prabhakar > -- > Kind regards, > > Sakari Ailus ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-06 17:11 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-06 17:11 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-media, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, LKML, Rob Herring, Laurent Pinchart, Pengutronix Kernel Team, NXP Linux Team, Mauro Carvalho Chehab, Shawn Guo, LAK, Maxime Ripard Hi Sakari, On Mon, Apr 6, 2020 at 5:51 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Prabhakar, > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > Even if some current systems where the driver is used are using 24 MHz > clock, that doesn't mean there wouldn't be systems using another frequency > that the driver does not support right now. > > The driver really should not set the frequency unless it gets it from DT, > but I think the preferred means is to use assigned-clock-rates instead, and > not to involve the driver with setting the frequency. > > Otherwise we'll make it impossible to support other frequencies, at least > without more or less random defaults. > Ouch! my previous version of patches switched the driver for using assigned-clock-rates but I was asked to not do so [1]. [1] https://patchwork.linuxtv.org/patch/62185/ Cheers, --Prabhakar > -- > Kind regards, > > Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:51 ` Sakari Ailus @ 2020-04-06 17:32 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:32 UTC (permalink / raw) To: Sakari Ailus Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Sakari, (CC'ing Maxime) On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > Even if some current systems where the driver is used are using 24 MHz > clock, that doesn't mean there wouldn't be systems using another frequency > that the driver does not support right now. > > The driver really should not set the frequency unless it gets it from DT, > but I think the preferred means is to use assigned-clock-rates instead, and > not to involve the driver with setting the frequency. > > Otherwise we'll make it impossible to support other frequencies, at least > without more or less random defaults. We're running in circles here. As the driver only supports 24MHz at the moment, the frequency should be set by the driver, as it's a driver limitation. We can then work on supporting additional frequencies, which will require DT to provide a list of supported frequencies for the system, but that can be done on top. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-06 17:32 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:32 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Sakari, (CC'ing Maxime) On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > Even if some current systems where the driver is used are using 24 MHz > clock, that doesn't mean there wouldn't be systems using another frequency > that the driver does not support right now. > > The driver really should not set the frequency unless it gets it from DT, > but I think the preferred means is to use assigned-clock-rates instead, and > not to involve the driver with setting the frequency. > > Otherwise we'll make it impossible to support other frequencies, at least > without more or less random defaults. We're running in circles here. As the driver only supports 24MHz at the moment, the frequency should be set by the driver, as it's a driver limitation. We can then work on supporting additional frequencies, which will require DT to provide a list of supported frequencies for the system, but that can be done on top. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 17:32 ` Laurent Pinchart @ 2020-04-07 6:22 ` Sakari Ailus -1 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-07 6:22 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Laurent, On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > (CC'ing Maxime) > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > driver enforce the required clock frequency. > > > > Even if some current systems where the driver is used are using 24 MHz > > clock, that doesn't mean there wouldn't be systems using another frequency > > that the driver does not support right now. > > > > The driver really should not set the frequency unless it gets it from DT, > > but I think the preferred means is to use assigned-clock-rates instead, and > > not to involve the driver with setting the frequency. > > > > Otherwise we'll make it impossible to support other frequencies, at least > > without more or less random defaults. > > We're running in circles here. > > As the driver only supports 24MHz at the moment, the frequency should be > set by the driver, as it's a driver limitation. We can then work on > supporting additional frequencies, which will require DT to provide a > list of supported frequencies for the system, but that can be done on > top. I guess it would be possible to use different external clock frequencies on a sensor in a given system but that seems to be a bit far fetched, to the extent I've never seen anyone doing that in practice. Originally, the driver set the frequency based on the clock-frequency property. If we're removing that but use a fixed frequency instead, then how is that going to work going forward when someone adds support for other frequencies in the driver and has a system requiring that, while there are some other platforms relying on the driver setting a particular frequency? Although, if you're saying that this driver only needs to work with DT that comes with the kernel and you don't care about DT binary compatibility, this would be fine. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 6:22 ` Sakari Ailus 0 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-07 6:22 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Laurent, On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > (CC'ing Maxime) > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > driver enforce the required clock frequency. > > > > Even if some current systems where the driver is used are using 24 MHz > > clock, that doesn't mean there wouldn't be systems using another frequency > > that the driver does not support right now. > > > > The driver really should not set the frequency unless it gets it from DT, > > but I think the preferred means is to use assigned-clock-rates instead, and > > not to involve the driver with setting the frequency. > > > > Otherwise we'll make it impossible to support other frequencies, at least > > without more or less random defaults. > > We're running in circles here. > > As the driver only supports 24MHz at the moment, the frequency should be > set by the driver, as it's a driver limitation. We can then work on > supporting additional frequencies, which will require DT to provide a > list of supported frequencies for the system, but that can be done on > top. I guess it would be possible to use different external clock frequencies on a sensor in a given system but that seems to be a bit far fetched, to the extent I've never seen anyone doing that in practice. Originally, the driver set the frequency based on the clock-frequency property. If we're removing that but use a fixed frequency instead, then how is that going to work going forward when someone adds support for other frequencies in the driver and has a system requiring that, while there are some other platforms relying on the driver setting a particular frequency? Although, if you're saying that this driver only needs to work with DT that comes with the kernel and you don't care about DT binary compatibility, this would be fine. -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-07 6:22 ` Sakari Ailus @ 2020-04-07 12:21 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-07 12:21 UTC (permalink / raw) To: Sakari Ailus Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Sakari, On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > > driver enforce the required clock frequency. > > > > > > Even if some current systems where the driver is used are using 24 MHz > > > clock, that doesn't mean there wouldn't be systems using another frequency > > > that the driver does not support right now. > > > > > > The driver really should not set the frequency unless it gets it from DT, > > > but I think the preferred means is to use assigned-clock-rates instead, and > > > not to involve the driver with setting the frequency. > > > > > > Otherwise we'll make it impossible to support other frequencies, at least > > > without more or less random defaults. > > > > We're running in circles here. > > > > As the driver only supports 24MHz at the moment, the frequency should be > > set by the driver, as it's a driver limitation. We can then work on > > supporting additional frequencies, which will require DT to provide a > > list of supported frequencies for the system, but that can be done on > > top. > > I guess it would be possible to use different external clock frequencies on > a sensor in a given system but that seems to be a bit far fetched, to the > extent I've never seen anyone doing that in practice. > > Originally, the driver set the frequency based on the clock-frequency > property. If we're removing that but use a fixed frequency instead, then > how is that going to work going forward when someone adds support for other > frequencies in the driver and has a system requiring that, while there are > some other platforms relying on the driver setting a particular frequency? The standard property for this is link-frequencies, not clock-frequency. Deprecating clock-frequency now paves the way to use the standard property later when/if someone implements support for additional frequencies. > Although, if you're saying that this driver only needs to work with DT that > comes with the kernel and you don't care about DT binary compatibility, > this would be fine. I believe this series to not break backward compatibility, as the driver only works with a 24MHz clock, so I expect all DTs to specify that. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 12:21 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-07 12:21 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Sakari, On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > > driver enforce the required clock frequency. > > > > > > Even if some current systems where the driver is used are using 24 MHz > > > clock, that doesn't mean there wouldn't be systems using another frequency > > > that the driver does not support right now. > > > > > > The driver really should not set the frequency unless it gets it from DT, > > > but I think the preferred means is to use assigned-clock-rates instead, and > > > not to involve the driver with setting the frequency. > > > > > > Otherwise we'll make it impossible to support other frequencies, at least > > > without more or less random defaults. > > > > We're running in circles here. > > > > As the driver only supports 24MHz at the moment, the frequency should be > > set by the driver, as it's a driver limitation. We can then work on > > supporting additional frequencies, which will require DT to provide a > > list of supported frequencies for the system, but that can be done on > > top. > > I guess it would be possible to use different external clock frequencies on > a sensor in a given system but that seems to be a bit far fetched, to the > extent I've never seen anyone doing that in practice. > > Originally, the driver set the frequency based on the clock-frequency > property. If we're removing that but use a fixed frequency instead, then > how is that going to work going forward when someone adds support for other > frequencies in the driver and has a system requiring that, while there are > some other platforms relying on the driver setting a particular frequency? The standard property for this is link-frequencies, not clock-frequency. Deprecating clock-frequency now paves the way to use the standard property later when/if someone implements support for additional frequencies. > Although, if you're saying that this driver only needs to work with DT that > comes with the kernel and you don't care about DT binary compatibility, > this would be fine. I believe this series to not break backward compatibility, as the driver only works with a 24MHz clock, so I expect all DTs to specify that. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-07 12:21 ` Laurent Pinchart @ 2020-04-07 15:14 ` Sakari Ailus -1 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-07 15:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Laurent, On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > > > driver enforce the required clock frequency. > > > > > > > > Even if some current systems where the driver is used are using 24 MHz > > > > clock, that doesn't mean there wouldn't be systems using another frequency > > > > that the driver does not support right now. > > > > > > > > The driver really should not set the frequency unless it gets it from DT, > > > > but I think the preferred means is to use assigned-clock-rates instead, and > > > > not to involve the driver with setting the frequency. > > > > > > > > Otherwise we'll make it impossible to support other frequencies, at least > > > > without more or less random defaults. > > > > > > We're running in circles here. > > > > > > As the driver only supports 24MHz at the moment, the frequency should be > > > set by the driver, as it's a driver limitation. We can then work on > > > supporting additional frequencies, which will require DT to provide a > > > list of supported frequencies for the system, but that can be done on > > > top. > > > > I guess it would be possible to use different external clock frequencies on > > a sensor in a given system but that seems to be a bit far fetched, to the > > extent I've never seen anyone doing that in practice. > > > > Originally, the driver set the frequency based on the clock-frequency > > property. If we're removing that but use a fixed frequency instead, then > > how is that going to work going forward when someone adds support for other > > frequencies in the driver and has a system requiring that, while there are > > some other platforms relying on the driver setting a particular frequency? > > The standard property for this is link-frequencies, not clock-frequency. > Deprecating clock-frequency now paves the way to use the standard > property later when/if someone implements support for additional > frequencies. The external clock frequency and link frequency are different indeed, but they are related. The link frequency has been selected in a way that it is possible to generate that exact frequency using the chosen external clock frequency. If you change the external clock frequency, chances are good there is no PLL configuration to generate that link frequency. > > > Although, if you're saying that this driver only needs to work with DT that > > comes with the kernel and you don't care about DT binary compatibility, > > this would be fine. > > I believe this series to not break backward compatibility, as the driver > only works with a 24MHz clock, so I expect all DTs to specify that. What you're still doing here is defining the DT bindings based on the current driver implementation, not the device properties. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 15:14 ` Sakari Ailus 0 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-07 15:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Laurent, On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > > > driver enforce the required clock frequency. > > > > > > > > Even if some current systems where the driver is used are using 24 MHz > > > > clock, that doesn't mean there wouldn't be systems using another frequency > > > > that the driver does not support right now. > > > > > > > > The driver really should not set the frequency unless it gets it from DT, > > > > but I think the preferred means is to use assigned-clock-rates instead, and > > > > not to involve the driver with setting the frequency. > > > > > > > > Otherwise we'll make it impossible to support other frequencies, at least > > > > without more or less random defaults. > > > > > > We're running in circles here. > > > > > > As the driver only supports 24MHz at the moment, the frequency should be > > > set by the driver, as it's a driver limitation. We can then work on > > > supporting additional frequencies, which will require DT to provide a > > > list of supported frequencies for the system, but that can be done on > > > top. > > > > I guess it would be possible to use different external clock frequencies on > > a sensor in a given system but that seems to be a bit far fetched, to the > > extent I've never seen anyone doing that in practice. > > > > Originally, the driver set the frequency based on the clock-frequency > > property. If we're removing that but use a fixed frequency instead, then > > how is that going to work going forward when someone adds support for other > > frequencies in the driver and has a system requiring that, while there are > > some other platforms relying on the driver setting a particular frequency? > > The standard property for this is link-frequencies, not clock-frequency. > Deprecating clock-frequency now paves the way to use the standard > property later when/if someone implements support for additional > frequencies. The external clock frequency and link frequency are different indeed, but they are related. The link frequency has been selected in a way that it is possible to generate that exact frequency using the chosen external clock frequency. If you change the external clock frequency, chances are good there is no PLL configuration to generate that link frequency. > > > Although, if you're saying that this driver only needs to work with DT that > > comes with the kernel and you don't care about DT binary compatibility, > > this would be fine. > > I believe this series to not break backward compatibility, as the driver > only works with a 24MHz clock, so I expect all DTs to specify that. What you're still doing here is defining the DT bindings based on the current driver implementation, not the device properties. -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-07 15:14 ` Sakari Ailus @ 2020-04-14 20:55 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-14 20:55 UTC (permalink / raw) To: Sakari Ailus Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Sakari, On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>> driver enforce the required clock frequency. > >>>> > >>>> Even if some current systems where the driver is used are using 24 MHz > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>> that the driver does not support right now. > >>>> > >>>> The driver really should not set the frequency unless it gets it from DT, > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>> not to involve the driver with setting the frequency. > >>>> > >>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>> without more or less random defaults. > >>> > >>> We're running in circles here. > >>> > >>> As the driver only supports 24MHz at the moment, the frequency should be > >>> set by the driver, as it's a driver limitation. We can then work on > >>> supporting additional frequencies, which will require DT to provide a > >>> list of supported frequencies for the system, but that can be done on > >>> top. > >> > >> I guess it would be possible to use different external clock frequencies on > >> a sensor in a given system but that seems to be a bit far fetched, to the > >> extent I've never seen anyone doing that in practice. > >> > >> Originally, the driver set the frequency based on the clock-frequency > >> property. If we're removing that but use a fixed frequency instead, then > >> how is that going to work going forward when someone adds support for other > >> frequencies in the driver and has a system requiring that, while there are > >> some other platforms relying on the driver setting a particular frequency? > > > > The standard property for this is link-frequencies, not clock-frequency. > > Deprecating clock-frequency now paves the way to use the standard > > property later when/if someone implements support for additional > > frequencies. > > The external clock frequency and link frequency are different indeed, but > they are related. The link frequency has been selected in a way that it is > possible to generate that exact frequency using the chosen external clock > frequency. If you change the external clock frequency, chances are good > there is no PLL configuration to generate that link frequency. But aren't we supposed to pick the clock frequency based on the link frequency specified in DT ? In any case, this policy needs to be carefully documented. > >> Although, if you're saying that this driver only needs to work with DT that > >> comes with the kernel and you don't care about DT binary compatibility, > >> this would be fine. > > > > I believe this series to not break backward compatibility, as the driver > > only works with a 24MHz clock, so I expect all DTs to specify that. > > What you're still doing here is defining the DT bindings based on the > current driver implementation, not the device properties. Quite the contrary, the device doesn't require any particular input clock frequency, so we're removing that from DT :-) Specifying the clock frequency in DT is in my opinion a manual workaround for not computing it at runtime based on the desired link frequency, while the link frequency is a property of the system as it specifies the range of link frequencies that are safe to use from an EMC point of view. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-14 20:55 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-14 20:55 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Sakari, On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>> driver enforce the required clock frequency. > >>>> > >>>> Even if some current systems where the driver is used are using 24 MHz > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>> that the driver does not support right now. > >>>> > >>>> The driver really should not set the frequency unless it gets it from DT, > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>> not to involve the driver with setting the frequency. > >>>> > >>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>> without more or less random defaults. > >>> > >>> We're running in circles here. > >>> > >>> As the driver only supports 24MHz at the moment, the frequency should be > >>> set by the driver, as it's a driver limitation. We can then work on > >>> supporting additional frequencies, which will require DT to provide a > >>> list of supported frequencies for the system, but that can be done on > >>> top. > >> > >> I guess it would be possible to use different external clock frequencies on > >> a sensor in a given system but that seems to be a bit far fetched, to the > >> extent I've never seen anyone doing that in practice. > >> > >> Originally, the driver set the frequency based on the clock-frequency > >> property. If we're removing that but use a fixed frequency instead, then > >> how is that going to work going forward when someone adds support for other > >> frequencies in the driver and has a system requiring that, while there are > >> some other platforms relying on the driver setting a particular frequency? > > > > The standard property for this is link-frequencies, not clock-frequency. > > Deprecating clock-frequency now paves the way to use the standard > > property later when/if someone implements support for additional > > frequencies. > > The external clock frequency and link frequency are different indeed, but > they are related. The link frequency has been selected in a way that it is > possible to generate that exact frequency using the chosen external clock > frequency. If you change the external clock frequency, chances are good > there is no PLL configuration to generate that link frequency. But aren't we supposed to pick the clock frequency based on the link frequency specified in DT ? In any case, this policy needs to be carefully documented. > >> Although, if you're saying that this driver only needs to work with DT that > >> comes with the kernel and you don't care about DT binary compatibility, > >> this would be fine. > > > > I believe this series to not break backward compatibility, as the driver > > only works with a 24MHz clock, so I expect all DTs to specify that. > > What you're still doing here is defining the DT bindings based on the > current driver implementation, not the device properties. Quite the contrary, the device doesn't require any particular input clock frequency, so we're removing that from DT :-) Specifying the clock frequency in DT is in my opinion a manual workaround for not computing it at runtime based on the desired link frequency, while the link frequency is a property of the system as it specifies the range of link frequencies that are safe to use from an EMC point of view. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-14 20:55 ` Laurent Pinchart @ 2020-04-14 20:56 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-14 20:56 UTC (permalink / raw) To: Sakari Ailus Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Sakari, On Tue, Apr 14, 2020 at 11:55:54PM +0300, Laurent Pinchart wrote: > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > >> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >>> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>>> driver enforce the required clock frequency. > >>>>> > >>>>> Even if some current systems where the driver is used are using 24 MHz > >>>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>>> that the driver does not support right now. > >>>>> > >>>>> The driver really should not set the frequency unless it gets it from DT, > >>>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>>> not to involve the driver with setting the frequency. > >>>>> > >>>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>>> without more or less random defaults. > >>>> > >>>> We're running in circles here. > >>>> > >>>> As the driver only supports 24MHz at the moment, the frequency should be > >>>> set by the driver, as it's a driver limitation. We can then work on > >>>> supporting additional frequencies, which will require DT to provide a > >>>> list of supported frequencies for the system, but that can be done on > >>>> top. > >>> > >>> I guess it would be possible to use different external clock frequencies on > >>> a sensor in a given system but that seems to be a bit far fetched, to the > >>> extent I've never seen anyone doing that in practice. > >>> > >>> Originally, the driver set the frequency based on the clock-frequency > >>> property. If we're removing that but use a fixed frequency instead, then > >>> how is that going to work going forward when someone adds support for other > >>> frequencies in the driver and has a system requiring that, while there are > >>> some other platforms relying on the driver setting a particular frequency? > >> > >> The standard property for this is link-frequencies, not clock-frequency. > >> Deprecating clock-frequency now paves the way to use the standard > >> property later when/if someone implements support for additional > >> frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? > > In any case, this policy needs to be carefully documented. And by this I mean in a central place, not leaving it to individual bindings. Maxime, we've previously discussed this issue privately on IRC, what's your opinion ? > >>> Although, if you're saying that this driver only needs to work with DT that > >>> comes with the kernel and you don't care about DT binary compatibility, > >>> this would be fine. > >> > >> I believe this series to not break backward compatibility, as the driver > >> only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-14 20:56 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-14 20:56 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Sakari, On Tue, Apr 14, 2020 at 11:55:54PM +0300, Laurent Pinchart wrote: > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > >> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >>> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>>> driver enforce the required clock frequency. > >>>>> > >>>>> Even if some current systems where the driver is used are using 24 MHz > >>>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>>> that the driver does not support right now. > >>>>> > >>>>> The driver really should not set the frequency unless it gets it from DT, > >>>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>>> not to involve the driver with setting the frequency. > >>>>> > >>>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>>> without more or less random defaults. > >>>> > >>>> We're running in circles here. > >>>> > >>>> As the driver only supports 24MHz at the moment, the frequency should be > >>>> set by the driver, as it's a driver limitation. We can then work on > >>>> supporting additional frequencies, which will require DT to provide a > >>>> list of supported frequencies for the system, but that can be done on > >>>> top. > >>> > >>> I guess it would be possible to use different external clock frequencies on > >>> a sensor in a given system but that seems to be a bit far fetched, to the > >>> extent I've never seen anyone doing that in practice. > >>> > >>> Originally, the driver set the frequency based on the clock-frequency > >>> property. If we're removing that but use a fixed frequency instead, then > >>> how is that going to work going forward when someone adds support for other > >>> frequencies in the driver and has a system requiring that, while there are > >>> some other platforms relying on the driver setting a particular frequency? > >> > >> The standard property for this is link-frequencies, not clock-frequency. > >> Deprecating clock-frequency now paves the way to use the standard > >> property later when/if someone implements support for additional > >> frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? > > In any case, this policy needs to be carefully documented. And by this I mean in a central place, not leaving it to individual bindings. Maxime, we've previously discussed this issue privately on IRC, what's your opinion ? > >>> Although, if you're saying that this driver only needs to work with DT that > >>> comes with the kernel and you don't care about DT binary compatibility, > >>> this would be fine. > >> > >> I believe this series to not break backward compatibility, as the driver > >> only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-14 20:55 ` Laurent Pinchart @ 2020-04-15 8:19 ` Maxime Ripard -1 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2020-04-15 8:19 UTC (permalink / raw) To: Laurent Pinchart Cc: Sakari Ailus, Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar [-- Attachment #1: Type: text/plain, Size: 4172 bytes --] On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > > >>>>> driver enforce the required clock frequency. > > >>>> > > >>>> Even if some current systems where the driver is used are using 24 MHz > > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > > >>>> that the driver does not support right now. > > >>>> > > >>>> The driver really should not set the frequency unless it gets it from DT, > > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > > >>>> not to involve the driver with setting the frequency. > > >>>> > > >>>> Otherwise we'll make it impossible to support other frequencies, at least > > >>>> without more or less random defaults. > > >>> > > >>> We're running in circles here. > > >>> > > >>> As the driver only supports 24MHz at the moment, the frequency should be > > >>> set by the driver, as it's a driver limitation. We can then work on > > >>> supporting additional frequencies, which will require DT to provide a > > >>> list of supported frequencies for the system, but that can be done on > > >>> top. > > >> > > >> I guess it would be possible to use different external clock frequencies on > > >> a sensor in a given system but that seems to be a bit far fetched, to the > > >> extent I've never seen anyone doing that in practice. > > >> > > >> Originally, the driver set the frequency based on the clock-frequency > > >> property. If we're removing that but use a fixed frequency instead, then > > >> how is that going to work going forward when someone adds support for other > > >> frequencies in the driver and has a system requiring that, while there are > > >> some other platforms relying on the driver setting a particular frequency? > > > > > > The standard property for this is link-frequencies, not clock-frequency. > > > Deprecating clock-frequency now paves the way to use the standard > > > property later when/if someone implements support for additional > > > frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? > > In any case, this policy needs to be carefully documented. > > > >> Although, if you're saying that this driver only needs to work with DT that > > >> comes with the kernel and you don't care about DT binary compatibility, > > >> this would be fine. > > > > > > I believe this series to not break backward compatibility, as the driver > > > only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. I 100% agree with this. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-15 8:19 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2020-04-15 8:19 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media [-- Attachment #1.1: Type: text/plain, Size: 4172 bytes --] On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > > >>>>> driver enforce the required clock frequency. > > >>>> > > >>>> Even if some current systems where the driver is used are using 24 MHz > > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > > >>>> that the driver does not support right now. > > >>>> > > >>>> The driver really should not set the frequency unless it gets it from DT, > > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > > >>>> not to involve the driver with setting the frequency. > > >>>> > > >>>> Otherwise we'll make it impossible to support other frequencies, at least > > >>>> without more or less random defaults. > > >>> > > >>> We're running in circles here. > > >>> > > >>> As the driver only supports 24MHz at the moment, the frequency should be > > >>> set by the driver, as it's a driver limitation. We can then work on > > >>> supporting additional frequencies, which will require DT to provide a > > >>> list of supported frequencies for the system, but that can be done on > > >>> top. > > >> > > >> I guess it would be possible to use different external clock frequencies on > > >> a sensor in a given system but that seems to be a bit far fetched, to the > > >> extent I've never seen anyone doing that in practice. > > >> > > >> Originally, the driver set the frequency based on the clock-frequency > > >> property. If we're removing that but use a fixed frequency instead, then > > >> how is that going to work going forward when someone adds support for other > > >> frequencies in the driver and has a system requiring that, while there are > > >> some other platforms relying on the driver setting a particular frequency? > > > > > > The standard property for this is link-frequencies, not clock-frequency. > > > Deprecating clock-frequency now paves the way to use the standard > > > property later when/if someone implements support for additional > > > frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? > > In any case, this policy needs to be carefully documented. > > > >> Although, if you're saying that this driver only needs to work with DT that > > >> comes with the kernel and you don't care about DT binary compatibility, > > >> this would be fine. > > > > > > I believe this series to not break backward compatibility, as the driver > > > only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. I 100% agree with this. Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-14 20:55 ` Laurent Pinchart @ 2020-04-15 16:27 ` Sakari Ailus -1 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-15 16:27 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Laurent, On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > > >>>>> driver enforce the required clock frequency. > > >>>> > > >>>> Even if some current systems where the driver is used are using 24 MHz > > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > > >>>> that the driver does not support right now. > > >>>> > > >>>> The driver really should not set the frequency unless it gets it from DT, > > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > > >>>> not to involve the driver with setting the frequency. > > >>>> > > >>>> Otherwise we'll make it impossible to support other frequencies, at least > > >>>> without more or less random defaults. > > >>> > > >>> We're running in circles here. > > >>> > > >>> As the driver only supports 24MHz at the moment, the frequency should be > > >>> set by the driver, as it's a driver limitation. We can then work on > > >>> supporting additional frequencies, which will require DT to provide a > > >>> list of supported frequencies for the system, but that can be done on > > >>> top. > > >> > > >> I guess it would be possible to use different external clock frequencies on > > >> a sensor in a given system but that seems to be a bit far fetched, to the > > >> extent I've never seen anyone doing that in practice. > > >> > > >> Originally, the driver set the frequency based on the clock-frequency > > >> property. If we're removing that but use a fixed frequency instead, then > > >> how is that going to work going forward when someone adds support for other > > >> frequencies in the driver and has a system requiring that, while there are > > >> some other platforms relying on the driver setting a particular frequency? > > > > > > The standard property for this is link-frequencies, not clock-frequency. > > > Deprecating clock-frequency now paves the way to use the standard > > > property later when/if someone implements support for additional > > > frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? No. In a general case there is no reliable way to come up with an external clock frequency based on another, different if related, frequency. > > In any case, this policy needs to be carefully documented. I thought after ten or so years this would be already an established practice. :-) I agree it should be documented. We don't seem to have specific documentation for camera sensor drivers at the moment. I can submit a patch... > > > >> Although, if you're saying that this driver only needs to work with DT that > > >> comes with the kernel and you don't care about DT binary compatibility, > > >> this would be fine. > > > > > > I believe this series to not break backward compatibility, as the driver > > > only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. The external clock frequency is significantly lower than the link frequency (usually), but it still comes out of the SoC (or a PMIC chip). The clock signal track on PCB as well as wiring may also be rather long, depending on where the camera sensor is --- quite possibly tens of centimetres. Therefore I wouldn't categorically rule out possible EMC issues with that one either. The bottom line is: use a known-good, safe frequency. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-15 16:27 ` Sakari Ailus 0 siblings, 0 replies; 66+ messages in thread From: Sakari Ailus @ 2020-04-15 16:27 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Laurent, On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the > > >>>>> driver enforce the required clock frequency. > > >>>> > > >>>> Even if some current systems where the driver is used are using 24 MHz > > >>>> clock, that doesn't mean there wouldn't be systems using another frequency > > >>>> that the driver does not support right now. > > >>>> > > >>>> The driver really should not set the frequency unless it gets it from DT, > > >>>> but I think the preferred means is to use assigned-clock-rates instead, and > > >>>> not to involve the driver with setting the frequency. > > >>>> > > >>>> Otherwise we'll make it impossible to support other frequencies, at least > > >>>> without more or less random defaults. > > >>> > > >>> We're running in circles here. > > >>> > > >>> As the driver only supports 24MHz at the moment, the frequency should be > > >>> set by the driver, as it's a driver limitation. We can then work on > > >>> supporting additional frequencies, which will require DT to provide a > > >>> list of supported frequencies for the system, but that can be done on > > >>> top. > > >> > > >> I guess it would be possible to use different external clock frequencies on > > >> a sensor in a given system but that seems to be a bit far fetched, to the > > >> extent I've never seen anyone doing that in practice. > > >> > > >> Originally, the driver set the frequency based on the clock-frequency > > >> property. If we're removing that but use a fixed frequency instead, then > > >> how is that going to work going forward when someone adds support for other > > >> frequencies in the driver and has a system requiring that, while there are > > >> some other platforms relying on the driver setting a particular frequency? > > > > > > The standard property for this is link-frequencies, not clock-frequency. > > > Deprecating clock-frequency now paves the way to use the standard > > > property later when/if someone implements support for additional > > > frequencies. > > > > The external clock frequency and link frequency are different indeed, but > > they are related. The link frequency has been selected in a way that it is > > possible to generate that exact frequency using the chosen external clock > > frequency. If you change the external clock frequency, chances are good > > there is no PLL configuration to generate that link frequency. > > But aren't we supposed to pick the clock frequency based on the link > frequency specified in DT ? No. In a general case there is no reliable way to come up with an external clock frequency based on another, different if related, frequency. > > In any case, this policy needs to be carefully documented. I thought after ten or so years this would be already an established practice. :-) I agree it should be documented. We don't seem to have specific documentation for camera sensor drivers at the moment. I can submit a patch... > > > >> Although, if you're saying that this driver only needs to work with DT that > > >> comes with the kernel and you don't care about DT binary compatibility, > > >> this would be fine. > > > > > > I believe this series to not break backward compatibility, as the driver > > > only works with a 24MHz clock, so I expect all DTs to specify that. > > > > What you're still doing here is defining the DT bindings based on the > > current driver implementation, not the device properties. > > Quite the contrary, the device doesn't require any particular input > clock frequency, so we're removing that from DT :-) Specifying the clock > frequency in DT is in my opinion a manual workaround for not computing > it at runtime based on the desired link frequency, while the link > frequency is a property of the system as it specifies the range of link > frequencies that are safe to use from an EMC point of view. The external clock frequency is significantly lower than the link frequency (usually), but it still comes out of the SoC (or a PMIC chip). The clock signal track on PCB as well as wiring may also be rather long, depending on where the camera sensor is --- quite possibly tens of centimetres. Therefore I wouldn't categorically rule out possible EMC issues with that one either. The bottom line is: use a known-good, safe frequency. -- Regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-15 16:27 ` Sakari Ailus @ 2020-04-17 2:09 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-17 2:09 UTC (permalink / raw) To: Sakari Ailus Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Maxime Ripard Hi Sakari, On Wed, Apr 15, 2020 at 07:27:22PM +0300, Sakari Ailus wrote: > On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > >> On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > >>> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >>>> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>>>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>>>> driver enforce the required clock frequency. > >>>>>> > >>>>>> Even if some current systems where the driver is used are using 24 MHz > >>>>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>>>> that the driver does not support right now. > >>>>>> > >>>>>> The driver really should not set the frequency unless it gets it from DT, > >>>>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>>>> not to involve the driver with setting the frequency. > >>>>>> > >>>>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>>>> without more or less random defaults. > >>>>> > >>>>> We're running in circles here. > >>>>> > >>>>> As the driver only supports 24MHz at the moment, the frequency should be > >>>>> set by the driver, as it's a driver limitation. We can then work on > >>>>> supporting additional frequencies, which will require DT to provide a > >>>>> list of supported frequencies for the system, but that can be done on > >>>>> top. > >>>> > >>>> I guess it would be possible to use different external clock frequencies on > >>>> a sensor in a given system but that seems to be a bit far fetched, to the > >>>> extent I've never seen anyone doing that in practice. > >>>> > >>>> Originally, the driver set the frequency based on the clock-frequency > >>>> property. If we're removing that but use a fixed frequency instead, then > >>>> how is that going to work going forward when someone adds support for other > >>>> frequencies in the driver and has a system requiring that, while there are > >>>> some other platforms relying on the driver setting a particular frequency? > >>> > >>> The standard property for this is link-frequencies, not clock-frequency. > >>> Deprecating clock-frequency now paves the way to use the standard > >>> property later when/if someone implements support for additional > >>> frequencies. > >> > >> The external clock frequency and link frequency are different indeed, but > >> they are related. The link frequency has been selected in a way that it is > >> possible to generate that exact frequency using the chosen external clock > >> frequency. If you change the external clock frequency, chances are good > >> there is no PLL configuration to generate that link frequency. > > > > But aren't we supposed to pick the clock frequency based on the link > > frequency specified in DT ? > > No. In a general case there is no reliable way to come up with an external > clock frequency based on another, different if related, frequency. > > > In any case, this policy needs to be carefully documented. > > I thought after ten or so years this would be already an established > practice. :-) > > I agree it should be documented. We don't seem to have specific > documentation for camera sensor drivers at the moment. I can submit a > patch... > > >>>> Although, if you're saying that this driver only needs to work with DT that > >>>> comes with the kernel and you don't care about DT binary compatibility, > >>>> this would be fine. > >>> > >>> I believe this series to not break backward compatibility, as the driver > >>> only works with a 24MHz clock, so I expect all DTs to specify that. > >> > >> What you're still doing here is defining the DT bindings based on the > >> current driver implementation, not the device properties. > > > > Quite the contrary, the device doesn't require any particular input > > clock frequency, so we're removing that from DT :-) Specifying the clock > > frequency in DT is in my opinion a manual workaround for not computing > > it at runtime based on the desired link frequency, while the link > > frequency is a property of the system as it specifies the range of link > > frequencies that are safe to use from an EMC point of view. > > The external clock frequency is significantly lower than the link frequency > (usually), but it still comes out of the SoC (or a PMIC chip). The clock > signal track on PCB as well as wiring may also be rather long, depending on > where the camera sensor is --- quite possibly tens of centimetres. > Therefore I wouldn't categorically rule out possible EMC issues with that > one either. That's a valid point. > The bottom line is: use a known-good, safe frequency. What if different input clock frequencies are needed to achieve different link frequencies ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-17 2:09 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-17 2:09 UTC (permalink / raw) To: Sakari Ailus Cc: Mark Rutland, devicetree, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Maxime Ripard, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Sakari, On Wed, Apr 15, 2020 at 07:27:22PM +0300, Sakari Ailus wrote: > On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote: > >> On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote: > >>> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote: > >>>> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote: > >>>>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote: > >>>>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > >>>>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > >>>>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > >>>>>>> 24MHz. So instead making clock-frequency as dt-property just let the > >>>>>>> driver enforce the required clock frequency. > >>>>>> > >>>>>> Even if some current systems where the driver is used are using 24 MHz > >>>>>> clock, that doesn't mean there wouldn't be systems using another frequency > >>>>>> that the driver does not support right now. > >>>>>> > >>>>>> The driver really should not set the frequency unless it gets it from DT, > >>>>>> but I think the preferred means is to use assigned-clock-rates instead, and > >>>>>> not to involve the driver with setting the frequency. > >>>>>> > >>>>>> Otherwise we'll make it impossible to support other frequencies, at least > >>>>>> without more or less random defaults. > >>>>> > >>>>> We're running in circles here. > >>>>> > >>>>> As the driver only supports 24MHz at the moment, the frequency should be > >>>>> set by the driver, as it's a driver limitation. We can then work on > >>>>> supporting additional frequencies, which will require DT to provide a > >>>>> list of supported frequencies for the system, but that can be done on > >>>>> top. > >>>> > >>>> I guess it would be possible to use different external clock frequencies on > >>>> a sensor in a given system but that seems to be a bit far fetched, to the > >>>> extent I've never seen anyone doing that in practice. > >>>> > >>>> Originally, the driver set the frequency based on the clock-frequency > >>>> property. If we're removing that but use a fixed frequency instead, then > >>>> how is that going to work going forward when someone adds support for other > >>>> frequencies in the driver and has a system requiring that, while there are > >>>> some other platforms relying on the driver setting a particular frequency? > >>> > >>> The standard property for this is link-frequencies, not clock-frequency. > >>> Deprecating clock-frequency now paves the way to use the standard > >>> property later when/if someone implements support for additional > >>> frequencies. > >> > >> The external clock frequency and link frequency are different indeed, but > >> they are related. The link frequency has been selected in a way that it is > >> possible to generate that exact frequency using the chosen external clock > >> frequency. If you change the external clock frequency, chances are good > >> there is no PLL configuration to generate that link frequency. > > > > But aren't we supposed to pick the clock frequency based on the link > > frequency specified in DT ? > > No. In a general case there is no reliable way to come up with an external > clock frequency based on another, different if related, frequency. > > > In any case, this policy needs to be carefully documented. > > I thought after ten or so years this would be already an established > practice. :-) > > I agree it should be documented. We don't seem to have specific > documentation for camera sensor drivers at the moment. I can submit a > patch... > > >>>> Although, if you're saying that this driver only needs to work with DT that > >>>> comes with the kernel and you don't care about DT binary compatibility, > >>>> this would be fine. > >>> > >>> I believe this series to not break backward compatibility, as the driver > >>> only works with a 24MHz clock, so I expect all DTs to specify that. > >> > >> What you're still doing here is defining the DT bindings based on the > >> current driver implementation, not the device properties. > > > > Quite the contrary, the device doesn't require any particular input > > clock frequency, so we're removing that from DT :-) Specifying the clock > > frequency in DT is in my opinion a manual workaround for not computing > > it at runtime based on the desired link frequency, while the link > > frequency is a property of the system as it specifies the range of link > > frequencies that are safe to use from an EMC point of view. > > The external clock frequency is significantly lower than the link frequency > (usually), but it still comes out of the SoC (or a PMIC chip). The clock > signal track on PCB as well as wiring may also be rather long, depending on > where the camera sensor is --- quite possibly tens of centimetres. > Therefore I wouldn't categorically rule out possible EMC issues with that > one either. That's a valid point. > The bottom line is: use a known-good, safe frequency. What if different input clock frequencies are needed to achieve different link frequencies ? -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 17:34 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:34 UTC (permalink / raw) To: Lad Prabhakar Cc: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index a6c17d15d754..52848fff8a08 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -61,6 +61,8 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +#define OV5645_XVCLK_FREQ 24000000 > + > /* regulator supplies */ > static const char * const ov5645_supply_name[] = { > "vdddo", /* Digital I/O (1.8V) supply */ > @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) > return PTR_ERR(ov5645->xclk); > } > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); > if (ret) { > - dev_err(dev, "could not get xclk frequency\n"); > + dev_err(dev, "could not set xclk frequency\n"); > return ret; > } > - I think you can keep the blank line here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > /* external clock must be 24MHz, allow 1% tolerance */ > + xclk_freq = clk_get_rate(ov5645->xclk); > if (xclk_freq < 23760000 || xclk_freq > 24240000) { > dev_err(dev, "external clock frequency %u is not supported\n", > xclk_freq); > return -EINVAL; > } > > - ret = clk_set_rate(ov5645->xclk, xclk_freq); > - if (ret) { > - dev_err(dev, "could not set xclk frequency\n"); > - return ret; > - } > - > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > ov5645->supplies[i].supply = ov5645_supply_name[i]; > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-06 17:34 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:34 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index a6c17d15d754..52848fff8a08 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -61,6 +61,8 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +#define OV5645_XVCLK_FREQ 24000000 > + > /* regulator supplies */ > static const char * const ov5645_supply_name[] = { > "vdddo", /* Digital I/O (1.8V) supply */ > @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) > return PTR_ERR(ov5645->xclk); > } > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); > if (ret) { > - dev_err(dev, "could not get xclk frequency\n"); > + dev_err(dev, "could not set xclk frequency\n"); > return ret; > } > - I think you can keep the blank line here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > /* external clock must be 24MHz, allow 1% tolerance */ > + xclk_freq = clk_get_rate(ov5645->xclk); > if (xclk_freq < 23760000 || xclk_freq > 24240000) { > dev_err(dev, "external clock frequency %u is not supported\n", > xclk_freq); > return -EINVAL; > } > > - ret = clk_set_rate(ov5645->xclk, xclk_freq); > - if (ret) { > - dev_err(dev, "could not set xclk frequency\n"); > - return ret; > - } > - > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > ov5645->supplies[i].supply = ov5645_supply_name[i]; > -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 17:34 ` Laurent Pinchart @ 2020-04-07 7:36 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:36 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, LAK Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:34 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 14 +++++--------- > > 1 file changed, 5 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index a6c17d15d754..52848fff8a08 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -61,6 +61,8 @@ > > #define OV5645_SDE_SAT_U 0x5583 > > #define OV5645_SDE_SAT_V 0x5584 > > > > +#define OV5645_XVCLK_FREQ 24000000 > > + > > /* regulator supplies */ > > static const char * const ov5645_supply_name[] = { > > "vdddo", /* Digital I/O (1.8V) supply */ > > @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) > > return PTR_ERR(ov5645->xclk); > > } > > > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); > > if (ret) { > > - dev_err(dev, "could not get xclk frequency\n"); > > + dev_err(dev, "could not set xclk frequency\n"); > > return ret; > > } > > - > > I think you can keep the blank line here. > Oops my bad will drop that in next version. Cheers, --Prabhakar > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > /* external clock must be 24MHz, allow 1% tolerance */ > > + xclk_freq = clk_get_rate(ov5645->xclk); > > if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > dev_err(dev, "external clock frequency %u is not supported\n", > > xclk_freq); > > return -EINVAL; > > } > > > > - ret = clk_set_rate(ov5645->xclk, xclk_freq); > > - if (ret) { > > - dev_err(dev, "could not set xclk frequency\n"); > > - return ret; > > - } > > - > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > > ov5645->supplies[i].supply = ov5645_supply_name[i]; > > > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 7:36 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:36 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, LKML, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, LAK, linux-media Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:34 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 14 +++++--------- > > 1 file changed, 5 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index a6c17d15d754..52848fff8a08 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -61,6 +61,8 @@ > > #define OV5645_SDE_SAT_U 0x5583 > > #define OV5645_SDE_SAT_V 0x5584 > > > > +#define OV5645_XVCLK_FREQ 24000000 > > + > > /* regulator supplies */ > > static const char * const ov5645_supply_name[] = { > > "vdddo", /* Digital I/O (1.8V) supply */ > > @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client) > > return PTR_ERR(ov5645->xclk); > > } > > > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ); > > if (ret) { > > - dev_err(dev, "could not get xclk frequency\n"); > > + dev_err(dev, "could not set xclk frequency\n"); > > return ret; > > } > > - > > I think you can keep the blank line here. > Oops my bad will drop that in next version. Cheers, --Prabhakar > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > /* external clock must be 24MHz, allow 1% tolerance */ > > + xclk_freq = clk_get_rate(ov5645->xclk); > > if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > dev_err(dev, "external clock frequency %u is not supported\n", > > xclk_freq); > > return -EINVAL; > > } > > > > - ret = clk_set_rate(ov5645->xclk, xclk_freq); > > - if (ret) { > > - dev_err(dev, "could not set xclk frequency\n"); > > - return ret; > > - } > > - > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > > ov5645->supplies[i].supply = ov5645_supply_name[i]; > > > > -- > Regards, > > Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-07 7:16 ` Geert Uytterhoeven -1 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:16 UTC (permalink / raw) To: Lad Prabhakar Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM, Lad Prabhakar Hi Prabhakar, On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> However, still wondering about the "xvclk" name above and in the definition below. Is this the naming from the datasheet? The DT bindings nor the driver use the "xvclk" naming. > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -61,6 +61,8 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +#define OV5645_XVCLK_FREQ 24000000 > + Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 7:16 ` Geert Uytterhoeven 0 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:16 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Linux Media Mailing List, Linux Kernel Mailing List, Lad Prabhakar, Rob Herring, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, NXP Linux Team Hi Prabhakar, On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > 24MHz. So instead making clock-frequency as dt-property just let the > driver enforce the required clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> However, still wondering about the "xvclk" name above and in the definition below. Is this the naming from the datasheet? The DT bindings nor the driver use the "xvclk" naming. > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -61,6 +61,8 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +#define OV5645_XVCLK_FREQ 24000000 > + Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-07 7:16 ` Geert Uytterhoeven @ 2020-04-07 7:40 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:40 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Lad Prabhakar, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM Hi Geert, Thank you for the review. On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > However, still wondering about the "xvclk" name above and in the definition > below. Is this the naming from the datasheet? > The DT bindings nor the driver use the "xvclk" naming. > xvclk naming is from the datasheet, although the 0v5645 datasheet on publicly available I have referred [1]/[2]. If I am not wrong all the ov sensors have the same naming convention as xvclk. [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf [2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf Cheers, --Prabhakar > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -61,6 +61,8 @@ > > #define OV5645_SDE_SAT_U 0x5583 > > #define OV5645_SDE_SAT_V 0x5584 > > > > +#define OV5645_XVCLK_FREQ 24000000 > > + > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 7:40 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:40 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, Linux Kernel Mailing List, Rob Herring, Linux Media Mailing List, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, NXP Linux Team Hi Geert, Thank you for the review. On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > 24MHz. So instead making clock-frequency as dt-property just let the > > driver enforce the required clock frequency. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > However, still wondering about the "xvclk" name above and in the definition > below. Is this the naming from the datasheet? > The DT bindings nor the driver use the "xvclk" naming. > xvclk naming is from the datasheet, although the 0v5645 datasheet on publicly available I have referred [1]/[2]. If I am not wrong all the ov sensors have the same naming convention as xvclk. [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf [2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf Cheers, --Prabhakar > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -61,6 +61,8 @@ > > #define OV5645_SDE_SAT_U 0x5583 > > #define OV5645_SDE_SAT_V 0x5584 > > > > +#define OV5645_XVCLK_FREQ 24000000 > > + > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property 2020-04-07 7:40 ` Lad, Prabhakar @ 2020-04-07 12:18 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-07 12:18 UTC (permalink / raw) To: Lad, Prabhakar Cc: Geert Uytterhoeven, Lad Prabhakar, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM Hi Prabhakar, On Tue, Apr 07, 2020 at 08:40:06AM +0100, Lad, Prabhakar wrote: > On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven wrote: > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar wrote: > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > driver enforce the required clock frequency. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > However, still wondering about the "xvclk" name above and in the definition > > below. Is this the naming from the datasheet? > > The DT bindings nor the driver use the "xvclk" naming. > > > xvclk naming is from the datasheet, although the 0v5645 datasheet on > publicly available I have referred [1]/[2]. > If I am not wrong all the ov sensors have the same naming convention as xvclk. > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > [2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf The clock in DT should really have been named xvclk, but it's too late to change that. We can follow one of two approaches, either naming everything xclk, and naming everything but the DT property xvclk. Both have pros and cons, feel free to pick your preferred option, but in any case a comment to explain the issue would be useful. > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -61,6 +61,8 @@ > > > #define OV5645_SDE_SAT_U 0x5583 > > > #define OV5645_SDE_SAT_V 0x5584 > > > > > > +#define OV5645_XVCLK_FREQ 24000000 > > > + -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property @ 2020-04-07 12:18 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-07 12:18 UTC (permalink / raw) To: Lad, Prabhakar Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, Linux Kernel Mailing List, Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, Linux Media Mailing List Hi Prabhakar, On Tue, Apr 07, 2020 at 08:40:06AM +0100, Lad, Prabhakar wrote: > On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven wrote: > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar wrote: > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to > > > 24MHz. So instead making clock-frequency as dt-property just let the > > > driver enforce the required clock frequency. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > However, still wondering about the "xvclk" name above and in the definition > > below. Is this the naming from the datasheet? > > The DT bindings nor the driver use the "xvclk" naming. > > > xvclk naming is from the datasheet, although the 0v5645 datasheet on > publicly available I have referred [1]/[2]. > If I am not wrong all the ov sensors have the same naming convention as xvclk. > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > [2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf The clock in DT should really have been named xvclk, but it's too late to change that. We can follow one of two approaches, either naming everything xclk, and naming everything but the DT property xvclk. Both have pros and cons, feel free to pick your preferred option, but in any case a comment to explain the issue would be useful. > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -61,6 +61,8 @@ > > > #define OV5645_SDE_SAT_U 0x5583 > > > #define OV5645_SDE_SAT_V 0x5584 > > > > > > +#define OV5645_XVCLK_FREQ 24000000 > > > + -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:42 ` Lad Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar PLL's on platforms might not be so accurate enough to generate the required clock frequency, so instead of erroring out on xvlck frequency mismatch just warn the user and continue ahead in probe. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/i2c/ov5645.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index 52848fff8a08..314760349adf 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) } /* external clock must be 24MHz, allow 1% tolerance */ xclk_freq = clk_get_rate(ov5645->xclk); - if (xclk_freq < 23760000 || xclk_freq > 24240000) { - dev_err(dev, "external clock frequency %u is not supported\n", - xclk_freq); - return -EINVAL; - } + if (xclk_freq < 23760000 || xclk_freq > 24240000) + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); for (i = 0; i < OV5645_NUM_SUPPLIES; i++) ov5645->supplies[i].supply = ov5645_supply_name[i]; -- 2.20.1 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media PLL's on platforms might not be so accurate enough to generate the required clock frequency, so instead of erroring out on xvlck frequency mismatch just warn the user and continue ahead in probe. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/media/i2c/ov5645.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index 52848fff8a08..314760349adf 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) } /* external clock must be 24MHz, allow 1% tolerance */ xclk_freq = clk_get_rate(ov5645->xclk); - if (xclk_freq < 23760000 || xclk_freq > 24240000) { - dev_err(dev, "external clock frequency %u is not supported\n", - xclk_freq); - return -EINVAL; - } + if (xclk_freq < 23760000 || xclk_freq > 24240000) + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); for (i = 0; i < OV5645_NUM_SUPPLIES; i++) ov5645->supplies[i].supply = ov5645_supply_name[i]; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 17:35 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:35 UTC (permalink / raw) To: Lad Prabhakar Cc: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:39PM +0100, Lad Prabhakar wrote: > PLL's on platforms might not be so accurate enough to generate the > required clock frequency, so instead of erroring out on xvlck frequency > mismatch just warn the user and continue ahead in probe. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/ov5645.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 52848fff8a08..314760349adf 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > } > /* external clock must be 24MHz, allow 1% tolerance */ > xclk_freq = clk_get_rate(ov5645->xclk); > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > - dev_err(dev, "external clock frequency %u is not supported\n", > - xclk_freq); > - return -EINVAL; > - } > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > ov5645->supplies[i].supply = ov5645_supply_name[i]; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch @ 2020-04-06 17:35 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:35 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:39PM +0100, Lad Prabhakar wrote: > PLL's on platforms might not be so accurate enough to generate the > required clock frequency, so instead of erroring out on xvlck frequency > mismatch just warn the user and continue ahead in probe. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/ov5645.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 52848fff8a08..314760349adf 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > } > /* external clock must be 24MHz, allow 1% tolerance */ > xclk_freq = clk_get_rate(ov5645->xclk); > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > - dev_err(dev, "external clock frequency %u is not supported\n", > - xclk_freq); > - return -EINVAL; > - } > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > ov5645->supplies[i].supply = ov5645_supply_name[i]; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-07 7:19 ` Geert Uytterhoeven -1 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:19 UTC (permalink / raw) To: Lad Prabhakar Cc: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM, Lad Prabhakar Hi Prabhakar, On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > PLL's on platforms might not be so accurate enough to generate the > required clock frequency, so instead of erroring out on xvlck frequency xvclk? (but see below) > mismatch just warn the user and continue ahead in probe. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Like for 2/5, what about the xvclk naming? > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > } > /* external clock must be 24MHz, allow 1% tolerance */ > xclk_freq = clk_get_rate(ov5645->xclk); > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > - dev_err(dev, "external clock frequency %u is not supported\n", > - xclk_freq); > - return -EINVAL; > - } > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); Calling it "xvclk" here will confuse the user, as the clock is named "xclk" in DT? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch @ 2020-04-07 7:19 ` Geert Uytterhoeven 0 siblings, 0 replies; 66+ messages in thread From: Geert Uytterhoeven @ 2020-04-07 7:19 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Linux Media Mailing List, Linux Kernel Mailing List, Lad Prabhakar, Rob Herring, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, NXP Linux Team Hi Prabhakar, On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > PLL's on platforms might not be so accurate enough to generate the > required clock frequency, so instead of erroring out on xvlck frequency xvclk? (but see below) > mismatch just warn the user and continue ahead in probe. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Like for 2/5, what about the xvclk naming? > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > } > /* external clock must be 24MHz, allow 1% tolerance */ > xclk_freq = clk_get_rate(ov5645->xclk); > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > - dev_err(dev, "external clock frequency %u is not supported\n", > - xclk_freq); > - return -EINVAL; > - } > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); Calling it "xvclk" here will confuse the user, as the clock is named "xclk" in DT? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch 2020-04-07 7:19 ` Geert Uytterhoeven @ 2020-04-07 7:43 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:43 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Lad Prabhakar, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, Linux Media Mailing List, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM Hi Geert, Thank you for the review. On Tue, Apr 7, 2020 at 8:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > PLL's on platforms might not be so accurate enough to generate the > > required clock frequency, so instead of erroring out on xvlck frequency > > xvclk? (but see below) > > > mismatch just warn the user and continue ahead in probe. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Like for 2/5, what about the xvclk naming? > > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > > } > > /* external clock must be 24MHz, allow 1% tolerance */ > > xclk_freq = clk_get_rate(ov5645->xclk); > > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > - dev_err(dev, "external clock frequency %u is not supported\n", > > - xclk_freq); > > - return -EINVAL; > > - } > > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); > > Calling it "xvclk" here will confuse the user, as the clock is named > "xclk" in DT? > Agreed Ill replace it with xclk in the warning. Cheers, --Prabhakar > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch @ 2020-04-07 7:43 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:43 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, Linux Kernel Mailing List, Rob Herring, Linux Media Mailing List, Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Linux ARM, NXP Linux Team Hi Geert, Thank you for the review. On Tue, Apr 7, 2020 at 8:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > PLL's on platforms might not be so accurate enough to generate the > > required clock frequency, so instead of erroring out on xvlck frequency > > xvclk? (but see below) > > > mismatch just warn the user and continue ahead in probe. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Like for 2/5, what about the xvclk naming? > > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -1103,11 +1103,8 @@ static int ov5645_probe(struct i2c_client *client) > > } > > /* external clock must be 24MHz, allow 1% tolerance */ > > xclk_freq = clk_get_rate(ov5645->xclk); > > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > - dev_err(dev, "external clock frequency %u is not supported\n", > > - xclk_freq); > > - return -EINVAL; > > - } > > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > > + dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n"); > > Calling it "xvclk" here will confuse the user, as the clock is named > "xclk" in DT? > Agreed Ill replace it with xclk in the warning. Cheers, --Prabhakar > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 4/5] ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:42 ` Lad Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar clock-frequency property has been deprecated in ov5645 binding. This patch makes sure it matches the bindings by dropping clock-frequency property from ov5645 node. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi index c070893c509e..fa01cad65335 100644 --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi @@ -126,7 +126,6 @@ reg = <0x3c>; clocks = <&clks IMX6QDL_CLK_CKO2>; clock-names = "xclk"; - clock-frequency = <24000000>; vdddo-supply = <®_1p8v>; vdda-supply = <®_2p8v>; vddd-supply = <®_1p5v>; -- 2.20.1 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 4/5] ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media clock-frequency property has been deprecated in ov5645 binding. This patch makes sure it matches the bindings by dropping clock-frequency property from ov5645 node. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi index c070893c509e..fa01cad65335 100644 --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi @@ -126,7 +126,6 @@ reg = <0x3c>; clocks = <&clks IMX6QDL_CLK_CKO2>; clock-names = "xclk"; - clock-frequency = <24000000>; vdddo-supply = <®_1p8v>; vdda-supply = <®_2p8v>; vddd-supply = <®_1p5v>; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v5 4/5] ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 17:36 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:36 UTC (permalink / raw) To: Lad Prabhakar Cc: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:40PM +0100, Lad Prabhakar wrote: > clock-frequency property has been deprecated in ov5645 binding. This patch > makes sure it matches the bindings by dropping clock-frequency property > from ov5645 node. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> This looks good to me, but could you also address imx6qdl-pico.dtsi ? With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > index c070893c509e..fa01cad65335 100644 > --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > @@ -126,7 +126,6 @@ > reg = <0x3c>; > clocks = <&clks IMX6QDL_CLK_CKO2>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > vdddo-supply = <®_1p8v>; > vdda-supply = <®_2p8v>; > vddd-supply = <®_1p5v>; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 4/5] ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node @ 2020-04-06 17:36 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:36 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:40PM +0100, Lad Prabhakar wrote: > clock-frequency property has been deprecated in ov5645 binding. This patch > makes sure it matches the bindings by dropping clock-frequency property > from ov5645 node. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> This looks good to me, but could you also address imx6qdl-pico.dtsi ? With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > index c070893c509e..fa01cad65335 100644 > --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > @@ -126,7 +126,6 @@ > reg = <0x3c>; > clocks = <&clks IMX6QDL_CLK_CKO2>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > vdddo-supply = <®_1p8v>; > vdda-supply = <®_2p8v>; > vddd-supply = <®_1p5v>; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 16:42 ` Lad Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar, Lad Prabhakar Convert ov5645 bindings to json-schema. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ 2 files changed, 126 insertions(+), 52 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ /dev/null @@ -1,52 +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". -- 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"; - - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml new file mode 100644 index 000000000000..d5cdcf9a1c76 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor + +maintainers: + - Sakari Ailus <sakari.ailus@linux.intel.com> + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> + +description: |- + 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. + +properties: + compatible: + const: ovti,ov5645 + + reg: + description: I2C device address + maxItems: 1 + + clocks: + description: External clock frequency should range between 6MHz to 27MHz. + maxItems: 1 + + clock-names: + items: + - const: xclk + + enable-gpios: + description: |- + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is + physically active low. + + reset-gpios: + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. + + vdddo-supply: + description: Chip digital IO regulator. + + vdda-supply: + description: Chip analog regulator. + + vddd-supply: + description: Chip digital core regulator. + + # See ../video-interfaces.txt for more details + port: + type: object + properties: + endpoint: + type: object + + properties: + data-lanes: + description: |- + The sensor supports either one-lane, or two-lane operation. + For one-lane operation the property must be set to <1> and + for two-lane operation the property must be set to <1 2>. + items: + - const: 1 + - const: 2 + + clock-lanes: + description: + should be set to <0> (clock lane on hardware lane 0). + items: + - const: 0 + + remote-endpoint: true + + required: + - data-lanes + - clock-lanes + - remote-endpoint + + additionalProperties: false + + additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - enable-gpios + - reset-gpios + - vdddo-supply + - vdda-supply + - vddd-supply + - port + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov5645: sensor@3c { + compatible = "ovti,ov5645"; + reg = <0x3c>; + clocks = <&ov5645_cl>; + clock-names = "xclk"; + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; + vdddo-supply = <&camera_dovdd_1v8>; + vdda-supply = <&camera_avdd_2v8>; + vddd-supply = <&camera_dvdd_1v2>; + + port { + ov5645_0: endpoint { + remote-endpoint = <&csi1_ep>; + clock-lanes = <0>; + data-lanes = <1 2>; + }; + }; + }; + }; + +... -- 2.20.1 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema @ 2020-04-06 16:42 ` Lad Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad Prabhakar @ 2020-04-06 16:42 UTC (permalink / raw) To: Laurent Pinchart, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham Cc: devicetree, Geert Uytterhoeven, linux-kernel, Lad Prabhakar, Lad Prabhakar, linux-arm-kernel, linux-media Convert ov5645 bindings to json-schema. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ 2 files changed, 126 insertions(+), 52 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ /dev/null @@ -1,52 +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". -- 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"; - - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml new file mode 100644 index 000000000000..d5cdcf9a1c76 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml @@ -0,0 +1,126 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor + +maintainers: + - Sakari Ailus <sakari.ailus@linux.intel.com> + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> + +description: |- + 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. + +properties: + compatible: + const: ovti,ov5645 + + reg: + description: I2C device address + maxItems: 1 + + clocks: + description: External clock frequency should range between 6MHz to 27MHz. + maxItems: 1 + + clock-names: + items: + - const: xclk + + enable-gpios: + description: |- + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is + physically active low. + + reset-gpios: + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. + + vdddo-supply: + description: Chip digital IO regulator. + + vdda-supply: + description: Chip analog regulator. + + vddd-supply: + description: Chip digital core regulator. + + # See ../video-interfaces.txt for more details + port: + type: object + properties: + endpoint: + type: object + + properties: + data-lanes: + description: |- + The sensor supports either one-lane, or two-lane operation. + For one-lane operation the property must be set to <1> and + for two-lane operation the property must be set to <1 2>. + items: + - const: 1 + - const: 2 + + clock-lanes: + description: + should be set to <0> (clock lane on hardware lane 0). + items: + - const: 0 + + remote-endpoint: true + + required: + - data-lanes + - clock-lanes + - remote-endpoint + + additionalProperties: false + + additionalProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - enable-gpios + - reset-gpios + - vdddo-supply + - vdda-supply + - vddd-supply + - port + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov5645: sensor@3c { + compatible = "ovti,ov5645"; + reg = <0x3c>; + clocks = <&ov5645_cl>; + clock-names = "xclk"; + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; + vdddo-supply = <&camera_dovdd_1v8>; + vdda-supply = <&camera_avdd_2v8>; + vddd-supply = <&camera_dvdd_1v2>; + + port { + ov5645_0: endpoint { + remote-endpoint = <&csi1_ep>; + clock-lanes = <0>; + data-lanes = <1 2>; + }; + }; + }; + }; + +... -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema 2020-04-06 16:42 ` Lad Prabhakar @ 2020-04-06 17:43 ` Laurent Pinchart -1 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:43 UTC (permalink / raw) To: Lad Prabhakar Cc: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > Convert ov5645 bindings to json-schema. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > 2 files changed, 126 insertions(+), 52 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ /dev/null > @@ -1,52 +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". > -- 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"; > - > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > new file mode 100644 > index 000000000000..d5cdcf9a1c76 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > @@ -0,0 +1,126 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > + > +maintainers: > + - Sakari Ailus <sakari.ailus@linux.intel.com> > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > + > +description: |- > + 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. > + > +properties: > + compatible: > + const: ovti,ov5645 > + > + reg: > + description: I2C device address > + maxItems: 1 > + > + clocks: > + description: External clock frequency should range between 6MHz to 27MHz. > + maxItems: 1 > + > + clock-names: > + items: > + - const: xclk > + > + enable-gpios: > + description: |- > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > + physically active low. > + > + reset-gpios: > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > + > + vdddo-supply: > + description: Chip digital IO regulator. > + > + vdda-supply: > + description: Chip analog regulator. > + > + vddd-supply: > + description: Chip digital core regulator. > + > + # See ../video-interfaces.txt for more details > + port: > + type: object > + properties: > + endpoint: > + type: object > + > + properties: > + data-lanes: Don't you need minItems: 1 maxItems: 2 here ? > + description: |- > + The sensor supports either one-lane, or two-lane operation. > + For one-lane operation the property must be set to <1> and > + for two-lane operation the property must be set to <1 2>. > + items: > + - const: 1 > + - const: 2 > + > + clock-lanes: Same here, maxItems: 1 ? > + description: > + should be set to <0> (clock lane on hardware lane 0). I think you can drop the description, with the items below it's clear that the value has to be <0>. > + items: > + - const: 0 > + > + remote-endpoint: true Should this be remote-endpoint: $ref: /schemas/types.yaml#/definitions/phandle > + > + required: > + - data-lanes > + - clock-lanes > + - remote-endpoint > + > + additionalProperties: false > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - enable-gpios > + - reset-gpios > + - vdddo-supply > + - vdda-supply > + - vddd-supply > + - port > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov5645: sensor@3c { > + compatible = "ovti,ov5645"; > + reg = <0x3c>; > + clocks = <&ov5645_cl>; > + clock-names = "xclk"; > + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; > + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; > + vdddo-supply = <&camera_dovdd_1v8>; > + vdda-supply = <&camera_avdd_2v8>; > + vddd-supply = <&camera_dvdd_1v2>; > + > + port { > + ov5645_0: endpoint { > + remote-endpoint = <&csi1_ep>; > + clock-lanes = <0>; > + data-lanes = <1 2>; > + }; > + }; > + }; > + }; > + > +... -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema @ 2020-04-06 17:43 ` Laurent Pinchart 0 siblings, 0 replies; 66+ messages in thread From: Laurent Pinchart @ 2020-04-06 17:43 UTC (permalink / raw) To: Lad Prabhakar Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, linux-kernel, Lad Prabhakar, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media Hi Prabhakar, Thank you for the patch. On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > Convert ov5645 bindings to json-schema. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > 2 files changed, 126 insertions(+), 52 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ /dev/null > @@ -1,52 +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". > -- 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"; > - > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > new file mode 100644 > index 000000000000..d5cdcf9a1c76 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > @@ -0,0 +1,126 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > + > +maintainers: > + - Sakari Ailus <sakari.ailus@linux.intel.com> > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > + > +description: |- > + 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. > + > +properties: > + compatible: > + const: ovti,ov5645 > + > + reg: > + description: I2C device address > + maxItems: 1 > + > + clocks: > + description: External clock frequency should range between 6MHz to 27MHz. > + maxItems: 1 > + > + clock-names: > + items: > + - const: xclk > + > + enable-gpios: > + description: |- > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > + physically active low. > + > + reset-gpios: > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > + > + vdddo-supply: > + description: Chip digital IO regulator. > + > + vdda-supply: > + description: Chip analog regulator. > + > + vddd-supply: > + description: Chip digital core regulator. > + > + # See ../video-interfaces.txt for more details > + port: > + type: object > + properties: > + endpoint: > + type: object > + > + properties: > + data-lanes: Don't you need minItems: 1 maxItems: 2 here ? > + description: |- > + The sensor supports either one-lane, or two-lane operation. > + For one-lane operation the property must be set to <1> and > + for two-lane operation the property must be set to <1 2>. > + items: > + - const: 1 > + - const: 2 > + > + clock-lanes: Same here, maxItems: 1 ? > + description: > + should be set to <0> (clock lane on hardware lane 0). I think you can drop the description, with the items below it's clear that the value has to be <0>. > + items: > + - const: 0 > + > + remote-endpoint: true Should this be remote-endpoint: $ref: /schemas/types.yaml#/definitions/phandle > + > + required: > + - data-lanes > + - clock-lanes > + - remote-endpoint > + > + additionalProperties: false > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - enable-gpios > + - reset-gpios > + - vdddo-supply > + - vdda-supply > + - vddd-supply > + - port > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov5645: sensor@3c { > + compatible = "ovti,ov5645"; > + reg = <0x3c>; > + clocks = <&ov5645_cl>; > + clock-names = "xclk"; > + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; > + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; > + vdddo-supply = <&camera_dovdd_1v8>; > + vdda-supply = <&camera_avdd_2v8>; > + vddd-supply = <&camera_dvdd_1v2>; > + > + port { > + ov5645_0: endpoint { > + remote-endpoint = <&csi1_ep>; > + clock-lanes = <0>; > + data-lanes = <1 2>; > + }; > + }; > + }; > + }; > + > +... -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema 2020-04-06 17:43 ` Laurent Pinchart @ 2020-04-07 7:46 ` Lad, Prabhakar -1 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:46 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, LAK Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:43 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > > Convert ov5645 bindings to json-schema. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > > 2 files changed, 126 insertions(+), 52 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ /dev/null > > @@ -1,52 +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". > > -- 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"; > > - > > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > new file mode 100644 > > index 000000000000..d5cdcf9a1c76 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > @@ -0,0 +1,126 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > > + > > +maintainers: > > + - Sakari Ailus <sakari.ailus@linux.intel.com> > > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + > > +description: |- > > + 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. > > + > > +properties: > > + compatible: > > + const: ovti,ov5645 > > + > > + reg: > > + description: I2C device address > > + maxItems: 1 > > + > > + clocks: > > + description: External clock frequency should range between 6MHz to 27MHz. > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: xclk > > + > > + enable-gpios: > > + description: |- > > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > > + physically active low. > > + > > + reset-gpios: > > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > > + > > + vdddo-supply: > > + description: Chip digital IO regulator. > > + > > + vdda-supply: > > + description: Chip analog regulator. > > + > > + vddd-supply: > > + description: Chip digital core regulator. > > + > > + # See ../video-interfaces.txt for more details > > + port: > > + type: object > > + properties: > > + endpoint: > > + type: object > > + > > + properties: > > + data-lanes: > > Don't you need > > minItems: 1 > maxItems: 2 > > here ? > Agreed will add that. > > + description: |- > > + The sensor supports either one-lane, or two-lane operation. > > + For one-lane operation the property must be set to <1> and > > + for two-lane operation the property must be set to <1 2>. > > + items: > > + - const: 1 > > + - const: 2 > > + > > + clock-lanes: > > Same here, > > maxItems: 1 > > ? > As above. > > + description: > > + should be set to <0> (clock lane on hardware lane 0). > > I think you can drop the description, with the items below it's clear > that the value has to be <0>. > OK > > + items: > > + - const: 0 > > + > > + remote-endpoint: true > > Should this be > > remote-endpoint: > $ref: /schemas/types.yaml#/definitions/phandle > OK shall replace it. Cheers, --Prabhakar > > + > > + required: > > + - data-lanes > > + - clock-lanes > > + - remote-endpoint > > + > > + additionalProperties: false > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - enable-gpios > > + - reset-gpios > > + - vdddo-supply > > + - vdda-supply > > + - vddd-supply > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov5645: sensor@3c { > > + compatible = "ovti,ov5645"; > > + reg = <0x3c>; > > + clocks = <&ov5645_cl>; > > + clock-names = "xclk"; > > + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; > > + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; > > + vdddo-supply = <&camera_dovdd_1v8>; > > + vdda-supply = <&camera_avdd_2v8>; > > + vddd-supply = <&camera_dvdd_1v2>; > > + > > + port { > > + ov5645_0: endpoint { > > + remote-endpoint = <&csi1_ep>; > > + clock-lanes = <0>; > > + data-lanes = <1 2>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema @ 2020-04-07 7:46 ` Lad, Prabhakar 0 siblings, 0 replies; 66+ messages in thread From: Lad, Prabhakar @ 2020-04-07 7:46 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Kieran Bingham, Lad Prabhakar, LKML, Rob Herring, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, LAK, linux-media Hi Laurent, Thank you for the review. On Mon, Apr 6, 2020 at 6:43 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > > Convert ov5645 bindings to json-schema. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > > 2 files changed, 126 insertions(+), 52 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ /dev/null > > @@ -1,52 +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". > > -- 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"; > > - > > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > new file mode 100644 > > index 000000000000..d5cdcf9a1c76 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > @@ -0,0 +1,126 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > > + > > +maintainers: > > + - Sakari Ailus <sakari.ailus@linux.intel.com> > > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + > > +description: |- > > + 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. > > + > > +properties: > > + compatible: > > + const: ovti,ov5645 > > + > > + reg: > > + description: I2C device address > > + maxItems: 1 > > + > > + clocks: > > + description: External clock frequency should range between 6MHz to 27MHz. > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: xclk > > + > > + enable-gpios: > > + description: |- > > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > > + physically active low. > > + > > + reset-gpios: > > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > > + > > + vdddo-supply: > > + description: Chip digital IO regulator. > > + > > + vdda-supply: > > + description: Chip analog regulator. > > + > > + vddd-supply: > > + description: Chip digital core regulator. > > + > > + # See ../video-interfaces.txt for more details > > + port: > > + type: object > > + properties: > > + endpoint: > > + type: object > > + > > + properties: > > + data-lanes: > > Don't you need > > minItems: 1 > maxItems: 2 > > here ? > Agreed will add that. > > + description: |- > > + The sensor supports either one-lane, or two-lane operation. > > + For one-lane operation the property must be set to <1> and > > + for two-lane operation the property must be set to <1 2>. > > + items: > > + - const: 1 > > + - const: 2 > > + > > + clock-lanes: > > Same here, > > maxItems: 1 > > ? > As above. > > + description: > > + should be set to <0> (clock lane on hardware lane 0). > > I think you can drop the description, with the items below it's clear > that the value has to be <0>. > OK > > + items: > > + - const: 0 > > + > > + remote-endpoint: true > > Should this be > > remote-endpoint: > $ref: /schemas/types.yaml#/definitions/phandle > OK shall replace it. Cheers, --Prabhakar > > + > > + required: > > + - data-lanes > > + - clock-lanes > > + - remote-endpoint > > + > > + additionalProperties: false > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - enable-gpios > > + - reset-gpios > > + - vdddo-supply > > + - vdda-supply > > + - vddd-supply > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov5645: sensor@3c { > > + compatible = "ovti,ov5645"; > > + reg = <0x3c>; > > + clocks = <&ov5645_cl>; > > + clock-names = "xclk"; > > + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>; > > + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>; > > + vdddo-supply = <&camera_dovdd_1v8>; > > + vdda-supply = <&camera_avdd_2v8>; > > + vddd-supply = <&camera_dvdd_1v2>; > > + > > + port { > > + ov5645_0: endpoint { > > + remote-endpoint = <&csi1_ep>; > > + clock-lanes = <0>; > > + data-lanes = <1 2>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > -- > Regards, > > Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema 2020-04-06 17:43 ` Laurent Pinchart @ 2020-04-15 14:43 ` Rob Herring -1 siblings, 0 replies; 66+ messages in thread From: Rob Herring @ 2020-04-15 14:43 UTC (permalink / raw) To: Laurent Pinchart Cc: Lad Prabhakar, Sakari Ailus, Mauro Carvalho Chehab, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Kieran Bingham, Geert Uytterhoeven, linux-media, devicetree, linux-kernel, linux-arm-kernel, Lad Prabhakar On Mon, Apr 06, 2020 at 08:43:46PM +0300, Laurent Pinchart wrote: > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > > Convert ov5645 bindings to json-schema. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > > 2 files changed, 126 insertions(+), 52 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ /dev/null > > @@ -1,52 +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". > > -- 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"; > > - > > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > new file mode 100644 > > index 000000000000..d5cdcf9a1c76 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > @@ -0,0 +1,126 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > > + > > +maintainers: > > + - Sakari Ailus <sakari.ailus@linux.intel.com> > > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + > > +description: |- > > + 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. > > + > > +properties: > > + compatible: > > + const: ovti,ov5645 > > + > > + reg: > > + description: I2C device address > > + maxItems: 1 > > + > > + clocks: > > + description: External clock frequency should range between 6MHz to 27MHz. > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: xclk > > + > > + enable-gpios: > > + description: |- > > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > > + physically active low. > > + > > + reset-gpios: > > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > > + > > + vdddo-supply: > > + description: Chip digital IO regulator. > > + > > + vdda-supply: > > + description: Chip analog regulator. > > + > > + vddd-supply: > > + description: Chip digital core regulator. > > + > > + # See ../video-interfaces.txt for more details > > + port: > > + type: object > > + properties: > > + endpoint: > > + type: object > > + > > + properties: > > + data-lanes: > > Don't you need > > minItems: 1 > maxItems: 2 > > here ? Yes. > > + description: |- > > + The sensor supports either one-lane, or two-lane operation. > > + For one-lane operation the property must be set to <1> and > > + for two-lane operation the property must be set to <1 2>. > > + items: > > + - const: 1 > > + - const: 2 > > + > > + clock-lanes: > > Same here, > > maxItems: 1 > > ? No, because the default is number of entries in 'items'. > > > + description: > > + should be set to <0> (clock lane on hardware lane 0). > > I think you can drop the description, with the items below it's clear > that the value has to be <0>. > > > + items: > > + - const: 0 > > + > > + remote-endpoint: true > > Should this be > > remote-endpoint: > $ref: /schemas/types.yaml#/definitions/phandle No, we can assume this has a type definition already. Rob ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema @ 2020-04-15 14:43 ` Rob Herring 0 siblings, 0 replies; 66+ messages in thread From: Rob Herring @ 2020-04-15 14:43 UTC (permalink / raw) To: Laurent Pinchart Cc: Mark Rutland, devicetree, Pengutronix Kernel Team, Geert Uytterhoeven, Fabio Estevam, Sascha Hauer, Lad Prabhakar, linux-kernel, Lad Prabhakar, Kieran Bingham, NXP Linux Team, Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media On Mon, Apr 06, 2020 at 08:43:46PM +0300, Laurent Pinchart wrote: > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote: > > Convert ov5645 bindings to json-schema. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../devicetree/bindings/media/i2c/ov5645.txt | 52 -------- > > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++ > > 2 files changed, 126 insertions(+), 52 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > > create mode 100644 Documentation/devicetree/bindings/media/i2c/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 a55bb728ea48..000000000000 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ /dev/null > > @@ -1,52 +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". > > -- 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"; > > - > > - 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/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > new file mode 100644 > > index 000000000000..d5cdcf9a1c76 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml > > @@ -0,0 +1,126 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor > > + > > +maintainers: > > + - Sakari Ailus <sakari.ailus@linux.intel.com> > > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + > > +description: |- > > + 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. > > + > > +properties: > > + compatible: > > + const: ovti,ov5645 > > + > > + reg: > > + description: I2C device address > > + maxItems: 1 > > + > > + clocks: > > + description: External clock frequency should range between 6MHz to 27MHz. > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: xclk > > + > > + enable-gpios: > > + description: |- > > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is > > + physically active low. > > + > > + reset-gpios: > > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB. > > + > > + vdddo-supply: > > + description: Chip digital IO regulator. > > + > > + vdda-supply: > > + description: Chip analog regulator. > > + > > + vddd-supply: > > + description: Chip digital core regulator. > > + > > + # See ../video-interfaces.txt for more details > > + port: > > + type: object > > + properties: > > + endpoint: > > + type: object > > + > > + properties: > > + data-lanes: > > Don't you need > > minItems: 1 > maxItems: 2 > > here ? Yes. > > + description: |- > > + The sensor supports either one-lane, or two-lane operation. > > + For one-lane operation the property must be set to <1> and > > + for two-lane operation the property must be set to <1 2>. > > + items: > > + - const: 1 > > + - const: 2 > > + > > + clock-lanes: > > Same here, > > maxItems: 1 > > ? No, because the default is number of entries in 'items'. > > > + description: > > + should be set to <0> (clock lane on hardware lane 0). > > I think you can drop the description, with the items below it's clear > that the value has to be <0>. > > > + items: > > + - const: 0 > > + > > + remote-endpoint: true > > Should this be > > remote-endpoint: > $ref: /schemas/types.yaml#/definitions/phandle No, we can assume this has a type definition already. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 66+ messages in thread
end of thread, other threads:[~2020-04-17 2:10 UTC | newest] Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-06 16:42 [PATCH v5 0/5] ov5645: Deprecate usage of the clock-frequency Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 16:42 ` [PATCH v5 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 17:30 ` Laurent Pinchart 2020-04-06 17:30 ` Laurent Pinchart 2020-04-07 7:35 ` Lad, Prabhakar 2020-04-07 7:35 ` Lad, Prabhakar 2020-04-07 7:14 ` Geert Uytterhoeven 2020-04-07 7:14 ` Geert Uytterhoeven 2020-04-14 13:55 ` Rob Herring 2020-04-14 13:55 ` Rob Herring 2020-04-06 16:42 ` [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 16:51 ` Sakari Ailus 2020-04-06 16:51 ` Sakari Ailus 2020-04-06 17:11 ` Lad, Prabhakar 2020-04-06 17:11 ` Lad, Prabhakar 2020-04-06 17:32 ` Laurent Pinchart 2020-04-06 17:32 ` Laurent Pinchart 2020-04-07 6:22 ` Sakari Ailus 2020-04-07 6:22 ` Sakari Ailus 2020-04-07 12:21 ` Laurent Pinchart 2020-04-07 12:21 ` Laurent Pinchart 2020-04-07 15:14 ` Sakari Ailus 2020-04-07 15:14 ` Sakari Ailus 2020-04-14 20:55 ` Laurent Pinchart 2020-04-14 20:55 ` Laurent Pinchart 2020-04-14 20:56 ` Laurent Pinchart 2020-04-14 20:56 ` Laurent Pinchart 2020-04-15 8:19 ` Maxime Ripard 2020-04-15 8:19 ` Maxime Ripard 2020-04-15 16:27 ` Sakari Ailus 2020-04-15 16:27 ` Sakari Ailus 2020-04-17 2:09 ` Laurent Pinchart 2020-04-17 2:09 ` Laurent Pinchart 2020-04-06 17:34 ` Laurent Pinchart 2020-04-06 17:34 ` Laurent Pinchart 2020-04-07 7:36 ` Lad, Prabhakar 2020-04-07 7:36 ` Lad, Prabhakar 2020-04-07 7:16 ` Geert Uytterhoeven 2020-04-07 7:16 ` Geert Uytterhoeven 2020-04-07 7:40 ` Lad, Prabhakar 2020-04-07 7:40 ` Lad, Prabhakar 2020-04-07 12:18 ` Laurent Pinchart 2020-04-07 12:18 ` Laurent Pinchart 2020-04-06 16:42 ` [PATCH v5 3/5] media: i2c: ov5645: Turn probe error into warning for xvclk frequency mismatch Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 17:35 ` Laurent Pinchart 2020-04-06 17:35 ` Laurent Pinchart 2020-04-07 7:19 ` Geert Uytterhoeven 2020-04-07 7:19 ` Geert Uytterhoeven 2020-04-07 7:43 ` Lad, Prabhakar 2020-04-07 7:43 ` Lad, Prabhakar 2020-04-06 16:42 ` [PATCH v5 4/5] ARM: dts: imx6qdl-wandboard: Drop clock-frequency property from ov5645 node Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 17:36 ` Laurent Pinchart 2020-04-06 17:36 ` Laurent Pinchart 2020-04-06 16:42 ` [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema Lad Prabhakar 2020-04-06 16:42 ` Lad Prabhakar 2020-04-06 17:43 ` Laurent Pinchart 2020-04-06 17:43 ` Laurent Pinchart 2020-04-07 7:46 ` Lad, Prabhakar 2020-04-07 7:46 ` Lad, Prabhakar 2020-04-15 14:43 ` Rob Herring 2020-04-15 14:43 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.