Re: [PATCH v5] clk: add CS2000 Fractional-N driver

[Andy Shevchenko]

On Tue, Oct 20, 2015 at 4:26 AM, Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>

Hmmâ Something wrong with send-email settings?

> This patch adds CS2000 Fractional-N driver as clock provider.

> +#define CH_SIZE_ERR(ch)                ((ch < 0) || (ch >= CH_MAX))
> +#define hw_to_priv(_hw)                container_of(_hw, struct cs2000_priv, hw)

> +#define priv_to_client(priv)   (priv->client)

to_client()?

> +#define priv_to_dev(priv)      (&(priv_to_client(priv)->dev))

to_dev() ?

> +static int cs2000_bset(struct cs2000_priv *priv, u8 addr, u8 mask, u8 val)
> +{
> +       s32 data;
> +
> +       data = cs2000_read(priv, addr);
> +       if (data < 0)
> +               return data;
> +
> +       data &= ~mask;
> +       data |= (val & mask);
> +
> +       return cs2000_write(priv, addr, data);
> +}
> +
> +static int cs2000_enable_dev_config(struct cs2000_priv *priv, bool enable)
> +{
> +       u32 val;
> +       int ret;
> +
> +       val = enable ? ENDEV1 : 0;

Put in expression below?

> +       ret = cs2000_bset(priv, DEVICE_CFG1, ENDEV1, val);
> +       if (ret < 0)
> +               return ret;
> +
> +       val = enable ? ENDEV2 : 0;

Same.

> +       ret = cs2000_bset(priv, GLOBAL_CFG,  ENDEV2, val);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int cs2000_clk_in_bound_rate(struct cs2000_priv *priv,
> +                                   u32 rate_in)
> +{
> +       u32 val;
> +
> +       if (rate_in >= 32000000 &&
> +           rate_in < 56000000)

One line here and below?

> +               val = 0x0;
> +       else if (rate_in >= 16000000 &&
> +                rate_in < 28000000)
> +               val = 0x1;
> +       else if (rate_in >= 8000000 &&
> +                rate_in < 14000000)
> +               val = 0x2;
> +       else
> +               return -EINVAL;
> +
> +       return cs2000_bset(priv, FUNC_CFG1, 0x3 << 3, val << 3);


> +}
> +
> +static int cs2000_wait_pll_lock(struct cs2000_priv *priv)
> +{
> +       struct device *dev = priv_to_dev(priv);
> +       s32 val;
> +       unsigned int i;
> +
> +       for (i = 0; i < 256; i++) {
> +               val = cs2000_read(priv, DEVICE_CTRL);
> +               if (val < 0)
> +                       return val;
> +               if (!(val & PLL_UNLOCK))
> +                       return 0;
> +               udelay(1);
> +       }

unsigned int i = 256;

do {
val = read();
â
while ((val & PLL_UNLOCK) && --i);

if (!i) {
 â
 return -EIO;

Actually -ETIMEDOUT ?

}
return 0;

> +
> +       dev_err(dev, "pll lock failed\n");
> +
> +       return -EIO;
> +}
> +
> +static int cs2000_clk_out_enable(struct cs2000_priv *priv, bool enable)
> +{
> +       u32 val = enable ? 0 : 0x3;

Redundant variable?

> +
> +       /* enable both AUX_OUT, CLK_OUT */
> +       return cs2000_write(priv, DEVICE_CTRL, val);
> +}
> +
> +static u32 cs2000_rate_to_ratio(u32 rate_in, u32 rate_out)
> +{
> +       u64 ratio;
> +
> +       /*
> +        * ratio = rate_out / rate_in * 2^20
> +        *
> +        * To avoid over flow, rate_out is u64
> +        * The result should be u32
> +        */
> +       ratio = (u64)rate_out << 20;
> +       do_div(ratio, rate_in);
> +
> +       return (u32)ratio;

No need to do explicit casting.

> +}
> +
> +static unsigned long cs2000_ratio_to_rate(u32 ratio, u32 rate_in)
> +{
> +       u64 rate_out;
> +
> +       /*
> +        * ratio = rate_out / rate_in * 2^20
> +        *
> +        * To avoid over flow, rate_out is u64
> +        * The result should be u32

u32 or unsigned long?

Btw, dots at the end of sentences.

> +        */
> +
> +       rate_out = (u64)ratio * rate_in;
> +       return (unsigned long)(rate_out >> 20);

Same.

> +}
> +
> +static int cs2000_ratio_set(struct cs2000_priv *priv,
> +                           int ch, u32 rate_in, u32 rate_out)
> +{
> +       u32 val;
> +       unsigned int i;
> +       int ret;
> +
> +       if (CH_SIZE_ERR(ch))
> +               return -EINVAL;
> +
> +       val = cs2000_rate_to_ratio(rate_in, rate_out);
> +       for (i = 0; i < 4; i++)

4 is magic, you have define already.

> +               ret = cs2000_write(priv,
> +                                  Ratio_Add(ch, i),
> +                                  Ratio_Val(val, i));
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static u32 cs2000_ratio_get(struct cs2000_priv *priv, int ch)
> +{
> +       u32 tmp, val;
> +       unsigned int i;
> +
> +       val = 0;
> +       for (i = 0; i < 4; i++) {

Same.

> +               tmp = cs2000_read(priv,
> +                                 Ratio_Add(ch, i));

One line?

> +               if (tmp < 0)
> +                       return 0;
> +
> +               val |= Val_Ratio(tmp, i);
> +       }
> +
> +       return val;
> +}
> +
> +static int cs2000_ratio_select(struct cs2000_priv *priv, int ch)
> +{
> +       int ret;
> +
> +       if (CH_SIZE_ERR(ch))
> +               return -EINVAL;
> +

> +       /*
> +        * FIXME
> +        *
> +        * this driver supports static ratio mode only
> +        * at this point
> +        */

One line?

> +       ret = cs2000_bset(priv, DEVICE_CFG1, RSEL_MASK, RSEL(ch));
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_write(priv, DEVICE_CFG2, 0x0);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static unsigned long cs2000_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long parent_rate)
> +{
> +       struct cs2000_priv *priv = hw_to_priv(hw);
> +       int ch = 0; /* it uses ch0 only at this point */
> +       u32 ratio;
> +
> +       ratio = cs2000_ratio_get(priv, ch);
> +
> +       return cs2000_ratio_to_rate(ratio, parent_rate);
> +}
> +
> +static long cs2000_round_rate(struct clk_hw *hw, unsigned long rate,
> +                             unsigned long *parent_rate)
> +{
> +       u32 ratio;
> +
> +       ratio = cs2000_rate_to_ratio(*parent_rate, rate);
> +
> +       return cs2000_ratio_to_rate(ratio, *parent_rate);
> +}
> +
> +static int __cs2000_set_rate(struct cs2000_priv *priv, int ch,
> +                            unsigned long rate, unsigned long parent_rate)
> +
> +{
> +       int ret;
> +
> +       ret = cs2000_clk_in_bound_rate(priv, parent_rate);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_ratio_set(priv, ch, parent_rate, rate);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_ratio_select(priv, ch);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int cs2000_set_rate(struct clk_hw *hw,
> +                          unsigned long rate, unsigned long parent_rate)
> +{
> +       struct cs2000_priv *priv = hw_to_priv(hw);
> +       int ch = 0; /* it uses ch0 only at this point */
> +
> +       return __cs2000_set_rate(priv, ch, rate, parent_rate);
> +}
> +
> +static int cs2000_enable(struct clk_hw *hw)
> +{
> +       struct cs2000_priv *priv = hw_to_priv(hw);
> +       int ret;
> +
> +       ret = cs2000_enable_dev_config(priv, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_clk_out_enable(priv, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_wait_pll_lock(priv);
> +       if (ret < 0)
> +               return ret;
> +
> +       return ret;
> +}
> +
> +static void cs2000_disable(struct clk_hw *hw)
> +{
> +       struct cs2000_priv *priv = hw_to_priv(hw);
> +
> +       cs2000_enable_dev_config(priv, false);
> +
> +       cs2000_clk_out_enable(priv, false);
> +}
> +
> +static u8 cs2000_get_parent(struct clk_hw *hw)
> +{
> +       /* always return REF_CLK */
> +       return REF_CLK;
> +}
> +
> +static const struct clk_ops cs2000_ops = {
> +       .get_parent     = cs2000_get_parent,
> +       .recalc_rate    = cs2000_recalc_rate,
> +       .round_rate     = cs2000_round_rate,
> +       .set_rate       = cs2000_set_rate,
> +       .prepare        = cs2000_enable,
> +       .unprepare      = cs2000_disable,
> +};
> +
> +static int cs2000_clk_get(struct cs2000_priv *priv)
> +{
> +       struct i2c_client *client = priv_to_client(priv);
> +       struct device *dev = &client->dev;
> +       struct clk *clk_in, *ref_clk;
> +
> +       clk_in = devm_clk_get(dev, "clk_in");
> +       /* not yet provided */
> +       if (IS_ERR(clk_in))
> +               return -EPROBE_DEFER;
> +
> +       ref_clk = devm_clk_get(dev, "ref_clk");
> +       /* not yet provided */
> +       if (IS_ERR(ref_clk))
> +               return -EPROBE_DEFER;
> +
> +       priv->clk_in    = clk_in;
> +       priv->ref_clk   = ref_clk;
> +
> +       return 0;
> +}
> +
> +static int cs2000_clk_register(struct cs2000_priv *priv)
> +{
> +       struct device *dev = priv_to_dev(priv);
> +       struct device_node *np = dev->of_node;
> +       struct clk_init_data init;
> +       const char *name = np->name;
> +       struct clk *clk;
> +       static const char *parent_names[CLK_MAX];
> +       int ch = 0; /* it uses ch0 only at this point */
> +       int rate;
> +       int ret;
> +
> +       of_property_read_string(np, "clock-output-names", &name);

What about device property API?

> +
> +       /*
> +        * set default rate as 1/1.
> +        * otherwise .set_rate which setup ratio
> +        * is never called if user requests 1/1 rate
> +        */
> +       rate = clk_get_rate(priv->ref_clk);
> +       ret = __cs2000_set_rate(priv, ch, rate, rate);
> +       if (ret < 0)
> +               return ret;
> +
> +       parent_names[CLK_IN]    = __clk_get_name(priv->clk_in);
> +       parent_names[REF_CLK]   = __clk_get_name(priv->ref_clk);
> +
> +       init.name               = name;
> +       init.ops                = &cs2000_ops;
> +       init.flags              = CLK_SET_RATE_GATE;
> +       init.parent_names       = parent_names;
> +       init.num_parents        = ARRAY_SIZE(parent_names);
> +
> +       priv->hw.init = &init;
> +
> +       clk = clk_register(dev, &priv->hw);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +       if (ret < 0) {
> +               clk_unregister(clk);
> +               return ret;
> +       }
> +
> +       priv->clk_out = clk;
> +
> +       return 0;
> +}
> +
> +static int cs2000_version_print(struct cs2000_priv *priv)
> +{
> +       struct i2c_client *client = priv_to_client(priv);
> +       struct device *dev = &client->dev;
> +       s32 val = cs2000_read(priv, DEVICE_ID);
> +       const char *revision;
> +

Move read here to see how val is assigned.

s32 val;

â

val = read();

> +       if (val < 0)
> +               return val;
> +
> +       /* CS2000 should be 0x0 */
> +       if (0 != (val >> 3))

if (val >> 3)

> +               return -EIO;
> +
> +       switch (val & 0x7) {

magic

> +       case 0x4:

magic

> +               revision = "B2 / B3";
> +               break;
> +       case 0x6:

magic

> +               revision = "C1";
> +               break;
> +       default:
> +               return -EIO;
> +       }
> +
> +       dev_info(dev, "revision - %s\n", revision);
> +
> +       return 0;
> +}
> +
> +static int cs2000_remove(struct i2c_client *client)
> +{
> +       struct cs2000_priv *priv = i2c_get_clientdata(client);
> +       struct device *dev = &client->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       of_clk_del_provider(np);
> +
> +       clk_unregister(priv->clk_out);
> +
> +       return 0;
> +}
> +
> +static int cs2000_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{
> +       struct cs2000_priv *priv;
> +       struct device *dev = &client->dev;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->client = client;
> +       i2c_set_clientdata(client, priv);
> +
> +       ret = cs2000_clk_get(priv);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_clk_register(priv);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = cs2000_version_print(priv);
> +       if (ret < 0)
> +               goto probe_err;
> +
> +       return 0;
> +
> +probe_err:
> +       cs2000_remove(client);


> +
> +       return ret;
> +}
> +
> +static struct i2c_driver cs2000_driver = {
> +       .driver = {
> +               .name = "cs2000-cp",
> +               .of_match_table = cs2000_of_match,
> +       },
> +       .probe          = cs2000_probe,
> +       .remove         = cs2000_remove,
> +       .id_table       = cs2000_id,
> +};
> +
> +module_i2c_driver(cs2000_driver);
> +
> +MODULE_DESCRIPTION("CS2000-CP driver");
> +MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/