> -----Original Message-----
> From: Varadarajan, Charulatha
> Sent: Monday, November 22, 2010 5:30 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@xxxxxxxxxxxxxxx
> Subject: Re: [PATCHv4 12/14] OMAP: dmtimer: switch-over to platform device
> driver
>
> On Mon, Nov 22, 2010 at 14:44, DebBarma, Tarun Kanti <tarun.kanti@xxxxxx>
> wrote:
> >> -----Original Message-----
> >> From: Varadarajan, Charulatha
> >> Sent: Monday, November 22, 2010 12:15 PM
> >> To: DebBarma, Tarun Kanti
> >> Cc: linux-omap@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCHv4 12/14] OMAP: dmtimer: switch-over to platform
> device
> >> driver
> >>
> >> <<snip>>
> >>
> >> > -/*
> >> > - * Reads timer registers in posted and non-posted mode. The posted
> mode
> >> bit
> >> > - * is encoded in reg. Note that in posted mode write pending bit
> must
> >> be
> >> > - * checked. Otherwise a read of a non completed write will produce
> an
> >> error.
> >> > +/**
> >> > + * omap_dm_timer_read_reg - read timer registers in posted and non-
> >> posted mode
> >> > + * @timer: timer pointer over which read operation to perform
> >> > + * @reg: lowest byte holds the register offset
> >> > + *
> >> > + * The posted mode bit is encoded in reg. Note that in posted mode
> >> write
> >> > + * pending bit must be checked. Otherwise a read of a non completed
> >> write
> >> > + * will produce an error.
> >> > */
> >> > static inline u32 omap_dm_timer_read_reg(struct omap_dm_timer
> *timer,
> >> u32 reg)
> >> > {
> >> > + struct dmtimer_platform_data *pdata = timer->pdev-
> >> >dev.platform_data;
> >> > +
> >> > + if (reg >= OMAP_TIMER_WAKEUP_EN_REG)
> >> > + reg += pdata->func_offst;
> >> > +
> >> > if (timer->posted)
> >> > - while (readl(timer->io_base +
> (OMAP_TIMER_WRITE_PEND_REG
> >> & 0xff))
> >> > - & (reg >> WPSHIFT))
> >> > + while (readl(timer->io_base +
> >> > + ((OMAP_TIMER_WRITE_PEND_REG + pdata-
> >func_offst)
> >> > + & 0xff)) & (reg >> WPSHIFT))
> >>
> >> You may add a timeout in this.
> > There are few reasons why I have not done:
> > (1) This was in this way from the beginning and I thought some analysis
> > Was done already for not adding timeout and so did not wish to alter
> that.
> > (2) wanted to keep low level functions as simple as possible by avoiding
> > As many checks as possible unless really needed.
> > (3) The client driver can take care of timeout. Even if timeout is
> > Introduced in this api, the client driver anyways have to check for the
> > Timeout.
>
> I don't think that client driver should take care of this. The client
> driver should
> check only for the return value. If timeout, return error.
So what I am saying is it can add the timeout instead of checking for the
Return value. This would keep the low level function simple.
Please try to understand that this is a common function called by all the
Client drivers associated with all the timers. If you can reduce a check
The benefit is tremendous.
--
Tarun
>
> In your case, one of the calls to omap_dm_timer_write_reg() is as given
> below:
> omap2_gp_timer_init()
> -> omap2_gp_clockevent_init()
> -> omap_dm_timer_request_specific()
> -> omap_dm_timer_prepare()
> -> omap_dm_timer_write_reg()
>
> Here timeout is not taken care.
>
> >
> > --
> > Tarun
> >>
> >> > cpu_relax();
> >> > return readl(timer->io_base + (reg & 0xff));
> >> > }
> >> >
> >> > -/*
> >> > - * Writes timer registers in posted and non-posted mode. The posted
> >> mode bit
> >> > - * is encoded in reg. Note that in posted mode the write pending bit
> >> must be
> >> > - * checked. Otherwise a write on a register which has a pending
> write
> >> will be
> >> > - * lost.
> >> > +/**
> >> > + * omap_dm_timer_write_reg - write timer registers in posted and
> non-
> >> posted mode
> >> > + * @timer: timer pointer over which write operation is to
> perform
> >> > + * @reg: lowest byte holds the register offset
> >> > + * @value: data to write into the register
> >> > + *
> >> > + * The posted mode bit is encoded in reg. Note that in posted mode
> the
> >> write
> >> > + * pending bit must be checked. Otherwise a write on a register
> which
> >> has a
> >> > + * pending write will be lost.
> >> > */
> >> > static void omap_dm_timer_write_reg(struct omap_dm_timer *timer, u32
> >> reg,
> >> > u32 value)
> >> > {
> >> > + struct dmtimer_platform_data *pdata = timer->pdev-
> >> >dev.platform_data;
> >> > +
> >> > + if (reg >= OMAP_TIMER_WAKEUP_EN_REG)
> >> > + reg += pdata->func_offst;
> >> > +
> >> > if (timer->posted)
> >> > - while (readl(timer->io_base +
> (OMAP_TIMER_WRITE_PEND_REG
> >> & 0xff))
> >> > - & (reg >> WPSHIFT))
> >> > + while (readl(timer->io_base +
> >> > + ((OMAP_TIMER_WRITE_PEND_REG + pdata-
> >func_offst)
> >> > + & 0xff)) & (reg >> WPSHIFT))
> >>
> >> Ditto.
> >>
> >> > cpu_relax();
> >> > writel(value, timer->io_base + (reg & 0xff));
> >> > }
> >>
> >>
> >> <<snip>>
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
