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

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

On 10/31/2013 05:46 PM, Nishanth Menon wrote:
> On 10/31/2013 09:40 AM, 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.
>>
>
> using function pointers might be an appropriate solution. in general a
> low level reg access api that uses two different approaches sounds a
> little.. umm.. fishy..

Initial work I did for v9 had clk_reg_ops struct in place which pretty 
much did this, however Mike recommended looking at the regmap so I did. 
I could put the reg_ops struct back and just have it use internally 
regmap if that would be better, but I guess we need comment from Mike 
how he wants this to be done.

We could have:

struct clk_reg_ops {
	int (*clk_readl)(u32 addr, u32 *val);
	int (*clk_writel)(u32 addr, u32 val);
};

struct clk_foo {
	...
	void __iomem *reg;
	struct clk_reg_ops *regops;
};

>
>>> regmap can also return error value that could also be handled as a result.
>>
>> True, however if the regmap fails for the clock code, not sure what we
>> can do but panic. If this code is expanded, it is probably better to not
>> inline it anymore.
>>
> let the compiler deal with that decision. regmap operation fail should
> be percollated back to initiator of the request. in some cases that
> will be ir-recoverable, but on other cases panic might be the right
> answer - at the low level we are in no position to make that distinction.

Currently, none of the clock drivers handle failures for regmap 
operations, I can of course add some sort of support for this given what 
we do with above point and what the API for the register accesses ends 
up like.

-Tero