On 05/19/2014 04:26 AM, Li.Xiubo@xxxxxxxxxxxxx wrote:
+#define FTM_CNTIN 0x4C
+
+static void __iomem *clksrc_base;
+static void __iomem *clkevt_base;
+static unsigned long peroidic_cyc;
+static unsigned long ps;
+bool big_endian;
+
Usually this is encaspulated in a structure.
struct ftm_clock_device {
void __iomem *clksrc_base;
...
};
+static inline u32 ftm_readl(void __iomem *addr)
+{
+ if (big_endian)
I am not a big fan of addressing global variables in the functions, so
if you can pass the structure pointer around here and the other
functions instead that would be nice.
Otherwise the patch sounds ok. Thanks for taking care of encapsulating
well the functions and commenting the code.
Yes, I did think so.
But some callbacks like ：
+ static u64 ftm_read_sched_clock(void)
+ {
+ return ftm_readl(clksrc_base + FTM_CNT);
+ }
Used by :
+ sched_clock_register(ftm_read_sched_clock,....);
If they are encapsulated in a structure, and should the struct instance
be one global instance too ? I'm doubting whether will this make sense ?
Actually, I plan in a near future to consolidate the code and factor out
some parts with a common structure across the different drivers. So even
if you address the base@ with the global instance but pass around the
structure as parameter, that will be ok because that will be less
modifications in the future. It is not a strong requirement, just put in
place some encapsulation to make the life easier for after.
+static int __init ftm_calc_closest_round_cyc(unsigned long freq)
+{
+ ps = 0;
+
+ do {
+ peroidic_cyc = DIV_ROUND_CLOSEST(freq, HZ * (1 << ps++));
+ } while (peroidic_cyc > 0xFFFF);
+
+ if (ps > 7) {
+ pr_err("ftm: the max prescaler is %lu > 7\n", ps);
+ return -EINVAL;
+ }
+
Can you explain how this error can happen ?
Yes, the hardware limitation of the 'ps' is 0~7, and the counter register
Is only using the lower 16 bits.
If the 'freq' value is too big here, then the periodic_cyc may exceed 0xFFFF.
Or should I add some comment here ?
Yes, a comment will be welcome.
Thanks
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
