On Tue, 2018-05-29 at 14:27 +0300, Jarkko Nikula wrote:
> Mixed timing parameter validation, calculation and their debug prints
> with HW initialization in i2c_dw_init_master() and i2c_dw_init_slave()
> has been bothering me some time.
>
> It makes function a little bit unclear to follow, doesn't show what
> steps
> are needed to do only once during probe and what are needed whenever
> HW
> needs to be reinitialized. Also those debug prints show information
> that
> doesn't change runtime and thus are also needlessly printed multiple
> times
> whenever HW is reinitialized.
>
> Thus let the i2c_dw_init_master() and i2c_dw_init_slave() to do only
> HW
> initialization and move out one time parameter setting and debug
> prints
> to separate functions which are called only during probe. Since SDA
> hold
> time setting is common to both master and slave code remove this
> duplication and move it to common code.
Just would like you to double check if it possible to split this to
couple or more smaller changes.
> +void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
> +{
> + u32 reg;
> + int ret = 0;
> +
> + ret = i2c_dw_acquire_lock(dev);
> + if (ret)
> + return;
> +
> + /* Configure SDA Hold Time if required */
> + reg = dw_readl(dev, DW_IC_COMP_VERSION);
> + if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
> + if (!dev->sda_hold_time) {
> + /* Keep previous hold time setting if no one
> set it */
> + dev->sda_hold_time = dw_readl(dev,
> DW_IC_SDA_HOLD);
> + }
Pewrhaps + blank line here ?
> + /*
> + * Workaround for avoiding TX arbitration lost in
> case I2C
> + * slave pulls SDA down "too quickly" after falling
> egde of
> + * SCL by enabling non-zero SDA RX hold.
> Specification says it
> + * extends incoming SDA low to high transition while
> SCL is
> + * high but it apprears to help also above issue.
> + */
> + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
> + dev->sda_hold_time |= 1 <<
> DW_IC_SDA_HOLD_RX_SHIFT;
> + } else if (dev->sda_hold_time) {
> + dev_warn(dev->dev,
> + "Hardware too old to adjust SDA hold
> time.\n");
> + dev->sda_hold_time = 0;
> + }
> +
> + i2c_dw_release_lock(dev);
> +}
> +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
> {
>
> + u32 comp_param1;
> u32 sda_falling_time, scl_falling_time;
> + int ret = 0;
Redundant assignment.
> ret = i2c_dw_acquire_lock(dev);
> if (ret)
> + return;
> + /* 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),
Ditto here and below.
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.
> + 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.
> + 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?
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
