diff for duplicates of <560E1DF6.9010901@topic.nl> diff --git a/a/1.txt b/N1/1.txt index c46288b..272c2f1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,4 +1,4 @@ -=EF=BB=BFOn 02-10-15 01:34, Stephen Boyd wrote: +On 02-10-15 01:34, Stephen Boyd wrote: > On 09/17, Mike Looijmans wrote: >> This patch adds the driver and devicetree documentation for the >> Silicon Labs SI514 clock generator chip. This is an I2C controlled @@ -8,10 +8,9 @@ > >> to 250MHz. >> ->> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> +>> Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org> >> --- ->> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/= -Documentation/devicetree/bindings/clock/silabs,si514.txt +>> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/Documentation/devicetree/bindings/clock/silabs,si514.txt >> new file mode 100644 >> index 0000000..05964d1 >> --- /dev/null @@ -20,8 +19,7 @@ Documentation/devicetree/bindings/clock/silabs,si514.txt >> +Binding for Silicon Labs 514 programmable I2C clock generator. >> + >> +Reference ->> +This binding uses the common clock binding[1]. Details about the device= - can be +>> +This binding uses the common clock binding[1]. Details about the device can be >> +found in the datasheet[2]. >> + >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt @@ -34,25 +32,22 @@ Documentation/devicetree/bindings/clock/silabs,si514.txt >> + - #clock-cells: From common clock bindings: Shall be 0. >> + >> +Optional properties: ->> + - clock-output-names: From common clock bindings. Recommended to be "s= -i514". ->> + - clock-frequency: Output frequency to generate. This defines the outp= -ut +>> + - clock-output-names: From common clock bindings. Recommended to be "si514". +>> + - clock-frequency: Output frequency to generate. This defines the output >> + frequency set during boot. It can be reprogrammed during >> + runtime through the common clock framework. > > Can we use assigned clock rates instead of this property? -I'll first need to learn what 'assigned clock rates' means. But I suspect t= -he=20 +I'll first need to learn what 'assigned clock rates' means. But I suspect the answer will be yes. >> + >> +Example: >> + si514: clock-generator@55 { ->> + reg =3D <0x55>; ->> + #clock-cells =3D <0>; ->> + compatible =3D "silabs,si514"; +>> + reg = <0x55>; +>> + #clock-cells = <0>; +>> + compatible = "silabs,si514"; >> + }; >> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig >> index 42f7120..6ac7deb5 100644 @@ -92,7 +87,7 @@ It calls various 'of_' methods unconditionally, so I'd think so. > I'd expect some sort of linux/clk.h include here if we're using > clk APIs. -It probably got dragged in by the clk-provider.h include, including it=20 +It probably got dragged in by the clk-provider.h include, including it specifically would be more appropriate. > @@ -117,43 +112,43 @@ specifically would be more appropriate. >> + /* Calculate LP1/LP2 according to table 13 in the datasheet */ >> + /* 65.259980246 */ >> + if ((settings->m_int < 65) || ->> + ((settings->m_int =3D=3D 65) && (settings->m_frac <=3D 139575831))) ->> + lp =3D 0x22; +>> + ((settings->m_int == 65) && (settings->m_frac <= 139575831))) +>> + lp = 0x22; >> + /* 67.859763463 */ >> + else if ((settings->m_int < 67) || ->> + ((settings->m_int =3D=3D 67) && (settings->m_frac <=3D 461581994))) ->> + lp =3D 0x23; +>> + ((settings->m_int == 67) && (settings->m_frac <= 461581994))) +>> + lp = 0x23; >> + /* 72.937624981 */ >> + else if ((settings->m_int < 72) || ->> + ((settings->m_int =3D=3D 72) && (settings->m_frac <=3D 503383578))) ->> + lp =3D 0x33; +>> + ((settings->m_int == 72) && (settings->m_frac <= 503383578))) +>> + lp = 0x33; >> + /* 75.843265046 */ >> + else if ((settings->m_int < 75) || ->> + ((settings->m_int =3D=3D 75) && (settings->m_frac <=3D 452724474))) ->> + lp =3D 0x34; +>> + ((settings->m_int == 75) && (settings->m_frac <= 452724474))) +>> + lp = 0x34; > > Drop the extra parenthesis on these if statements. > >> + else ->> + lp =3D 0x44; +>> + lp = 0x44; >> + ->> + err =3D regmap_write(data->regmap, SI514_REG_LP, lp); +>> + err = regmap_write(data->regmap, SI514_REG_LP, lp); >> + if (err < 0) >> + return err; >> + ->> + reg[0] =3D settings->m_frac & 0xff; ->> + reg[1] =3D (settings->m_frac >> 8) & 0xff; ->> + reg[2] =3D (settings->m_frac >> 16) & 0xff; ->> + reg[3] =3D ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff; ->> + reg[4] =3D (settings->m_int >> 3) & 0xff; ->> + reg[5] =3D (settings->hs_div) & 0xff; ->> + reg[6] =3D (((settings->hs_div) >> 8) | +>> + reg[0] = settings->m_frac & 0xff; +>> + reg[1] = (settings->m_frac >> 8) & 0xff; +>> + reg[2] = (settings->m_frac >> 16) & 0xff; +>> + reg[3] = ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff; +>> + reg[4] = (settings->m_int >> 3) & 0xff; +>> + reg[5] = (settings->hs_div) & 0xff; +>> + reg[6] = (((settings->hs_div) >> 8) | >> + (settings->ls_div_bits << 4)) & 0xff; > > The & 0xff part is redundant. Assignment truncates for us. > >> + ->> + err =3D regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2); +>> + err = regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2); >> + if (err < 0) >> + return err; >> + /* Writing to SI514_REG_M_INT_FRAC triggers the clock change, so that @@ -177,36 +172,36 @@ specifically would be more appropriate. >> + return -EINVAL; >> + >> + /* Determine the minimum value of LS_DIV and resulting target freq. */ ->> + ls_freq =3D frequency; ->> + if (frequency >=3D (FVCO_MIN / HS_DIV_MAX)) ->> + settings->ls_div_bits =3D 0; +>> + ls_freq = frequency; +>> + if (frequency >= (FVCO_MIN / HS_DIV_MAX)) +>> + settings->ls_div_bits = 0; >> + else { ->> + res =3D 1; ->> + tmp =3D 2 * HS_DIV_MAX; ->> + while (tmp <=3D (HS_DIV_MAX*32)) { +>> + res = 1; +>> + tmp = 2 * HS_DIV_MAX; +>> + while (tmp <= (HS_DIV_MAX*32)) { > > Please add some space here between HS_DIV_MAX and 32. > ->> + if ((frequency * tmp) >=3D FVCO_MIN) +>> + if ((frequency * tmp) >= FVCO_MIN) >> + break; /* We're done */ > > Yep, that's what break in a loop usually means. > >> + ++res; ->> + tmp <<=3D 1; +>> + tmp <<= 1; >> + } ->> + settings->ls_div_bits =3D res; ->> + ls_freq =3D frequency << res; +>> + settings->ls_div_bits = res; +>> + ls_freq = frequency << res; >> + } >> + >> + /* Determine minimum HS_DIV, round up to even number */ ->> + settings->hs_div =3D DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1; +>> + settings->hs_div = DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1; >> + ->> + /* M =3D LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */ ->> + m =3D ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2); +>> + /* M = LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */ +>> + m = ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2); >> + do_div(m, FXO); ->> + settings->m_frac =3D (u32)m & (BIT(29) - 1); ->> + settings->m_int =3D (u32)(m >> 29); +>> + settings->m_frac = (u32)m & (BIT(29) - 1); +>> + settings->m_int = (u32)(m >> 29); >> + >> + return 0; >> +} @@ -214,7 +209,7 @@ specifically would be more appropriate. >> +/* Calculate resulting frequency given the register settings */ >> +static unsigned long si514_calc_rate(struct clk_si514_muldiv *settings) >> +{ ->> + u64 m =3D settings->m_frac | ((u64)settings->m_int << 29); +>> + u64 m = settings->m_frac | ((u64)settings->m_int << 29); >> + >> + return ((u32)(((m * FXO) + (FXO/2)) >> 29)) / > @@ -231,7 +226,7 @@ I'll refactor it into something more readable. >> + > [...] >> + ->> + err =3D si514_calc_muldiv(&settings, rate); +>> + err = si514_calc_muldiv(&settings, rate); >> + if (err) >> + return err; >> + @@ -239,8 +234,7 @@ I'll refactor it into something more readable. >> +} >> + >> +/* Update output frequency for big frequency changes (> 1000 ppm). ->> + * The chip supports <1000ppm changes "on the fly", we haven't implemen= -ted +>> + * The chip supports <1000ppm changes "on the fly", we haven't implemented >> + * that here. */ > > Please fix comment style. @@ -248,23 +242,22 @@ ted >> +static int si514_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ ->> + struct clk_si514 *data =3D to_clk_si514(hw); +>> + struct clk_si514 *data = to_clk_si514(hw); >> + struct clk_si514_muldiv settings; >> + int err; >> + ->> + err =3D si514_calc_muldiv(&settings, rate); +>> + err = si514_calc_muldiv(&settings, rate); >> + if (err) >> + return err; >> + >> + si514_enable_output(data, false); >> + ->> + err =3D si514_set_muldiv(data, &settings); +>> + err = si514_set_muldiv(data, &settings); >> + if (err < 0) >> + return err; /* Undefined state now, best to leave disabled */ >> + >> + /* Trigger calibration */ ->> + err =3D regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FC= -AL); +>> + err = regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FCAL); >> + >> + /* Applying a new frequency can take up to 10ms */ >> + usleep_range(10000, 12000); @@ -276,25 +269,22 @@ AL); Good question, I don't recall why I made this choice. -For a client, when set_rate returns error, it would expect either the clock= -=20 -still running at the old frequency, or not running at all. From that=20 -perspective, since the PLL has been reprogrammed, the better choice would b= -e=20 -to leave it disabled. Expected behaviour for the client is going to be to f= -ix=20 +For a client, when set_rate returns error, it would expect either the clock +still running at the old frequency, or not running at all. From that +perspective, since the PLL has been reprogrammed, the better choice would be +to leave it disabled. Expected behaviour for the client is going to be to fix the I2C bus problems and try again. >> + return err; >> +} >> + ->> +static const struct clk_ops si514_clk_ops =3D { ->> + .recalc_rate =3D si514_recalc_rate, ->> + .round_rate =3D si514_round_rate, ->> + .set_rate =3D si514_set_rate, +>> +static const struct clk_ops si514_clk_ops = { +>> + .recalc_rate = si514_recalc_rate, +>> + .round_rate = si514_round_rate, +>> + .set_rate = si514_set_rate, >> +}; >> + ->> +static struct regmap_config si514_regmap_config =3D { +>> +static struct regmap_config si514_regmap_config = { > > const? > @@ -313,8 +303,7 @@ the I2C bus problems and try again. >> + > -I'll post a v2 patch with the requested changes soon. Thank you for your re= -view. +I'll post a v2 patch with the requested changes soon. Thank you for your review. Mike. @@ -329,7 +318,16 @@ Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 Telefax: +31 (0) 499 33 69 70 -E-mail: mike.looijmans@topicproducts.com +E-mail: mike.looijmans-yhtFebqMsb9it5bFGTN0CAC/G2K4zDHf@public.gmane.org Website: www.topicproducts.com Please consider the environment before printing this e-mail + + + + + +-- +To unsubscribe from this list: send the line "unsubscribe devicetree" in +the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org +More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/a/content_digest b/N1/content_digest index 54604f8..e8bb89f 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,17 +1,18 @@ "ref\01442478495-23522-1-git-send-email-mike.looijmans@topic.nl\0" "ref\01442487891-20477-1-git-send-email-mike.looijmans@topic.nl\0" "ref\020151001233407.GW19319@codeaurora.org\0" - "From\0Mike Looijmans <mike.looijmans@topic.nl>\0" + "ref\020151001233407.GW19319-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org\0" + "From\0Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>\0" "Subject\0Re: [PATCH v2] Add driver for the si514 clock generator chip\0" "Date\0Fri, 2 Oct 2015 08:02:30 +0200\0" - "To\0Stephen Boyd <sboyd@codeaurora.org>\0" - "Cc\0<mturquette@baylibre.com>" - <linux-clk@vger.kernel.org> - <devicetree@vger.kernel.org> - " <linux-kernel@vger.kernel.org>\0" + "To\0Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>\0" + "Cc\0mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org" + linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org + devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org + " linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org\0" "\00:1\0" "b\0" - "=EF=BB=BFOn 02-10-15 01:34, Stephen Boyd wrote:\n" + "\357\273\277On 02-10-15 01:34, Stephen Boyd wrote:\n" "> On 09/17, Mike Looijmans wrote:\n" ">> This patch adds the driver and devicetree documentation for the\n" ">> Silicon Labs SI514 clock generator chip. This is an I2C controlled\n" @@ -21,10 +22,9 @@ ">\n" ">> to 250MHz.\n" ">>\n" - ">> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>\n" + ">> Signed-off-by: Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>\n" ">> ---\n" - ">> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/=\n" - "Documentation/devicetree/bindings/clock/silabs,si514.txt\n" + ">> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/Documentation/devicetree/bindings/clock/silabs,si514.txt\n" ">> new file mode 100644\n" ">> index 0000000..05964d1\n" ">> --- /dev/null\n" @@ -33,8 +33,7 @@ ">> +Binding for Silicon Labs 514 programmable I2C clock generator.\n" ">> +\n" ">> +Reference\n" - ">> +This binding uses the common clock binding[1]. Details about the device=\n" - " can be\n" + ">> +This binding uses the common clock binding[1]. Details about the device can be\n" ">> +found in the datasheet[2].\n" ">> +\n" ">> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt\n" @@ -47,25 +46,22 @@ ">> + - #clock-cells: From common clock bindings: Shall be 0.\n" ">> +\n" ">> +Optional properties:\n" - ">> + - clock-output-names: From common clock bindings. Recommended to be \"s=\n" - "i514\".\n" - ">> + - clock-frequency: Output frequency to generate. This defines the outp=\n" - "ut\n" + ">> + - clock-output-names: From common clock bindings. Recommended to be \"si514\".\n" + ">> + - clock-frequency: Output frequency to generate. This defines the output\n" ">> +\t\t frequency set during boot. It can be reprogrammed during\n" ">> +\t\t runtime through the common clock framework.\n" ">\n" "> Can we use assigned clock rates instead of this property?\n" "\n" - "I'll first need to learn what 'assigned clock rates' means. But I suspect t=\n" - "he=20\n" + "I'll first need to learn what 'assigned clock rates' means. But I suspect the \n" "answer will be yes.\n" "\n" ">> +\n" ">> +Example:\n" ">> +\tsi514: clock-generator@55 {\n" - ">> +\t\treg =3D <0x55>;\n" - ">> +\t\t#clock-cells =3D <0>;\n" - ">> +\t\tcompatible =3D \"silabs,si514\";\n" + ">> +\t\treg = <0x55>;\n" + ">> +\t\t#clock-cells = <0>;\n" + ">> +\t\tcompatible = \"silabs,si514\";\n" ">> +\t};\n" ">> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig\n" ">> index 42f7120..6ac7deb5 100644\n" @@ -105,7 +101,7 @@ "> I'd expect some sort of linux/clk.h include here if we're using\n" "> clk APIs.\n" "\n" - "It probably got dragged in by the clk-provider.h include, including it=20\n" + "It probably got dragged in by the clk-provider.h include, including it \n" "specifically would be more appropriate.\n" "\n" ">\n" @@ -130,43 +126,43 @@ ">> +\t/* Calculate LP1/LP2 according to table 13 in the datasheet */\n" ">> +\t/* 65.259980246 */\n" ">> +\tif ((settings->m_int < 65) ||\n" - ">> +\t\t((settings->m_int =3D=3D 65) && (settings->m_frac <=3D 139575831)))\n" - ">> +\t\tlp =3D 0x22;\n" + ">> +\t\t((settings->m_int == 65) && (settings->m_frac <= 139575831)))\n" + ">> +\t\tlp = 0x22;\n" ">> +\t/* 67.859763463 */\n" ">> +\telse if ((settings->m_int < 67) ||\n" - ">> +\t\t((settings->m_int =3D=3D 67) && (settings->m_frac <=3D 461581994)))\n" - ">> +\t\tlp =3D 0x23;\n" + ">> +\t\t((settings->m_int == 67) && (settings->m_frac <= 461581994)))\n" + ">> +\t\tlp = 0x23;\n" ">> +\t/* 72.937624981 */\n" ">> +\telse if ((settings->m_int < 72) ||\n" - ">> +\t\t((settings->m_int =3D=3D 72) && (settings->m_frac <=3D 503383578)))\n" - ">> +\t\tlp =3D 0x33;\n" + ">> +\t\t((settings->m_int == 72) && (settings->m_frac <= 503383578)))\n" + ">> +\t\tlp = 0x33;\n" ">> +\t/* 75.843265046 */\n" ">> +\telse if ((settings->m_int < 75) ||\n" - ">> +\t\t((settings->m_int =3D=3D 75) && (settings->m_frac <=3D 452724474)))\n" - ">> +\t\tlp =3D 0x34;\n" + ">> +\t\t((settings->m_int == 75) && (settings->m_frac <= 452724474)))\n" + ">> +\t\tlp = 0x34;\n" ">\n" "> Drop the extra parenthesis on these if statements.\n" ">\n" ">> +\telse\n" - ">> +\t\tlp =3D 0x44;\n" + ">> +\t\tlp = 0x44;\n" ">> +\n" - ">> +\terr =3D regmap_write(data->regmap, SI514_REG_LP, lp);\n" + ">> +\terr = regmap_write(data->regmap, SI514_REG_LP, lp);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err;\n" ">> +\n" - ">> +\treg[0] =3D settings->m_frac & 0xff;\n" - ">> +\treg[1] =3D (settings->m_frac >> 8) & 0xff;\n" - ">> +\treg[2] =3D (settings->m_frac >> 16) & 0xff;\n" - ">> +\treg[3] =3D ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff;\n" - ">> +\treg[4] =3D (settings->m_int >> 3) & 0xff;\n" - ">> +\treg[5] =3D (settings->hs_div) & 0xff;\n" - ">> +\treg[6] =3D (((settings->hs_div) >> 8) |\n" + ">> +\treg[0] = settings->m_frac & 0xff;\n" + ">> +\treg[1] = (settings->m_frac >> 8) & 0xff;\n" + ">> +\treg[2] = (settings->m_frac >> 16) & 0xff;\n" + ">> +\treg[3] = ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff;\n" + ">> +\treg[4] = (settings->m_int >> 3) & 0xff;\n" + ">> +\treg[5] = (settings->hs_div) & 0xff;\n" + ">> +\treg[6] = (((settings->hs_div) >> 8) |\n" ">> +\t\t\t(settings->ls_div_bits << 4)) & 0xff;\n" ">\n" "> The & 0xff part is redundant. Assignment truncates for us.\n" ">\n" ">> +\n" - ">> +\terr =3D regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2);\n" + ">> +\terr = regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err;\n" ">> +\t/* Writing to SI514_REG_M_INT_FRAC triggers the clock change, so that\n" @@ -190,36 +186,36 @@ ">> +\t\treturn -EINVAL;\n" ">> +\n" ">> +\t/* Determine the minimum value of LS_DIV and resulting target freq. */\n" - ">> +\tls_freq =3D frequency;\n" - ">> +\tif (frequency >=3D (FVCO_MIN / HS_DIV_MAX))\n" - ">> +\t\tsettings->ls_div_bits =3D 0;\n" + ">> +\tls_freq = frequency;\n" + ">> +\tif (frequency >= (FVCO_MIN / HS_DIV_MAX))\n" + ">> +\t\tsettings->ls_div_bits = 0;\n" ">> +\telse {\n" - ">> +\t\tres =3D 1;\n" - ">> +\t\ttmp =3D 2 * HS_DIV_MAX;\n" - ">> +\t\twhile (tmp <=3D (HS_DIV_MAX*32)) {\n" + ">> +\t\tres = 1;\n" + ">> +\t\ttmp = 2 * HS_DIV_MAX;\n" + ">> +\t\twhile (tmp <= (HS_DIV_MAX*32)) {\n" ">\n" "> Please add some space here between HS_DIV_MAX and 32.\n" ">\n" - ">> +\t\t\tif ((frequency * tmp) >=3D FVCO_MIN)\n" + ">> +\t\t\tif ((frequency * tmp) >= FVCO_MIN)\n" ">> +\t\t\t\tbreak; /* We're done */\n" ">\n" "> Yep, that's what break in a loop usually means.\n" ">\n" ">> +\t\t\t++res;\n" - ">> +\t\t\ttmp <<=3D 1;\n" + ">> +\t\t\ttmp <<= 1;\n" ">> +\t\t}\n" - ">> +\t\tsettings->ls_div_bits =3D res;\n" - ">> +\t\tls_freq =3D frequency << res;\n" + ">> +\t\tsettings->ls_div_bits = res;\n" + ">> +\t\tls_freq = frequency << res;\n" ">> +\t}\n" ">> +\n" ">> +\t/* Determine minimum HS_DIV, round up to even number */\n" - ">> +\tsettings->hs_div =3D DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1;\n" + ">> +\tsettings->hs_div = DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1;\n" ">> +\n" - ">> +\t/* M =3D LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */\n" - ">> +\tm =3D ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2);\n" + ">> +\t/* M = LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */\n" + ">> +\tm = ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2);\n" ">> +\tdo_div(m, FXO);\n" - ">> +\tsettings->m_frac =3D (u32)m & (BIT(29) - 1);\n" - ">> +\tsettings->m_int =3D (u32)(m >> 29);\n" + ">> +\tsettings->m_frac = (u32)m & (BIT(29) - 1);\n" + ">> +\tsettings->m_int = (u32)(m >> 29);\n" ">> +\n" ">> +\treturn 0;\n" ">> +}\n" @@ -227,7 +223,7 @@ ">> +/* Calculate resulting frequency given the register settings */\n" ">> +static unsigned long si514_calc_rate(struct clk_si514_muldiv *settings)\n" ">> +{\n" - ">> +\tu64 m =3D settings->m_frac | ((u64)settings->m_int << 29);\n" + ">> +\tu64 m = settings->m_frac | ((u64)settings->m_int << 29);\n" ">> +\n" ">> +\treturn ((u32)(((m * FXO) + (FXO/2)) >> 29)) /\n" ">\n" @@ -244,7 +240,7 @@ ">> +\n" "> [...]\n" ">> +\n" - ">> +\terr =3D si514_calc_muldiv(&settings, rate);\n" + ">> +\terr = si514_calc_muldiv(&settings, rate);\n" ">> +\tif (err)\n" ">> +\t\treturn err;\n" ">> +\n" @@ -252,8 +248,7 @@ ">> +}\n" ">> +\n" ">> +/* Update output frequency for big frequency changes (> 1000 ppm).\n" - ">> + * The chip supports <1000ppm changes \"on the fly\", we haven't implemen=\n" - "ted\n" + ">> + * The chip supports <1000ppm changes \"on the fly\", we haven't implemented\n" ">> + * that here. */\n" ">\n" "> Please fix comment style.\n" @@ -261,23 +256,22 @@ ">> +static int si514_set_rate(struct clk_hw *hw, unsigned long rate,\n" ">> +\t\tunsigned long parent_rate)\n" ">> +{\n" - ">> +\tstruct clk_si514 *data =3D to_clk_si514(hw);\n" + ">> +\tstruct clk_si514 *data = to_clk_si514(hw);\n" ">> +\tstruct clk_si514_muldiv settings;\n" ">> +\tint err;\n" ">> +\n" - ">> +\terr =3D si514_calc_muldiv(&settings, rate);\n" + ">> +\terr = si514_calc_muldiv(&settings, rate);\n" ">> +\tif (err)\n" ">> +\t\treturn err;\n" ">> +\n" ">> +\tsi514_enable_output(data, false);\n" ">> +\n" - ">> +\terr =3D si514_set_muldiv(data, &settings);\n" + ">> +\terr = si514_set_muldiv(data, &settings);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err; /* Undefined state now, best to leave disabled */\n" ">> +\n" ">> +\t/* Trigger calibration */\n" - ">> +\terr =3D regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FC=\n" - "AL);\n" + ">> +\terr = regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FCAL);\n" ">> +\n" ">> +\t/* Applying a new frequency can take up to 10ms */\n" ">> +\tusleep_range(10000, 12000);\n" @@ -289,25 +283,22 @@ "\n" "Good question, I don't recall why I made this choice.\n" "\n" - "For a client, when set_rate returns error, it would expect either the clock=\n" - "=20\n" - "still running at the old frequency, or not running at all. From that=20\n" - "perspective, since the PLL has been reprogrammed, the better choice would b=\n" - "e=20\n" - "to leave it disabled. Expected behaviour for the client is going to be to f=\n" - "ix=20\n" + "For a client, when set_rate returns error, it would expect either the clock \n" + "still running at the old frequency, or not running at all. From that \n" + "perspective, since the PLL has been reprogrammed, the better choice would be \n" + "to leave it disabled. Expected behaviour for the client is going to be to fix \n" "the I2C bus problems and try again.\n" "\n" ">> +\treturn err;\n" ">> +}\n" ">> +\n" - ">> +static const struct clk_ops si514_clk_ops =3D {\n" - ">> +\t.recalc_rate =3D si514_recalc_rate,\n" - ">> +\t.round_rate =3D si514_round_rate,\n" - ">> +\t.set_rate =3D si514_set_rate,\n" + ">> +static const struct clk_ops si514_clk_ops = {\n" + ">> +\t.recalc_rate = si514_recalc_rate,\n" + ">> +\t.round_rate = si514_round_rate,\n" + ">> +\t.set_rate = si514_set_rate,\n" ">> +};\n" ">> +\n" - ">> +static struct regmap_config si514_regmap_config =3D {\n" + ">> +static struct regmap_config si514_regmap_config = {\n" ">\n" "> const?\n" ">\n" @@ -326,8 +317,7 @@ ">> +\n" ">\n" "\n" - "I'll post a v2 patch with the requested changes soon. Thank you for your re=\n" - "view.\n" + "I'll post a v2 patch with the requested changes soon. Thank you for your review.\n" "\n" "Mike.\n" "\n" @@ -342,9 +332,18 @@ "Postbus 440, NL-5680 AK Best\n" "Telefoon: +31 (0) 499 33 69 79\n" "Telefax: +31 (0) 499 33 69 70\n" - "E-mail: mike.looijmans@topicproducts.com\n" + "E-mail: mike.looijmans-yhtFebqMsb9it5bFGTN0CAC/G2K4zDHf@public.gmane.org\n" "Website: www.topicproducts.com\n" "\n" - Please consider the environment before printing this e-mail + "Please consider the environment before printing this e-mail\n" + "\n" + "\n" + "\n" + "\n" + "\n" + "--\n" + "To unsubscribe from this list: send the line \"unsubscribe devicetree\" in\n" + "the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org\n" + More majordomo info at http://vger.kernel.org/majordomo-info.html -b3470054f77f66c5a0ac90c16ce10de0dedb6d888d2221e5ef00038659427cb4 +cbb3e28fd93e09b9f85d97f4cc124a8afbf3cd9318a6081e5c380d599a331711
diff --git a/a/1.txt b/N2/1.txt index c46288b..eb0e5d7 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,4 +1,4 @@ -=EF=BB=BFOn 02-10-15 01:34, Stephen Boyd wrote: +On 02-10-15 01:34, Stephen Boyd wrote: > On 09/17, Mike Looijmans wrote: >> This patch adds the driver and devicetree documentation for the >> Silicon Labs SI514 clock generator chip. This is an I2C controlled @@ -10,8 +10,7 @@ >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- ->> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/= -Documentation/devicetree/bindings/clock/silabs,si514.txt +>> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/Documentation/devicetree/bindings/clock/silabs,si514.txt >> new file mode 100644 >> index 0000000..05964d1 >> --- /dev/null @@ -20,8 +19,7 @@ Documentation/devicetree/bindings/clock/silabs,si514.txt >> +Binding for Silicon Labs 514 programmable I2C clock generator. >> + >> +Reference ->> +This binding uses the common clock binding[1]. Details about the device= - can be +>> +This binding uses the common clock binding[1]. Details about the device can be >> +found in the datasheet[2]. >> + >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt @@ -34,25 +32,22 @@ Documentation/devicetree/bindings/clock/silabs,si514.txt >> + - #clock-cells: From common clock bindings: Shall be 0. >> + >> +Optional properties: ->> + - clock-output-names: From common clock bindings. Recommended to be "s= -i514". ->> + - clock-frequency: Output frequency to generate. This defines the outp= -ut +>> + - clock-output-names: From common clock bindings. Recommended to be "si514". +>> + - clock-frequency: Output frequency to generate. This defines the output >> + frequency set during boot. It can be reprogrammed during >> + runtime through the common clock framework. > > Can we use assigned clock rates instead of this property? -I'll first need to learn what 'assigned clock rates' means. But I suspect t= -he=20 +I'll first need to learn what 'assigned clock rates' means. But I suspect the answer will be yes. >> + >> +Example: >> + si514: clock-generator@55 { ->> + reg =3D <0x55>; ->> + #clock-cells =3D <0>; ->> + compatible =3D "silabs,si514"; +>> + reg = <0x55>; +>> + #clock-cells = <0>; +>> + compatible = "silabs,si514"; >> + }; >> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig >> index 42f7120..6ac7deb5 100644 @@ -92,7 +87,7 @@ It calls various 'of_' methods unconditionally, so I'd think so. > I'd expect some sort of linux/clk.h include here if we're using > clk APIs. -It probably got dragged in by the clk-provider.h include, including it=20 +It probably got dragged in by the clk-provider.h include, including it specifically would be more appropriate. > @@ -117,43 +112,43 @@ specifically would be more appropriate. >> + /* Calculate LP1/LP2 according to table 13 in the datasheet */ >> + /* 65.259980246 */ >> + if ((settings->m_int < 65) || ->> + ((settings->m_int =3D=3D 65) && (settings->m_frac <=3D 139575831))) ->> + lp =3D 0x22; +>> + ((settings->m_int == 65) && (settings->m_frac <= 139575831))) +>> + lp = 0x22; >> + /* 67.859763463 */ >> + else if ((settings->m_int < 67) || ->> + ((settings->m_int =3D=3D 67) && (settings->m_frac <=3D 461581994))) ->> + lp =3D 0x23; +>> + ((settings->m_int == 67) && (settings->m_frac <= 461581994))) +>> + lp = 0x23; >> + /* 72.937624981 */ >> + else if ((settings->m_int < 72) || ->> + ((settings->m_int =3D=3D 72) && (settings->m_frac <=3D 503383578))) ->> + lp =3D 0x33; +>> + ((settings->m_int == 72) && (settings->m_frac <= 503383578))) +>> + lp = 0x33; >> + /* 75.843265046 */ >> + else if ((settings->m_int < 75) || ->> + ((settings->m_int =3D=3D 75) && (settings->m_frac <=3D 452724474))) ->> + lp =3D 0x34; +>> + ((settings->m_int == 75) && (settings->m_frac <= 452724474))) +>> + lp = 0x34; > > Drop the extra parenthesis on these if statements. > >> + else ->> + lp =3D 0x44; +>> + lp = 0x44; >> + ->> + err =3D regmap_write(data->regmap, SI514_REG_LP, lp); +>> + err = regmap_write(data->regmap, SI514_REG_LP, lp); >> + if (err < 0) >> + return err; >> + ->> + reg[0] =3D settings->m_frac & 0xff; ->> + reg[1] =3D (settings->m_frac >> 8) & 0xff; ->> + reg[2] =3D (settings->m_frac >> 16) & 0xff; ->> + reg[3] =3D ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff; ->> + reg[4] =3D (settings->m_int >> 3) & 0xff; ->> + reg[5] =3D (settings->hs_div) & 0xff; ->> + reg[6] =3D (((settings->hs_div) >> 8) | +>> + reg[0] = settings->m_frac & 0xff; +>> + reg[1] = (settings->m_frac >> 8) & 0xff; +>> + reg[2] = (settings->m_frac >> 16) & 0xff; +>> + reg[3] = ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff; +>> + reg[4] = (settings->m_int >> 3) & 0xff; +>> + reg[5] = (settings->hs_div) & 0xff; +>> + reg[6] = (((settings->hs_div) >> 8) | >> + (settings->ls_div_bits << 4)) & 0xff; > > The & 0xff part is redundant. Assignment truncates for us. > >> + ->> + err =3D regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2); +>> + err = regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2); >> + if (err < 0) >> + return err; >> + /* Writing to SI514_REG_M_INT_FRAC triggers the clock change, so that @@ -177,36 +172,36 @@ specifically would be more appropriate. >> + return -EINVAL; >> + >> + /* Determine the minimum value of LS_DIV and resulting target freq. */ ->> + ls_freq =3D frequency; ->> + if (frequency >=3D (FVCO_MIN / HS_DIV_MAX)) ->> + settings->ls_div_bits =3D 0; +>> + ls_freq = frequency; +>> + if (frequency >= (FVCO_MIN / HS_DIV_MAX)) +>> + settings->ls_div_bits = 0; >> + else { ->> + res =3D 1; ->> + tmp =3D 2 * HS_DIV_MAX; ->> + while (tmp <=3D (HS_DIV_MAX*32)) { +>> + res = 1; +>> + tmp = 2 * HS_DIV_MAX; +>> + while (tmp <= (HS_DIV_MAX*32)) { > > Please add some space here between HS_DIV_MAX and 32. > ->> + if ((frequency * tmp) >=3D FVCO_MIN) +>> + if ((frequency * tmp) >= FVCO_MIN) >> + break; /* We're done */ > > Yep, that's what break in a loop usually means. > >> + ++res; ->> + tmp <<=3D 1; +>> + tmp <<= 1; >> + } ->> + settings->ls_div_bits =3D res; ->> + ls_freq =3D frequency << res; +>> + settings->ls_div_bits = res; +>> + ls_freq = frequency << res; >> + } >> + >> + /* Determine minimum HS_DIV, round up to even number */ ->> + settings->hs_div =3D DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1; +>> + settings->hs_div = DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1; >> + ->> + /* M =3D LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */ ->> + m =3D ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2); +>> + /* M = LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */ +>> + m = ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2); >> + do_div(m, FXO); ->> + settings->m_frac =3D (u32)m & (BIT(29) - 1); ->> + settings->m_int =3D (u32)(m >> 29); +>> + settings->m_frac = (u32)m & (BIT(29) - 1); +>> + settings->m_int = (u32)(m >> 29); >> + >> + return 0; >> +} @@ -214,7 +209,7 @@ specifically would be more appropriate. >> +/* Calculate resulting frequency given the register settings */ >> +static unsigned long si514_calc_rate(struct clk_si514_muldiv *settings) >> +{ ->> + u64 m =3D settings->m_frac | ((u64)settings->m_int << 29); +>> + u64 m = settings->m_frac | ((u64)settings->m_int << 29); >> + >> + return ((u32)(((m * FXO) + (FXO/2)) >> 29)) / > @@ -231,7 +226,7 @@ I'll refactor it into something more readable. >> + > [...] >> + ->> + err =3D si514_calc_muldiv(&settings, rate); +>> + err = si514_calc_muldiv(&settings, rate); >> + if (err) >> + return err; >> + @@ -239,8 +234,7 @@ I'll refactor it into something more readable. >> +} >> + >> +/* Update output frequency for big frequency changes (> 1000 ppm). ->> + * The chip supports <1000ppm changes "on the fly", we haven't implemen= -ted +>> + * The chip supports <1000ppm changes "on the fly", we haven't implemented >> + * that here. */ > > Please fix comment style. @@ -248,23 +242,22 @@ ted >> +static int si514_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ ->> + struct clk_si514 *data =3D to_clk_si514(hw); +>> + struct clk_si514 *data = to_clk_si514(hw); >> + struct clk_si514_muldiv settings; >> + int err; >> + ->> + err =3D si514_calc_muldiv(&settings, rate); +>> + err = si514_calc_muldiv(&settings, rate); >> + if (err) >> + return err; >> + >> + si514_enable_output(data, false); >> + ->> + err =3D si514_set_muldiv(data, &settings); +>> + err = si514_set_muldiv(data, &settings); >> + if (err < 0) >> + return err; /* Undefined state now, best to leave disabled */ >> + >> + /* Trigger calibration */ ->> + err =3D regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FC= -AL); +>> + err = regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FCAL); >> + >> + /* Applying a new frequency can take up to 10ms */ >> + usleep_range(10000, 12000); @@ -276,25 +269,22 @@ AL); Good question, I don't recall why I made this choice. -For a client, when set_rate returns error, it would expect either the clock= -=20 -still running at the old frequency, or not running at all. From that=20 -perspective, since the PLL has been reprogrammed, the better choice would b= -e=20 -to leave it disabled. Expected behaviour for the client is going to be to f= -ix=20 +For a client, when set_rate returns error, it would expect either the clock +still running at the old frequency, or not running at all. From that +perspective, since the PLL has been reprogrammed, the better choice would be +to leave it disabled. Expected behaviour for the client is going to be to fix the I2C bus problems and try again. >> + return err; >> +} >> + ->> +static const struct clk_ops si514_clk_ops =3D { ->> + .recalc_rate =3D si514_recalc_rate, ->> + .round_rate =3D si514_round_rate, ->> + .set_rate =3D si514_set_rate, +>> +static const struct clk_ops si514_clk_ops = { +>> + .recalc_rate = si514_recalc_rate, +>> + .round_rate = si514_round_rate, +>> + .set_rate = si514_set_rate, >> +}; >> + ->> +static struct regmap_config si514_regmap_config =3D { +>> +static struct regmap_config si514_regmap_config = { > > const? > @@ -313,8 +303,7 @@ the I2C bus problems and try again. >> + > -I'll post a v2 patch with the requested changes soon. Thank you for your re= -view. +I'll post a v2 patch with the requested changes soon. Thank you for your review. Mike. diff --git a/a/content_digest b/N2/content_digest index 54604f8..3e7ec27 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -11,7 +11,7 @@ " <linux-kernel@vger.kernel.org>\0" "\00:1\0" "b\0" - "=EF=BB=BFOn 02-10-15 01:34, Stephen Boyd wrote:\n" + "\357\273\277On 02-10-15 01:34, Stephen Boyd wrote:\n" "> On 09/17, Mike Looijmans wrote:\n" ">> This patch adds the driver and devicetree documentation for the\n" ">> Silicon Labs SI514 clock generator chip. This is an I2C controlled\n" @@ -23,8 +23,7 @@ ">>\n" ">> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>\n" ">> ---\n" - ">> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/=\n" - "Documentation/devicetree/bindings/clock/silabs,si514.txt\n" + ">> diff --git a/Documentation/devicetree/bindings/clock/silabs,si514.txt b/Documentation/devicetree/bindings/clock/silabs,si514.txt\n" ">> new file mode 100644\n" ">> index 0000000..05964d1\n" ">> --- /dev/null\n" @@ -33,8 +32,7 @@ ">> +Binding for Silicon Labs 514 programmable I2C clock generator.\n" ">> +\n" ">> +Reference\n" - ">> +This binding uses the common clock binding[1]. Details about the device=\n" - " can be\n" + ">> +This binding uses the common clock binding[1]. Details about the device can be\n" ">> +found in the datasheet[2].\n" ">> +\n" ">> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt\n" @@ -47,25 +45,22 @@ ">> + - #clock-cells: From common clock bindings: Shall be 0.\n" ">> +\n" ">> +Optional properties:\n" - ">> + - clock-output-names: From common clock bindings. Recommended to be \"s=\n" - "i514\".\n" - ">> + - clock-frequency: Output frequency to generate. This defines the outp=\n" - "ut\n" + ">> + - clock-output-names: From common clock bindings. Recommended to be \"si514\".\n" + ">> + - clock-frequency: Output frequency to generate. This defines the output\n" ">> +\t\t frequency set during boot. It can be reprogrammed during\n" ">> +\t\t runtime through the common clock framework.\n" ">\n" "> Can we use assigned clock rates instead of this property?\n" "\n" - "I'll first need to learn what 'assigned clock rates' means. But I suspect t=\n" - "he=20\n" + "I'll first need to learn what 'assigned clock rates' means. But I suspect the \n" "answer will be yes.\n" "\n" ">> +\n" ">> +Example:\n" ">> +\tsi514: clock-generator@55 {\n" - ">> +\t\treg =3D <0x55>;\n" - ">> +\t\t#clock-cells =3D <0>;\n" - ">> +\t\tcompatible =3D \"silabs,si514\";\n" + ">> +\t\treg = <0x55>;\n" + ">> +\t\t#clock-cells = <0>;\n" + ">> +\t\tcompatible = \"silabs,si514\";\n" ">> +\t};\n" ">> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig\n" ">> index 42f7120..6ac7deb5 100644\n" @@ -105,7 +100,7 @@ "> I'd expect some sort of linux/clk.h include here if we're using\n" "> clk APIs.\n" "\n" - "It probably got dragged in by the clk-provider.h include, including it=20\n" + "It probably got dragged in by the clk-provider.h include, including it \n" "specifically would be more appropriate.\n" "\n" ">\n" @@ -130,43 +125,43 @@ ">> +\t/* Calculate LP1/LP2 according to table 13 in the datasheet */\n" ">> +\t/* 65.259980246 */\n" ">> +\tif ((settings->m_int < 65) ||\n" - ">> +\t\t((settings->m_int =3D=3D 65) && (settings->m_frac <=3D 139575831)))\n" - ">> +\t\tlp =3D 0x22;\n" + ">> +\t\t((settings->m_int == 65) && (settings->m_frac <= 139575831)))\n" + ">> +\t\tlp = 0x22;\n" ">> +\t/* 67.859763463 */\n" ">> +\telse if ((settings->m_int < 67) ||\n" - ">> +\t\t((settings->m_int =3D=3D 67) && (settings->m_frac <=3D 461581994)))\n" - ">> +\t\tlp =3D 0x23;\n" + ">> +\t\t((settings->m_int == 67) && (settings->m_frac <= 461581994)))\n" + ">> +\t\tlp = 0x23;\n" ">> +\t/* 72.937624981 */\n" ">> +\telse if ((settings->m_int < 72) ||\n" - ">> +\t\t((settings->m_int =3D=3D 72) && (settings->m_frac <=3D 503383578)))\n" - ">> +\t\tlp =3D 0x33;\n" + ">> +\t\t((settings->m_int == 72) && (settings->m_frac <= 503383578)))\n" + ">> +\t\tlp = 0x33;\n" ">> +\t/* 75.843265046 */\n" ">> +\telse if ((settings->m_int < 75) ||\n" - ">> +\t\t((settings->m_int =3D=3D 75) && (settings->m_frac <=3D 452724474)))\n" - ">> +\t\tlp =3D 0x34;\n" + ">> +\t\t((settings->m_int == 75) && (settings->m_frac <= 452724474)))\n" + ">> +\t\tlp = 0x34;\n" ">\n" "> Drop the extra parenthesis on these if statements.\n" ">\n" ">> +\telse\n" - ">> +\t\tlp =3D 0x44;\n" + ">> +\t\tlp = 0x44;\n" ">> +\n" - ">> +\terr =3D regmap_write(data->regmap, SI514_REG_LP, lp);\n" + ">> +\terr = regmap_write(data->regmap, SI514_REG_LP, lp);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err;\n" ">> +\n" - ">> +\treg[0] =3D settings->m_frac & 0xff;\n" - ">> +\treg[1] =3D (settings->m_frac >> 8) & 0xff;\n" - ">> +\treg[2] =3D (settings->m_frac >> 16) & 0xff;\n" - ">> +\treg[3] =3D ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff;\n" - ">> +\treg[4] =3D (settings->m_int >> 3) & 0xff;\n" - ">> +\treg[5] =3D (settings->hs_div) & 0xff;\n" - ">> +\treg[6] =3D (((settings->hs_div) >> 8) |\n" + ">> +\treg[0] = settings->m_frac & 0xff;\n" + ">> +\treg[1] = (settings->m_frac >> 8) & 0xff;\n" + ">> +\treg[2] = (settings->m_frac >> 16) & 0xff;\n" + ">> +\treg[3] = ((settings->m_frac >> 24) | (settings->m_int << 5)) & 0xff;\n" + ">> +\treg[4] = (settings->m_int >> 3) & 0xff;\n" + ">> +\treg[5] = (settings->hs_div) & 0xff;\n" + ">> +\treg[6] = (((settings->hs_div) >> 8) |\n" ">> +\t\t\t(settings->ls_div_bits << 4)) & 0xff;\n" ">\n" "> The & 0xff part is redundant. Assignment truncates for us.\n" ">\n" ">> +\n" - ">> +\terr =3D regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2);\n" + ">> +\terr = regmap_bulk_write(data->regmap, SI514_REG_HS_DIV, reg + 5, 2);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err;\n" ">> +\t/* Writing to SI514_REG_M_INT_FRAC triggers the clock change, so that\n" @@ -190,36 +185,36 @@ ">> +\t\treturn -EINVAL;\n" ">> +\n" ">> +\t/* Determine the minimum value of LS_DIV and resulting target freq. */\n" - ">> +\tls_freq =3D frequency;\n" - ">> +\tif (frequency >=3D (FVCO_MIN / HS_DIV_MAX))\n" - ">> +\t\tsettings->ls_div_bits =3D 0;\n" + ">> +\tls_freq = frequency;\n" + ">> +\tif (frequency >= (FVCO_MIN / HS_DIV_MAX))\n" + ">> +\t\tsettings->ls_div_bits = 0;\n" ">> +\telse {\n" - ">> +\t\tres =3D 1;\n" - ">> +\t\ttmp =3D 2 * HS_DIV_MAX;\n" - ">> +\t\twhile (tmp <=3D (HS_DIV_MAX*32)) {\n" + ">> +\t\tres = 1;\n" + ">> +\t\ttmp = 2 * HS_DIV_MAX;\n" + ">> +\t\twhile (tmp <= (HS_DIV_MAX*32)) {\n" ">\n" "> Please add some space here between HS_DIV_MAX and 32.\n" ">\n" - ">> +\t\t\tif ((frequency * tmp) >=3D FVCO_MIN)\n" + ">> +\t\t\tif ((frequency * tmp) >= FVCO_MIN)\n" ">> +\t\t\t\tbreak; /* We're done */\n" ">\n" "> Yep, that's what break in a loop usually means.\n" ">\n" ">> +\t\t\t++res;\n" - ">> +\t\t\ttmp <<=3D 1;\n" + ">> +\t\t\ttmp <<= 1;\n" ">> +\t\t}\n" - ">> +\t\tsettings->ls_div_bits =3D res;\n" - ">> +\t\tls_freq =3D frequency << res;\n" + ">> +\t\tsettings->ls_div_bits = res;\n" + ">> +\t\tls_freq = frequency << res;\n" ">> +\t}\n" ">> +\n" ">> +\t/* Determine minimum HS_DIV, round up to even number */\n" - ">> +\tsettings->hs_div =3D DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1;\n" + ">> +\tsettings->hs_div = DIV_ROUND_UP(FVCO_MIN >> 1, ls_freq) << 1;\n" ">> +\n" - ">> +\t/* M =3D LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */\n" - ">> +\tm =3D ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2);\n" + ">> +\t/* M = LS_DIV x HS_DIV x frequency / F_XO (in fixed-point) */\n" + ">> +\tm = ((u64)(ls_freq * settings->hs_div) << 29) + (FXO/2);\n" ">> +\tdo_div(m, FXO);\n" - ">> +\tsettings->m_frac =3D (u32)m & (BIT(29) - 1);\n" - ">> +\tsettings->m_int =3D (u32)(m >> 29);\n" + ">> +\tsettings->m_frac = (u32)m & (BIT(29) - 1);\n" + ">> +\tsettings->m_int = (u32)(m >> 29);\n" ">> +\n" ">> +\treturn 0;\n" ">> +}\n" @@ -227,7 +222,7 @@ ">> +/* Calculate resulting frequency given the register settings */\n" ">> +static unsigned long si514_calc_rate(struct clk_si514_muldiv *settings)\n" ">> +{\n" - ">> +\tu64 m =3D settings->m_frac | ((u64)settings->m_int << 29);\n" + ">> +\tu64 m = settings->m_frac | ((u64)settings->m_int << 29);\n" ">> +\n" ">> +\treturn ((u32)(((m * FXO) + (FXO/2)) >> 29)) /\n" ">\n" @@ -244,7 +239,7 @@ ">> +\n" "> [...]\n" ">> +\n" - ">> +\terr =3D si514_calc_muldiv(&settings, rate);\n" + ">> +\terr = si514_calc_muldiv(&settings, rate);\n" ">> +\tif (err)\n" ">> +\t\treturn err;\n" ">> +\n" @@ -252,8 +247,7 @@ ">> +}\n" ">> +\n" ">> +/* Update output frequency for big frequency changes (> 1000 ppm).\n" - ">> + * The chip supports <1000ppm changes \"on the fly\", we haven't implemen=\n" - "ted\n" + ">> + * The chip supports <1000ppm changes \"on the fly\", we haven't implemented\n" ">> + * that here. */\n" ">\n" "> Please fix comment style.\n" @@ -261,23 +255,22 @@ ">> +static int si514_set_rate(struct clk_hw *hw, unsigned long rate,\n" ">> +\t\tunsigned long parent_rate)\n" ">> +{\n" - ">> +\tstruct clk_si514 *data =3D to_clk_si514(hw);\n" + ">> +\tstruct clk_si514 *data = to_clk_si514(hw);\n" ">> +\tstruct clk_si514_muldiv settings;\n" ">> +\tint err;\n" ">> +\n" - ">> +\terr =3D si514_calc_muldiv(&settings, rate);\n" + ">> +\terr = si514_calc_muldiv(&settings, rate);\n" ">> +\tif (err)\n" ">> +\t\treturn err;\n" ">> +\n" ">> +\tsi514_enable_output(data, false);\n" ">> +\n" - ">> +\terr =3D si514_set_muldiv(data, &settings);\n" + ">> +\terr = si514_set_muldiv(data, &settings);\n" ">> +\tif (err < 0)\n" ">> +\t\treturn err; /* Undefined state now, best to leave disabled */\n" ">> +\n" ">> +\t/* Trigger calibration */\n" - ">> +\terr =3D regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FC=\n" - "AL);\n" + ">> +\terr = regmap_write(data->regmap, SI514_REG_CONTROL, SI514_CONTROL_FCAL);\n" ">> +\n" ">> +\t/* Applying a new frequency can take up to 10ms */\n" ">> +\tusleep_range(10000, 12000);\n" @@ -289,25 +282,22 @@ "\n" "Good question, I don't recall why I made this choice.\n" "\n" - "For a client, when set_rate returns error, it would expect either the clock=\n" - "=20\n" - "still running at the old frequency, or not running at all. From that=20\n" - "perspective, since the PLL has been reprogrammed, the better choice would b=\n" - "e=20\n" - "to leave it disabled. Expected behaviour for the client is going to be to f=\n" - "ix=20\n" + "For a client, when set_rate returns error, it would expect either the clock \n" + "still running at the old frequency, or not running at all. From that \n" + "perspective, since the PLL has been reprogrammed, the better choice would be \n" + "to leave it disabled. Expected behaviour for the client is going to be to fix \n" "the I2C bus problems and try again.\n" "\n" ">> +\treturn err;\n" ">> +}\n" ">> +\n" - ">> +static const struct clk_ops si514_clk_ops =3D {\n" - ">> +\t.recalc_rate =3D si514_recalc_rate,\n" - ">> +\t.round_rate =3D si514_round_rate,\n" - ">> +\t.set_rate =3D si514_set_rate,\n" + ">> +static const struct clk_ops si514_clk_ops = {\n" + ">> +\t.recalc_rate = si514_recalc_rate,\n" + ">> +\t.round_rate = si514_round_rate,\n" + ">> +\t.set_rate = si514_set_rate,\n" ">> +};\n" ">> +\n" - ">> +static struct regmap_config si514_regmap_config =3D {\n" + ">> +static struct regmap_config si514_regmap_config = {\n" ">\n" "> const?\n" ">\n" @@ -326,8 +316,7 @@ ">> +\n" ">\n" "\n" - "I'll post a v2 patch with the requested changes soon. Thank you for your re=\n" - "view.\n" + "I'll post a v2 patch with the requested changes soon. Thank you for your review.\n" "\n" "Mike.\n" "\n" @@ -347,4 +336,4 @@ "\n" Please consider the environment before printing this e-mail -b3470054f77f66c5a0ac90c16ce10de0dedb6d888d2221e5ef00038659427cb4 +fedcf57601ebed0f52b1abc10181cb798a4555894a973670dc9aec89fcec7c2b
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.