> > +#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 ? > > +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 ? Thanks very much, BRs Xiubo N�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z��w���?�����&�)ߢf��^jǫy�m��@A�a��� 0��h���i