Hi Daniel,
On Mon, Jan 26, 2015 at 10:43:43AM +0100, Daniel Lezcano wrote:
[...]
> >+static struct dc_dev_t {
> >+ void __iomem *timer_base;
> >+ u32 ticks_per_jiffy;
> >+} dc_dev;
>
> Hi Baruch,
>
> your code is valid but I think there is a misunderstanding when we talked
> about the encapsulation.
>
> I was expecting something like:
>
> static struct digicolor_timer {
> void __iomem *base;
> u32 ticks_per_jiffy;
> int channel;
> struct clock_event_device ce;
> };
>
> struct digicolor_timer *dg_timer(struct clock_event_device *ce)
> {
> return container_of(ce, struct digicolor_timer, ce);
> }
>
> So the timer could be accessed directly from the clock_event_device
> structure. The exception is the 'sched_read' and the init function where you
> will have to address directly the global variable.
>
> Perhaps, that would be nice if you take the opportunity to encapsulate the
> enable/disable functions.
>
> static inline void dg_timer_disable(struct clock_event_device *ce)
> {
> struct digital_timer *dt = dg_timer(ce);
> writeb(CONTROL_DISABLE, dt->base + CONTROL(dt->channel));
> }
>
> static inline void dg_timer_enable(struct clock_event_device *ce, int mode)
> {
> struct digital_timer *dt = dg_timer(ce);
> writeb(CONTROL_ENABLE | mode, dt->base, CONTROL(dt->channel));
> }
>
> >+
> >+static void digicolor_clkevt_mode(enum clock_event_mode mode,
> >+ struct clock_event_device *clk)
>
> IMO, 'clk' argument name is confusing. I would suggest to use 'ce' or
> whatever.
>
> >+{
> >+ switch (mode) {
> >+ case CLOCK_EVT_MODE_PERIODIC:
> >+ writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
> >+ writel(dc_dev.ticks_per_jiffy,
> >+ dc_dev.timer_base + COUNT(TIMER_C));
> >+ writeb(CONTROL_ENABLE | CONTROL_MODE_PERIODIC,
> >+ dc_dev.timer_base + CONTROL(TIMER_C));
>
> Becomes:
>
> dg_timer_disable(ce);
> dg_timer_enable(ce, CONTROL_MODE_PERIODIC);
>
> ...
>
> >+ break;
> >+ case CLOCK_EVT_MODE_ONESHOT:
> >+ writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
> >+ writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
> >+ dc_dev.timer_base + CONTROL(TIMER_C));
>
>
> Becomes:
> dg_timer_disable(ce);
> dg_timer_enable(ce, CONTROL_MODE_ONESHOT);
>
>
> >+ break;
> >+ case CLOCK_EVT_MODE_UNUSED:
> >+ case CLOCK_EVT_MODE_SHUTDOWN:
> >+ default:
> >+ writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
>
> Becomes:
> dg_timer_disable(ce);
>
> >+ break;
> >+ }
> >+}
> >+
> >+static int digicolor_clkevt_next_event(unsigned long evt,
> >+ struct clock_event_device *unused)
> >+{
> >+ writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
> >+ writel(evt, dc_dev.timer_base + COUNT(TIMER_C));
> >+ writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
> >+ dc_dev.timer_base + CONTROL(TIMER_C));
>
> Becomes:
> dg_timer_disable(ce);
> writel(evt, dg_timer(ce)->base + COUNT(dg_timer(ce)->base));
> dg_timer_enable(ce, CONTROL_MODE_ONESHOT);
> >+ return 0;
> >+}
> >+
> >+static struct clock_event_device digicolor_clockevent = {
> >+ .name = "digicolor_tick",
> >+ .rating = 340,
> >+ .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> >+ .set_mode = digicolor_clkevt_mode,
> >+ .set_next_event = digicolor_clkevt_next_event,
> >+};
>
>
> struct digicolor_timer dg_timer = {
>
> .ce = {
> .name = "digicolor_tick",
> .rating = 340,
> .features = CLOCK_EVT_FEAT_PERIODIC |
> CLOCK_EVT_FEAT_ONESHOT,
> .set_mode = digicolor_clkevt_mode,
> .set_next_event = digicolor_clkevt_next_event,
> },
> .channel = TIMER_C,
> };
>
> What do you think ?
OK. I'll give it a try.
Thanks for your prompt response and thorough review.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
