On Tue, Mar 01, 2011 at 11:00:09AM +0100, Thomas Gleixner wrote:
> On Tue, 1 Mar 2011, Sascha Hauer wrote:
> > On Mon, Feb 28, 2011 at 07:33:05PM +0100, Thomas Gleixner wrote:
> > > > +void ipu_idmac_put(struct ipu_channel *channel)
> > > > +{
> > > > + dev_dbg(ipu_dev, "%s %d\n", __func__, channel->num);
> > >
> > > Do we really need this debug stuff in all these functions ?
> >
> > Reading this comment I expected tons of dev_dbg in the driver. The one
> > you mentioned above (plus the corresponding one in ipu_idmac_get) are
> > indeed not particularly useful, but do you think there is still too much
> > debug code left?
>
> Well, I don't see a point in having useless debug stuff around.
> > > > + DECLARE_IPU_IRQ_BITMAP(irqs);
> > >
> > > Why the hell do we need this? It's a bog standard bitmap, right ?
> >
> > It's defined as:
> >
> > #define DECLARE_IPU_IRQ_BITMAP(name) DECLARE_BITMAP(name, IPU_IRQ_COUNT)
> >
> > So yes, it's a standard bitmask. It can be used in client drivers
> > aswell. Where's the problem of adding a define for this so that client
> > drivers do not have to care about the size of the bitmap?
>
> That's nonsense. You have to know about the size of the bitmap for any
> operation on it. Or are you going to provide wrappers around
> bitmap_zero() and all other possible bitmap_* functions as well?
Ok, you're right.
>
> > >
> > > > + bitmap_zero(irqs, IPU_IRQ_COUNT);
> > > > + ret = ipu_submodules_init(pdev, ipu_base, ipu_clk);
> > > > + if (ret)
> > > > + goto failed_submodules_init;
> > > > +
> > > > + /* Set sync refresh channels as high priority */
> > > > + ipu_idmac_write(0x18800000, IDMAC_CHA_PRI(0));
> > >
> > > Hmm, this random prio setting here is odd.
> >
> > This is 1:1 from the Freescale Kernel and I never thought about it. We
> > can remove it and see what happens. Maybe then some day we'll learn
> > *why* this is done.
>
> Well, the point is to move that to the init function which deals with
> IDMAC and not have it at some random place in the code.
Ok, will move it there.
>
> > > > + /* Wait for DC triple buffer to empty */
> > > > + if (dc_channels[dc_chan].di == 0)
> > > > + while ((__raw_readl(DC_STAT) & 0x00000002)
> > > > + != 0x00000002) {
> > > > + msleep(2);
> > > > + timeout -= 2;
> > > > + if (timeout <= 0)
> > > > + break;
> > >
> > > So we poll stuff which is updated from some other function ?
> >
> > We poll the DC_STAT register here which is updated from the hardware.
>
> And there is no interrupt for this ?
Given the sheer amount of interrupt bits it's a bit surprising, but no,
I haven't found an interrupt for this (This of course doesn't mean it
doesn't exist...)
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
