lkml.org 
[lkml]   [2021]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 05/11] clk: Introduce clk-tps68470 driver
From
Hi,

On 10/25/21 13:24, Andy Shevchenko wrote:
> On Mon, Oct 25, 2021 at 12:42 PM Hans de Goede <hdegoede@redhat.com> 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

\
 
 \ /
  Last update: 2021-11-01 11:29    [W:0.087 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site