Re: [PATCH] clk: Add driver for the si544 clock generator chip

[Stephen Boyd]

Quoting Mike Looijmans (2018-03-13 01:43:59)
> diff --git a/Documentation/devicetree/bindings/clock/silabs,si544.txt b/Documentation/devicetree/bindings/clock/silabs,si544.txt
> new file mode 100644
> index 0000000..eec1787
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/silabs,si544.txt
> @@ -0,0 +1,25 @@
> +Binding for Silicon Labs 544 programmable I2C clock generator.
> +
> +Reference
> +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
> +[2] Si544 datasheet
> +    https://www.silabs.com/documents/public/data-sheets/si544-datasheet.pdf

Can this reference stuff go to the bottom of this document?

> +
> +Required properties:
> + - compatible: One of "silabs,si514a", "silabs,si514b" "silabs,si514c" according
> +               to the speed grade of the chip.
> + - reg: I2C device address.
> + - #clock-cells: From common clock bindings: Shall be 0.
> +
> +Optional properties:
> + - clock-output-names: From common clock bindings. Recommended to be "si544".
> +
> +Example:
> +       si544: clock-generator@55 {

I'm not sure clock-generator is in the list of node names, but we have
some of these already so alright.

> +               reg = <0x55>;
> +               #clock-cells = <0>;
> +               compatible = "silabs,si544b";
> +       };
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 98ce9fc..5c7dc8e 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -91,6 +91,16 @@ config COMMON_CLK_SI514
>           This driver supports the Silicon Labs 514 programmable clock
>           generator.
>  
> +config COMMON_CLK_SI544
> +       tristate "Clock driver for SiLabs 544 devices"
> +       depends on I2C
> +       depends on OF

Does it depend on anything in OF to build?

> +       select REGMAP_I2C
> +       help
> +       ---help---
> +         This driver supports the Silicon Labs 544 programmable clock
> +         generator.
> +
>  config COMMON_CLK_SI570
>         tristate "Clock driver for SiLabs 570 and compatible devices"
>         depends on I2C
> diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
> new file mode 100644
> index 0000000..1947e48
> --- /dev/null
> +++ b/drivers/clk/clk-si544.c
> @@ -0,0 +1,421 @@
> +/*
> + * Driver for Silicon Labs Si544 Programmable Oscillator
> + *
> + * Copyright (C) 2018 Topic Embedded Products
> + *
> + * Author: Mike Looijmans <mike.looijmans@xxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Can we have SPDX style license here?

> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* I2C registers (decimal as in datasheet) */

Heh.

> +#define SI544_REG_CONTROL      7
> +#define SI544_REG_OE_STATE     17
> +#define SI544_REG_HS_DIV       23
> +#define SI544_REG_LS_HS_DIV    24
> +#define SI544_REG_FBDIV0       26
> +#define SI544_REG_FBDIV8       27
> +#define SI544_REG_FBDIV16      28
> +#define SI544_REG_FBDIV24      29
> +#define SI544_REG_FBDIV32      30
> +#define SI544_REG_FBDIV40      31
> +#define SI544_REG_FCAL_OVR     69
> +#define SI544_REG_ADPLL_DELTA_M0       231
> +#define SI544_REG_ADPLL_DELTA_M8       232
> +#define SI544_REG_ADPLL_DELTA_M16      233
> +#define SI544_REG_PAGE_SELECT  255
> +
> +/* Register values */
> +#define SI544_CONTROL_RESET    BIT(7)
> +#define SI544_CONTROL_MS_ICAL2 BIT(3)
> +
> +#define SI544_OE_STATE_ODC_OE  BIT(0)
> +
> +/* Max freq depends on speed grade */
> +#define SI544_MIN_FREQ     200000U
> +
> +/* Si544 Internal oscilator runs at 55.05 MHz */
> +#define FXO              55050000U
> +
> +/* VCO range is 10.8 .. 12.1 GHz, max depends on speed grade */
> +#define FVCO_MIN       10800000000ULL
> +
> +#define HS_DIV_MAX     2046
> +#define HS_DIV_MAX_ODD 33
> +
> +enum si544_speed_grade {
> +       si544a,
> +       si544b,
> +       si544c,
> +};
> +
> +struct clk_si544 {
> +       struct clk_hw hw;
> +       struct regmap *regmap;
> +       struct i2c_client *i2c_client;
> +       enum si544_speed_grade speed_grade;
> +};
> +#define to_clk_si544(_hw)      container_of(_hw, struct clk_si544, hw)
> +
> +/* Multiplier/divider settings */
> +struct clk_si544_muldiv {
> +       u32 fb_div_frac;  /* Integer part of feedback divider (32 bits) */
> +       u16 fb_div_int;  /* Fractional part of feedback divider (11 bits) */
> +       u16 hs_div; /* 1st divider, 5..2046, must be even when >33 */
> +       u8 ls_div_bits; /* 2nd divider, as 2^x, range 0..5 */
> +};

Can you use kernel-doc style instead of inline comments please.

> +
> +static bool is_valid_frequency(const struct clk_si544 *data,
> +       unsigned long frequency)
> +{
> +       unsigned long max_freq;
> +
> +       if (frequency < SI544_MIN_FREQ)
> +               return false;
> +
> +       switch (data->speed_grade) {
> +       case si544a:
> +               max_freq = 1500000000;
> +               break;
> +       case si544b:
> +               max_freq = 800000000;
> +               break;
> +       case si544c:
> +               max_freq = 350000000;
> +               break;
> +       default:
> +               return false;

Drop this default case and let the compiler complain if we get a new
enum to handle.

> +       }
> +
> +       if (frequency > max_freq)
> +               return false;
> +
> +       return true;

Replace with 'return frequency <= max_freq'?

> +}
> +
> +/* Calculate divider settings for a given frequency */
> +static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
> +       unsigned long frequency)
> +{
> +       u64 vco;
> +       u32 ls_freq;
> +       u32 tmp;
> +       u8 res;
> +
> +       /* Determine the minimum value of LS_DIV and resulting target freq. */
> +       ls_freq = frequency;
> +       settings->ls_div_bits = 0;
> +
> +       if (frequency >= (FVCO_MIN / HS_DIV_MAX))

Maybe FVCO_MIN/HS_DIV_MAX has a name that can be defined? MIN_DIV_FREQ?

> +               settings->ls_div_bits = 0;
> +       else {

Style: Please add braces on the if arm too.

> +               res = 1;
> +               tmp = 2 * HS_DIV_MAX;
> +               while (tmp <= (HS_DIV_MAX * 32)) {
> +                       if ((frequency * tmp) >= FVCO_MIN)
> +                               break;
> +                       ++res;
> +                       tmp <<= 1;
> +               }
> +               settings->ls_div_bits = res;
> +               ls_freq = frequency << res;
> +       }
> +
> +       /* Determine minimum HS_DIV by rounding up */
> +       vco = FVCO_MIN + ls_freq - 1;
> +       do_div(vco, ls_freq);
> +       settings->hs_div = vco;
> +       /* round up to even number if needed */
> +       if ((settings->hs_div > HS_DIV_MAX_ODD) && (settings->hs_div & 1))

Drop useless parenthesis please.

> +               ++settings->hs_div;
> +       /* Calculate VCO frequency (in 10..12GHz range) */
> +       vco = (u64)ls_freq * settings->hs_div;
> +       /* Calculate the integer part of the feedback divider */
> +       tmp = do_div(vco, FXO);
> +       settings->fb_div_int = vco;
> +       /* And the fractional bits using the remainder */
> +       vco = (u64)tmp << 32;
> +       do_div(vco, FXO);
> +       settings->fb_div_frac = vco;

This is all packed together. Please add a newline before each comment to
make it easier to read.

> +
> +       return 0;
> +}
> +
> +/* Calculate resulting frequency given the register settings */
> +static unsigned long si544_calc_rate(struct clk_si544_muldiv *settings)
> +{
> +       u32 d = settings->hs_div * BIT(settings->ls_div_bits);
> +       u64 vco;
> +
> +       /* Calculate VCO while preventing overflow */
> +       vco = (u64)settings->fb_div_int * FXO;
> +       vco += (((u64)settings->fb_div_frac * FXO) + (FXO / 2)) >> 32;

Yay for the smatch checking stuff on the list! Try to put things on more
lines to keep stuff easier to follow.

> +
> +       do_div(vco, d);
> +
> +       return vco;
> +}
> +
> +static unsigned long si544_recalc_rate(struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct clk_si544 *data = to_clk_si544(hw);
> +       struct clk_si544_muldiv settings;
> +       int err;
> +
> +       err = si544_get_muldiv(data, &settings);
> +       if (err) {
> +               dev_err(&data->i2c_client->dev, "unable to retrieve settings\n");

This error message could become quite spammy given that we typically
call recalc_rate() many times.

> +               return 0;
> +       }
> +
> +       return si544_calc_rate(&settings);
> +}
> +
> +static long si544_round_rate(struct clk_hw *hw, unsigned long rate,
> +               unsigned long *parent_rate)
> +{
> +       struct clk_si544 *data = to_clk_si544(hw);
> +       struct clk_si544_muldiv settings;
> +       int err;
> +
> +       if (!rate)
> +               return 0;

Why? Do certain drivers call round_rate() with 0 and we want to ignore
those cases and not return an error?

> +
> +       if (!is_valid_frequency(data, rate))
> +               return -EINVAL;
> +
> +       err = si544_calc_muldiv(&settings, rate);
> +       if (err)
> +               return err;
> +
> +       return si544_calc_rate(&settings);
> +}
> +
> +
[...]
> +
> +static int si544_probe(struct i2c_client *client,
> +               const struct i2c_device_id *id)
> +{
> +       struct clk_si544 *data;
> +       struct clk_init_data init;
> +       int err;
> +
> +       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       init.ops = &si544_clk_ops;
> +       init.flags = 0;
> +       init.num_parents = 0;
> +       data->hw.init = &init;
> +       data->i2c_client = client;
> +       data->speed_grade = id->driver_data;
> +
> +       if (of_property_read_string(client->dev.of_node, "clock-output-names",
> +                       &init.name))
> +               init.name = client->dev.of_node->name;
> +
> +       data->regmap = devm_regmap_init_i2c(client, &si544_regmap_config);
> +       if (IS_ERR(data->regmap)) {
> +               dev_err(&client->dev, "failed to allocate register map\n");

Do we need another allocation failure message?

> +               return PTR_ERR(data->regmap);
> +       }
> +
> +       i2c_set_clientdata(client, data);
> +
> +       /* Select page 0, just to be sure, there appear to be no more */
> +       err = regmap_write(data->regmap, SI544_REG_PAGE_SELECT, 0);
> +       if (err < 0)
> +               return err;
> +
> +       err = devm_clk_hw_register(&client->dev, &data->hw);
> +       if (err) {
> +               dev_err(&client->dev, "clock registration failed\n");
> +               return err;
> +       }
> +       err = of_clk_add_hw_provider(client->dev.of_node, of_clk_hw_simple_get,

devm_of_clk_add_hw_provider()?

> +                                    &data->hw);
> +       if (err) {
> +               dev_err(&client->dev, "unable to add clk provider\n");
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int si544_remove(struct i2c_client *client)
> +{
> +       of_clk_del_provider(client->dev.of_node);
> +       return 0;
> +}

Drop?