RE: [PATCHv4 12/14] OMAP: dmtimer: switch-over to platform device driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux