Messages in this thread | | | Date | Mon, 19 May 2014 11:08:20 +0200 | From | Daniel Lezcano <> | Subject | Re: [PATCHv3 3/3] clocksource: Add Freescale FlexTimer Module (FTM) timer support |
| |
On 05/19/2014 04:26 AM, Li.Xiubo@freescale.com 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |