Re: [PATCHv9 01/43] clk: Add support for regmap register read/write

[Tero Kristo <t-kristo@ti.com>]

On 11/05/2013 11:43 PM, Gerhard Sittig wrote:
> On Thu, Oct 31, 2013 at 16:40 +0200, Tero Kristo wrote:
>>
>> On 10/31/2013 04:03 PM, Nishanth Menon wrote:
>>> On 10/25/2013 10:56 AM, Tero Kristo wrote:
>>> [...]
>>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>>> index 7e59253..63ff78c 100644
>>>> --- a/include/linux/clk-provider.h
>>>> +++ b/include/linux/clk-provider.h
>>>
>>> [...]
>>>> -static inline u32 clk_readl(u32 __iomem *reg)
>>>> +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap)
>>>>   {
>>>> -	return readl(reg);
>>>> +	u32 val;
>>>> +
>>>> +	if (regmap)
>>>> +		regmap_read(regmap, (u32)reg, &val);
>>>> +	else
>>>> +		val = readl(reg);
>>>> +	return val;
>>>>   }
>>>>
>>>> -static inline void clk_writel(u32 val, u32 __iomem *reg)
>>>> +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap)
>>>>   {
>>>> -	writel(val, reg);
>>>> +	if (regmap)
>>>> +		regmap_write(regmap, (u32)reg, val);
>>>> +	else
>>>> +		writel(val, reg);
>>>>   }
>>>>
>>>>   #endif /* CONFIG_COMMON_CLK */
>>>>
>>>
>>> Might it not be better to introduce regmap variants?
>>> static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap
>>> *regmap)
>>> and corresponding readl? that allows cleaner readability for clk
>>> drivers that use regmap and those that dont.
>>
>> Well, doing that will introduce a lot of redundant code, as the
>> checks for the presence of regmap must be copied all over the place.
>> With this patch, all the generic clock drivers support internally
>> both regmap or non-regmap register accesses.
>
> Please keep in mind that the "identity" of clk_readl() and
> readl() only applies in the current source code (ARM only use of
> common CCF primitives), while patches are pending (currently
> under review and receiving further improvement) which introduce
> several alternative implementations of clk_readl() depending on
> the platform.  Making all of them duplicate the "regmap vs direct
> register access" branch would be as bad.  Keeping one set of
> clk_readl() and clk_writel() routines and adding #ifdefs around
> the direct register access would be rather ugly, and I understand
> that preprocessor ifdef counts should get reduced instead of
> introduced.

So, it might be better to provide platform / or clock specific 
clk_reg_ops or something, so we can cover all the possible cases easily.

The requirement from TI SoC point of view is that:

1) need to be able to specify custom register read/write ops
2) need to be able to provide an index parameter to the read/write ops
3) need to be able to provide a register offset to the read/write ops

This could be done in following way for example:

clk_readl / clk_writel would be accessed through a single platform 
specific struct which provides function pointers to both.

The content of 'reg' provided to the clk_readl / clk_writel would be 
internally interpreted as a

struct omap_clk_reg_internal {
	u16 offset;
	u16 index;
};

... the index part would be used to select the appropriate regmap to 
use, and TI SoC:s would be using 3...4 regmaps for actually accessing 
the physical registers themselves.

-Tero

>
>
> virtually yours
> Gerhard Sittig
>