linux-kernel - Re: [PATCH v8 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: <1432108775.12796.11.camel@mtksdaap41>
Date:	Wed, 20 May 2015 15:59:35 +0800
From:	Eddie Huang <eddie.huang@...iatek.com>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
CC:	Mark Rutland <mark.rutland@....com>,
	Xudong Chen <xudong.chen@...iatek.com>,
	<srv_heupstream@...iatek.com>, Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	"Wolfram Sang" <wsa@...-dreams.de>,
	Liguo Zhang <liguo.zhang@...iatek.com>,
	<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	<linux-mediatek@...ts.infradead.org>, <linux-i2c@...r.kernel.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	Kumar Gala <galak@...eaurora.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 2/3] I2C: mediatek: Add driver for MediaTek I2C
 controller

Hi Uwe,

On Wed, 2015-05-20 at 09:11 +0200, Uwe Kleine-König wrote:
> Hello Eddie,
> 
> On Wed, May 20, 2015 at 10:40:11AM +0800, Eddie Huang wrote:
> > On Mon, 2015-05-18 at 20:43 +0200, Uwe Kleine-König wrote:
> > > On Tue, May 19, 2015 at 12:40:08AM +0800, Eddie Huang wrote:
> > > > +/* calculate i2c port speed */
> > > It would be nice to summarize the clock frequency settings here.
> > > Something like:
> > > 
> > > 	/*
> > > 	 * The input clock is divided by the value specified in the
> > > 	 * device tree as clock-div. The actual bus speed is then
> > > 	 * derived from this frequency by the following formula:
> > > 	 *  ....
> > > 
> > > This would make it possible to verify your calculations below.
> > 
> > The comment will be:
> > /*
> >  * khz: I2C bus clock
> >  * hclk: The input clock is divided by the value specified in the 
> >  *       device tree as clock-div
> and which one of the two clocks you're writing about is hclk now? I
> assume the divided one.
> >  * div = (sample_cnt + 1) * (step_cnt + 1)
> >  * khz = (hclk / 2) / div
> khz for the 2nd time.
> 
> >  *
> >  * The calculation is to get div value that let result of 
> >  * ((hclk / 2) / div) most approach and less than khz
> >  */
> I imagined something more hardware related. A list of register (or
> register bit fields) that influence the frequency and a formula

> 	i2c_freq = parent_clk / clock-div * (...)
> 
> (It seems to be a bit more complicated here as there are two registers
> involved that are set differently depending on the target frequency.)

Yes, hardware is a little complicated. I rewrite the comment:

/*
 * Calculate i2c port speed
 *
 * Hardware design:
 * i2c_bus_freq = parent_clk / (clock-div * 2 * (sample_cnt) *
(step_cnt))
 * clock-div: fixed in hardware, but may be various in different SoCs
 *
 * The calculation want to pick the highest bus frequency that is still 
 * less than or equal to i2c->speed_hz. and the calculation try to get 
 * sample_cnt and step_cnt to fill in hardware register.
 */

> 
> > > > +static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int clk_src_in_hz)
> clk_src_in_hz is the module's input rate already divided by clock-div.
> This clock-div value is fixed in hardware and unchangeable, right?
Yes

> Maybe give that divided clock a nice name?
I don't know, after new comment, maybe this name is ok.

> The target frequency is i2c->speed_hz, so among the possible frequencies
> we want to pick the highest one that is still less than or equal
> i2c->speed_hz, right?
Right.

> 
> > > > +		/* Set the hign speed mode register */
> I just notice s/hign/high/ here.
> 
Thanks, will fix it.

Eddie


--
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/