linux-kernel - Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller

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]

Message-ID: <20150330172309.GB9742@pengutronix.de>
Date:	Mon, 30 Mar 2015 19:23:09 +0200
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Eddie Huang <eddie.huang@...iatek.com>
Cc:	Mark Rutland <mark.rutland@....com>,
	Wolfram Sang <wsa@...-dreams.de>,
	Doug Anderson <dianders@...omium.org>,
	linux-i2c@...r.kernel.org, David Box <david.e.box@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Jean Delvare <jdelvare@...e.de>,
	Xudong Chen <xudong.chen@...iatek.com>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Pawel Moll <pawel.moll@....com>,
	Liguo Zhang <liguo.zhang@...iatek.com>,
	Wei Yan <sledge.yanwei@...wei.com>,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Neelesh Gupta <neelegup@...ux.vnet.ibm.com>,
	devicetree@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Beniamino Galvani <b.galvani@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	linux-mediatek@...ts.infradead.org,
	Matthias Brugger <matthias.bgg@...il.com>,
	linux-arm-kernel@...ts.infradead.org, srv_heupstream@...iatek.com,
	Anders Berg <anders.berg@...gotech.com>,
	Jim Cromie <jim.cromie@...il.com>,
	Simon Glass <sjg@...omium.org>,
	Max Schwarz <max.schwarz@...ine.de>,
	linux-kernel@...r.kernel.org, Sascha Hauer <kernel@...gutronix.de>,
	Kumar Gala <galak@...eaurora.org>
Subject: Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C
 controller

On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote:
> Hi Sascha,
> 
> > 
> > [...]
> > 
> > > +	if (i2c->speed_hz > 400000)
> > > +		control_reg |= I2C_CONTROL_RS;
> > > +	if (i2c->op == I2C_MASTER_WRRD)
> > > +		control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > > +	mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
> > > +
> > > +	/* set start condition */
> > > +	if (i2c->speed_hz <= 100000)
> > > +		mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
> > > +	else
> > > +		mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
> > > +
> > > +	if (~control_reg & I2C_CONTROL_RS)
> > > +		mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
> > 
> > speed <= 400000 here to make this more obvious?
> There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> to keep it.

Still it looks strange. You only ever write this default value to the
register. Putting this register write under an if() seems bogus since
the same value will be in the register the next time this code is
executed. It looks like you should move this register write to some
initialization function.

> > > +
> > > +	/* Enable interrupt */
> > > +	mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> > > +		i2c, OFFSET_INTR_MASK);
> > 
> > Why do you enable/disable interrupts for each transfer? Enabling them
> > once and just acknowledge them in the interrupt handler should be
> > enough.
> This can avoid unwanted I2C interrupt. For example, I2C transfer error,
> and cause timeout, I2C driver report error to caller. Then I2C error
> interrupt happen.

So isn't the same unwanted interrupt then just delayed until you enable
the interrupts again? Is this something that really happens or just
something you think that might happen?

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/