Re: [PATCH v4 2/7] clk: at91: add I2S clock mux driver

From: Stephen Boyd
Date: Thu May 31 2018 - 11:26:48 EST


Quoting Codrin Ciubotariu (2018-05-25 05:34:23)
> This driver is a simple muxing driver that controls the
> I2S's clock input by using syscon/regmap to change the parrent.

s/parrent/parent/

> The available inputs can be Peripheral clock and Generated clock.

Why capitalize peripheral and generated?

>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
> ---
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 1254bf9..903f23c 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -27,6 +27,7 @@ config SOC_SAMA5D2
> select HAVE_AT91_H32MX
> select HAVE_AT91_GENERATED_CLK
> select HAVE_AT91_AUDIO_PLL
> + select HAVE_AT91_I2S_MUX_CLK
> select PINCTRL_AT91PIO4
> help
> Select this if ou are using one of Microchip's SAMA5D2 family SoC.
> @@ -129,6 +130,9 @@ config HAVE_AT91_GENERATED_CLK
> config HAVE_AT91_AUDIO_PLL
> bool
>
> +config HAVE_AT91_I2S_MUX_CLK
> + bool
> +
> config SOC_SAM_V4_V5
> bool
>

I guess this is OK.

> diff --git a/drivers/clk/at91/Makefile b/drivers/clk/at91/Makefile
> index 082596f..facc169 100644
> --- a/drivers/clk/at91/Makefile
> +++ b/drivers/clk/at91/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_HAVE_AT91_USB_CLK) += clk-usb.o
> obj-$(CONFIG_HAVE_AT91_SMD) += clk-smd.o
> obj-$(CONFIG_HAVE_AT91_H32MX) += clk-h32mx.o
> obj-$(CONFIG_HAVE_AT91_GENERATED_CLK) += clk-generated.o
> +obj-$(CONFIG_HAVE_AT91_I2S_MUX_CLK) += clk-i2s-mux.o
> diff --git a/drivers/clk/at91/clk-i2s-mux.c b/drivers/clk/at91/clk-i2s-mux.c
> new file mode 100644
> index 0000000..2d56ded
> --- /dev/null
> +++ b/drivers/clk/at91/clk-i2s-mux.c
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Microchip Technology Inc,
> + * Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
> + *
> + *
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include <soc/at91/atmel-sfr.h>
> +
> +#define I2S_BUS_NR 2
> +
> +struct clk_i2s_mux {
> + struct clk_hw hw;
> + struct regmap *regmap;
> + u32 bus_id;

Can be a u8?

> +};
> +
> +#define to_clk_i2s_mux(hw) container_of(hw, struct clk_i2s_mux, hw)
> +
> +static u8 clk_i2s_mux_get_parent(struct clk_hw *hw)
> +{
> + struct clk_i2s_mux *mux = to_clk_i2s_mux(hw);
> + u32 val;
> +
> + regmap_read(mux->regmap, AT91_SFR_I2SCLKSEL, &val);
> +
> + return (val & BIT(mux->bus_id)) >> mux->bus_id;
> +}
> +
> +static int clk_i2s_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct clk_i2s_mux *mux = to_clk_i2s_mux(hw);
> +
> + return regmap_update_bits(mux->regmap, AT91_SFR_I2SCLKSEL,
> + BIT(mux->bus_id), index << mux->bus_id);
> +}
> +
> +const struct clk_ops clk_i2s_mux_ops = {

static?

> + .get_parent = clk_i2s_mux_get_parent,
> + .set_parent = clk_i2s_mux_set_parent,
> + .determine_rate = __clk_mux_determine_rate,
> +};
> +
> +static struct clk_hw * __init
> +at91_clk_i2s_mux_register(struct regmap *regmap, const char *name,
> + const char * const *parent_names,
> + unsigned int num_parents, u32 bus_id)
> +{
> + struct clk_init_data init = {};
> + struct clk_i2s_mux *i2s_ck;
> + int ret;
> +
> + i2s_ck = kzalloc(sizeof(*i2s_ck), GFP_KERNEL);
> + if (!i2s_ck)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &clk_i2s_mux_ops;
> + init.parent_names = parent_names;
> + init.num_parents = num_parents;
> + init.flags = CLK_IGNORE_UNUSED;

Really? Why?

> +
> + i2s_ck->hw.init = &init;
> + i2s_ck->bus_id = bus_id;
> + i2s_ck->regmap = regmap;
> +
> + ret = clk_hw_register(NULL, &i2s_ck->hw);
> + if (ret) {
> + kfree(i2s_ck);
> + return ERR_PTR(ret);
> + }
> +
> + return &i2s_ck->hw;
> +}