linux-kernel - Re: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 8 Nov 2011 20:06:49 +0200
From:	Felipe Balbi <balbi@...com>
To:	"Voss, Nikolaus" <N.Voss@...nmann.de>
Cc:	"'balbi@...com'" <balbi@...com>,
	"'linux-i2c@...r.kernel.org'" <linux-i2c@...r.kernel.org>,
	"'linux-arm-kernel@...ts.infradead.org'" 
	<linux-arm-kernel@...ts.infradead.org>,
	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
	"'ben-linux@...ff.org'" <ben-linux@...ff.org>,
	"'khali@...ux-fr.org'" <khali@...ux-fr.org>,
	"'nicolas.ferre@...el.com'" <nicolas.ferre@...el.com>,
	"'rmallon@...il.com'" <rmallon@...il.com>
Subject: Re: [PATCH V3 2/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

On Tue, Nov 08, 2011 at 04:49:07PM +0100, Voss, Nikolaus wrote:
> > > > > +#include <mach/at91_twi.h>
> > > > > +#include <mach/board.h>
> > > > > +#include <mach/cpu.h>
> > > >
> > > > avoid including <mach/*> on drivers.
> > >
> > > Should I move at91_twi.h to include/linux (omap does it like this,
> > > other use the mach-include)?
> > 
> > maybe, is at91_twi.h some sort of platform_data ? there's
> > <linux/platform_data/...> for that.
> 
> It contains hardware register definitions, not really platform data.
> So linux/i2c-at91.h (like linux/i2c-{omap,pxe,...}) would be the right place?

if it's only register definitions, does it need to be in a header ? I
mean, is anyone outside of this driver trying to access those registers?
Otherwise they could sit on the C source file itself.

If there's anyone else which needs those register definitions then
<linux/i2c/at91.h> seems like a good place (??)

> > > > > +	if (irqstatus & AT91_TWI_TXCOMP) {
> > > > > +		at91_disable_twi_interrupts(dev);
> > > > > +		dev->transfer_status = status;
> > > > > +		complete(&dev->cmd_complete);
> > > > > +	}
> > > > > +	else if (irqstatus & AT91_TWI_RXRDY) {
> > > > > +		at91_twi_read_next_byte(dev);
> > > > > +	}
> > > > > +	else if (irqstatus & AT91_TWI_TXRDY) {
> > > > > +		at91_twi_write_next_byte(dev);
> > > > > +	}
> > > > > +	else {
> > > > > +		return IRQ_NONE;
> > > >
> > > > coding style is wrong. Also, are those IRQ events really mutually
> > exclusive ??
> > >
> > > These are indeed mutually exclusive (semantically).
> > 
> > so you couldn't have AT91_TWI_TXCOMP and AT91_TWI_RXRDY set when you read
> > irqstatus ?
> 
> Yes, I do have this, but in this constellation only TXCOMP is relevant and
> all other flags can be ignored (because the transfer is finished).

I asked about different directions exactly because of that. My question
was if you could have a TX Complete and RX Ready IRQs simultaneously.

The way you coded this IRQ handler is like a priority encoder, meaning
that you will ignore all other bits once you find the first set bit. I'm
wondering if you shouldn't drop the "else" as most of the IRQ handlers
do.

But it's your driver anyway, I don't know how this controller behaves.
Just found it a bit worrying that you ignore all other IRQ status bits.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)