Hi,
On 10/25/21 13:24, Andy Shevchenko wrote:
> On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
>> the kernel the Regulators and Clocks are controlled by an OpRegion
>> driver designed to work with power control methods defined in ACPI, but
>> some platforms lack those methods, meaning drivers need to be able to
>> consume the resources of these chips through the usual frameworks.
>>
>> This commit adds a driver for the clocks provided by the tps68470,
>> and is designed to bind to the platform_device registered by the
>> intel_skl_int3472 module.
>
> ...
>
>> +/*
>> + * The PLL is used to multiply the crystal oscillator
>> + * frequency range of 3 MHz to 27 MHz by a programmable
>> + * factor of F = (M/N)*(1/P) such that the output
>> + * available at the HCLK_A or HCLK_B pins are in the range
>> + * of 4 MHz to 64 MHz in increments of 0.1 MHz
>
> Missed (grammatical) period.
Thx, fixed for v5.
>
>> + *
>> + * hclk_# = osc_in * (((plldiv*2)+320) / (xtaldiv+30)) * (1 / 2^postdiv)
>> + *
>> + * PLL_REF_CLK should be as close as possible to 100kHz
>> + * PLL_REF_CLK = input clk / XTALDIV[7:0] + 30)
>> + *
>> + * PLL_VCO_CLK = (PLL_REF_CLK * (plldiv*2 + 320))
>> + *
>> + * BOOST should be as close as possible to 2Mhz
>> + * BOOST = PLL_VCO_CLK / (BOOSTDIV[4:0] + 16) *
>> + *
>> + * BUCK should be as close as possible to 5.2Mhz
>> + * BUCK = PLL_VCO_CLK / (BUCKDIV[3:0] + 5)
>> + *
>> + * osc_in xtaldiv plldiv postdiv hclk_#
>> + * 20Mhz 170 32 1 19.2Mhz
>> + * 20Mhz 170 40 1 20Mhz
>> + * 20Mhz 170 80 1 24Mhz
>
>> + *
>
> Redundant empty line.
Removed for v5.
>> + */
>
> ...
>
>> + /* disable clock first */
>
> Disable
> first...
>
>> + /* and then tri-state the clock outputs */
>
> ...and
Fixed for v5.
> ...
>
>> + for (i = 0; i < ARRAY_SIZE(clk_freqs); i++) {
>> + diff = clk_freqs[i].freq - rate;
>> + if (diff == 0)
>> + return i;
>
>> + diff = abs(diff);
>
> This needs a comment why higher (lower) frequency is okay.
This function is called in 2 places:
1. From tps68470_clk_round_rate(), where higher/lower clearly is ok,
(see the function name) so no comment needed.
2. From tps68470_clk_set_rate() where it is NOT ok and this is
enforced in the caller:
unsigned int idx = tps68470_clk_cfg_lookup(rate);
if (rate != clk_freqs[idx].freq)
return -EINVAL;
This is not easy to describe in a comment, while being obvious
if someone looking at this actually looks at the callers.
>
>> + if (diff < best_diff) {
>> + best_diff = diff;
>> + best_idx = i;
>> + }
>> + }
>
> ...
>
>> + if (pdata) {
>> + ret = devm_clk_hw_register_clkdev(&pdev->dev,
>> + &tps68470_clkdata->clkout_hw,
>> + pdata->consumer_con_id,
>> + pdata->consumer_dev_name);
>
> if (ret)
> return ret;
>
>> + }
>> +
>> + return ret;
>
> return 0;
That was the code in v2, but Stephen (the clk maintainer) asked to
simplify it to its current form, so I'm not going to change this back.
Regards,
Hans
