From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuninori Morimoto Date: Wed, 13 May 2015 00:08:50 +0000 Subject: Re: [PATCH 5/7 v4] mmc: sh_mmcif: calculate best clock with parent clock Message-Id: <87k2wdfihl.wl%kuninori.morimoto.gx@renesas.com> List-Id: References: <873840a4ch.wl%kuninori.morimoto.gx@renesas.com> <87d22vgt8u.wl%kuninori.morimoto.gx@renesas.com> <874mo7gsza.wl%kuninori.morimoto.gx@renesas.com> <8472238.ZvlxlsYeIr@avalon> In-Reply-To: <8472238.ZvlxlsYeIr@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Laurent Pinchart Cc: Mike Turquette , Ulf Hansson , Simon , linux-mmc , Magnus , Linux-SH , kobayashi , devicetree@vger.kernel.org Hi Laurent Thank you for your feedback > > diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > > b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt index > > 299081f..eb50f4e 100644 > > --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > > +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > > @@ -14,6 +14,8 @@ Required properties: > > > > - clocks: reference to the functional clock > > > > +- clk-range: parent clock range > > + > > I think the idea of a DT property to specify the admissible range for clock > frequencies is good. However, I have a couple of small comments regarding the > implementation. > > First of all, this doesn't seem Renesas-specific to me (feel free to disagree, > but in that case the property should probably be called renesas,clk-range). > Wouldn't it make sense to specify the property in > Documentation/devicetree/bindings/clock/clock-bindings.txt instead ? > > Then, I think the documentation should be clarified a bit, as "parent clock > range" is probably a bit vague for people who haven't followed the development > of this patch series. How about something like "range of admissible > frequencies for the input clock" ? > > There's already a clock-ranges property with a totally different meaning. > That's quite confusing, it would thus be good to rename clk-range. How about > clock-freq-range or clock-frequency-range ? > > Finally, if a DT node references several clocks in its clocks property, we > need a way to specify the range for each of them. I didn't tell you about this, but actually I created v5 patch (it is under test stage now, I will send it to ML soon) which doesn't add new DT property. This v4 added new property for clock frequencies, and it was Geert's idea. This is just my opinion, and I don't know this is good match for DT, but, this clock frequencies range depends on each SoC, and we are already using SoC based "compatible" on DT. This means we can (should ?) get each SoC specific feature (which is not related to each platform/board) from "compatible". Thus, v5 patch gets clock frequency range from SoC based "compatible". Thank you for your help, please check my next v5 patch. Best regards --- Kuninori Morimoto