| Message ID | 1584133954-6953-2-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ov5645: Switch to assigned-clock-rates | expand |
Hi Prabhakar, Thank you for the patch. On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > Use assigned-clock-rates to specify the clock rate. Also mark > clock-frequency property as deprecated. I would phrase it the other way around, this patch mainly deprecates clock-frequency, and as a side effect recommends usage of assigned-clock-rates. "Deprecate usage of the clock-frequency propertly. The preferred method to set clock rates is to use assigned-clock-rates." > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > index 72ad992..e62fe82 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > @@ -8,7 +8,7 @@ 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. > +- clock-frequency (deprecated): Frequency of the xclk clock. I would drop this completely. Drivers need to ensure backward compatibility, but DT bindings should only document the latest version, the history is available in git. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> While at it, can I enlist you to convert these bindings to yaml ? :-) > - 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 +37,8 @@ Example: > > clocks = <&clks 200>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > + assigned-clocks = <&clks 200>; > + assigned-clock-rates = <24000000>; > > vdddo-supply = <&camera_dovdd_1v8>; > vdda-supply = <&camera_avdd_2v8>;
Hi Laurent, Thank you for the quick review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 13 March 2020 21:20 > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; > Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari > Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>; > Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia > <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux- > media@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to > assigned-clock-rates > > Hi Prabhakar, > > Thank you for the patch. > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > Use assigned-clock-rates to specify the clock rate. Also mark > > clock-frequency property as deprecated. > > I would phrase it the other way around, this patch mainly deprecates clock- > frequency, and as a side effect recommends usage of assigned-clock-rates. > > "Deprecate usage of the clock-frequency propertly. The preferred method > to set clock rates is to use assigned-clock-rates." > Agreed will do that. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > lad.rj@bp.renesas.com> > > --- > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > index 72ad992..e62fe82 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > @@ -8,7 +8,7 @@ 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. > > +- clock-frequency (deprecated): Frequency of the xclk clock. > > I would drop this completely. Drivers need to ensure backward compatibility, > but DT bindings should only document the latest version, the history is > available in git. > Sure will drop it. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > While at it, can I enlist you to convert these bindings to yaml ? :-) > Sure will do the honours
Hi Prabhakar, On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote: > On Sent: 13 March 2020 21:20, Laurent Pinchart wrote: > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > clock-frequency property as deprecated. > > > > I would phrase it the other way around, this patch mainly deprecates clock- > > frequency, and as a side effect recommends usage of assigned-clock-rates. > > > > "Deprecate usage of the clock-frequency propertly. The preferred method > > to set clock rates is to use assigned-clock-rates." > > Agreed will do that. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > index 72ad992..e62fe82 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > @@ -8,7 +8,7 @@ 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. > > > +- clock-frequency (deprecated): Frequency of the xclk clock. > > > > I would drop this completely. Drivers need to ensure backward compatibility, > > but DT bindings should only document the latest version, the history is > > available in git. > > > Sure will drop it. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > While at it, can I enlist you to convert these bindings to yaml ? :-) > > > Sure will do the honours
Hi Laurent, On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote: > > On Sent: 13 March 2020 21:20, Laurent Pinchart wrote: > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > > clock-frequency property as deprecated. > > > > > > I would phrase it the other way around, this patch mainly deprecates clock- > > > frequency, and as a side effect recommends usage of assigned-clock-rates. > > > > > > "Deprecate usage of the clock-frequency propertly. The preferred method > > > to set clock rates is to use assigned-clock-rates." > > > > Agreed will do that. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > index 72ad992..e62fe82 100644 > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > @@ -8,7 +8,7 @@ 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. > > > > +- clock-frequency (deprecated): Frequency of the xclk clock. > > > > > > I would drop this completely. Drivers need to ensure backward compatibility, > > > but DT bindings should only document the latest version, the history is > > > available in git. > > > > > Sure will drop it. > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > While at it, can I enlist you to convert these bindings to yaml ? :-) > > > > > Sure will do the honours , will make sure yaml patch is ontop of this patch too. > Shall I enlist you as the maintainer in the json-schema ? dt_binding_check says 'maintainers' is a required property. Cheers, --Prabhakar Lad > Thank you :-) > > > > > - 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 +37,8 @@ Example: > > > > > > > > clocks = <&clks 200>; > > > > clock-names = "xclk"; > > > > -clock-frequency = <24000000>; > > > > +assigned-clocks = <&clks 200>; > > > > +assigned-clock-rates = <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
Hi Prabhakar, On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote: > On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote: > > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote: > >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote: > >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > >>>> Use assigned-clock-rates to specify the clock rate. Also mark > >>>> clock-frequency property as deprecated. > >>> > >>> I would phrase it the other way around, this patch mainly deprecates clock- > >>> frequency, and as a side effect recommends usage of assigned-clock-rates. > >>> > >>> "Deprecate usage of the clock-frequency propertly. The preferred method > >>> to set clock rates is to use assigned-clock-rates." > >> > >> Agreed will do that. > >> > >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>> --- > >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > >>>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > >>>> index 72ad992..e62fe82 100644 > >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > >>>> @@ -8,7 +8,7 @@ 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. > >>>> +- clock-frequency (deprecated): Frequency of the xclk clock. > >>> > >>> I would drop this completely. Drivers need to ensure backward compatibility, > >>> but DT bindings should only document the latest version, the history is > >>> available in git. > >> > >> Sure will drop it. > >> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>> While at it, can I enlist you to convert these bindings to yaml ? :-) > >>> > >> Sure will do the honours , will make sure yaml patch is ontop of this patch too. > > Shall I enlist you as the maintainer in the json-schema ? > dt_binding_check says 'maintainers' is a required property. Do you want to be the new maintainer ? :-) Sakari is maintaining sensor drivers (in his spare time though) so maybe he could be listed in the DT bindings too if he wants. Otherwise, I could do it. > > Thank you :-) > > > >>>> - 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 +37,8 @@ Example: > >>>> > >>>> clocks = <&clks 200>; > >>>> clock-names = "xclk"; > >>>> -clock-frequency = <24000000>; > >>>> +assigned-clocks = <&clks 200>; > >>>> +assigned-clock-rates = <24000000>; > >>>> > >>>> vdddo-supply = <&camera_dovdd_1v8>; > >>>> vdda-supply = <&camera_avdd_2v8>;
Hi Laurent, On Wed, Mar 18, 2020 at 10:33 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote: > > On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote: > > > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote: > > >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote: > > >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > >>>> Use assigned-clock-rates to specify the clock rate. Also mark > > >>>> clock-frequency property as deprecated. > > >>> > > >>> I would phrase it the other way around, this patch mainly deprecates clock- > > >>> frequency, and as a side effect recommends usage of assigned-clock-rates. > > >>> > > >>> "Deprecate usage of the clock-frequency propertly. The preferred method > > >>> to set clock rates is to use assigned-clock-rates." > > >> > > >> Agreed will do that. > > >> > > >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > >>>> --- > > >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > >>>> 1 file changed, 3 insertions(+), 2 deletions(-) > > >>>> > > >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > >>>> index 72ad992..e62fe82 100644 > > >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > >>>> @@ -8,7 +8,7 @@ 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. > > >>>> +- clock-frequency (deprecated): Frequency of the xclk clock. > > >>> > > >>> I would drop this completely. Drivers need to ensure backward compatibility, > > >>> but DT bindings should only document the latest version, the history is > > >>> available in git. > > >> > > >> Sure will drop it. > > >> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>> > > >>> While at it, can I enlist you to convert these bindings to yaml ? :-) > > >>> > > >> Sure will do the honours , will make sure yaml patch is ontop of this patch too. > > > > Shall I enlist you as the maintainer in the json-schema ? > > dt_binding_check says 'maintainers' is a required property. > > Do you want to be the new maintainer ? :-) Sakari is maintaining sensor > drivers (in his spare time though) so maybe he could be listed in the DT > bindings too if he wants. Otherwise, I could do it. > OK I will add myself and Sakari for now and post a v4. Cheers, --Prabhakar > > > Thank you :-) > > > > > >>>> - 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 +37,8 @@ Example: > > >>>> > > >>>> clocks = <&clks 200>; > > >>>> clock-names = "xclk"; > > >>>> -clock-frequency = <24000000>; > > >>>> +assigned-clocks = <&clks 200>; > > >>>> +assigned-clock-rates = <24000000>; > > >>>> > > >>>> vdddo-supply = <&camera_dovdd_1v8>; > > >>>> vdda-supply = <&camera_avdd_2v8>; > > -- > Regards, > > Laurent Pinchart
Hi, On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > Use assigned-clock-rates to specify the clock rate. Also mark > clock-frequency property as deprecated. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > index 72ad992..e62fe82 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > @@ -8,7 +8,7 @@ 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. > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > clocks = <&clks 200>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > + assigned-clocks = <&clks 200>; > + assigned-clock-rates = <24000000>; > > vdddo-supply = <&camera_dovdd_1v8>; > vdda-supply = <&camera_avdd_2v8>; clock-frequency is quite different from assigned-clock-rates though, semantically speaking. clock-frequency is only about what the clock frequency is, while assigned-clock-rates will change the rate as well, and you have no idea how long it will last. If you want to retrieve that through the clock framework, then just making clock-frequency optional is enough and falling back to clk_get_rate on the clocks property already provided is enough. Maxime
Hi Maxime, On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > Use assigned-clock-rates to specify the clock rate. Also mark > > clock-frequency property as deprecated. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > index 72ad992..e62fe82 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > @@ -8,7 +8,7 @@ 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. > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > clocks = <&clks 200>; > > clock-names = "xclk"; > > - clock-frequency = <24000000>; > > + assigned-clocks = <&clks 200>; > > + assigned-clock-rates = <24000000>; > > > > vdddo-supply = <&camera_dovdd_1v8>; > > vdda-supply = <&camera_avdd_2v8>; > > clock-frequency is quite different from assigned-clock-rates though, > semantically speaking. clock-frequency is only about what the clock > frequency is, while assigned-clock-rates will change the rate as well, > and you have no idea how long it will last. The driver currently reads the clock-frequency property and then calls clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I think it's less of a hack than what we currently have. As discussed on IRC, maybe the best option in this specific case is to drop clock-frequency and assigned-clok-rates, and call clk_set_rate() with a hardcoded frequency of 24MHz in the driver, as that's the only frequency the driver supports. > If you want to retrieve that through the clock framework, then just > making clock-frequency optional is enough and falling back to > clk_get_rate on the clocks property already provided is enough.
Hi Maxime, Thank you for the review. On Thu, Mar 19, 2020 at 12:45 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > Use assigned-clock-rates to specify the clock rate. Also mark > > clock-frequency property as deprecated. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > index 72ad992..e62fe82 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > @@ -8,7 +8,7 @@ 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. > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > clocks = <&clks 200>; > > clock-names = "xclk"; > > - clock-frequency = <24000000>; > > + assigned-clocks = <&clks 200>; > > + assigned-clock-rates = <24000000>; > > > > vdddo-supply = <&camera_dovdd_1v8>; > > vdda-supply = <&camera_avdd_2v8>; > > clock-frequency is quite different from assigned-clock-rates though, > semantically speaking. clock-frequency is only about what the clock > frequency is, while assigned-clock-rates will change the rate as well, > and you have no idea how long it will last. > Agreed clock-frequency tells whats the clock frequency, wrt ov5645 driver this property was read and and the clock rate was changed accordingly as per the value being passed. So switching to assigned-clock-rates does bypass of clock rate being set in the ov5645 driver [1] as the framework does it. > If you want to retrieve that through the clock framework, then just > making clock-frequency optional is enough and falling back to > clk_get_rate on the clocks property already provided is enough. > As done in patch [1] ? Fyi I have posted a v4 [2] to ML. [1] https://patchwork.linuxtv.org/patch/62378/ [2] https://patchwork.linuxtv.org/project/linux-media/list/?series=1990 Cheers, --Prabhakar > Maxime > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Laurent, On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Maxime, > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > clock-frequency property as deprecated. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > index 72ad992..e62fe82 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > @@ -8,7 +8,7 @@ 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. > > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > > > clocks = <&clks 200>; > > > clock-names = "xclk"; > > > - clock-frequency = <24000000>; > > > + assigned-clocks = <&clks 200>; > > > + assigned-clock-rates = <24000000>; > > > > > > vdddo-supply = <&camera_dovdd_1v8>; > > > vdda-supply = <&camera_avdd_2v8>; > > > > clock-frequency is quite different from assigned-clock-rates though, > > semantically speaking. clock-frequency is only about what the clock > > frequency is, while assigned-clock-rates will change the rate as well, > > and you have no idea how long it will last. > > The driver currently reads the clock-frequency property and then calls > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I > think it's less of a hack than what we currently have. > > As discussed on IRC, maybe the best option in this specific case is to > drop clock-frequency and assigned-clok-rates, and call clk_set_rate() > with a hardcoded frequency of 24MHz in the driver, as that's the only > frequency the driver supports. > Does this mean any driver which has a fixed clock requirement shouldn't be a DT property and should be just handled by the drivers internally ? Cheers, --Prabhakar > > If you want to retrieve that through the clock framework, then just > > making clock-frequency optional is enough and falling back to > > clk_get_rate on the clocks property already provided is enough. > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote: > Hi Laurent, > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Maxime, > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > > clock-frequency property as deprecated. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > index 72ad992..e62fe82 100644 > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > @@ -8,7 +8,7 @@ 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. > > > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > > > > > clocks = <&clks 200>; > > > > clock-names = "xclk"; > > > > - clock-frequency = <24000000>; > > > > + assigned-clocks = <&clks 200>; > > > > + assigned-clock-rates = <24000000>; > > > > > > > > vdddo-supply = <&camera_dovdd_1v8>; > > > > vdda-supply = <&camera_avdd_2v8>; > > > > > > clock-frequency is quite different from assigned-clock-rates though, > > > semantically speaking. clock-frequency is only about what the clock > > > frequency is, while assigned-clock-rates will change the rate as well, > > > and you have no idea how long it will last. > > > > The driver currently reads the clock-frequency property and then calls > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I > > think it's less of a hack than what we currently have. > > > > As discussed on IRC, maybe the best option in this specific case is to > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate() > > with a hardcoded frequency of 24MHz in the driver, as that's the only > > frequency the driver supports. > > > Does this mean any driver which has a fixed clock requirement shouldn't be a > DT property and should be just handled by the drivers internally ? It's hard to give a generic policy, but here, the hardware is pretty flexible since it can deal with anything between 6MHz to 50-something MHz, it's the driver that chooses to enforce a 24MHz and be pedantic about it, so it's up to the driver to enforce that policy, not to the DT since it's essentially a software limitation, not a hardware one. Maxime
Hi Maxime, On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote: > > Hi Laurent, > > > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Maxime, > > > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > > > clock-frequency property as deprecated. > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > --- > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > index 72ad992..e62fe82 100644 > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > @@ -8,7 +8,7 @@ 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. > > > > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > > > > > > > clocks = <&clks 200>; > > > > > clock-names = "xclk"; > > > > > - clock-frequency = <24000000>; > > > > > + assigned-clocks = <&clks 200>; > > > > > + assigned-clock-rates = <24000000>; > > > > > > > > > > vdddo-supply = <&camera_dovdd_1v8>; > > > > > vdda-supply = <&camera_avdd_2v8>; > > > > > > > > clock-frequency is quite different from assigned-clock-rates though, > > > > semantically speaking. clock-frequency is only about what the clock > > > > frequency is, while assigned-clock-rates will change the rate as well, > > > > and you have no idea how long it will last. > > > > > > The driver currently reads the clock-frequency property and then calls > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I > > > think it's less of a hack than what we currently have. > > > > > > As discussed on IRC, maybe the best option in this specific case is to > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate() > > > with a hardcoded frequency of 24MHz in the driver, as that's the only > > > frequency the driver supports. > > > > > Does this mean any driver which has a fixed clock requirement shouldn't be a > > DT property and should be just handled by the drivers internally ? > > It's hard to give a generic policy, but here, the hardware is pretty > flexible since it can deal with anything between 6MHz to 50-something > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic > about it, so it's up to the driver to enforce that policy, not to the > DT since it's essentially a software limitation, not a hardware one. > Thank you for the clarification, Ill drop patches 1-4 from this series. Cheers, --Prabhakar > Maxime > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Prabhakar, On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote: > On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote: > > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote: > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > > > > clock-frequency property as deprecated. > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > --- > > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > index 72ad992..e62fe82 100644 > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > @@ -8,7 +8,7 @@ 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. > > > > > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > > > > > > > > > clocks = <&clks 200>; > > > > > > clock-names = "xclk"; > > > > > > - clock-frequency = <24000000>; > > > > > > + assigned-clocks = <&clks 200>; > > > > > > + assigned-clock-rates = <24000000>; > > > > > > > > > > > > vdddo-supply = <&camera_dovdd_1v8>; > > > > > > vdda-supply = <&camera_avdd_2v8>; > > > > > > > > > > clock-frequency is quite different from assigned-clock-rates though, > > > > > semantically speaking. clock-frequency is only about what the clock > > > > > frequency is, while assigned-clock-rates will change the rate as well, > > > > > and you have no idea how long it will last. > > > > > > > > The driver currently reads the clock-frequency property and then calls > > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I > > > > think it's less of a hack than what we currently have. > > > > > > > > As discussed on IRC, maybe the best option in this specific case is to > > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate() > > > > with a hardcoded frequency of 24MHz in the driver, as that's the only > > > > frequency the driver supports. > > > > > > > Does this mean any driver which has a fixed clock requirement shouldn't be a > > > DT property and should be just handled by the drivers internally ? > > > > It's hard to give a generic policy, but here, the hardware is pretty > > flexible since it can deal with anything between 6MHz to 50-something > > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic > > about it, so it's up to the driver to enforce that policy, not to the > > DT since it's essentially a software limitation, not a hardware one. > > Thank you for the clarification, Ill drop patches 1-4 from this series. That's the whole series... :-) I think you should keep patch 1/4 but just remove the clock-frequency from the bindings, then remove it from the DT files, and patch the driver to set the clock rate to 24MHz unconditionally in patch 4/4.
Hi Laurent, On Tue, Mar 24, 2020 at 4:12 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote: > > On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote: > > > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote: > > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote: > > > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote: > > > > > > > Use assigned-clock-rates to specify the clock rate. Also mark > > > > > > > clock-frequency property as deprecated. > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > --- > > > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- > > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > > index 72ad992..e62fe82 100644 > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > > > > > > > @@ -8,7 +8,7 @@ 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. > > > > > > > +- clock-frequency (deprecated): 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 +37,8 @@ Example: > > > > > > > > > > > > > > clocks = <&clks 200>; > > > > > > > clock-names = "xclk"; > > > > > > > - clock-frequency = <24000000>; > > > > > > > + assigned-clocks = <&clks 200>; > > > > > > > + assigned-clock-rates = <24000000>; > > > > > > > > > > > > > > vdddo-supply = <&camera_dovdd_1v8>; > > > > > > > vdda-supply = <&camera_avdd_2v8>; > > > > > > > > > > > > clock-frequency is quite different from assigned-clock-rates though, > > > > > > semantically speaking. clock-frequency is only about what the clock > > > > > > frequency is, while assigned-clock-rates will change the rate as well, > > > > > > and you have no idea how long it will last. > > > > > > > > > > The driver currently reads the clock-frequency property and then calls > > > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I > > > > > think it's less of a hack than what we currently have. > > > > > > > > > > As discussed on IRC, maybe the best option in this specific case is to > > > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate() > > > > > with a hardcoded frequency of 24MHz in the driver, as that's the only > > > > > frequency the driver supports. > > > > > > > > > Does this mean any driver which has a fixed clock requirement shouldn't be a > > > > DT property and should be just handled by the drivers internally ? > > > > > > It's hard to give a generic policy, but here, the hardware is pretty > > > flexible since it can deal with anything between 6MHz to 50-something > > > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic > > > about it, so it's up to the driver to enforce that policy, not to the > > > DT since it's essentially a software limitation, not a hardware one. > > > > Thank you for the clarification, Ill drop patches 1-4 from this series. > > That's the whole series... :-) I think you should keep patch 1/4 but > just remove the clock-frequency from the bindings, then remove it from > the DT files, and patch the driver to set the clock rate to 24MHz > unconditionally in patch 4/4. > My bad I was referring to v4 series patch 5/5 which converts dt bindings to json schema. I'll shall post a v5 as suggested above. Cheers, --Prabhakar > -- > Regards, > > Laurent Pinchart
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt index 72ad992..e62fe82 100644 --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt @@ -8,7 +8,7 @@ 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. +- clock-frequency (deprecated): 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 +37,8 @@ Example: clocks = <&clks 200>; clock-names = "xclk"; - clock-frequency = <24000000>; + assigned-clocks = <&clks 200>; + assigned-clock-rates = <24000000>; vdddo-supply = <&camera_dovdd_1v8>; vdda-supply = <&camera_avdd_2v8>;
Use assigned-clock-rates to specify the clock rate. Also mark clock-frequency property as deprecated. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)