Quoting Mark Rutland (2013-06-26 01:40:58)
> Hi,
>
> I have a couple of minor comments.
>
> On Fri, Jun 21, 2013 at 07:14:14AM +0100, Mike Turquette wrote:
> > Device Tree binding for the basic clock multiplexer, plus the setup
> > function to register the clock. Based on the existing fixed-clock
> > binding.
> >
> > Includes minor beautification of clk-provider.h where some whitespace is
> > added and of_fixed_factor_clock_setup is relocated to maintain a
> > consistent style.
> >
> > Tero Kristo contributed helpful bug fixes to this patch.
> >
> > Signed-off-by: Mike Turquette <mturque...@linaro.org>
> > ---
> > Changes since v2:
> > * added hiword-mask property to the binding
> > * changed bit-shift property from u8 to u32 in the dt binding
> >
> > Changes since v1:
> > * pass shift value into clk_register_mux_table
> > * s/multiplexor/multiplexer/
> > * removed debug prints
> > * mask is u32, shift is u8
> > * DT property names use dashes instead of underscores
> > * DT property names are more verbose
> > * shift property is optional in binding and can be auto-generated from a
> > full 32-bit mask
> >
> > .../devicetree/bindings/clock/mux-clock.txt | 79
> > ++++++++++++++++++++++
> > drivers/clk/clk-mux.c | 65 +++++++++++++++++-
> > include/linux/clk-provider.h | 5 +-
> > 3 files changed, 147 insertions(+), 2 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
> >
> > diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt
> > b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > new file mode 100644
> > index 0000000..54cb8d1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
> > @@ -0,0 +1,79 @@
> > +Binding for simple mux clock.
> > +
> > +This binding uses the common clock binding[1]. It assumes a
> > +register-mapped multiplexer with multiple input clock signals or
> > +parents, one of which can be selected as output. This clock does not
> > +gate or adjust the parent rate via a divider or multiplier.
> > +
> > +By default the "clocks" property lists the parents in the same order
> > +as they are programmed into the regster. E.g:
> > +
> > + clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
> > +
> > +results in programming the register as follows:
> > +
> > +register value selected parent clock
> > +0 foo_clock
> > +1 bar_clock
> > +2 baz_clock
> > +
> > +Some clock controller IPs do not allow a value of zero to be programmed
> > +into the register, instead indexing begins at 1. The optional property
> > +"index_one" modified the scheme as follows:
>
> I assume this is meant to be "index-starts-at-one", given the code and
> changelog?
>
> > +
> > +register value selected clock parent
> > +1 foo_clock
> > +2 bar_clock
> > +3 baz_clock
> > +
> > +Additionally an optional table of bit and parent pairs may be supplied
> > +like so:
> > +
> > + table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
> > +
> > +where the first value in the pair is the parent clock and the second
> > +value is the bitfield to be programmed into the register.
> > +
> > +The binding must provide the register to control the mux and the mask
> > +for the corresponding control bits. Optionally the number of bits to
> > +shift that mask if necessary. If the shift value is missing it is the
> > +same as supplying a zero shift.
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : shall be "mux-clock".
> > +- #clock-cells : from common clock binding; shall be set to 0.
> > +- clocks : link phandles of parent clocks
> > +- reg : base address for register controlling adjustable mux
> > +- bit-mask : arbitrary bitmask for programming the adjustable mux
> > +
> > +Optional properties:
> > +- clock-output-names : From common clock binding.
> > +- table : array of integer pairs defining parents & bitfield values
> > +- bit-shift : number of bits to shift the bit-mask, defaults to
> > + (ffs(mask) - 1) if not present
> > +- index-starts-at-one : valid input select programming starts at 1, not
> > + zero
> > +- hiword-mask : lower half of the register programs the mux, upper half
> > + of the register indicates bits that were updated in the lower half
> > +
> > +Examples:
> > + clock: clock@4a008100 {
> > + compatible = "mux-clock";
> > + #clock-cells = <0>;
> > + clocks = <&clock_foo>, <&clock_bar>, <&clock_baz>;
> > + reg = <0x4a008100 0x4>
> > + mask = <0x3>;
> > + index_one;
>
> And here too?
>
> [...]
>
> > +/**
> > + * of_mux_clk_setup() - Setup function for simple mux rate clock
> > + */
> > +void of_mux_clk_setup(struct device_node *node)
> > +{
> > + struct clk *clk;
> > + const char *clk_name = node->name;
> > + void __iomem *reg;
> > + int num_parents;
> > + const char **parent_names;
> > + int i;
> > + u8 clk_mux_flags = 0;
> > + u32 mask = 0;
> > + u32 shift = 0;
> > +
> > + of_property_read_string(node, "clock-output-names", &clk_name);
> > +
> > + num_parents = of_clk_get_parent_count(node);
> > + if (num_parents < 1) {
> > + pr_err("%s: mux-clock %s must have parent(s)\n",
> > + __func__, node->name);
> > + return;
> > + }
> > +
> > + parent_names = kzalloc((sizeof(char*) * num_parents),
> > + GFP_KERNEL);
> > +
> > + for (i = 0; i < num_parents; i++)
> > + parent_names[i] = of_clk_get_parent_name(node, i);
> > +
> > + reg = of_iomap(node, 0);
>
> Would it not be a good idea to check this isn't NULL before we pass it
> to clk_register_mux_table later?
>
> > +
> > + if (of_property_read_u32(node, "bit-mask", &mask)) {
> > + pr_err("%s: missing bit-mask property for %s\n", __func__,
> > node->name);
> > + return;
> > + }
> > +
> > + if (of_property_read_u32(node, "bit-shift", &shift)) {
> > + shift = __ffs(mask);
> > + pr_debug("%s: bit-shift property defaults to 0x%x for %s\n",
> > + __func__, shift, node->name);
> > + }
> > +
> > + if (of_property_read_bool(node, "index-starts-at-one"))
> > + clk_mux_flags |= CLK_MUX_INDEX_ONE;
> > +
> > + if (of_property_read_bool(node, "hiword-mask"))
> > + clk_mux_flags |= CLK_MUX_HIWORD_MASK;
> > +
> > + clk = clk_register_mux_table(NULL, clk_name, parent_names,
> > num_parents,
> > + 0, reg, (u8) shift, mask, clk_mux_flags,
> > + NULL, NULL);
>
> Why do you need the (u8) cast on shift? Isn't that implicit?
All of your suggestions are good. I'll roll them into the next version.
Regards,
Mike
>
> Thanks,
> Mark.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
