On Tue, Dec 08, 2009 at 03:53:24, Kevin Hilman wrote:
> "Nori, Sekhar" <nsek...@ti.com> writes:
>
> > On Thu, Nov 26, 2009 at 00:54:38, Kevin Hilman wrote:
> >> Sekhar Nori <nsek...@ti.com> writes:
> >>
[...]
> >> > +static struct davinci_pm_config *pdata;
> >> > +
> >> > +static void davinci_sram_push(void *dest, void *src, unsigned int size)
> >> > +{
> >> > + memcpy(dest, src, size);
> >> > + flush_icache_range((unsigned long)dest, (unsigned long)(dest +
> >> > size));
> >> > +}
> >> > +
> >> > +static inline void davinci_pm_and(void __iomem *base, int offset,
> >> > unsigned and)
> >> > +{
> >> > + unsigned val = __raw_readl(base + offset);
> >> > + val &= and;
> >> > + __raw_writel(val, base + offset);
> >> > +}
> >> > +
> >> > +static inline void davinci_pm_or(void __iomem *base, int offset,
> >> > unsigned or)
> >> > +{
> >> > + unsigned val = __raw_readl(base + offset);
> >> > + val |= or;
> >> > + __raw_writel(val, base + offset);
> >> > +}
> >> > +
> >> > +static inline void davinci_pm_modify(void __iomem *base, int offset,
> >> > + unsigned and, unsigned or)
> >> > +{
> >> > + unsigned val = __raw_readl(base + offset);
> >> > + val &= and;
> >> > + val |= or;
> >> > + __raw_writel(val, base + offset);
> >> > +}
> >>
> >> I'm not sure these helper functions help in readability.
> >>
> >> I think the functions would be more readable using accessing the
> >> registers directly, and could also be more efficient. e.g. a single
> >> read up front followed by modify/write accesses.
> >>
> >> At least in the suspend func below, all the accesses are to the same
> >> register, so i don't see the value in these helper funcs.
> >
> > This comes from a desire to transliterate the user guide
> > step-by-step procedure into C code. Also, it is not always
> > possible to just read once and write once as there is a
> > requirement to wait after writing certain bits (reset wait,
> > lock wait).
> >
> > Let me know, I can always redo the way you suggest and we
> > can then choose the one that looks better.
>
> I think those helper functions do not help readability. Just do the
> raw read/modify/writes and I think it will greatly improve
> readability.
>
> If you want to track the docs, you could use comments for that
> including a reference to the doc and relevant sections.
>
> >>
> >> > +static void davinci_pm_suspend(void)
> >> > +{
> >> > + DEFINE_SPINLOCK(lock);
> >>
> >> What is this local lock protecting? and why is an IRQ-save lock. I'm
> >> guessing what you is to be sure this is an IRQs off section, but I'm
> >> pretty sure the higher level suspend code has disabled IRQs by now.
> >>
> >
> > Honestly, I just saw that preempt_count() is still 0 in
> > davinci_pm_suspend() and went ahead with the locking. I
> > will check and remove the locking.
>
> If the upper layers of the suspend code don't disable interrupts, then
> I suggest you just use a local_irq_save/restore block here instad of a
> spinlock.
Thanks Kevin. I will rework this patch and post.
Regards,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
