lkml.org 
[lkml]   [2011]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC,PATCH 1/3] Add a common struct clk
    Date
    Hi Ryan,

    > > +int clk_prepare(struct clk *clk)
    > > +{
    > > + int ret = 0;
    > > +
    > > + if (!clk->ops->prepare)
    > > + return 0;
    >
    > If there is no ops->prepare function then we never increment
    > prepare_count, which means that driver writers can get sloppy if they
    > know that ops->prepare is no-op on their platform since they will not
    > get warned for omitting clk_prepare.

    Yeah, as discussed in other replies, it's probably best that we do the
    counting unconditionally. I've removed these optimisations - I think we'd best
    enforce the checking here, at least at the introduction of this API.

    > Also, why are the warnings added in a separate patch rather than being
    > rolled into this patch?

    Just splitting things up; the warnings were the most discussed issue
    previously, so I wanted to separate that discussion from the API side.

    > Again, you should still increment enable_count even if ops->enabled is a
    > no-op since it provides valuable warnings when clk_enable/disable calls
    > are not matched correctly.

    Yep, as above.

    > > +unsigned long clk_get_rate(struct clk *clk)
    > > +{
    > > + if (clk->ops->get_rate)
    > > + return clk->ops->get_rate(clk);
    >
    > Possibly we should shadow the clock rate if ops->get_rate is no-op? So
    > clock initialisation and clk_set_rate store the rate in the shadow
    > field, and then do:
    >
    > if (clk->ops->get_rate)
    > return clk->ops->get_rate(clk);
    > return clk->shadow_rate;
    >
    > Because the API is generic, driver writers should reasonably expect that
    > clk_get_rate will return something valid without having to know the
    > platform implementation details. It may also be worth having a warning
    > to let the user know that the returned rate may be approximate.

    I'd prefer to require that get_rate is implemented as an op, rather than
    allowing two methods for retrieving the rate of the clock.

    > > + return 0;
    > > +}
    > > +EXPORT_SYMBOL_GPL(clk_get_rate);
    > > +
    > > +int __clk_get(struct clk *clk)
    > > +{
    > > + if (clk->ops->get)
    > > + return clk->ops->get(clk);
    > > + return 1;
    > > +}
    > > +EXPORT_SYMBOL_GPL(__clk_get);
    > > +
    > > +void clk_put(struct clk *clk)
    > > +{
    > > + if (clk->ops->put)
    > > + clk->ops->put(clk);
    > > +}
    > > +EXPORT_SYMBOL_GPL(clk_put);
    >
    > This has probably been covered, and I have probably missed it, but why
    > don't the generic clk_get/put functions do ref-counting? Drivers must
    > have matched clk_get/put calls so it should work like enable/prepare
    > counting right?

    clk_get is used to find a clock; most implementations will not use this for
    refcounting.

    However, for the case where clocks are dynamically allocated, we need clk_put
    to do any possible freeing. There's an existing API for this type of reference
    counting (kref), so for the cases where this matters, the clock
    implementations can use that.

    > > + * The choice of atomic or non-atomic clock depends on how the clock is
    > > enabled. + * Typically, you'll want to use a non-atomic clock. For
    > > clocks that need to be + * enabled/disabled in interrupt context, use
    > > CLK_ATOMIC. Note that atomic + * clocks with parents will typically
    > > cascade enable/disable operations to + * their parent, so the parent of
    > > an atomic clock *must* be atomic too.
    >
    > This comment seems out of date now that we have the prepare/enable
    > semantics?

    Yep, will update.

    > > + * @unprepare: Release the clock from its prepared state. This will
    > > typically + * undo any work done in the @prepare callback. Called
    > > with + * clk->prepare_lock held.
    >
    > I think you need to make it more clear the prepare/unprepare must be
    > called from a sleepable context.

    The documentation on clk_ops is intended for the clock implementor, so it's
    not really the right place to descibe the caller's requirements.

    Indeed, the documentation for clk_prepare & clk_unprepare describe the
    caller's requirements for these (and contain the words "This function may
    sleep").

    > > + * Typically, drivers will call clk_prepare when a clock may be needed
    > > later + * (eg. when a device is opened), and clk_enable when the clock
    > > is actually + * required (eg. from an interrupt).
    >
    > Drivers _must_ call clk_prepare before clk_enable (not typically)?

    This 'typically' is about the actual placement of the clk_prepare and
    clk_enable calls in the driver code, but I will clarify.

    > > +/**
    > > + * __clk_get - update clock-specific refcounter
    > > + *
    > > + * @clk: The clock to refcount
    > > + *
    > > + * Before a clock is returned from clk_get, this function should be
    > > called + * to update any clock-specific refcounting.
    > > + *
    > > + * Returns non-zero on success, zero on failure.
    > > + *
    > > + * Drivers should not need this function; it is only needed by the
    > > + * arch-specific clk_get() implementations.
    > > + */
    > > +int __clk_get(struct clk *clk);
    >
    > I don't understand this. Are architectures supposed to provide a
    > function called clk_get? Doesn't this break the whole idea of having a
    > common struct clk?

    clk_get() is now provided in drivers/clk/clkdev.c; the arch-specific part of
    this comment is old (I'll remove it).

    Thanks for taking the time to review, I appreciate it.

    Cheers,


    Jeremy


    \
     
     \ /
      Last update: 2011-02-08 03:57    [W:0.028 / U:0.616 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site