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

[Codrin Ciubotariu]

On 31.05.2018 18:26, Stephen Boyd wrote:
> 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/

I will fix it.

> > The available inputs can be Peripheral clock and Generated clock.
>
> Why capitalize peripheral and generated?

In DS, at I2S block these clocks appear defined with capital letters... 
I will fix it.

> > 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?

I think so, I will cast out_value parameter of of_property_read_u32() to 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?

Yes, I will fix it.

> > +       .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?

I am thinking that there is no need to gate this clock, since there is 
no way to gate this clock in HW.

> > +
> > +       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;
> > +}

Thank you for your review. I will wait a few more days for more comments 
on this series and send a V5 afterwards.

Best regards,
Codrin