linux-kernel - Re: [PATCH v2 2/2] clk: fsl-sai: new driver

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: <20191224080536.B0C99206CB@mail.kernel.org>
Date:   Tue, 24 Dec 2019 00:05:35 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Michael Walle <michael@...le.cc>, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Michael Walle <michael@...le.cc>
Subject: Re: [PATCH v2 2/2] clk: fsl-sai: new driver

Quoting Michael Walle (2019-12-09 15:33:05)
> diff --git a/drivers/clk/clk-fsl-sai.c b/drivers/clk/clk-fsl-sai.c
> new file mode 100644
> index 000000000000..b92054d15ab1
> --- /dev/null
> +++ b/drivers/clk/clk-fsl-sai.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Freescale SAI BCLK as a generic clock driver
> + *
> + * Copyright 2019 Kontron Europe GmbH
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define I2S_CSR                0x00
> +#define I2S_CR2                0x08
> +#define CSR_BCE_BIT    28
> +#define CR2_BCD                BIT(24)
> +#define CR2_DIV_SHIFT  0
> +#define CR2_DIV_WIDTH  8
> +
> +struct fsl_sai_clk {
> +       struct clk_divider div;
> +       struct clk_gate gate;
> +       spinlock_t lock;
> +};
> +
> +static void __init fsl_sai_clk_setup(struct device_node *node)
> +{
> +       const char *clk_name = node->name;
> +       struct fsl_sai_clk *sai_clk;
> +       unsigned int num_parents;
> +       const char *parent_name;
> +       void __iomem *base;
> +       struct clk_hw *hw;
> +
> +       num_parents = of_clk_get_parent_count(node);
> +       if (!num_parents) {
> +               pr_err("%s: no parent found", clk_name);
> +               return;
> +       }
> +
> +       parent_name = of_clk_get_parent_name(node, 0);

Could this use the new way of specifying clk parents so that we don't
have to query DT for parent names and just let the core framework do it
whenever it needs to?

> +
> +       sai_clk = kzalloc(sizeof(*sai_clk), GFP_KERNEL);
> +       if (!sai_clk)
> +               return;
> +
> +       base = of_iomap(node, 0);
> +       if (base == NULL) {
> +               pr_err("%s: failed to map register space", clk_name);
> +               goto err;
> +       }
> +
> +       spin_lock_init(&sai_clk->lock);
> +
> +       sai_clk->gate.reg = base + I2S_CSR;
> +       sai_clk->gate.bit_idx = CSR_BCE_BIT;
> +       sai_clk->gate.lock = &sai_clk->lock;
> +
> +       sai_clk->div.reg = base + I2S_CR2;
> +       sai_clk->div.shift = CR2_DIV_SHIFT;
> +       sai_clk->div.width = CR2_DIV_WIDTH;
> +       sai_clk->div.lock = &sai_clk->lock;
> +
> +       /* set clock direction, we are the BCLK master */

Should this configuration come from DT somehow?

> +       writel(CR2_BCD, base + I2S_CR2);
> +
> +       hw = clk_hw_register_composite(NULL, clk_name, &parent_name, 1,
> +                                      NULL, NULL,
> +                                      &sai_clk->div.hw, &clk_divider_ops,
> +                                      &sai_clk->gate.hw, &clk_gate_ops,
> +                                      CLK_SET_RATE_GATE);
> +       if (IS_ERR(hw))
> +               goto err;
> +
> +       of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw);
> +
> +       return;
> +
> +err:
> +       kfree(sai_clk);
> +}
> +
> +CLK_OF_DECLARE(fsl_sai_clk, "fsl,vf610-sai-clock", fsl_sai_clk_setup);

Is there a reason this can't be a platform device driver?