Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Stephen Boyd]

Quoting Kuninori Morimoto (2018-10-09 19:16:48)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 292056b..9cfeb0e 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -299,5 +299,6 @@ source "drivers/clk/sunxi-ng/Kconfig"
>  source "drivers/clk/tegra/Kconfig"
>  source "drivers/clk/ti/Kconfig"
>  source "drivers/clk/uniphier/Kconfig"
> +source "drivers/clk/nxp/Kconfig"

Please sort this alphabetically.

>  
>  endmenu
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index ed344eb..fcfd42a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -84,7 +84,7 @@ obj-$(CONFIG_ARCH_MMP)                        += mmp/
>  endif
>  obj-y                                  += mvebu/
>  obj-$(CONFIG_ARCH_MXS)                 += mxs/
> -obj-$(CONFIG_COMMON_CLK_NXP)           += nxp/
> +obj-y                                  += nxp/

There's the CONFIG_COMMON_CLK_NXP config symbol that could go into the
drivers/clk/nxp/Kconfig file then and also wrap the Makefile contents in
the nxp subdirectory too? We don't want to lose that config option and
start enabling this driver all the time.

>  obj-$(CONFIG_MACH_PISTACHIO)           += pistachio/
>  obj-$(CONFIG_COMMON_CLK_PXA)           += pxa/
>  obj-$(CONFIG_COMMON_CLK_QCOM)          += qcom/
> diff --git a/drivers/clk/nxp/clk-74aup1g157gw.c b/drivers/clk/nxp/clk-74aup1g157gw.c
> new file mode 100644
> index 0000000..78199bb
> --- /dev/null
> +++ b/drivers/clk/nxp/clk-74aup1g157gw.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 Renesas Solutions Corp.
> +// Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> +//
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#define CLK_I_NUM 2
> +struct clk_priv {
> +       struct clk_hw hw;
> +       struct device *dev;
> +       struct clk *i[CLK_I_NUM];
> +       struct gpio_desc *sel;
> +       long (*round_rate)(struct clk_hw *hw, unsigned long rate,
> +                          unsigned long *parent_rate);
> +};
> +#define hw_to_priv(_hw) container_of(_hw, struct clk_priv, hw)
> +#define priv_to_dev(priv) priv->dev

Nitpick: This macro is just obfuscating, there isn't any container_of()
usage going on so please remove it.

> +#define for_each_iclk(i)                       \
> +       for ((i) = 0; (i) < CLK_I_NUM; (i)++)

Nitpick: This macros isn't making things any shorter, so please remove
it.

> +
> +static int clk74_set_rate(struct clk_hw *hw, unsigned long rate,
> +                         unsigned long parent_rate)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +       struct device *dev = priv_to_dev(priv);
> +       int i;
> +
> +       for_each_iclk(i) {
> +               if (rate == clk_get_rate(priv->i[i])) {
> +                       dev_dbg(dev, "set rate %lu as i%d\n", rate, i);
> +                       gpiod_set_value_cansleep(priv->sel, i);
> +                       return 0;
> +               }
> +       }
> +
> +       dev_err(dev, "unsupported rate %lu\n", rate);
> +       return -EIO;
> +}
> +
> +static long clk74_round_rate_close(struct clk_hw *hw, unsigned long rate,
> +                            unsigned long *parent_rate)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +       struct device *dev = priv_to_dev(priv);
> +       unsigned long min = ~0;
> +       unsigned long ret = 0;
> +       int i;
> +
> +       /*
> +        * select closest rate
> +        */
> +       for_each_iclk(i) {
> +               unsigned long irate = clk_get_rate(priv->i[i]);
> +               unsigned long diff = abs(rate - irate);
> +
> +               if (min > diff) {
> +                       min = diff;
> +                       ret = irate;
> +               }
> +       }
> +
> +       dev_dbg(dev, "(close)round rate %lu\n", ret);
> +
> +       return ret;
> +}
> +
> +static long clk74_round_rate_audio(struct clk_hw *hw, unsigned long rate,
> +                            unsigned long *parent_rate)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +       struct device *dev = priv_to_dev(priv);
> +       unsigned long ret = 0;
> +       int is_8k = 0;
> +       int i;
> +
> +       /*
> +        * select 48kHz or 44.1kHz category rate
> +        */
> +       if (!(rate % 8000))
> +               is_8k = 1;
> +
> +       for_each_iclk(i) {
> +               unsigned long irate = clk_get_rate(priv->i[i]);
> +
> +               if (( is_8k && !(irate % 8000)) ||
> +                   (!is_8k &&  (irate % 8000))) {
> +                       ret = irate;
> +               }
> +       }
> +
> +       dev_dbg(dev, "(audio)round rate %lu\n", ret);
> +
> +       return ret;
> +}
> +
> +static long clk74_round_rate(struct clk_hw *hw, unsigned long rate,
> +                            unsigned long *parent_rate)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +
> +       return priv->round_rate(hw, rate, parent_rate);
> +}
> +
> +static unsigned long clk74_recalc_rate(struct clk_hw *hw,
> +                                    unsigned long parent_rate)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +       struct device *dev = priv_to_dev(priv);
> +       unsigned long rate;
> +       int i = gpiod_get_raw_value_cansleep(priv->sel);
> +
> +       rate = clk_get_rate(priv->i[i]);
> +
> +       dev_dbg(dev, "recalc rate %lu as i%d\n", rate, i);
> +
> +       return rate;
> +}
> +
> +static u8 clk74_get_parent(struct clk_hw *hw)
> +{
> +       struct clk_priv *priv = hw_to_priv(hw);
> +
> +       return gpiod_get_raw_value_cansleep(priv->sel);
> +}
> +
> +static const struct clk_ops clk74_ops = {
> +       .set_rate       = clk74_set_rate,
> +       .round_rate     = clk74_round_rate,
> +       .recalc_rate    = clk74_recalc_rate,
> +       .get_parent     = clk74_get_parent,
> +};

Can this all be handled by the 'gpio-mux-clock' compatible/driver? I
suppose it may need an update to add the rounding policy that you want
via some sort of DT property, but otherwise it would be fine?

> +
> +static const char * const clk74_in_name[CLK_I_NUM] = {
> +       "i0",
> +       "i1",
> +};
> +
> +static int clk74_probe(struct platform_device *pdev)
> +{
> +       struct clk *clk;
> +       struct clk_init_data init;
> +       struct clk_priv *priv;
> +       struct device *dev = &pdev->dev;
> +       const char *parent_names[CLK_I_NUM];
> +       int i, ret;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENODEV;

You mean -ENOMEM?

> +
> +       for_each_iclk(i) {
> +               clk = devm_clk_get(dev, clk74_in_name[i]);
> +               if (IS_ERR(clk))
> +                       return -EPROBE_DEFER;

Please return actual error code instead of overriding and returning
probe defer.

> +               priv->i[i] = clk;
> +               parent_names[i] = __clk_get_name(clk);
> +       }
> +
> +       memset(&init, 0, sizeof(init));
> +       init.name               = "74aup1g157gw";
> +       init.ops                = &clk74_ops;
> +       init.parent_names       = parent_names;
> +       init.num_parents        = CLK_I_NUM;
> +
> +       priv->hw.init           = &init;
> +       priv->dev               = dev;
> +       priv->round_rate        = of_device_get_match_data(dev);
> +       priv->sel               = devm_gpiod_get(dev, "sel", 0);
> +       if (IS_ERR(priv->sel))
> +               return -EPROBE_DEFER;

Please return the actual error.

> +
> +       gpiod_direction_output(priv->sel, 0);
> +
> +       ret = devm_clk_hw_register(dev, &priv->hw);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &priv->hw);
> +       if (ret < 0)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       dev_info(dev, "probed\n");

Please remove noise like this.

> +
> +       return 0;
> +}
> +
> +#define OF_ID(name, func) { .compatible = name, .data = func }
> +static const struct of_device_id clk74_of_match[] = {
> +       OF_ID("nxp,74aup1g157gw-clk",           clk74_round_rate_close),
> +       OF_ID("nxp,74aup1g157gw-audio-clk",     clk74_round_rate_audio),

Nitpick: I'd prefer to not have the macro.