Re: [PATCH v3 1/3] clk: Add regmap support

[Matthias Brugger]

2015-06-16 12:23 GMT+02:00 Joachim  Eastwood <manabian@xxxxxxxxx>:
> On 9 June 2015 at 17:38, Matthias Brugger <matthias.bgg@xxxxxxxxx> wrote:
>> Some devices like SoCs from Mediatek need to use the clock
>> through a regmap interface.
>> This patch adds regmap support for the simple multiplexer clock,
>> the divider clock and the clock gate code.
>>
>> Signed-off-by: Matthias Brugger <matthias.bgg@xxxxxxxxx>
>> ---
>>  drivers/clk/Makefile         |   1 +
>>  drivers/clk/clk-divider.c    |  71 ++++++++++++++++++++------
>>  drivers/clk/clk-gate.c       |  60 +++++++++++++++++-----
>>  drivers/clk/clk-io.c         |  62 +++++++++++++++++++++++
>>  drivers/clk/clk-io.h         |  13 +++++
>>  drivers/clk/clk-mux.c        |  94 +++++++++++++++++++++++++++++------
>>  include/linux/clk-provider.h | 115 +++++++++++++++++++++++++++++++++++++++++--
>>  7 files changed, 373 insertions(+), 43 deletions(-)
>>  create mode 100644 drivers/clk/clk-io.c
>>  create mode 100644 drivers/clk/clk-io.h
> [...]
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 2e5df06..22acfd5 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -14,6 +14,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>>  #include <linux/of.h>
>> +#include <linux/regmap.h>
>>
>>  #ifdef CONFIG_COMMON_CLK
>>
>> @@ -31,6 +32,7 @@
>>  #define CLK_GET_RATE_NOCACHE   BIT(6) /* do not use the cached clk rate */
>>  #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
>>  #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
>> +#define CLK_USE_REGMAP BIT(9) /* clock uses regmap to access its registers */
>>
>>  struct clk_hw;
>>  struct clk_core;
>> @@ -271,6 +273,8 @@ void of_fixed_clk_setup(struct device_node *np);
>>   *
>>   * @hw:                handle between common and hardware-specific interfaces
>>   * @reg:       register controlling gate
>> + * @regmap:    regmap used to control the gate
>> + * @offset:    offset inside the regmap
>>   * @bit_idx:   single bit controlling gate
>>   * @flags:     hardware-specific flags
>>   * @lock:      register lock
>> @@ -288,7 +292,11 @@ void of_fixed_clk_setup(struct device_node *np);
>>   */
>>  struct clk_gate {
>>         struct clk_hw hw;
>> -       void __iomem    *reg;
>> +       union {
>> +               void __iomem    *reg;
>> +               struct regmap   *regmap;
>> +       };
>> +       u32             offset;
>
> Maybe using a named union here would be nice here. Something like:
> union clk_io {
>                void __iomem    *reg;
>                struct regmap   *regmap;
> };
>
> And you could use this in the clk_gate, clk_mux, and clk_div
> structures as well as your clk_io_* functions.
> Having both void __iomem *reg and struct regmap *regmap as function
> parameters seems a bit awkward to me.
>
> Or maybe even as a structure:
> struct clk_io {
>                union {
>                               void __iomem    *reg;
>                               struct regmap   *regmap;
>                };
>                u32 offset;
> }
>
> What do you think?
>

The problem with this approach is, that I would have to change all
clocks which use clk_mux, clk_gate or clk_divider.
Up to now, i tried to find a solution which does not change the
existing interface.
So I would pretty much prefer to hear the opinion from the clk
maintainers first before starting the crusade. :)

Thanks,
Matthias

-- 
motzblog.wordpress.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/