diff for duplicates of <1502297028.2586.4.camel@synopsys.com> diff --git a/a/1.txt b/N1/1.txt index e14f006..d4c2dad 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,39 +1,74 @@ -SGkgU3RlcGhlbizCoA0KdGhhbmtzIGZvciByZXNwb25kLCBteSBjb21tZW50cyBhcmUgaW5saW5l -ZCBiZWxvdy4NCg0KT24gVGh1LCAyMDE3LTA4LTAzIGF0IDE4OjUzIC0wNzAwLCBTdGVwaGVuIEJv -eWQgd3JvdGU6DQo+IE9uIDA3LzE0LCBFdWdlbml5IFBhbHRzZXYgd3JvdGU6DQoNCj4gWy4uLl0N -Cj4gPiArCWRldl9kYmcoY2xrLT5kZXYsICJ3cml0ZSBjb25maWd1cmFyaW9uOiAweCV4IiwgdmFs -KTsNCj7CoA0KPiBPciBqdXN0IHVzZSAlI3ggdG8gYWRkIHRoZSAweCBwYXJ0Lg0KDQpUaGFua3Ms -IEkgZG9uJ3Qga25vdyBhYm91dCB0aGlzIG9wdGlvbi4NCj7CoA0KPiBbLi4uXQ0KPg0KPiA+ICsJ -LyogaW5wdXQgZGl2aWRlciA9IHJlZy5pZGl2ICsgMSAqLw0KPiA+ICsJaWRpdiA9IDEgKyAoKHZh -bCAmIENHVV9QTExfQ1RSTF9JRElWX01BU0spID4+DQo+ID4gQ0dVX1BMTF9DVFJMX0lESVZfU0hJ -RlQpOw0KPiA+ICsJLyogZmIgZGl2aWRlciA9IDIqKHJlZy5mYmRpdiArIDEpICovDQo+ID4gKwlm -YmRpdiA9IDIgKiAoMSArICgodmFsICYgQ0dVX1BMTF9DVFJMX0ZCRElWX01BU0spID4+DQo+ID4g -Q0dVX1BMTF9DVFJMX0ZCRElWX1NISUZUKSk7DQo+ID4gKwkvKiBvdXRwdXQgZGl2aWRlciA9IDJe -KHJlZy5vZGl2KSAqLw0KPiA+ICsJb2RpdiA9IDEgPDwgKCh2YWwgJiBDR1VfUExMX0NUUkxfT0RJ -Vl9NQVNLKSA+Pg0KPiA+IENHVV9QTExfQ1RSTF9PRElWX1NISUZUKTsNCj4NCj4gTWF5YmUganVz -dCBkcm9wIHRoZXNlIGNvbW1lbnRzLiBUaGV5J3JlIGp1c3QgcmVwZWF0aW5nIHRoZSBjb2RlLg0K -DQpBY3R1YWxseSBJIHdvdWxkIHByZWZlciB0byBrZWVwIHRoZW0sIGFzICIyXihyZWcub2Rpdiki -IGlzIG1vcmXCoA0KaHVtYW4tcmVhZGFibGUgdGhlbg0KIjEgPDwgKCh2YWwgJiBDR1VfUExMX0NU -UkxfT0RJVl9NQVNLKSA+PiBDR1VfUExMX0NUUkxfT0RJVl9TSElGVCkiDQoNCj4gPiArDQo+ID4g -KwlyYXRlID0gKHU2NClwYXJlbnRfcmF0ZSAqIGZiZGl2Ow0KPiA+ICsJZG9fZGl2KHJhdGUsIGlk -aXYgKiBvZGl2KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmF0ZTsNCj4gPiArfQ0KPiA+ICsNCj4g -PiArc3RhdGljIGxvbmcgaHNka19wbGxfcm91bmRfcmF0ZShzdHJ1Y3QgY2xrX2h3ICpodywgdW5z -aWduZWQgbG9uZw0KPiA+IHJhdGUsDQo+ID4gKwkJCQl1bnNpZ25lZCBsb25nICpwcmF0ZSkNCj4g -PiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlsb25nIGJlc3RfcmF0ZTsNCj4gPiArCXN0cnVjdCBo -c2RrX3BsbF9jbGsgKmNsayA9IHRvX2hzZGtfcGxsX2Nsayhodyk7DQo+ID4gKwljb25zdCBzdHJ1 -Y3QgaHNka19wbGxfY2ZnICpwbGxfY2ZnID0gY2xrLT5wbGxfY2ZnOw0KPiA+ICsNCj4gPiArCWlm -IChwbGxfY2ZnWzBdLnJhdGUgPT0gMCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsNCj7CoA0KPiBU -aGlzIGhhcHBlbnM/DQoNCk9ubHkgaWYgd2UgYWRkIGJhZCBoc2RrX3BsbF9jZmcgdGFibGUuIEJ1 -dCBpdCBpcyBxdWl0ZSBjb2xkIGNvZGUgLSB3ZQ0KY2hhbmdlIHBsbCBjb25maWd1cmF0aW9uIHF1 -aXRlIHJhcmUsIHNvIG1heWJlIGl0J3MgYmV0dGVyIHRvIGtlZXAgdGhpcw0KYXNzZXJ0Pw0KDQo+ -ID4gKw0KPiA+ICsJYmVzdF9yYXRlID0gcGxsX2NmZ1swXS5yYXRlOw0KPiA+ICsNCj4gPiArCWZv -ciAoaSA9IDE7IHBsbF9jZmdbaV0ucmF0ZSAhPSAwOyBpKyspIHsNCj4gPiArCQlpZiAoYWJzKHJh -dGUgLSBwbGxfY2ZnW2ldLnJhdGUpIDwgYWJzKHJhdGUgLQ0KPiA+IGJlc3RfcmF0ZSkpDQo+wqAN -Cj4gQWxyaWdodCwgcmF0ZSBpcyB1bnNpZ25lZCBsb25nLCBhbmQgYmVzdF9yYXRlIGlzIGxvbmcu -IGFicygpIGRvZXMNCj4gdGhlIHJpZ2h0IHRoaW5nIGhlcmUsIGJ1dCBpdCBtYWtlcyBtZSBoYXZl -IHRvIHRoaW5rIHR3aWNlIGlmDQo+IGJlc3RfcmF0ZSBjYW4gYmUgbmVnYXRpdmUgYW5kIHRoZW4g -dGhpcyBpcyBhIGxhcmdlciBudW1iZXIsIG5vdCBhDQo+IHNtYWxsZXIgb25lLiBDYW4gd2UgbWFr -ZSBiZXN0X3JhdGUgdW5zaWduZWQgbG9uZyBpbiB0aGlzDQo+IGZ1bmN0aW9uPw0KDQpZZXMsIHdl -IGNhbi4NCkFueXdheSBpdCdzIGEgYml0IHN0cmFuZ2Ugd2hhdCByYXRlIGlzIHVuc2lnbmVkIGxv -bmcgYW5kIHJvdW5kX3JhdGUNCnJldHVybiB2YWx1ZSBpcyBsb25nLg0KLS3CoA0KwqBFdWdlbml5 -IFBhbHRzZXY= +Hi Stephen,? +thanks for respond, my comments are inlined below. + +On Thu, 2017-08-03@18:53 -0700, Stephen Boyd wrote: +> On 07/14, Eugeniy Paltsev wrote: + +> [...] +> > + dev_dbg(clk->dev, "write configurarion: 0x%x", val); +>? +> Or just use %#x to add the 0x part. + +Thanks, I don't know about this option. +>? +> [...] +> +> > + /* input divider = reg.idiv + 1 */ +> > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >> +> > CGU_PLL_CTRL_IDIV_SHIFT); +> > + /* fb divider = 2*(reg.fbdiv + 1) */ +> > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >> +> > CGU_PLL_CTRL_FBDIV_SHIFT)); +> > + /* output divider = 2^(reg.odiv) */ +> > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> +> > CGU_PLL_CTRL_ODIV_SHIFT); +> +> Maybe just drop these comments. They're just repeating the code. + +Actually I would prefer to keep them, as "2^(reg.odiv)" is more? +human-readable then +"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)" + +> > + +> > + rate = (u64)parent_rate * fbdiv; +> > + do_div(rate, idiv * odiv); +> > + +> > + return rate; +> > +} +> > + +> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long +> > rate, +> > + unsigned long *prate) +> > +{ +> > + int i; +> > + long best_rate; +> > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw); +> > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg; +> > + +> > + if (pll_cfg[0].rate == 0) +> > + return -EINVAL; +>? +> This happens? + +Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we +change pll configuration quite rare, so maybe it's better to keep this +assert? + +> > + +> > + best_rate = pll_cfg[0].rate; +> > + +> > + for (i = 1; pll_cfg[i].rate != 0; i++) { +> > + if (abs(rate - pll_cfg[i].rate) < abs(rate - +> > best_rate)) +>? +> Alright, rate is unsigned long, and best_rate is long. abs() does +> the right thing here, but it makes me have to think twice if +> best_rate can be negative and then this is a larger number, not a +> smaller one. Can we make best_rate unsigned long in this +> function? + +Yes, we can. +Anyway it's a bit strange what rate is unsigned long and round_rate +return value is long. +--? +?Eugeniy Paltsev diff --git a/a/content_digest b/N1/content_digest index f7ec218..f9aed5f 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,56 +1,84 @@ "ref\020170714153128.8945-1-Eugeniy.Paltsev@synopsys.com\0" "ref\020170804015351.GW2146@codeaurora.org\0" - "From\0Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>\0" - "Subject\0Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver\0" + "From\0Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)\0" + "Subject\0[PATCH] ARC: clk: introduce HSDKv1 pll driver\0" "Date\0Wed, 9 Aug 2017 16:43:49 +0000\0" - "To\0sboyd@codeaurora.org <sboyd@codeaurora.org>\0" - "Cc\0Eugeniy.Paltsev@synopsys.com <Eugeniy.Paltsev@synopsys.com>" - linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> - mark.rutland@arm.com <mark.rutland@arm.com> - mturquette@baylibre.com <mturquette@baylibre.com> - robh+dt@kernel.org <robh+dt@kernel.org> - linux-clk@vger.kernel.org <linux-clk@vger.kernel.org> - " linux-snps-arc@lists.infradead.org <linux-snps-arc@lists.infradead.org>\0" + "To\0linux-snps-arc@lists.infradead.org\0" "\00:1\0" "b\0" - "SGkgU3RlcGhlbizCoA0KdGhhbmtzIGZvciByZXNwb25kLCBteSBjb21tZW50cyBhcmUgaW5saW5l\n" - "ZCBiZWxvdy4NCg0KT24gVGh1LCAyMDE3LTA4LTAzIGF0IDE4OjUzIC0wNzAwLCBTdGVwaGVuIEJv\n" - "eWQgd3JvdGU6DQo+IE9uIDA3LzE0LCBFdWdlbml5IFBhbHRzZXYgd3JvdGU6DQoNCj4gWy4uLl0N\n" - "Cj4gPiArCWRldl9kYmcoY2xrLT5kZXYsICJ3cml0ZSBjb25maWd1cmFyaW9uOiAweCV4IiwgdmFs\n" - "KTsNCj7CoA0KPiBPciBqdXN0IHVzZSAlI3ggdG8gYWRkIHRoZSAweCBwYXJ0Lg0KDQpUaGFua3Ms\n" - "IEkgZG9uJ3Qga25vdyBhYm91dCB0aGlzIG9wdGlvbi4NCj7CoA0KPiBbLi4uXQ0KPg0KPiA+ICsJ\n" - "LyogaW5wdXQgZGl2aWRlciA9IHJlZy5pZGl2ICsgMSAqLw0KPiA+ICsJaWRpdiA9IDEgKyAoKHZh\n" - "bCAmIENHVV9QTExfQ1RSTF9JRElWX01BU0spID4+DQo+ID4gQ0dVX1BMTF9DVFJMX0lESVZfU0hJ\n" - "RlQpOw0KPiA+ICsJLyogZmIgZGl2aWRlciA9IDIqKHJlZy5mYmRpdiArIDEpICovDQo+ID4gKwlm\n" - "YmRpdiA9IDIgKiAoMSArICgodmFsICYgQ0dVX1BMTF9DVFJMX0ZCRElWX01BU0spID4+DQo+ID4g\n" - "Q0dVX1BMTF9DVFJMX0ZCRElWX1NISUZUKSk7DQo+ID4gKwkvKiBvdXRwdXQgZGl2aWRlciA9IDJe\n" - "KHJlZy5vZGl2KSAqLw0KPiA+ICsJb2RpdiA9IDEgPDwgKCh2YWwgJiBDR1VfUExMX0NUUkxfT0RJ\n" - "Vl9NQVNLKSA+Pg0KPiA+IENHVV9QTExfQ1RSTF9PRElWX1NISUZUKTsNCj4NCj4gTWF5YmUganVz\n" - "dCBkcm9wIHRoZXNlIGNvbW1lbnRzLiBUaGV5J3JlIGp1c3QgcmVwZWF0aW5nIHRoZSBjb2RlLg0K\n" - "DQpBY3R1YWxseSBJIHdvdWxkIHByZWZlciB0byBrZWVwIHRoZW0sIGFzICIyXihyZWcub2Rpdiki\n" - "IGlzIG1vcmXCoA0KaHVtYW4tcmVhZGFibGUgdGhlbg0KIjEgPDwgKCh2YWwgJiBDR1VfUExMX0NU\n" - "UkxfT0RJVl9NQVNLKSA+PiBDR1VfUExMX0NUUkxfT0RJVl9TSElGVCkiDQoNCj4gPiArDQo+ID4g\n" - "KwlyYXRlID0gKHU2NClwYXJlbnRfcmF0ZSAqIGZiZGl2Ow0KPiA+ICsJZG9fZGl2KHJhdGUsIGlk\n" - "aXYgKiBvZGl2KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmF0ZTsNCj4gPiArfQ0KPiA+ICsNCj4g\n" - "PiArc3RhdGljIGxvbmcgaHNka19wbGxfcm91bmRfcmF0ZShzdHJ1Y3QgY2xrX2h3ICpodywgdW5z\n" - "aWduZWQgbG9uZw0KPiA+IHJhdGUsDQo+ID4gKwkJCQl1bnNpZ25lZCBsb25nICpwcmF0ZSkNCj4g\n" - "PiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlsb25nIGJlc3RfcmF0ZTsNCj4gPiArCXN0cnVjdCBo\n" - "c2RrX3BsbF9jbGsgKmNsayA9IHRvX2hzZGtfcGxsX2Nsayhodyk7DQo+ID4gKwljb25zdCBzdHJ1\n" - "Y3QgaHNka19wbGxfY2ZnICpwbGxfY2ZnID0gY2xrLT5wbGxfY2ZnOw0KPiA+ICsNCj4gPiArCWlm\n" - "IChwbGxfY2ZnWzBdLnJhdGUgPT0gMCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsNCj7CoA0KPiBU\n" - "aGlzIGhhcHBlbnM/DQoNCk9ubHkgaWYgd2UgYWRkIGJhZCBoc2RrX3BsbF9jZmcgdGFibGUuIEJ1\n" - "dCBpdCBpcyBxdWl0ZSBjb2xkIGNvZGUgLSB3ZQ0KY2hhbmdlIHBsbCBjb25maWd1cmF0aW9uIHF1\n" - "aXRlIHJhcmUsIHNvIG1heWJlIGl0J3MgYmV0dGVyIHRvIGtlZXAgdGhpcw0KYXNzZXJ0Pw0KDQo+\n" - "ID4gKw0KPiA+ICsJYmVzdF9yYXRlID0gcGxsX2NmZ1swXS5yYXRlOw0KPiA+ICsNCj4gPiArCWZv\n" - "ciAoaSA9IDE7IHBsbF9jZmdbaV0ucmF0ZSAhPSAwOyBpKyspIHsNCj4gPiArCQlpZiAoYWJzKHJh\n" - "dGUgLSBwbGxfY2ZnW2ldLnJhdGUpIDwgYWJzKHJhdGUgLQ0KPiA+IGJlc3RfcmF0ZSkpDQo+wqAN\n" - "Cj4gQWxyaWdodCwgcmF0ZSBpcyB1bnNpZ25lZCBsb25nLCBhbmQgYmVzdF9yYXRlIGlzIGxvbmcu\n" - "IGFicygpIGRvZXMNCj4gdGhlIHJpZ2h0IHRoaW5nIGhlcmUsIGJ1dCBpdCBtYWtlcyBtZSBoYXZl\n" - "IHRvIHRoaW5rIHR3aWNlIGlmDQo+IGJlc3RfcmF0ZSBjYW4gYmUgbmVnYXRpdmUgYW5kIHRoZW4g\n" - "dGhpcyBpcyBhIGxhcmdlciBudW1iZXIsIG5vdCBhDQo+IHNtYWxsZXIgb25lLiBDYW4gd2UgbWFr\n" - "ZSBiZXN0X3JhdGUgdW5zaWduZWQgbG9uZyBpbiB0aGlzDQo+IGZ1bmN0aW9uPw0KDQpZZXMsIHdl\n" - "IGNhbi4NCkFueXdheSBpdCdzIGEgYml0IHN0cmFuZ2Ugd2hhdCByYXRlIGlzIHVuc2lnbmVkIGxv\n" - "bmcgYW5kIHJvdW5kX3JhdGUNCnJldHVybiB2YWx1ZSBpcyBsb25nLg0KLS3CoA0KwqBFdWdlbml5\n" - IFBhbHRzZXY= + "Hi Stephen,?\n" + "thanks for respond, my comments are inlined below.\n" + "\n" + "On Thu, 2017-08-03@18:53 -0700, Stephen Boyd wrote:\n" + "> On 07/14, Eugeniy Paltsev wrote:\n" + "\n" + "> [...]\n" + "> > +\tdev_dbg(clk->dev, \"write configurarion: 0x%x\", val);\n" + ">?\n" + "> Or just use %#x to add the 0x part.\n" + "\n" + "Thanks, I don't know about this option.\n" + ">?\n" + "> [...]\n" + ">\n" + "> > +\t/* input divider = reg.idiv + 1 */\n" + "> > +\tidiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>\n" + "> > CGU_PLL_CTRL_IDIV_SHIFT);\n" + "> > +\t/* fb divider = 2*(reg.fbdiv + 1) */\n" + "> > +\tfbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>\n" + "> > CGU_PLL_CTRL_FBDIV_SHIFT));\n" + "> > +\t/* output divider = 2^(reg.odiv) */\n" + "> > +\todiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>\n" + "> > CGU_PLL_CTRL_ODIV_SHIFT);\n" + ">\n" + "> Maybe just drop these comments. They're just repeating the code.\n" + "\n" + "Actually I would prefer to keep them, as \"2^(reg.odiv)\" is more?\n" + "human-readable then\n" + "\"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)\"\n" + "\n" + "> > +\n" + "> > +\trate = (u64)parent_rate * fbdiv;\n" + "> > +\tdo_div(rate, idiv * odiv);\n" + "> > +\n" + "> > +\treturn rate;\n" + "> > +}\n" + "> > +\n" + "> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long\n" + "> > rate,\n" + "> > +\t\t\t\tunsigned long *prate)\n" + "> > +{\n" + "> > +\tint i;\n" + "> > +\tlong best_rate;\n" + "> > +\tstruct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);\n" + "> > +\tconst struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;\n" + "> > +\n" + "> > +\tif (pll_cfg[0].rate == 0)\n" + "> > +\t\treturn -EINVAL;\n" + ">?\n" + "> This happens?\n" + "\n" + "Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we\n" + "change pll configuration quite rare, so maybe it's better to keep this\n" + "assert?\n" + "\n" + "> > +\n" + "> > +\tbest_rate = pll_cfg[0].rate;\n" + "> > +\n" + "> > +\tfor (i = 1; pll_cfg[i].rate != 0; i++) {\n" + "> > +\t\tif (abs(rate - pll_cfg[i].rate) < abs(rate -\n" + "> > best_rate))\n" + ">?\n" + "> Alright, rate is unsigned long, and best_rate is long. abs() does\n" + "> the right thing here, but it makes me have to think twice if\n" + "> best_rate can be negative and then this is a larger number, not a\n" + "> smaller one. Can we make best_rate unsigned long in this\n" + "> function?\n" + "\n" + "Yes, we can.\n" + "Anyway it's a bit strange what rate is unsigned long and round_rate\n" + "return value is long.\n" + "--?\n" + ?Eugeniy Paltsev -fa8217bb7df3efe5c1208233f97ac8643c3683eceb928c5a77e67a1bb9cb3038 +f7c7d05e5d05e260d837bf22916caaf55cb58747fbbea0ac80b42c832da60b57
diff --git a/a/1.txt b/N2/1.txt index e14f006..fbfc8b4 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,39 +1,74 @@ -SGkgU3RlcGhlbizCoA0KdGhhbmtzIGZvciByZXNwb25kLCBteSBjb21tZW50cyBhcmUgaW5saW5l -ZCBiZWxvdy4NCg0KT24gVGh1LCAyMDE3LTA4LTAzIGF0IDE4OjUzIC0wNzAwLCBTdGVwaGVuIEJv -eWQgd3JvdGU6DQo+IE9uIDA3LzE0LCBFdWdlbml5IFBhbHRzZXYgd3JvdGU6DQoNCj4gWy4uLl0N -Cj4gPiArCWRldl9kYmcoY2xrLT5kZXYsICJ3cml0ZSBjb25maWd1cmFyaW9uOiAweCV4IiwgdmFs -KTsNCj7CoA0KPiBPciBqdXN0IHVzZSAlI3ggdG8gYWRkIHRoZSAweCBwYXJ0Lg0KDQpUaGFua3Ms -IEkgZG9uJ3Qga25vdyBhYm91dCB0aGlzIG9wdGlvbi4NCj7CoA0KPiBbLi4uXQ0KPg0KPiA+ICsJ -LyogaW5wdXQgZGl2aWRlciA9IHJlZy5pZGl2ICsgMSAqLw0KPiA+ICsJaWRpdiA9IDEgKyAoKHZh -bCAmIENHVV9QTExfQ1RSTF9JRElWX01BU0spID4+DQo+ID4gQ0dVX1BMTF9DVFJMX0lESVZfU0hJ -RlQpOw0KPiA+ICsJLyogZmIgZGl2aWRlciA9IDIqKHJlZy5mYmRpdiArIDEpICovDQo+ID4gKwlm -YmRpdiA9IDIgKiAoMSArICgodmFsICYgQ0dVX1BMTF9DVFJMX0ZCRElWX01BU0spID4+DQo+ID4g -Q0dVX1BMTF9DVFJMX0ZCRElWX1NISUZUKSk7DQo+ID4gKwkvKiBvdXRwdXQgZGl2aWRlciA9IDJe -KHJlZy5vZGl2KSAqLw0KPiA+ICsJb2RpdiA9IDEgPDwgKCh2YWwgJiBDR1VfUExMX0NUUkxfT0RJ -Vl9NQVNLKSA+Pg0KPiA+IENHVV9QTExfQ1RSTF9PRElWX1NISUZUKTsNCj4NCj4gTWF5YmUganVz -dCBkcm9wIHRoZXNlIGNvbW1lbnRzLiBUaGV5J3JlIGp1c3QgcmVwZWF0aW5nIHRoZSBjb2RlLg0K -DQpBY3R1YWxseSBJIHdvdWxkIHByZWZlciB0byBrZWVwIHRoZW0sIGFzICIyXihyZWcub2Rpdiki -IGlzIG1vcmXCoA0KaHVtYW4tcmVhZGFibGUgdGhlbg0KIjEgPDwgKCh2YWwgJiBDR1VfUExMX0NU -UkxfT0RJVl9NQVNLKSA+PiBDR1VfUExMX0NUUkxfT0RJVl9TSElGVCkiDQoNCj4gPiArDQo+ID4g -KwlyYXRlID0gKHU2NClwYXJlbnRfcmF0ZSAqIGZiZGl2Ow0KPiA+ICsJZG9fZGl2KHJhdGUsIGlk -aXYgKiBvZGl2KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmF0ZTsNCj4gPiArfQ0KPiA+ICsNCj4g -PiArc3RhdGljIGxvbmcgaHNka19wbGxfcm91bmRfcmF0ZShzdHJ1Y3QgY2xrX2h3ICpodywgdW5z -aWduZWQgbG9uZw0KPiA+IHJhdGUsDQo+ID4gKwkJCQl1bnNpZ25lZCBsb25nICpwcmF0ZSkNCj4g -PiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlsb25nIGJlc3RfcmF0ZTsNCj4gPiArCXN0cnVjdCBo -c2RrX3BsbF9jbGsgKmNsayA9IHRvX2hzZGtfcGxsX2Nsayhodyk7DQo+ID4gKwljb25zdCBzdHJ1 -Y3QgaHNka19wbGxfY2ZnICpwbGxfY2ZnID0gY2xrLT5wbGxfY2ZnOw0KPiA+ICsNCj4gPiArCWlm -IChwbGxfY2ZnWzBdLnJhdGUgPT0gMCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsNCj7CoA0KPiBU -aGlzIGhhcHBlbnM/DQoNCk9ubHkgaWYgd2UgYWRkIGJhZCBoc2RrX3BsbF9jZmcgdGFibGUuIEJ1 -dCBpdCBpcyBxdWl0ZSBjb2xkIGNvZGUgLSB3ZQ0KY2hhbmdlIHBsbCBjb25maWd1cmF0aW9uIHF1 -aXRlIHJhcmUsIHNvIG1heWJlIGl0J3MgYmV0dGVyIHRvIGtlZXAgdGhpcw0KYXNzZXJ0Pw0KDQo+ -ID4gKw0KPiA+ICsJYmVzdF9yYXRlID0gcGxsX2NmZ1swXS5yYXRlOw0KPiA+ICsNCj4gPiArCWZv -ciAoaSA9IDE7IHBsbF9jZmdbaV0ucmF0ZSAhPSAwOyBpKyspIHsNCj4gPiArCQlpZiAoYWJzKHJh -dGUgLSBwbGxfY2ZnW2ldLnJhdGUpIDwgYWJzKHJhdGUgLQ0KPiA+IGJlc3RfcmF0ZSkpDQo+wqAN -Cj4gQWxyaWdodCwgcmF0ZSBpcyB1bnNpZ25lZCBsb25nLCBhbmQgYmVzdF9yYXRlIGlzIGxvbmcu -IGFicygpIGRvZXMNCj4gdGhlIHJpZ2h0IHRoaW5nIGhlcmUsIGJ1dCBpdCBtYWtlcyBtZSBoYXZl -IHRvIHRoaW5rIHR3aWNlIGlmDQo+IGJlc3RfcmF0ZSBjYW4gYmUgbmVnYXRpdmUgYW5kIHRoZW4g -dGhpcyBpcyBhIGxhcmdlciBudW1iZXIsIG5vdCBhDQo+IHNtYWxsZXIgb25lLiBDYW4gd2UgbWFr -ZSBiZXN0X3JhdGUgdW5zaWduZWQgbG9uZyBpbiB0aGlzDQo+IGZ1bmN0aW9uPw0KDQpZZXMsIHdl -IGNhbi4NCkFueXdheSBpdCdzIGEgYml0IHN0cmFuZ2Ugd2hhdCByYXRlIGlzIHVuc2lnbmVkIGxv -bmcgYW5kIHJvdW5kX3JhdGUNCnJldHVybiB2YWx1ZSBpcyBsb25nLg0KLS3CoA0KwqBFdWdlbml5 -IFBhbHRzZXY= +Hi Stephen, +thanks for respond, my comments are inlined below. + +On Thu, 2017-08-03 at 18:53 -0700, Stephen Boyd wrote: +> On 07/14, Eugeniy Paltsev wrote: + +> [...] +> > + dev_dbg(clk->dev, "write configurarion: 0x%x", val); +> +> Or just use %#x to add the 0x part. + +Thanks, I don't know about this option. +> +> [...] +> +> > + /* input divider = reg.idiv + 1 */ +> > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >> +> > CGU_PLL_CTRL_IDIV_SHIFT); +> > + /* fb divider = 2*(reg.fbdiv + 1) */ +> > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >> +> > CGU_PLL_CTRL_FBDIV_SHIFT)); +> > + /* output divider = 2^(reg.odiv) */ +> > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> +> > CGU_PLL_CTRL_ODIV_SHIFT); +> +> Maybe just drop these comments. They're just repeating the code. + +Actually I would prefer to keep them, as "2^(reg.odiv)" is more +human-readable then +"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)" + +> > + +> > + rate = (u64)parent_rate * fbdiv; +> > + do_div(rate, idiv * odiv); +> > + +> > + return rate; +> > +} +> > + +> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long +> > rate, +> > + unsigned long *prate) +> > +{ +> > + int i; +> > + long best_rate; +> > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw); +> > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg; +> > + +> > + if (pll_cfg[0].rate == 0) +> > + return -EINVAL; +> +> This happens? + +Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we +change pll configuration quite rare, so maybe it's better to keep this +assert? + +> > + +> > + best_rate = pll_cfg[0].rate; +> > + +> > + for (i = 1; pll_cfg[i].rate != 0; i++) { +> > + if (abs(rate - pll_cfg[i].rate) < abs(rate - +> > best_rate)) +> +> Alright, rate is unsigned long, and best_rate is long. abs() does +> the right thing here, but it makes me have to think twice if +> best_rate can be negative and then this is a larger number, not a +> smaller one. Can we make best_rate unsigned long in this +> function? + +Yes, we can. +Anyway it's a bit strange what rate is unsigned long and round_rate +return value is long. +-- + Eugeniy Paltsev diff --git a/a/content_digest b/N2/content_digest index f7ec218..a206ff4 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -13,44 +13,79 @@ " linux-snps-arc@lists.infradead.org <linux-snps-arc@lists.infradead.org>\0" "\00:1\0" "b\0" - "SGkgU3RlcGhlbizCoA0KdGhhbmtzIGZvciByZXNwb25kLCBteSBjb21tZW50cyBhcmUgaW5saW5l\n" - "ZCBiZWxvdy4NCg0KT24gVGh1LCAyMDE3LTA4LTAzIGF0IDE4OjUzIC0wNzAwLCBTdGVwaGVuIEJv\n" - "eWQgd3JvdGU6DQo+IE9uIDA3LzE0LCBFdWdlbml5IFBhbHRzZXYgd3JvdGU6DQoNCj4gWy4uLl0N\n" - "Cj4gPiArCWRldl9kYmcoY2xrLT5kZXYsICJ3cml0ZSBjb25maWd1cmFyaW9uOiAweCV4IiwgdmFs\n" - "KTsNCj7CoA0KPiBPciBqdXN0IHVzZSAlI3ggdG8gYWRkIHRoZSAweCBwYXJ0Lg0KDQpUaGFua3Ms\n" - "IEkgZG9uJ3Qga25vdyBhYm91dCB0aGlzIG9wdGlvbi4NCj7CoA0KPiBbLi4uXQ0KPg0KPiA+ICsJ\n" - "LyogaW5wdXQgZGl2aWRlciA9IHJlZy5pZGl2ICsgMSAqLw0KPiA+ICsJaWRpdiA9IDEgKyAoKHZh\n" - "bCAmIENHVV9QTExfQ1RSTF9JRElWX01BU0spID4+DQo+ID4gQ0dVX1BMTF9DVFJMX0lESVZfU0hJ\n" - "RlQpOw0KPiA+ICsJLyogZmIgZGl2aWRlciA9IDIqKHJlZy5mYmRpdiArIDEpICovDQo+ID4gKwlm\n" - "YmRpdiA9IDIgKiAoMSArICgodmFsICYgQ0dVX1BMTF9DVFJMX0ZCRElWX01BU0spID4+DQo+ID4g\n" - "Q0dVX1BMTF9DVFJMX0ZCRElWX1NISUZUKSk7DQo+ID4gKwkvKiBvdXRwdXQgZGl2aWRlciA9IDJe\n" - "KHJlZy5vZGl2KSAqLw0KPiA+ICsJb2RpdiA9IDEgPDwgKCh2YWwgJiBDR1VfUExMX0NUUkxfT0RJ\n" - "Vl9NQVNLKSA+Pg0KPiA+IENHVV9QTExfQ1RSTF9PRElWX1NISUZUKTsNCj4NCj4gTWF5YmUganVz\n" - "dCBkcm9wIHRoZXNlIGNvbW1lbnRzLiBUaGV5J3JlIGp1c3QgcmVwZWF0aW5nIHRoZSBjb2RlLg0K\n" - "DQpBY3R1YWxseSBJIHdvdWxkIHByZWZlciB0byBrZWVwIHRoZW0sIGFzICIyXihyZWcub2Rpdiki\n" - "IGlzIG1vcmXCoA0KaHVtYW4tcmVhZGFibGUgdGhlbg0KIjEgPDwgKCh2YWwgJiBDR1VfUExMX0NU\n" - "UkxfT0RJVl9NQVNLKSA+PiBDR1VfUExMX0NUUkxfT0RJVl9TSElGVCkiDQoNCj4gPiArDQo+ID4g\n" - "KwlyYXRlID0gKHU2NClwYXJlbnRfcmF0ZSAqIGZiZGl2Ow0KPiA+ICsJZG9fZGl2KHJhdGUsIGlk\n" - "aXYgKiBvZGl2KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmF0ZTsNCj4gPiArfQ0KPiA+ICsNCj4g\n" - "PiArc3RhdGljIGxvbmcgaHNka19wbGxfcm91bmRfcmF0ZShzdHJ1Y3QgY2xrX2h3ICpodywgdW5z\n" - "aWduZWQgbG9uZw0KPiA+IHJhdGUsDQo+ID4gKwkJCQl1bnNpZ25lZCBsb25nICpwcmF0ZSkNCj4g\n" - "PiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlsb25nIGJlc3RfcmF0ZTsNCj4gPiArCXN0cnVjdCBo\n" - "c2RrX3BsbF9jbGsgKmNsayA9IHRvX2hzZGtfcGxsX2Nsayhodyk7DQo+ID4gKwljb25zdCBzdHJ1\n" - "Y3QgaHNka19wbGxfY2ZnICpwbGxfY2ZnID0gY2xrLT5wbGxfY2ZnOw0KPiA+ICsNCj4gPiArCWlm\n" - "IChwbGxfY2ZnWzBdLnJhdGUgPT0gMCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsNCj7CoA0KPiBU\n" - "aGlzIGhhcHBlbnM/DQoNCk9ubHkgaWYgd2UgYWRkIGJhZCBoc2RrX3BsbF9jZmcgdGFibGUuIEJ1\n" - "dCBpdCBpcyBxdWl0ZSBjb2xkIGNvZGUgLSB3ZQ0KY2hhbmdlIHBsbCBjb25maWd1cmF0aW9uIHF1\n" - "aXRlIHJhcmUsIHNvIG1heWJlIGl0J3MgYmV0dGVyIHRvIGtlZXAgdGhpcw0KYXNzZXJ0Pw0KDQo+\n" - "ID4gKw0KPiA+ICsJYmVzdF9yYXRlID0gcGxsX2NmZ1swXS5yYXRlOw0KPiA+ICsNCj4gPiArCWZv\n" - "ciAoaSA9IDE7IHBsbF9jZmdbaV0ucmF0ZSAhPSAwOyBpKyspIHsNCj4gPiArCQlpZiAoYWJzKHJh\n" - "dGUgLSBwbGxfY2ZnW2ldLnJhdGUpIDwgYWJzKHJhdGUgLQ0KPiA+IGJlc3RfcmF0ZSkpDQo+wqAN\n" - "Cj4gQWxyaWdodCwgcmF0ZSBpcyB1bnNpZ25lZCBsb25nLCBhbmQgYmVzdF9yYXRlIGlzIGxvbmcu\n" - "IGFicygpIGRvZXMNCj4gdGhlIHJpZ2h0IHRoaW5nIGhlcmUsIGJ1dCBpdCBtYWtlcyBtZSBoYXZl\n" - "IHRvIHRoaW5rIHR3aWNlIGlmDQo+IGJlc3RfcmF0ZSBjYW4gYmUgbmVnYXRpdmUgYW5kIHRoZW4g\n" - "dGhpcyBpcyBhIGxhcmdlciBudW1iZXIsIG5vdCBhDQo+IHNtYWxsZXIgb25lLiBDYW4gd2UgbWFr\n" - "ZSBiZXN0X3JhdGUgdW5zaWduZWQgbG9uZyBpbiB0aGlzDQo+IGZ1bmN0aW9uPw0KDQpZZXMsIHdl\n" - "IGNhbi4NCkFueXdheSBpdCdzIGEgYml0IHN0cmFuZ2Ugd2hhdCByYXRlIGlzIHVuc2lnbmVkIGxv\n" - "bmcgYW5kIHJvdW5kX3JhdGUNCnJldHVybiB2YWx1ZSBpcyBsb25nLg0KLS3CoA0KwqBFdWdlbml5\n" - IFBhbHRzZXY= + "Hi Stephen,\302\240\n" + "thanks for respond, my comments are inlined below.\n" + "\n" + "On Thu, 2017-08-03 at 18:53 -0700, Stephen Boyd wrote:\n" + "> On 07/14, Eugeniy Paltsev wrote:\n" + "\n" + "> [...]\n" + "> > +\tdev_dbg(clk->dev, \"write configurarion: 0x%x\", val);\n" + ">\302\240\n" + "> Or just use %#x to add the 0x part.\n" + "\n" + "Thanks, I don't know about this option.\n" + ">\302\240\n" + "> [...]\n" + ">\n" + "> > +\t/* input divider = reg.idiv + 1 */\n" + "> > +\tidiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>\n" + "> > CGU_PLL_CTRL_IDIV_SHIFT);\n" + "> > +\t/* fb divider = 2*(reg.fbdiv + 1) */\n" + "> > +\tfbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>\n" + "> > CGU_PLL_CTRL_FBDIV_SHIFT));\n" + "> > +\t/* output divider = 2^(reg.odiv) */\n" + "> > +\todiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>\n" + "> > CGU_PLL_CTRL_ODIV_SHIFT);\n" + ">\n" + "> Maybe just drop these comments. They're just repeating the code.\n" + "\n" + "Actually I would prefer to keep them, as \"2^(reg.odiv)\" is more\302\240\n" + "human-readable then\n" + "\"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)\"\n" + "\n" + "> > +\n" + "> > +\trate = (u64)parent_rate * fbdiv;\n" + "> > +\tdo_div(rate, idiv * odiv);\n" + "> > +\n" + "> > +\treturn rate;\n" + "> > +}\n" + "> > +\n" + "> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long\n" + "> > rate,\n" + "> > +\t\t\t\tunsigned long *prate)\n" + "> > +{\n" + "> > +\tint i;\n" + "> > +\tlong best_rate;\n" + "> > +\tstruct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);\n" + "> > +\tconst struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;\n" + "> > +\n" + "> > +\tif (pll_cfg[0].rate == 0)\n" + "> > +\t\treturn -EINVAL;\n" + ">\302\240\n" + "> This happens?\n" + "\n" + "Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we\n" + "change pll configuration quite rare, so maybe it's better to keep this\n" + "assert?\n" + "\n" + "> > +\n" + "> > +\tbest_rate = pll_cfg[0].rate;\n" + "> > +\n" + "> > +\tfor (i = 1; pll_cfg[i].rate != 0; i++) {\n" + "> > +\t\tif (abs(rate - pll_cfg[i].rate) < abs(rate -\n" + "> > best_rate))\n" + ">\302\240\n" + "> Alright, rate is unsigned long, and best_rate is long. abs() does\n" + "> the right thing here, but it makes me have to think twice if\n" + "> best_rate can be negative and then this is a larger number, not a\n" + "> smaller one. Can we make best_rate unsigned long in this\n" + "> function?\n" + "\n" + "Yes, we can.\n" + "Anyway it's a bit strange what rate is unsigned long and round_rate\n" + "return value is long.\n" + "--\302\240\n" + "\302\240Eugeniy Paltsev" -fa8217bb7df3efe5c1208233f97ac8643c3683eceb928c5a77e67a1bb9cb3038 +06c5d2b209b7d74e869d231ead505bb5417a1d9bb1232ca72fbd7429ce5d4973
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.