lkml.org 
[lkml]   [2018]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 2/7] clk: at91: add I2S clock mux driver
From
Date
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

\
 
 \ /
  Last update: 2018-06-04 10:21    [W:0.145 / U:0.552 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site