From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756426AbcAOIes (ORCPT ); Fri, 15 Jan 2016 03:34:48 -0500 Received: from lucky1.263xmail.com ([211.157.147.133]:51735 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752397AbcAOIep (ORCPT ); Fri, 15 Jan 2016 03:34:45 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: wdc@rock-chips.com X-FST-TO: linux-kernel@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: wdc@rock-chips.com X-UNIQUE-TAG: <1b885577f710e411ac531ee172a2e253> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v3 2/4] i2c: rk3x: add ops to caculate i2c clocks To: Andy Shevchenko , David Wu References: <1452774699-57455-1-git-send-email-david.wu@rock-chips.com> <1452774699-57455-3-git-send-email-david.wu@rock-chips.com> Cc: =?UTF-8?Q?Heiko_St=c3=bcbner?= , Wolfram Sang , Douglas Anderson , Tao Huang , Chris Zhong , cf@rock-chips.com, Jianqun Xu , Lin Huang , linux-arm Mailing List , linux-rockchip@lists.infradead.org, linux-i2c@vger.kernel.org, "linux-kernel@vger.kernel.org" From: "David.Wu" Message-ID: <5698AF24.2030202@rock-chips.com> Date: Fri, 15 Jan 2016 16:34:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andy, 在 2016/1/14 21:19, Andy Shevchenko 写道: > On Thu, Jan 14, 2016 at 2:31 PM, David Wu wrote: >> From: David Wu >> >> I2c Controller of rk3x is updated for the rules to caculate clocks. >> So it need a new method to caculate i2c clock timing information >> for new version. The current method is defined as v0, and new is >> v1, next maybe v2...... > Thanks for an update. My comments below. > >> --- a/drivers/i2c/busses/i2c-rk3x.c >> +++ b/drivers/i2c/busses/i2c-rk3x.c >> @@ -90,10 +95,27 @@ struct rk3x_i2c_soc_data { >> int grf_offset; >> }; >> >> +/** >> + * struct rk3x_priv_i2c_timings - rk3x I2C timing information >> + * @div_low: Divider output for low >> + * @div_high: Divider output for high >> + */ >> +struct rk3x_priv_i2c_timings { >> + unsigned long div_low; >> + unsigned long div_high; >> +}; >> + >> +struct rk3x_i2c_ops { >> + int (*calc_clock)(unsigned long, >> + struct i2c_timings *, >> + struct rk3x_priv_i2c_timings *); >> +}; >> + >> struct rk3x_i2c { >> struct i2c_adapter adap; >> struct device *dev; >> struct rk3x_i2c_soc_data *soc_data; >> + struct rk3x_i2c_ops ops; > Not much sense for now to have a struct for one member. Yes, it looks ok not to do change here. >> /* Hardware resources */ >> void __iomem *regs; >> @@ -102,6 +124,7 @@ struct rk3x_i2c { >> > What about > /* I2C timing settings */ > struct i2c_timings t; > > /* Divider settings */ > int (*calc_divs)(unsigned long, > struct i2c_timings *, > unsigned long *div_low, > unsigned long *div_high); > unsigned long div_low; > unsigned long div_high; > ? > > As far as I understand you still calculate divider values. > >> /* Synchronization & notification */ >> spinlock_t lock; >> @@ -431,21 +454,20 @@ out: >> } >> >> /** >> - * Calculate divider values for desired SCL frequency >> + * Calculate timing clock info values for desired SCL frequency >> * >> * @clk_rate: I2C input clock rate >> - * @t_input: Known I2C timing information. >> - * @div_low: Divider output for low >> - * @div_high: Divider output for high > For now seems better to leave this prototype. > >> + * @t_input: Known I2C timing information >> + * @t_output: Caculated rk3x private timing information that would >> + * be written into regs >> * >> * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case >> * a best-effort divider value is returned in divs. If the target rate is >> * too high, we silently use the highest possible rate. >> */ >> -static int rk3x_i2c_calc_divs(unsigned long clk_rate, >> - struct i2c_timings *t_input, >> - unsigned long *div_low, >> - unsigned long *div_high) >> +static int rk3x_i2c_v0_calc_clock(unsigned long clk_rate, >> + struct i2c_timings *t_input, >> + struct rk3x_priv_i2c_timings *t_output) >> { >> unsigned long spec_min_low_ns, spec_min_high_ns; >> unsigned long spec_setup_start, spec_max_data_hold_ns; > >> @@ -583,25 +605,25 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, >> >> /* Give low the "ideal" and give high whatever extra is left */ >> extra_low_div = ideal_low_div - min_low_div; >> - *div_low = ideal_low_div; >> - *div_high = min_high_div + (extra_div - extra_low_div); >> + t_output->div_low = ideal_low_div; >> + t_output->div_high = min_high_div + (extra_div - extra_low_div); >> } >> >> /* >> * Adjust to the fact that the hardware has an implicit "+1". >> * NOTE: Above calculations always produce div_low > 0 and div_high > 0. >> */ >> - *div_low = *div_low - 1; >> - *div_high = *div_high - 1; >> + t_output->div_low = t_output->div_low - 1; > -= 1 > >> + t_output->div_high = t_output->div_high - 1; > Ditto. > > But without change of prototype those no needed. > >> /* Maximum divider supported by hw is 0xffff */ >> - if (*div_low > 0xffff) { >> - *div_low = 0xffff; >> + if (t_output->div_low > 0xffff) { >> + t_output->div_low = 0xffff; >> ret = -EINVAL; >> } >> >> - if (*div_high > 0xffff) { >> - *div_high = 0xffff; >> + if (t_output->div_high > 0xffff) { >> + t_output->div_high = 0xffff; >> ret = -EINVAL; >> } >> >> @@ -610,19 +632,21 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, >> >> static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate) >> { >> - unsigned long div_low, div_high; >> u64 t_low_ns, t_high_ns; >> int ret; >> >> - ret = rk3x_i2c_calc_divs(clk_rate, &i2c->t, &div_low, &div_high); >> + ret = i2c->ops.calc_clock(clk_rate, &i2c->t, &i2c->t_priv); >> WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->t.bus_freq_hz); >> >> clk_enable(i2c->clk); >> - i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV); >> + i2c_writel(i2c, (i2c->t_priv.div_high << 16) | >> + (i2c->t_priv.div_low & 0xffff), REG_CLKDIV); >> clk_disable(i2c->clk); >> >> - t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, clk_rate); >> - t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, clk_rate); >> + t_low_ns = div_u64(((u64)i2c->t_priv.div_low + 1) * 8 * 1000000000, >> + clk_rate); >> + t_high_ns = div_u64(((u64)i2c->t_priv.div_high + 1) * 8 * 1000000000, >> + clk_rate); >> dev_dbg(i2c->dev, >> "CLK %lukhz, Req %uns, Act low %lluns high %lluns\n", >> clk_rate / 1000, >> @@ -652,12 +676,11 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long >> { >> struct clk_notifier_data *ndata = data; >> struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb); >> - unsigned long div_low, div_high; >> >> switch (event) { >> case PRE_RATE_CHANGE: >> - if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, >> - &div_low, &div_high) != 0) >> + if (i2c->ops.calc_clock(ndata->new_rate, &i2c->t, >> + &i2c->t_priv) != 0) >> return NOTIFY_STOP; >> >> /* scale up */ >> @@ -861,6 +884,7 @@ static int rk3x_i2c_probe(struct platform_device *pdev) >> u32 value; >> int irq; >> unsigned long clk_rate; >> + unsigned int version; > No need to have a separate variable… > >> + version = (readl(i2c->regs + REG_CON) & VERSION_MASK) >> VERSION_SHIFT; >> + if (version == RK3X_I2C_V0) >> + i2c->ops.calc_clock = rk3x_i2c_v0_calc_clock; > value = readl(i2c->regs + REG_CON); > if ((value & VERSION_MASK) >> VERSION_SHIFT) == RK3X_I2C_V0) > i2c->ops.calc_clock = rk3x_i2c_v0_calc_clock; >