Re: [PATCH v2 2/2] clk: fsl-sai: new driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Michael Walle (2019-12-09 15:33:05)
> diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c
> new file mode 100644
> index 000000000000..b92054d15ab1
> --- /dev/null
> +++ b/drivers/clk/clk-fsl-sai.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Freescale SAI BCLK as a generic clock driver
> + *
> + * Copyright 2019 Kontron Europe GmbH
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define I2S_CSR                0x00
> +#define I2S_CR2                0x08
> +#define CSR_BCE_BIT    28
> +#define CR2_BCD                BIT(24)
> +#define CR2_DIV_SHIFT  0
> +#define CR2_DIV_WIDTH  8
> +
> +struct fsl_sai_clk {
> +       struct clk_divider div;
> +       struct clk_gate gate;
> +       spinlock_t lock;
> +};
> +
> +static void __init fsl_sai_clk_setup(struct device_node *node)
> +{
> +       const char *clk_name = node->name;
> +       struct fsl_sai_clk *sai_clk;
> +       unsigned int num_parents;
> +       const char *parent_name;
> +       void __iomem *base;
> +       struct clk_hw *hw;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (!num_parents) {
> +               pr_err("%s: no parent found", clk_name);
> +               return;
> +       }
> +
> +       parent_name = of_clk_get_parent_name(node, 0);

Could this use the new way of specifying clk parents so that we don't
have to query DT for parent names and just let the core framework do it
whenever it needs to?

> +
> +       sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL);
> +       if (!sai_clk)
> +               return;
> +
> +       base = of_iomap(node, 0);
> +       if (base == NULL) {
> +               pr_err("%s: failed to map register space", clk_name);
> +               goto err;
> +       }
> +
> +       spin_lock_init(&sai_clk->lock);
> +
> +       sai_clk->gate.reg = base + I2S_CSR;
> +       sai_clk->gate.bit_idx = CSR_BCE_BIT;
> +       sai_clk->gate.lock = &sai_clk->lock;
> +
> +       sai_clk->div.reg = base + I2S_CR2;
> +       sai_clk->div.shift = CR2_DIV_SHIFT;
> +       sai_clk->div.width = CR2_DIV_WIDTH;
> +       sai_clk->div.lock = &sai_clk->lock;
> +
> +       /* set clock direction, we are the BCLK master */

Should this configuration come from DT somehow?

> +       writel(CR2_BCD, base + I2S_CR2);
> +
> +       hw = clk_hw_register_composite(NULL, clk_name, &parent_name, 1,
> +                                      NULL, NULL,
> +                                      &sai_clk->div.hw, &clk_divider_ops,
> +                                      &sai_clk->gate.hw, &clk_gate_ops,
> +                                      CLK_SET_RATE_GATE);
> +       if (IS_ERR(hw))
> +               goto err;
> +
> +       of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw);
> +
> +       return;
> +
> +err:
> +       kfree(sai_clk);
> +}
> +
> +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", fsl_sai_clk_setup);

Is there a reason this can't be a platform device driver?