lkml.org 
[lkml]   [2011]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC 6/8] of: add clock providers
    On Sun, Nov 13, 2011 at 7:14 PM, Richard Zhao <richard.zhao@linaro.org> wrote:
    > On Tue, Nov 08, 2011 at 08:49:42PM -0600, Rob Herring wrote:
    >> > +For example:
    >> > +
    >> > +    oscillator {
    >> > +        #clock-cells = <1>;
    >> > +        clock-output-name = "ckil", "ckih";
    >> > +    };
    >> > +
    >> > +- this node defines a device with two clock outputs, the first named
    >> > +  "ckil" and the second named "ckih".  Consumer nodes always reference
    >> > +  clocks by index. The names should reflect the clock output signal
    >> > +  names for the device.
    >> > +
    >> > +==Clock consumers==
    >> > +
    >> > +Required properties:
    >> > +clock-input:       List of phandle and clock specifier pairs, one pair
    >> > +           for each clock input to the device.
    >> > +clock-input-name: List of clock input name strings sorted in the same
    >> > +           order as the clock-input property.  Consumers drivers
    >> > +           will use clock-input-name to match clock input names
    >> > +           with clock-input specifiers.
    >> > +
    >> > +For example:
    >> > +
    >> > +    uart {
    >> > +        clock-input = <&osc 1> <&ref 0>;
    >> > +        clock-input-name = "baud", "register";
    >> > +    };
    >>
    >> This is duplicated below.

    I'm okay with that. It's not entirely the same, and I think having
    several examples is helpful.


    >> > +    /* external oscillator */
    >> > +    osc: oscillator {
    >> > +        compatible = "fixed-clock";
    >> > +        #clock-cells = <1>;
    >> > +        frequency  = <32678>;
    >>
    >> The code is using "clock-frequency" here.

    Fixed

    >>
    >> > +        clock-output-name = "osc";
    >> > +    };
    >> > +
    >> > +    /* phase-locked-loop device, generates a higher frequency clock
    >> > +     * from the external oscillator reference */
    >> > +    pll: pll {
    >> > +        compatible = "some-pll-interface"
    >> > +        #clock-cells = <1>;
    >> > +        clock-input = <&osc 0>;
    >>
    >> There's a mismatch in #clock-cells size and this.
    >
    > osc #clock-cells is 1. "0" is one cell too. Nothing wrong. Correct my understanding?

    Richard is correct. The #clock-cells value needs to be 1, which in
    the clock binding means one cell for the phandle and one cell for the
    attached arguments. Exactly the same as the gpio binding.

    >> > +static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
    >> > +{
    >> > +   struct of_clk_provider *provider;
    >> > +   struct clk *clk = NULL;
    >> > +
    >> > +   /* Check if we have such a provider in our array */
    >> > +   mutex_lock(&of_clk_lock);
    >> > +   list_for_each_entry(provider, &of_clk_providers, link) {
    >> > +           if (provider->node == clkspec->np)
    >> > +                   clk = provider->get(clkspec, provider->data);
    >>
    >> How about:
    >>
    >> if (provider->get)
    >>       clk = provider->get(clkspec, provider->data);
    >> else
    >>       clk = provider->data;

    I'd rather make it a requirement for every clock driver to provide the
    get hook; even if it uses a stock version that simply returns ->data.

    Thanks for the review.

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

    \
     
     \ /
      Last update: 2011-11-16 20:09    [W:2.288 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site