Re: [PATCH v4 05/11] clk: Introduce clk-tps68470 driver

[Hans de Goede]

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