Messages in this thread | | | Date | Wed, 01 Jan 2020 16:15:32 +0100 | From | Michael Walle <> | Subject | Re: [PATCH v2 2/2] clk: fsl-sai: new driver |
| |
Hi Stephen,
thanks for the review.
Am 2019-12-24 09:05, schrieb Stephen Boyd: > 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?
you mean specifying parent_data with .index = 0? Seems like clk_composite does not support this. The parent can only be specified by supplying the clock names.
I could add that in a separate patch. What do you think about the following new functions, where a driver can use parent_data instead of parent_names.
+struct clk *clk_register_composite_pdata(struct device *dev, const char *name, + const struct clk_parent_data *parent_data, + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, + unsigned long flags);
+struct clk_hw *clk_hw_register_composite_pdata(struct device *dev, + const char *name, const struct clk_parent_data *parent_data, + struct clk_hw *mux_hw, const struct clk_ops *mux_ops, + struct clk_hw *rate_hw, const struct clk_ops *rate_ops, + struct clk_hw *gate_hw, const struct clk_ops *gate_ops, + unsigned long flags);
>> + >> + 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?
No, we are always master, because as a slave, there would be no clock output ;)
>> + 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?
I don't think so, the user will be a sound codec for now. I'll convert it to a platform device, in that case I could also use the devm_ variants.
-michael
| |