| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1516211221.2608.140.camel@baylibre.com>
Date: Wed, 17 Jan 2018 18:47:01 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: David Lechner <david@...hnology.com>,
Stephen Boyd <sboyd@...eaurora.org>,
Michael Turquette <mturquette@...libre.com>,
linux-clk@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, Vladimir Zapolskiy <vz@...ia.com>,
Sylvain Lemieux <slemieux.tyco@...il.com>,
Andy Gross <andy.gross@...aro.org>,
David Brown <david.brown@...aro.org>
Subject: Re: [3/5] clk: divider: add divider_ro_round_rate helper
On Thu, 2018-01-11 at 17:08 -0600, David Lechner wrote:
> On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> > Like divider_round_rate, a couple a of driver are doing more or less
> > the same operation to round the rate of the divider when it is read-only.
> >
> > We can factor this code so let's provide an helper function for this
>
> Perhaps you could also make use of this new helper here (clk-divider.c):
>
> const struct clk_ops clk_divider_ro_ops = {
> .recalc_rate = clk_divider_recalc_rate,
> .round_rate = clk_divider_round_rate,
> };
> EXPORT_SYMBOL_GPL(clk_divider_ro_ops);
The helper is used in this driver and ro_ops will use it.
I don't get this comment, could you be more specific
>
> >
> > Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> > ---
> > drivers/clk/clk-divider.c | 43 ++++++++++++++++++++++++++++---------------
> > include/linux/clk-provider.h | 15 +++++++++++++++
> > 2 files changed, 43 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index a851d3e04c7f..3eb2b27f3513 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > }
> > EXPORT_SYMBOL_GPL(divider_round_rate_parent);
> >
>
> It is nice to have documentation comments on public functions. Especially
> in this case where @prate is an in/out parameter and @table is optional.
Sure, but divider_round_rate_parent was already and undocumented. There is no
reason to change this is this particular patch (it is not the topic)
The RO helper being the counter part of this one, it did not do it differently
I suppose a documentation could be added in another patch.
However, I don't know if there is policy regarding the documentation of
functions internal to CCF. Maybe Stephen can tell us ?
Cheers
Jerome
>
> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > + unsigned long rate, unsigned long *prate,
> > + const struct clk_div_table *table, u8 width,
> > + unsigned long flags, unsigned int val)
> > +{
> > + int div;
> > +
> > + div = _get_div(table, val, flags, width);
> > +
> > + /* Even a read-only clock can propagate a rate change */
> > + if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > + if (!parent)
> > + return -EINVAL;
> > +
> > + *prate = clk_hw_round_rate(parent, rate * div);
> > + }
> > +
> > + return DIV_ROUND_UP_ULL((u64)*prate, div);
> > +}
> > +EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
> > +
> > +
> > static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> > unsigned long *prate)
> > {
> > struct clk_divider *divider = to_clk_divider(hw);
> > - struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> > - int bestdiv;
> > + u32 val;
> >
> > /* if read only, just return current value */
> > if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > - bestdiv = clk_readl(divider->reg) >> divider->shift;
> > - bestdiv &= div_mask(divider->width);
> > - bestdiv = _get_div(divider->table, bestdiv, divider->flags,
> > - divider->width);
> > -
> > - /* Even a read-only clock can propagate a rate change */
> > - if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > - if (!hw_parent)
> > - return -EINVAL;
> > -
> > - *prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
> > - }
> > + val = clk_readl(divider->reg) >> divider->shift;
> > + val &= div_mask(divider->width);
> >
> > - return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
> > + return divider_ro_round_rate(hw, rate, prate, divider->table,
> > + divider->width, divider->flags,
> > + val);
> > }
> >
> > return divider_round_rate(hw, rate, prate, divider->table,
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 175a62a15619..eb2c3a035e98 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > unsigned long rate, unsigned long *prate,
> > const struct clk_div_table *table,
> > u8 width, unsigned long flags);
> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > + unsigned long rate, unsigned long *prate,
> > + const struct clk_div_table *table, u8 width,
> > + unsigned long flags, unsigned int val);
> > int divider_get_val(unsigned long rate, unsigned long parent_rate,
> > const struct clk_div_table *table, u8 width,
> > unsigned long flags);
> > @@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
> > rate, prate, table, width, flags);
> > }
> >
>
> Same here (about documentation comment).
>
> > +static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *prate,
> > + const struct clk_div_table *table,
> > + u8 width, unsigned long flags,
> > + unsigned int val)
> > +{
> > + return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
> > + rate, prate, table, width, flags,
> > + val);
> > +}
> > +
> > /*
> > * FIXME clock api without lock protection
> > */
> >
>
>
Powered by blists - more mailing lists