linux-kernel - Re: [PATCH 3/8] Add a mfd IPUv3 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110301113155.GO29521@pengutronix.de>
Date:	Tue, 1 Mar 2011 12:31:55 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-fbdev@...r.kernel.org, Paul Mundt <lethal@...ux-sh.org>,
	Samuel Ortiz <sameo@...ux.intel.com>
Subject: Re: [PATCH 3/8] Add a mfd IPUv3 driver

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/