On Fri, 2018-06-01 at 16:47 +0300, Jarkko Nikula wrote:
> SDA hold time configuration is common to both master and slave code.
> It
> is also something that can be done once during probe and do only
> register write when HW needs to be reinitialized.
>
> Remove duplication and move SDA hold time configuration to common
> code.
> It will be called from slave probe and for master code from a new
> i2c_dw_set_timings_master() to where we will populate more probe time
> timing parameter setting.
>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
(Some minor issues / questions below)
> Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-designware-common.c | 35
> ++++++++++++++++++++++
> drivers/i2c/busses/i2c-designware-core.h | 1 +
> drivers/i2c/busses/i2c-designware-master.c | 30 ++++++-------------
> drivers/i2c/busses/i2c-designware-slave.c | 24 ++-------------
> 4 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-common.c
> b/drivers/i2c/busses/i2c-designware-common.c
> index c22654cce1df..1b542d3e5a2e 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -183,6 +183,41 @@ u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf,
> int offset)
> return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 +
> offset;
> }
>
> +void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
> +{
> + u32 reg;
> + int ret = 0;
Redundant assignment.
> +
> + 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);
> + }
> +
> + /*
> + * 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);
> +}
> +
> void __i2c_dw_disable(struct dw_i2c_dev *dev)
> {
> int timeout = 100;
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 5d54f70710ad..831b3d62b32c 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -298,6 +298,7 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int
> offset);
> int i2c_dw_set_reg_access(struct dw_i2c_dev *dev);
> u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int
> offset);
> u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
> +void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev);
> unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
> int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare);
> int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-master.c
> b/drivers/i2c/busses/i2c-designware-master.c
> index 3a7c184f24c8..b89bc8a5b0b1 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -45,6 +45,11 @@ static void i2c_dw_configure_fifo_master(struct
> dw_i2c_dev *dev)
> dw_writel(dev, dev->master_cfg, DW_IC_CON);
> }
>
> +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
> +{
> + i2c_dw_set_sda_hold(dev);
> +}
> +
Looking into patch 6 I'm not sure this split is required here and not
there.
Have you checked which one looks better?
> /**
> * i2c_dw_init() - Initialize the designware I2C master hardware
> * @dev: device private data
> @@ -56,7 +61,7 @@ static void i2c_dw_configure_fifo_master(struct
> dw_i2c_dev *dev)
> static int i2c_dw_init_master(struct dw_i2c_dev *dev)
> {
> u32 hcnt, lcnt;
> - u32 reg, comp_param1;
> + u32 comp_param1;
> u32 sda_falling_time, scl_falling_time;
> int ret;
>
> @@ -132,27 +137,9 @@ static int i2c_dw_init_master(struct dw_i2c_dev
> *dev)
> }
> }
>
> - /* 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);
> - }
> - /*
> - * 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;
> + /* Write SDA hold time if supported */
> + if (dev->sda_hold_time)
> dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> - } else if (dev->sda_hold_time) {
> - dev_warn(dev->dev,
> - "Hardware too old to adjust SDA hold
> time.\n");
> - }
>
> i2c_dw_configure_fifo_master(dev);
> i2c_dw_release_lock(dev);
> @@ -671,6 +658,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> if (ret)
> return ret;
>
> + i2c_dw_set_timings_master(dev);
> ret = dev->init(dev);
> if (ret)
> return ret;
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c
> b/drivers/i2c/busses/i2c-designware-slave.c
> index a1f802001e1f..6ba265db4eb0 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -51,7 +51,6 @@ static void i2c_dw_configure_fifo_slave(struct
> dw_i2c_dev *dev)
> */
> static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
> {
> - u32 reg;
> int ret;
>
> ret = i2c_dw_acquire_lock(dev);
> @@ -61,27 +60,9 @@ static int i2c_dw_init_slave(struct dw_i2c_dev
> *dev)
> /* Disable the adapter. */
> __i2c_dw_disable(dev);
>
> - /* 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);
> - }
> - /*
> - * 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;
> + /* Write SDA hold time if supported */
> + if (dev->sda_hold_time)
> dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> - } else {
> - dev_warn(dev->dev,
> - "Hardware too old to adjust SDA hold
> time.\n");
> - }
>
> i2c_dw_configure_fifo_slave(dev);
> i2c_dw_release_lock(dev);
> @@ -287,6 +268,7 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
> if (ret)
> return ret;
>
> + i2c_dw_set_sda_hold(dev);
> ret = dev->init(dev);
> if (ret)
> return ret;
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
