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

[Codrin Ciubotariu <codrin.ciubotariu@microchip.com>]

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@microchip.com>
>> ---
>> 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@microchip.com>
>> + *
>> + *
>> + */
>> +
>> +#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