Charu and Benoit,
> -----Original Message-----
> From: Varadarajan, Charulatha
> Sent: Monday, November 22, 2010 6:05 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 17:38, DebBarma, Tarun Kanti <tarun.kanti@xxxxxx>
> wrote:
> >
> >> -----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.
>
> I do not agree. If low level driver functions are using these common
> read/write
> functions, there is a possibility of getting struck up in the while loop
> since
> there is no timeout. If you think that there is no possibility of timeout
> when
> called from low level driver functions, remove the while loop unless if it
> is
> really required.
>
> If these functions uses while loops, please have a timeout error.
> Otherwise,
> analyze and remove the while loop from these functions.
As long as we use *posted* mode we need the while loop check.
So the timeout can be added inside the while loop.
Benoit,
Under this circumstance I am feeling no more value for having
Separate set of low level functions for accessing the interrupt
Registers, viz. omap_dm_timer_read_intr_reg() and
omap_dm_timer_write_intr_reg() which were added to keep the
low level functions simple.
So can I go ahead and merge them within omap_dm_timer_read_reg()
And omap_dm_timer_write_reg()?
--
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
