linux-kernel - Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <155621927685.15276.16453873579880842057@swboyd.mtv.corp.google.com>
Date:   Thu, 25 Apr 2019 12:07:56 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Joseph Lo <josephl@...dia.com>,
        Mark Rutland <mark.rutland@....com>,
        Michael Turquette <mturquette@...libre.com>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Prashant Gaikwad <pgaikwad@...dia.com>,
        Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>
Cc:     devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

Quoting Dmitry Osipenko (2019-04-14 13:20:06)
> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c
> new file mode 100644
> index 000000000000..35b67a9989c8
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-tegra20-emc.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bits.h>
> +#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include "clk.h"
> +
> +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK     GENMASK(7, 0)
> +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK         GENMASK(31, 30)
[...]
> +
> +static const struct clk_ops tegra_clk_emc_ops = {
> +       .recalc_rate = emc_recalc_rate,
> +       .get_parent = emc_get_parent,
> +       .set_parent = emc_set_parent,
> +       .set_rate = emc_set_rate,
> +       .set_rate_and_parent = emc_set_rate_and_parent,
> +       .determine_rate = emc_determine_rate,
> +};
> +
> +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb)

Why can't we have type safety on these function pointers?

> +{
> +       struct clk *clk = __clk_lookup("emc");
> +       struct tegra_clk_emc *emc;
> +       struct clk_hw *hw;
> +
> +       if (clk) {
> +               hw = __clk_get_hw(clk);
> +               emc = to_tegra_clk_emc(hw);
> +
> +               emc->round_cb = round_cb;
> +               emc->arg_cb = arg_cb;
> +       }
> +}
> +
> +bool tegra20_clk_emc_driver_available(void)
> +{
> +       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in
this driver somehow?

> +       struct tegra_clk_emc *emc;
> +       struct clk_hw *hw;
> +
> +       if (clk) {
> +               hw = __clk_get_hw(clk);

This gets further to the point, we don't prefer to see drivers use
__clk_get_hw() unless they absolutely need to. Maybe I don't understand
the driver structure, but it seems like maybe the driver that's
providing the callbacks could be the same driver that's registering
these clks, and thus everything could be inside one file so that we
don't have to pass around callbacks and clk_hw pointers? Commit text
says "this is how it's been" but that's not a reason to keep doing it.

> +               emc = to_tegra_clk_emc(hw);
> +