Hi Stephen,
Thank you for the review.
On 10/13/21 21:12, Stephen Boyd wrote:
> Quoting Hans de Goede (2021-10-10 11:57:01)
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index c5b3dc97396a..7dffecac83d1 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -169,6 +169,12 @@ config COMMON_CLK_CDCE706
>> help
>> This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
>>
>> +config COMMON_CLK_TPS68470
>> + tristate "Clock Driver for TI TPS68470 PMIC"
>> + depends on I2C && REGMAP_I2C && INTEL_SKL_INT3472
>
> Pretty sure REGMAP_I2C should be selected, not depended on.
>
> Can it
>
> depends on INTEL_SKL_INT3472 || COMPILE_TEST
>
> so that we don't have to enable the intel specific config to compile
> test this driver?
Ack, all fixed for v4.
>> + help
>> + This driver supports the clocks provided by TPS68470
>> +
>> config COMMON_CLK_CDCE925
>> tristate "Clock driver for TI CDCE913/925/937/949 devices"
>> depends on I2C
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index e42312121e51..6b6a88ae1425 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -63,6 +63,7 @@ obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o
>> obj-$(CONFIG_COMMON_CLK_STM32F) += clk-stm32f4.o
>> obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o
>> obj-$(CONFIG_COMMON_CLK_STM32MP157) += clk-stm32mp1.o
>> +obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o
>> obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
>> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
>> obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o
>> diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c
>> new file mode 100644
>> index 000000000000..27e8cbd0f60e
>> --- /dev/null
>> +++ b/drivers/clk/clk-tps68470.c
>> @@ -0,0 +1,256 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Clock driver for TPS68470 PMIC
>> + *
>> + * Copyright (C) 2018 Intel Corporation
>> + *
>> + * Authors:
>> + * Zaikuo Wang <zaikuo.wang@xxxxxxxxx>
>> + * Tianshu Qiu <tian.shu.qiu@xxxxxxxxx>
>> + * Jian Xu Zheng <jian.xu.zheng@xxxxxxxxx>
>> + * Yuning Pu <yuning.pu@xxxxxxxxx>
>> + * Antti Laakso <antti.laakso@xxxxxxxxx>
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/platform_data/tps68470.h>
>> +#include <linux/regmap.h>
>> +
>> +#define TPS68470_CLK_NAME "tps68470-clk"
>> +
>> +#define to_tps68470_clkdata(clkd) \
>> + container_of(clkd, struct tps68470_clkdata, clkout_hw)
>> +
>> +struct tps68470_clkout_freqs {
>> + unsigned long freq;
>> + unsigned int xtaldiv;
>> + unsigned int plldiv;
>> + unsigned int postdiv;
>> + unsigned int buckdiv;
>> + unsigned int boostdiv;
>> +} clk_freqs[] = {
>> +/*
>> + * 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
>> + *
>> + * 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
>> + *
>> + */
>> + { 19200000, 170, 32, 1, 2, 3 },
>> + { 20000000, 170, 40, 1, 3, 4 },
>> + { 24000000, 170, 80, 1, 4, 8 },
>> +};
>> +
>> +struct tps68470_clkdata {
>> + struct clk_hw clkout_hw;
>> + struct regmap *regmap;
>> + struct clk *clk;
>> + int clk_cfg_idx;
>> +};
>> +
>> +static int tps68470_clk_is_prepared(struct clk_hw *hw)
>> +{
>> + struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
>> + int val;
>> +
>> + if (regmap_read(clkdata->regmap, TPS68470_REG_PLLCTL, &val))
>> + return 0;
>> +
>> + return val & TPS68470_PLL_EN_MASK;
>> +}
>> +
>> +static int tps68470_clk_prepare(struct clk_hw *hw)
>> +{
>> + struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
>> + int idx = clkdata->clk_cfg_idx;
>> +
>> + regmap_write(clkdata->regmap, TPS68470_REG_BOOSTDIV, clk_freqs[idx].boostdiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_BUCKDIV, clk_freqs[idx].buckdiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_PLLSWR, TPS68470_PLLSWR_DEFAULT);
>> + regmap_write(clkdata->regmap, TPS68470_REG_XTALDIV, clk_freqs[idx].xtaldiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_PLLDIV, clk_freqs[idx].plldiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV, clk_freqs[idx].postdiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV2, clk_freqs[idx].postdiv);
>> + regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG2, TPS68470_CLKCFG2_DRV_STR_2MA);
>> +
>> + regmap_write(clkdata->regmap, TPS68470_REG_PLLCTL,
>> + TPS68470_OSC_EXT_CAP_DEFAULT << TPS68470_OSC_EXT_CAP_SHIFT |
>> + TPS68470_CLK_SRC_XTAL << TPS68470_CLK_SRC_SHIFT);
>> +
>> + regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1,
>> + (TPS68470_PLL_OUTPUT_ENABLE <<
>> + TPS68470_OUTPUT_A_SHIFT) |
>> + (TPS68470_PLL_OUTPUT_ENABLE <<
>> + TPS68470_OUTPUT_B_SHIFT));
>> +
>> + regmap_update_bits(clkdata->regmap, TPS68470_REG_PLLCTL,
>> + TPS68470_PLL_EN_MASK, TPS68470_PLL_EN_MASK);
>> +
>> + return 0;
>> +}
>> +
>> +static void tps68470_clk_unprepare(struct clk_hw *hw)
>> +{
>> + struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
>> +
>> + /* disable clock first*/
>> + regmap_update_bits(clkdata->regmap, TPS68470_REG_PLLCTL, TPS68470_PLL_EN_MASK, 0);
>> +
>> + /* write hw defaults */
>
> Is it necessary to reset the registers to 0? Can the comment indicate
> why it is necessary instead of stating what the code is doing?
As mentioned in the commit msg this driver started out of tree, this part
comes unmodified from the out of tree driver.
After inspecting the datasheet you are right and most of these register
clears are not necessary to disable the clock.
Only the clearing of TPS68470_REG_CLKCFG1 is necesary to tristate the
clk output pin. I will remove the rest and add a comment about the
clearing of TPS68470_REG_CLKCFG1.
>
>> + regmap_write(clkdata->regmap, TPS68470_REG_BOOSTDIV, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_BUCKDIV, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_PLLSWR, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_XTALDIV, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_PLLDIV, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG2, 0);
>> + regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1, 0);
>> +}
>> +
>> +static unsigned long tps68470_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>> +{
>> + struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
>> +
>> + return clk_freqs[clkdata->clk_cfg_idx].freq;
>> +}
>> +
>> +static int tps68470_clk_cfg_lookup(unsigned long rate)
>
> unsigned? Doesn't seem to return negative numbers.
Will fix for v4.
>
>> +{
>> + long diff, best_diff = LONG_MAX;
>> + int i, best_idx = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(clk_freqs); i++) {
>> + diff = clk_freqs[i].freq - rate;
>> + if (diff == 0)
>> + return i;
>> +
>> + diff = abs(diff);
>> + if (diff < best_diff) {
>> + best_diff = diff;
>> + best_idx = i;
>> + }
>> + }
>> +
>> + return best_idx;
>> +}
>> +
>> +static long tps68470_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long *parent_rate)
>> +{
>> + int idx = tps68470_clk_cfg_lookup(rate);
>
> unsigned?
Will fix for v4.
>
>> +
>> + return clk_freqs[idx].freq;
>> +}
>> +
>> +static int tps68470_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
>> + int idx = tps68470_clk_cfg_lookup(rate);
>> +
>> + if (rate != clk_freqs[idx].freq)
>> + return -EINVAL;
>> +
>> + clkdata->clk_cfg_idx = idx;
>
> Newline here please.
Done.
> Also, why isn't this function actually writing
> hardware?
set_rate can only be called when the clock is disabled, all the
necessary values are programmed based on the clk_cfg_idx in
tps68470_clk_prepare().
Note there is no enable() since enable() may not sleep and
this device is interfaced over I2C, so the clock is already
enabled from the prepare() op.
>
>> + return 0;
>> +}
>> +
>> +static const struct clk_ops tps68470_clk_ops = {
>> + .is_prepared = tps68470_clk_is_prepared,
>> + .prepare = tps68470_clk_prepare,
>> + .unprepare = tps68470_clk_unprepare,
>> + .recalc_rate = tps68470_clk_recalc_rate,
>> + .round_rate = tps68470_clk_round_rate,
>> + .set_rate = tps68470_clk_set_rate,
>> +};
>> +
>> +static struct clk_init_data tps68470_clk_initdata = {
>
> const?
Ack.
>
>> + .name = TPS68470_CLK_NAME,
>> + .ops = &tps68470_clk_ops,
>> +};
>> +
>> +static int tps68470_clk_probe(struct platform_device *pdev)
>> +{
>> + struct tps68470_clk_platform_data *pdata = pdev->dev.platform_data;
>> + struct tps68470_clkdata *tps68470_clkdata;
>> + int ret;
>> +
>> + tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata),
>> + GFP_KERNEL);
>> + if (!tps68470_clkdata)
>> + return -ENOMEM;
>> +
>> + tps68470_clkdata->regmap = dev_get_drvdata(pdev->dev.parent);
>> + tps68470_clkdata->clkout_hw.init = &tps68470_clk_initdata;
>> + tps68470_clkdata->clk = devm_clk_register(&pdev->dev, &tps68470_clkdata->clkout_hw);
>
> Please use devm_clk_hw_register()
Good idea, done for v4.
>
>> + if (IS_ERR(tps68470_clkdata->clk))
>> + return PTR_ERR(tps68470_clkdata->clk);
>> +
>> + ret = devm_clk_hw_register_clkdev(&pdev->dev, &tps68470_clkdata->clkout_hw,
>> + TPS68470_CLK_NAME, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + 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;
>
> Drop these two lines?
>
>> + }
>> +
>> + return 0;
>
> And then
>
> return ret;
Done for v4.
Regards,
Hans
