On 05/29/2018 10:14 PM, Andy Shevchenko wrote:
Just would like you to double check if it possible to split this to
couple or more smaller changes.
I was thinking to have i2c_dw_set_sda_hold() as a separate patch but
decided otherwise. Maybe it makes sense as it is not plain code move and
I can add the blank line you suggested.
+ /* Calculate SCL timing parameters for standard mode if not
set */
+ if (!dev->ss_hcnt || !dev->ss_lcnt) {
+ dev->ss_hcnt =
+ i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
You can consider to use
dev->ss_hcnt = dev->ss_hcnt ?: instead of conditional
(rationale: smaller indentation level)
+ dev->ss_lcnt =
+ i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
I wouldn't change the semantics now. Now if either timing parameter is
missing then we calculate both.
Moreover the question if in all cases the first argument is
i2c_dw_clk_rate(dev)? It might be useful to factor this into the callee.
+ dev->fs_hcnt = dev->fp_hcnt;
+ dev->fs_lcnt = dev->fp_lcnt;
Dunno if it makes sense to introduce something like
struct i2c_dw_cnt {
... low;
... high;
};
...and convert the driver to use it.
I'll check that.
+ dev_dbg(dev->dev, "Fast Mode%s HCNT:LCNT = %d:%d\n",
+ dev->clk_freq == 1000000 ? " Plus" : "",
+ dev->fs_hcnt, dev->fs_lcnt);
I would rather go with switch-case or alike and put full mode name into
string literals. Moreover you may use same literal in the code after
all.
True.
+ if (dev->sda_hold_time)
dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
Btw, what happens on older hardware that doesn't support SDA_HOLD
register when you try to write 0 there?
if (dev->sda_hold_time) covers that and dev->sda_hold_time is set to
zero on that HW.
--
Jarkko
