[PATCH v7 0/4] i2c: npcm: read/write operation, checkpatch

[PATCH v7 0/4] i2c: npcm: read/write operation, checkpatch

[Tyrone Ting]

This patchset includes the following fixes:

- Enable the target functionality in the interrupt handling routine 
  when the i2c transfer is about to finish.
- Correct the read/write operation procedure.
- Introduce a software flag to handle the bus error (BER) condition
  which is not caused by the i2c transfer.
- Modify timeout calculation.
- Assign the client address earlier logically.
- Use an i2c frequency table for the frequency parameters assignment.
- Coding style fix.

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Addressed comments from:
- Andy Shevchenko : https://lore.kernel.org/lkml/ZwkFWVC3_5xr6OQW
  @smile.fi.intel.com/
- Andy Shevchenko : https://lore.kernel.org/lkml/ZwkFwABviY8ClyUo
  @smile.fi.intel.com/

Changes since version 6:
- Modify code comments.
- Remove redundant code check.
- Remove i2c address mask.

Changes since version 5:
- Correct "EAGAIN" to "-EAGAIN" in the commit message.
- Configure the bus->dest_addr by calling i2c_8bit_addr_from_msg()
  and remove the I2C_M_RD flag when calling i2c_recover_bus().
- Fix the commit message which meets a too small wrapping limit.

Changes since version 4:
- Add more description for the codes.
- Modify the term "function" to "function()" in the commit message
and code comments.

Changes since version 3:
- Remove "Bug fixes" in the cover letter title.
- Modify the term "function" to "func()" in the commit message and
  code comments.
- Correct the coding style.

Changes since version 2:
- Add more explanations in the commit message and code modification.
- Use lower character names for declarations.
- Remove Fixes tags in commits which are not to fix bugs.

Changes since version 1:
- Restore the npcm_i2caddr array length to fix the smatch warning.
- Remove unused variables.
- Handle the condition where scl_table_cnt reaches to the maximum value.
- Fix the checkpatch warning.

Charles Boyer (1):
  i2c: npcm: Enable slave in eob interrupt

Tyrone Ting (3):
  i2c: npcm: Modify timeout evaluation mechanism
  i2c: npcm: Modify the client address assignment
  i2c: npcm: use i2c frequency table

 drivers/i2c/busses/i2c-npcm7xx.c | 424 ++++++++++++++++++++++++-------
 1 file changed, 328 insertions(+), 96 deletions(-)


base-commit: 663bff1ddfe4ecbba3bcf4a74646bb388b1ad5b2
-- 
2.34.1

[PATCH v7 1/4] i2c: npcm: Modify timeout evaluation mechanism

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

The users want to connect a lot of masters on the same bus.
This timeout is used to determine the time it takes to take bus ownership.
The transactions are very long, so waiting 35ms is not enough.

Increase the timeout and treat it as the total timeout, including retries.
The total timeout is 2 seconds now.

The i2c core layer will have chances to retry to call the i2c driver
transfer function if the i2c driver reports that the bus is busy and
returns -EAGAIN.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 482a0074d448..c96a25d37c14 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2132,19 +2132,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		}
 	}
 
-	/*
-	 * Adaptive TimeOut: estimated time in usec + 100% margin:
-	 * 2: double the timeout for clock stretching case
-	 * 9: bits per transaction (including the ack/nack)
-	 */
-	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
-	timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
 	if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
 		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
 		return -EINVAL;
 	}
 
-	time_left = jiffies + timeout + 1;
+	time_left = jiffies + bus->adap.timeout / bus->adap.retries + 1;
 	do {
 		/*
 		 * we must clear slave address immediately when the bus is not
@@ -2192,6 +2185,14 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
 				       write_data, read_data, read_PEC,
 				       read_block)) {
+		/*
+		 * Adaptive TimeOut: estimated time in usec + 100% margin:
+		 * 2: double the timeout for clock stretching case
+		 * 9: bits per transaction (including the ack/nack)
+		 */
+		timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
+		timeout = max_t(unsigned long, bus->adap.timeout / bus->adap.retries,
+				usecs_to_jiffies(timeout_usec));
 		time_left = wait_for_completion_timeout(&bus->cmd_complete,
 							timeout);
 
@@ -2317,7 +2318,12 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	adap = &bus->adap;
 	adap->owner = THIS_MODULE;
 	adap->retries = 3;
-	adap->timeout = msecs_to_jiffies(35);
+	/*
+	 * The users want to connect a lot of masters on the same bus.
+	 * This timeout is used to determine the time it takes to take bus ownership.
+	 * The transactions are very long, so waiting 35ms is not enough.
+	 */
+	adap->timeout = 2 * HZ;
 	adap->algo = &npcm_i2c_algo;
 	adap->quirks = &npcm_i2c_quirks;
 	adap->algo_data = bus;
-- 
2.34.1

[PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

Store the client address earlier since it might get called in
the i2c_recover_bus() logic flow at the early stage of
npcm_i2c_master_xfer().

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index c96a25d37c14..a9a9b21f1f0b 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2155,6 +2155,16 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 	} while (time_is_after_jiffies(time_left) && bus_busy);
 
+	/*
+	 * Previously, the 7-bit address was stored and being converted to
+	 * the address of event in the following call to npcm_i2c_master_start_xmit().
+	 *
+	 * Since there are cases that the i2c_recover_bus() gets called at the
+	 * early stage of npcm_i2c_master_xfer(), the address of event is stored
+	 * and then used in the i2c_recover_bus().
+	 */
+	bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
+
 	/*
 	 * Check the BER (bus error) state, when ber_state is true, it means that the module
 	 * detects the bus error which is caused by some factor like that the electricity
@@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	 * bus is busy.
 	 */
 	if (bus_busy || bus->ber_state) {
+		/*
+		 * Since the transfer might be a read operation, remove the I2C_M_RD flag
+		 * from the bus->dest_addr for the i2c_recover_bus() call later.
+		 *
+		 * The i2c_recover_bus() uses the address in a write direction to recover
+		 * the i2c bus if some error condition occurs.
+		 */
+		bus->dest_addr &= ~I2C_M_RD;
+
 		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 		npcm_i2c_reset(bus);
 		i2c_recover_bus(adap);
@@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	npcm_i2c_init_params(bus);
-	bus->dest_addr = slave_addr;
 	bus->msgs = msgs;
 	bus->msgs_num = num;
 	bus->cmd_err = 0;
-- 
2.34.1

[PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

Modify i2c frequency from table parameters for NPCM i2c modules.

Supported frequencies are:

1. 100KHz
2. 400KHz
3. 1MHz

The original equations were tested on a variety of chips and base clocks.
Since we added devices that use higher frequencies of the module we
saw that there is a mismatch between the equation and the actual
results on the bus itself, measured on scope.

Meanwhile, the equations were not accurate to begin with.
They are an approximation of the ideal value. The ideal value is
calculated per frequency of the core module.

So instead of using the equations we did an optimization per module
frequency, verified on a device.

Most of the work was focused on the rise time of the SCL and SDA,
which depends on external load of the bus and PU.

Different PCB designs, or specifically to this case: the number
and type of targets on the bus, impact the required values for
the timing registers.

Users can recalculate the numbers for each bus and get an even better
optimization, but our users chose not to.

We manually picked values per frequency that match the entire valid
range of targets (from 1 to max number). Then we check against the
AMR described in SMB spec and make sure that none of the values
is exceeding.

This process was led by the chip architect and included a lot of testing.

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 374 ++++++++++++++++++++++++-------
 1 file changed, 288 insertions(+), 86 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index a9a9b21f1f0b..7c13f9f6014a 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -263,6 +263,265 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define I2C_FREQ_MIN_HZ			10000
 #define I2C_FREQ_MAX_HZ			I2C_MAX_FAST_MODE_PLUS_FREQ
 
+struct smb_timing_t {
+	u32 core_clk;
+	u8 hldt;
+	u8 dbcnt;
+	u16 sclfrq;
+	u8 scllt;
+	u8 sclht;
+	bool fast_mode;
+};
+
+static struct smb_timing_t smb_timing_100khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4,
+		.sclfrq = 0xFB, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x9D, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x7E, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x79, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x65, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 30000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x4C, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 29000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x49, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 26000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x42, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x3F, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x3D, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x33, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 16180000, .hldt = 0x2A, .dbcnt = 0x1,
+		.sclfrq = 0x29, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 15000000, .hldt = 0x23, .dbcnt = 0x1,
+		.sclfrq = 0x26, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 13000000, .hldt = 0x1D, .dbcnt = 0x1,
+		.sclfrq = 0x21, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 12000000, .hldt = 0x1B, .dbcnt = 0x1,
+		.sclfrq = 0x1F, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 10000000, .hldt = 0x18, .dbcnt = 0x1,
+		.sclfrq = 0x1A, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 9000000, .hldt = 0x16, .dbcnt = 0x1,
+		.sclfrq = 0x17, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 8090000, .hldt = 0x14, .dbcnt = 0x1,
+		.sclfrq = 0x15, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 7500000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x13, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 6500000, .hldt = 0xE, .dbcnt = 0x1,
+		.sclfrq = 0x11, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+	{
+		.core_clk = 4000000, .hldt = 0x9, .dbcnt = 0x1,
+		.sclfrq = 0xB, .scllt = 0x0, .sclht = 0x0,
+		.fast_mode = false,
+	},
+};
+
+static struct smb_timing_t smb_timing_400khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x3,
+		.sclfrq = 0x0, .scllt = 0x47, .sclht = 0x35,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0x2A, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0x2C, .sclht = 0x22,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0x21, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x24, .sclht = 0x1B,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x1E, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x24, .sclht = 0x19,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x1B, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x1E, .sclht = 0x14,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 33000000, .hldt = 0x15, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x19, .sclht = 0x11,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 30000000, .hldt = 0x15, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x19, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 29000000, .hldt = 0x11, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x15, .sclht = 0x10,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 26000000, .hldt = 0x10, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x13, .sclht = 0xE,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0xF, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x13, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0xD, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x12, .sclht = 0xD,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0xB, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xF, .sclht = 0xA,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 16180000, .hldt = 0xA, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xC, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 15000000, .hldt = 0x9, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xB, .sclht = 0x8,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 13000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x7,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 12000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x6,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 10000000, .hldt = 0x6, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x8, .sclht = 0x5,
+		.fast_mode = true,
+	},
+};
+
+static struct smb_timing_t smb_timing_1000khz[] = {
+	{
+		.core_clk = 100000000, .hldt = 0x15, .dbcnt = 0x4,
+		.sclfrq = 0x0, .scllt = 0x1C, .sclht = 0x15,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 62500000, .hldt = 0xF, .dbcnt = 0x3,
+		.sclfrq = 0x0, .scllt = 0x11, .sclht = 0xE,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 50000000, .hldt = 0xA, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xE, .sclht = 0xB,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 48000000, .hldt = 0x9, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xD, .sclht = 0xB,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 41000000, .hldt = 0x9, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xC, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 40000000, .hldt = 0x8, .dbcnt = 0x2,
+		.sclfrq = 0x0, .scllt = 0xB, .sclht = 0x9,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 33000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0xA, .sclht = 0x7,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 25000000, .hldt = 0x4, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x7, .sclht = 0x6,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 24000000, .hldt = 0x7, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x8, .sclht = 0x5,
+		.fast_mode = true,
+	},
+	{
+		.core_clk = 20000000, .hldt = 0x4, .dbcnt = 0x1,
+		.sclfrq = 0x0, .scllt = 0x6, .sclht = 0x4,
+		.fast_mode = true,
+	},
+};
+
 struct npcm_i2c_data {
 	u8 fifo_size;
 	u32 segctl_init_val;
@@ -1805,102 +2064,45 @@ static void npcm_i2c_recovery_init(struct i2c_adapter *_adap)
  */
 static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 {
-	u32  k1 = 0;
-	u32  k2 = 0;
-	u8   dbnct = 0;
-	u32  sclfrq = 0;
-	u8   hldt = 7;
+	struct  smb_timing_t *smb_timing;
+	u8   scl_table_cnt = 0, table_size = 0;
 	u8   fast_mode = 0;
-	u32  src_clk_khz;
-	u32  bus_freq_khz;
 
-	src_clk_khz = bus->apb_clk / 1000;
-	bus_freq_khz = bus_freq_hz / 1000;
 	bus->bus_freq = bus_freq_hz;
 
-	/* 100KHz and below: */
-	if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
-		sclfrq = src_clk_khz / (bus_freq_khz * 4);
-
-		if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
-			return -EDOM;
-
-		if (src_clk_khz >= 40000)
-			hldt = 17;
-		else if (src_clk_khz >= 12500)
-			hldt = 15;
-		else
-			hldt = 7;
-	}
-
-	/* 400KHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
-		sclfrq = 0;
+	switch (bus_freq_hz) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		smb_timing = smb_timing_100khz;
+		table_size = ARRAY_SIZE(smb_timing_100khz);
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		smb_timing = smb_timing_400khz;
+		table_size = ARRAY_SIZE(smb_timing_400khz);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		if (src_clk_khz < 7500)
-			/* 400KHZ cannot be supported for core clock < 7.5MHz */
-			return -EDOM;
-
-		else if (src_clk_khz >= 50000) {
-			k1 = 80;
-			k2 = 48;
-			hldt = 12;
-			dbnct = 7;
-		}
-
-		/* Master or Slave with frequency > 25MHz */
-		else if (src_clk_khz > 25000) {
-			hldt = clk_coef(src_clk_khz, 300) + 7;
-			k1 = clk_coef(src_clk_khz, 1600);
-			k2 = clk_coef(src_clk_khz, 900);
-		}
-	}
-
-	/* 1MHz: */
-	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
-		sclfrq = 0;
+		break;
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		smb_timing = smb_timing_1000khz;
+		table_size = ARRAY_SIZE(smb_timing_1000khz);
 		fast_mode = I2CCTL3_400K_MODE;
-
-		/* 1MHZ cannot be supported for core clock < 24 MHz */
-		if (src_clk_khz < 24000)
-			return -EDOM;
-
-		k1 = clk_coef(src_clk_khz, 620);
-		k2 = clk_coef(src_clk_khz, 380);
-
-		/* Core clk > 40 MHz */
-		if (src_clk_khz > 40000) {
-			/*
-			 * Set HLDT:
-			 * SDA hold time:  (HLDT-7) * T(CLK) >= 120
-			 * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
-			 */
-			hldt = clk_coef(src_clk_khz, 120) + 7;
-		} else {
-			hldt = 7;
-			dbnct = 2;
-		}
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	/* Frequency larger than 1 MHz is not supported */
-	else
-		return -EINVAL;
+	for (scl_table_cnt = 0; scl_table_cnt < table_size; scl_table_cnt++)
+		if (bus->apb_clk >= smb_timing[scl_table_cnt].core_clk)
+			break;
 
-	if (bus_freq_hz >= I2C_MAX_FAST_MODE_FREQ) {
-		k1 = round_up(k1, 2);
-		k2 = round_up(k2 + 1, 2);
-		if (k1 < SCLFRQ_MIN || k1 > SCLFRQ_MAX ||
-		    k2 < SCLFRQ_MIN || k2 > SCLFRQ_MAX)
-			return -EDOM;
-	}
+	if (scl_table_cnt == table_size)
+		return -EINVAL;
 
 	/* write sclfrq value. bits [6:0] are in I2CCTL2 reg */
-	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),
+	iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, smb_timing[scl_table_cnt].sclfrq & 0x7F),
 		 bus->reg + NPCM_I2CCTL2);
 
 	/* bits [8:7] are in I2CCTL3 reg */
-	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
+	iowrite8(FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7) & 0x3) |
+		 fast_mode,
 		 bus->reg + NPCM_I2CCTL3);
 
 	/* Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5 */
@@ -1912,13 +2114,13 @@ static int npcm_i2c_init_clk(struct npcm_i2c *bus, u32 bus_freq_hz)
 		 * k1 = 2 * SCLLT7-0 -> Low Time  = k1 / 2
 		 * k2 = 2 * SCLLT7-0 -> High Time = k2 / 2
 		 */
-		iowrite8(k1 / 2, bus->reg + NPCM_I2CSCLLT);
-		iowrite8(k2 / 2, bus->reg + NPCM_I2CSCLHT);
+		iowrite8(smb_timing[scl_table_cnt].scllt, bus->reg + NPCM_I2CSCLLT);
+		iowrite8(smb_timing[scl_table_cnt].sclht, bus->reg + NPCM_I2CSCLHT);
 
-		iowrite8(dbnct, bus->reg + NPCM_I2CCTL5);
+		iowrite8(smb_timing[scl_table_cnt].dbcnt, bus->reg + NPCM_I2CCTL5);
 	}
 
-	iowrite8(hldt, bus->reg + NPCM_I2CCTL4);
+	iowrite8(smb_timing[scl_table_cnt].hldt, bus->reg + NPCM_I2CCTL4);
 
 	/* Return to Bank 1, and stay there by default: */
 	npcm_i2c_select_bank(bus, I2C_BANK_1);
-- 
2.34.1

[PATCH v7 4/4] i2c: npcm: Enable slave in eob interrupt

[Tyrone Ting]

From: Charles Boyer <Charles.Boyer@fii-usa.com>

Nuvoton slave enable was in user space API call master_xfer, so it is
subject to delays from the OS scheduler. If the BMC is not enabled for
slave mode in time for master to send response, then it will NAK the
address match. Then the PLDM request timeout occurs.

If the slave enable is moved to the EOB interrupt service routine, then
the BMC can be ready in slave mode by the time it needs to receive a
response.

Signed-off-by: Charles Boyer <Charles.Boyer@fii-usa.com>
Signed-off-by: Vivekanand Veeracholan <vveerach@google.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 7c13f9f6014a..1551f9755ce4 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -1925,6 +1925,12 @@ static int npcm_i2c_int_master_handler(struct npcm_i2c *bus)
 	    (FIELD_GET(NPCM_I2CCST3_EO_BUSY,
 		       ioread8(bus->reg + NPCM_I2CCST3)))) {
 		npcm_i2c_irq_handle_eob(bus);
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+		/* reenable slave if it was enabled */
+		if (bus->slave)
+			iowrite8(bus->slave->addr | NPCM_I2CADDR_SAEN,
+				 bus->reg + NPCM_I2CADDR1);
+#endif
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Paul Menzel]

Dear Tyrone,


Thank you for your patch.

Am 21.10.24 um 08:27 schrieb Tyrone Ting:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> Store the client address earlier since it might get called in
> the i2c_recover_bus() logic flow at the early stage of
> npcm_i2c_master_xfer().

Thank you for the description. For the summary/title it’d be great, if 
you were more specific. For example:

i2c: npcm: Assign client address earlier for `i2c_recover_bus()`

It’d be great if you noted the commit, your patch fixes, so it’s clear 
since when the problem has been present.

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> Reviewed-by: Tali Perry <tali.perry1@gmail.com>
> ---
>   drivers/i2c/busses/i2c-npcm7xx.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index c96a25d37c14..a9a9b21f1f0b 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -2155,6 +2155,16 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   
>   	} while (time_is_after_jiffies(time_left) && bus_busy);
>   
> +	/*
> +	 * Previously, the 7-bit address was stored and being converted to
> +	 * the address of event in the following call to npcm_i2c_master_start_xmit().
> +	 *
> +	 * Since there are cases that the i2c_recover_bus() gets called at the
> +	 * early stage of npcm_i2c_master_xfer(), the address of event is stored
> +	 * and then used in the i2c_recover_bus().
> +	 */
> +	bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> +
>   	/*
>   	 * Check the BER (bus error) state, when ber_state is true, it means that the module
>   	 * detects the bus error which is caused by some factor like that the electricity
> @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   	 * bus is busy.
>   	 */
>   	if (bus_busy || bus->ber_state) {
> +		/*
> +		 * Since the transfer might be a read operation, remove the I2C_M_RD flag
> +		 * from the bus->dest_addr for the i2c_recover_bus() call later.
> +		 *
> +		 * The i2c_recover_bus() uses the address in a write direction to recover
> +		 * the i2c bus if some error condition occurs.
> +		 */
> +		bus->dest_addr &= ~I2C_M_RD;
> +
>   		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
>   		npcm_i2c_reset(bus);
>   		i2c_recover_bus(adap);
> @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   	}
>   
>   	npcm_i2c_init_params(bus);
> -	bus->dest_addr = slave_addr;
>   	bus->msgs = msgs;
>   	bus->msgs_num = num;
>   	bus->cmd_err = 0;


Kind regards,

Paul

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Paul:

Thank you for your comment.

It'll be addressed in the next patch set.

Paul Menzel <pmenzel@molgen.mpg.de> 於 2024年10月21日 週一 下午3:01寫道：
>
> Dear Tyrone,
>
>
> Thank you for your patch.
>
> Am 21.10.24 um 08:27 schrieb Tyrone Ting:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Store the client address earlier since it might get called in
> > the i2c_recover_bus() logic flow at the early stage of
> > npcm_i2c_master_xfer().
>
> Thank you for the description. For the summary/title it’d be great, if
> you were more specific. For example:
>
> i2c: npcm: Assign client address earlier for `i2c_recover_bus()`
>
> It’d be great if you noted the commit, your patch fixes, so it’s clear
> since when the problem has been present.
>
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > Reviewed-by: Tali Perry <tali.perry1@gmail.com>
> > ---
> >   drivers/i2c/busses/i2c-npcm7xx.c | 20 +++++++++++++++++++-
> >   1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index c96a25d37c14..a9a9b21f1f0b 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -2155,6 +2155,16 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >
> >       } while (time_is_after_jiffies(time_left) && bus_busy);
> >
> > +     /*
> > +      * Previously, the 7-bit address was stored and being converted to
> > +      * the address of event in the following call to npcm_i2c_master_start_xmit().
> > +      *
> > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > +      * early stage of npcm_i2c_master_xfer(), the address of event is stored
> > +      * and then used in the i2c_recover_bus().
> > +      */
> > +     bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> > +
> >       /*
> >        * Check the BER (bus error) state, when ber_state is true, it means that the module
> >        * detects the bus error which is caused by some factor like that the electricity
> > @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >        * bus is busy.
> >        */
> >       if (bus_busy || bus->ber_state) {
> > +             /*
> > +              * Since the transfer might be a read operation, remove the I2C_M_RD flag
> > +              * from the bus->dest_addr for the i2c_recover_bus() call later.
> > +              *
> > +              * The i2c_recover_bus() uses the address in a write direction to recover
> > +              * the i2c bus if some error condition occurs.
> > +              */
> > +             bus->dest_addr &= ~I2C_M_RD;
> > +
> >               iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> >               npcm_i2c_reset(bus);
> >               i2c_recover_bus(adap);
> > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >       }
> >
> >       npcm_i2c_init_params(bus);
> > -     bus->dest_addr = slave_addr;
> >       bus->msgs = msgs;
> >       bus->msgs_num = num;
> >       bus->cmd_err = 0;
>
>
> Kind regards,
>
> Paul

Have a nice day.

Regards,
Tyrone

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Andi Shyti]

Hi Tyrone,

On Tue, Oct 22, 2024 at 04:08:46PM +0800, Tyrone Ting wrote:
> Hi Paul:
> 
> Thank you for your comment.
> 
> It'll be addressed in the next patch set.

No need to resend, I can take care of it.

Andi

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Andi Shyti]

Hi Tyrone,

...

> +	/*
> +	 * Previously, the 7-bit address was stored and being converted to
> +	 * the address of event in the following call to npcm_i2c_master_start_xmit().

Do we care how it was done previously? I think this is not a
useful information as the code readers will se the code the way
it is now, not the way it was done "previously".

(there is a related comment at the end)

> +	 * Since there are cases that the i2c_recover_bus() gets called at the
> +	 * early stage of npcm_i2c_master_xfer(), the address of event is stored
> +	 * and then used in the i2c_recover_bus().

I could rephrase this sentence to something like:

/*
 * Store the address early in a global position to ensure it is
 * accessible for a potential call to i2c_recover_bus().
 */

> +	 */
> +	bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> +
>  	/*
>  	 * Check the BER (bus error) state, when ber_state is true, it means that the module
>  	 * detects the bus error which is caused by some factor like that the electricity
> @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	 * bus is busy.
>  	 */
>  	if (bus_busy || bus->ber_state) {
> +		/*
> +		 * Since the transfer might be a read operation, remove the I2C_M_RD flag
> +		 * from the bus->dest_addr for the i2c_recover_bus() call later.
> +		 *
> +		 * The i2c_recover_bus() uses the address in a write direction to recover
> +		 * the i2c bus if some error condition occurs.
> +		 */
> +		bus->dest_addr &= ~I2C_M_RD;
> +
>  		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
>  		npcm_i2c_reset(bus);
>  		i2c_recover_bus(adap);
> @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	}
>  
>  	npcm_i2c_init_params(bus);
> -	bus->dest_addr = slave_addr;

We can now get rid of slave_addr. It's just used in
npcm_i2c_master_start_xmit(). Right?

Andi

>  	bus->msgs = msgs;
>  	bus->msgs_num = num;
>  	bus->cmd_err = 0;
> -- 
> 2.34.1
> 

Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Andi Shyti]

Hi Tyrone,

...

> -	/* 100KHz and below: */
> -	if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> -		sclfrq = src_clk_khz / (bus_freq_khz * 4);
> -
> -		if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> -			return -EDOM;
> -
> -		if (src_clk_khz >= 40000)
> -			hldt = 17;
> -		else if (src_clk_khz >= 12500)
> -			hldt = 15;
> -		else
> -			hldt = 7;
> -	}
> -
> -	/* 400KHz: */
> -	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> -		sclfrq = 0;
> +	switch (bus_freq_hz) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
> +		smb_timing = smb_timing_100khz;
> +		table_size = ARRAY_SIZE(smb_timing_100khz);
> +		break;
> +	case I2C_MAX_FAST_MODE_FREQ:
> +		smb_timing = smb_timing_400khz;
> +		table_size = ARRAY_SIZE(smb_timing_400khz);
>  		fast_mode = I2CCTL3_400K_MODE;
> -
> -		if (src_clk_khz < 7500)
> -			/* 400KHZ cannot be supported for core clock < 7.5MHz */
> -			return -EDOM;
> -
> -		else if (src_clk_khz >= 50000) {
> -			k1 = 80;
> -			k2 = 48;
> -			hldt = 12;
> -			dbnct = 7;
> -		}
> -
> -		/* Master or Slave with frequency > 25MHz */
> -		else if (src_clk_khz > 25000) {
> -			hldt = clk_coef(src_clk_khz, 300) + 7;
> -			k1 = clk_coef(src_clk_khz, 1600);
> -			k2 = clk_coef(src_clk_khz, 900);
> -		}
> -	}
> -
> -	/* 1MHz: */
> -	else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> -		sclfrq = 0;
> +		break;
> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> +		smb_timing = smb_timing_1000khz;
> +		table_size = ARRAY_SIZE(smb_timing_1000khz);
>  		fast_mode = I2CCTL3_400K_MODE;
> -
> -		/* 1MHZ cannot be supported for core clock < 24 MHz */
> -		if (src_clk_khz < 24000)
> -			return -EDOM;
> -
> -		k1 = clk_coef(src_clk_khz, 620);
> -		k2 = clk_coef(src_clk_khz, 380);
> -
> -		/* Core clk > 40 MHz */
> -		if (src_clk_khz > 40000) {
> -			/*
> -			 * Set HLDT:
> -			 * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> -			 * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> -			 */
> -			hldt = clk_coef(src_clk_khz, 120) + 7;
> -		} else {
> -			hldt = 7;
> -			dbnct = 2;
> -		}
> +		break;
> +	default:
> +		return -EINVAL;

There is here a slight change of behaiour which is not mentioned
in the commit log. Before the user could set a bus_freq_hz which
had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
precisely that.

Do we want to check what the user has set in the DTS?

(Or am I missing something?)

Thanks,
Andi

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

HI Andi:

Thank you for your help on this.

Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午5:28寫道：
>
> Hi Tyrone,
>
> On Tue, Oct 22, 2024 at 04:08:46PM +0800, Tyrone Ting wrote:
> > Hi Paul:
> >
> > Thank you for your comment.
> >
> > It'll be addressed in the next patch set.
>
> No need to resend, I can take care of it.
>
> Andi

Have a nice day.

Regards,
Tyrone

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andi:

Thank you for your comments.

Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午6:04寫道：
>
> Hi Tyrone,
>
> ...
>
> > +     /*
> > +      * Previously, the 7-bit address was stored and being converted to
> > +      * the address of event in the following call to npcm_i2c_master_start_xmit().
>
> Do we care how it was done previously? I think this is not a
> useful information as the code readers will se the code the way
> it is now, not the way it was done "previously".
>

Yes, it's not a useful information anymore.

> (there is a related comment at the end)
>
> > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > +      * early stage of npcm_i2c_master_xfer(), the address of event is stored
> > +      * and then used in the i2c_recover_bus().
>
> I could rephrase this sentence to something like:
>
> /*
>  * Store the address early in a global position to ensure it is
>  * accessible for a potential call to i2c_recover_bus().
>  */

Understood. Thank you for your help on this.

>
> > +      */
> > +     bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> > +
> >       /*
> >        * Check the BER (bus error) state, when ber_state is true, it means that the module
> >        * detects the bus error which is caused by some factor like that the electricity
> > @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >        * bus is busy.
> >        */
> >       if (bus_busy || bus->ber_state) {
> > +             /*
> > +              * Since the transfer might be a read operation, remove the I2C_M_RD flag
> > +              * from the bus->dest_addr for the i2c_recover_bus() call later.
> > +              *
> > +              * The i2c_recover_bus() uses the address in a write direction to recover
> > +              * the i2c bus if some error condition occurs.
> > +              */
> > +             bus->dest_addr &= ~I2C_M_RD;
> > +
> >               iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> >               npcm_i2c_reset(bus);
> >               i2c_recover_bus(adap);
> > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >       }
> >
> >       npcm_i2c_init_params(bus);
> > -     bus->dest_addr = slave_addr;
>
> We can now get rid of slave_addr. It's just used in
> npcm_i2c_master_start_xmit(). Right?

Yes, slave_addr is just used as the link
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
suggests with this patch.

>
> Andi
>
> >       bus->msgs = msgs;
> >       bus->msgs_num = num;
> >       bus->cmd_err = 0;
> > --
> > 2.34.1
> >

Thank you.

Regards,
Tyrone

Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Hi Andi:

Thank you for your comments.

Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午6:20寫道：
>
> Hi Tyrone,
>
> ...
>
> > -     /* 100KHz and below: */
> > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > -
> > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > -                     return -EDOM;
> > -
> > -             if (src_clk_khz >= 40000)
> > -                     hldt = 17;
> > -             else if (src_clk_khz >= 12500)
> > -                     hldt = 15;
> > -             else
> > -                     hldt = 7;
> > -     }
> > -
> > -     /* 400KHz: */
> > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > -             sclfrq = 0;
> > +     switch (bus_freq_hz) {
> > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > +             smb_timing = smb_timing_100khz;
> > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > +             break;
> > +     case I2C_MAX_FAST_MODE_FREQ:
> > +             smb_timing = smb_timing_400khz;
> > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> >               fast_mode = I2CCTL3_400K_MODE;
> > -
> > -             if (src_clk_khz < 7500)
> > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > -                     return -EDOM;
> > -
> > -             else if (src_clk_khz >= 50000) {
> > -                     k1 = 80;
> > -                     k2 = 48;
> > -                     hldt = 12;
> > -                     dbnct = 7;
> > -             }
> > -
> > -             /* Master or Slave with frequency > 25MHz */
> > -             else if (src_clk_khz > 25000) {
> > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > -                     k1 = clk_coef(src_clk_khz, 1600);
> > -                     k2 = clk_coef(src_clk_khz, 900);
> > -             }
> > -     }
> > -
> > -     /* 1MHz: */
> > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > -             sclfrq = 0;
> > +             break;
> > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > +             smb_timing = smb_timing_1000khz;
> > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> >               fast_mode = I2CCTL3_400K_MODE;
> > -
> > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > -             if (src_clk_khz < 24000)
> > -                     return -EDOM;
> > -
> > -             k1 = clk_coef(src_clk_khz, 620);
> > -             k2 = clk_coef(src_clk_khz, 380);
> > -
> > -             /* Core clk > 40 MHz */
> > -             if (src_clk_khz > 40000) {
> > -                     /*
> > -                      * Set HLDT:
> > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > -                      */
> > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > -             } else {
> > -                     hldt = 7;
> > -                     dbnct = 2;
> > -             }
> > +             break;
> > +     default:
> > +             return -EINVAL;
>
> There is here a slight change of behaiour which is not mentioned
> in the commit log. Before the user could set a bus_freq_hz which
> had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> precisely that.
>
> Do we want to check what the user has set in the DTS?

The driver checks the bus frequency the user sets in the DTS.

Please refer to the links:
1. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1995
2. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2002

>
> (Or am I missing something?)
>
> Thanks,
> Andi

Thank you.

Regards,
Tyrone

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andi:

Sorry to bother you.

May I have your comments about my feedback on these patches?

It'll be great to know if I need to prepare the next patch set for reviewing.

Tyrone Ting <warp5tw@gmail.com> 於 2024年10月25日 週五 上午9:43寫道：
>
> Hi Andi:
>
> Thank you for your comments.
>
> Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午6:04寫道：
> >
> > Hi Tyrone,
> >
> > ...
> >
> > > +     /*
> > > +      * Previously, the 7-bit address was stored and being converted to
> > > +      * the address of event in the following call to npcm_i2c_master_start_xmit().
> >
> > Do we care how it was done previously? I think this is not a
> > useful information as the code readers will se the code the way
> > it is now, not the way it was done "previously".
> >
>
> Yes, it's not a useful information anymore.
>
> > (there is a related comment at the end)
> >
> > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > +      * early stage of npcm_i2c_master_xfer(), the address of event is stored
> > > +      * and then used in the i2c_recover_bus().
> >
> > I could rephrase this sentence to something like:
> >
> > /*
> >  * Store the address early in a global position to ensure it is
> >  * accessible for a potential call to i2c_recover_bus().
> >  */
>
> Understood. Thank you for your help on this.
>
> >
> > > +      */
> > > +     bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> > > +
> > >       /*
> > >        * Check the BER (bus error) state, when ber_state is true, it means that the module
> > >        * detects the bus error which is caused by some factor like that the electricity
> > > @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >        * bus is busy.
> > >        */
> > >       if (bus_busy || bus->ber_state) {
> > > +             /*
> > > +              * Since the transfer might be a read operation, remove the I2C_M_RD flag
> > > +              * from the bus->dest_addr for the i2c_recover_bus() call later.
> > > +              *
> > > +              * The i2c_recover_bus() uses the address in a write direction to recover
> > > +              * the i2c bus if some error condition occurs.
> > > +              */
> > > +             bus->dest_addr &= ~I2C_M_RD;
> > > +
> > >               iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> > >               npcm_i2c_reset(bus);
> > >               i2c_recover_bus(adap);
> > > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >       }
> > >
> > >       npcm_i2c_init_params(bus);
> > > -     bus->dest_addr = slave_addr;
> >
> > We can now get rid of slave_addr. It's just used in
> > npcm_i2c_master_start_xmit(). Right?
>
> Yes, slave_addr is just used as the link
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
> suggests with this patch.
>
> >
> > Andi
> >
> > >       bus->msgs = msgs;
> > >       bus->msgs_num = num;
> > >       bus->cmd_err = 0;
> > > --
> > > 2.34.1
> > >
>
> Thank you.
>
> Regards,
> Tyrone

Have a nice day.

Thank you.

Regards,
Tyrone

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andi:

May I have your comments about my feedback on these patches?

Tyrone Ting <warp5tw@gmail.com> 於 2024年10月29日 週二 下午4:50寫道：
>
> Hi Andi:
>
> Sorry to bother you.
>
> May I have your comments about my feedback on these patches?
>
> It'll be great to know if I need to prepare the next patch set for reviewing.
>
> Tyrone Ting <warp5tw@gmail.com> 於 2024年10月25日 週五 上午9:43寫道：
> >
> > Hi Andi:
> >
> > Thank you for your comments.
> >
> > Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午6:04寫道：
> > >
> > > Hi Tyrone,
> > >
> > > ...
> > >
> > > > +     /*
> > > > +      * Previously, the 7-bit address was stored and being converted to
> > > > +      * the address of event in the following call to npcm_i2c_master_start_xmit().
> > >
> > > Do we care how it was done previously? I think this is not a
> > > useful information as the code readers will se the code the way
> > > it is now, not the way it was done "previously".
> > >
> >
> > Yes, it's not a useful information anymore.
> >
> > > (there is a related comment at the end)
> > >
> > > > +      * Since there are cases that the i2c_recover_bus() gets called at the
> > > > +      * early stage of npcm_i2c_master_xfer(), the address of event is stored
> > > > +      * and then used in the i2c_recover_bus().
> > >
> > > I could rephrase this sentence to something like:
> > >
> > > /*
> > >  * Store the address early in a global position to ensure it is
> > >  * accessible for a potential call to i2c_recover_bus().
> > >  */
> >
> > Understood. Thank you for your help on this.
> >
> > >
> > > > +      */
> > > > +     bus->dest_addr = i2c_8bit_addr_from_msg(msg0);
> > > > +
> > > >       /*
> > > >        * Check the BER (bus error) state, when ber_state is true, it means that the module
> > > >        * detects the bus error which is caused by some factor like that the electricity
> > > > @@ -2165,6 +2175,15 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >        * bus is busy.
> > > >        */
> > > >       if (bus_busy || bus->ber_state) {
> > > > +             /*
> > > > +              * Since the transfer might be a read operation, remove the I2C_M_RD flag
> > > > +              * from the bus->dest_addr for the i2c_recover_bus() call later.
> > > > +              *
> > > > +              * The i2c_recover_bus() uses the address in a write direction to recover
> > > > +              * the i2c bus if some error condition occurs.
> > > > +              */
> > > > +             bus->dest_addr &= ~I2C_M_RD;
> > > > +
> > > >               iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
> > > >               npcm_i2c_reset(bus);
> > > >               i2c_recover_bus(adap);
> > > > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >       }
> > > >
> > > >       npcm_i2c_init_params(bus);
> > > > -     bus->dest_addr = slave_addr;
> > >
> > > We can now get rid of slave_addr. It's just used in
> > > npcm_i2c_master_start_xmit(). Right?
> >
> > Yes, slave_addr is just used as the link
> > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
> > suggests with this patch.
> >
> > >
> > > Andi
> > >
> > > >       bus->msgs = msgs;
> > > >       bus->msgs_num = num;
> > > >       bus->cmd_err = 0;
> > > > --
> > > > 2.34.1
> > > >
> >
> > Thank you.
> >
> > Regards,
> > Tyrone
>
> Have a nice day.
>
> Thank you.
>
> Regards,
> Tyrone

Thank you.

Regards,
Tyrone

Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Tyrone Ting]

Hi Andi:

May I have your comments about my feedback on these patches?

Tyrone Ting <warp5tw@gmail.com> 於 2024年10月25日 週五 上午9:46寫道：
>
> Hi Andi:
>
> Thank you for your comments.
>
> Andi Shyti <andi.shyti@kernel.org> 於 2024年10月24日 週四 下午6:20寫道：
> >
> > Hi Tyrone,
> >
> > ...
> >
> > > -     /* 100KHz and below: */
> > > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > > -
> > > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > > -                     return -EDOM;
> > > -
> > > -             if (src_clk_khz >= 40000)
> > > -                     hldt = 17;
> > > -             else if (src_clk_khz >= 12500)
> > > -                     hldt = 15;
> > > -             else
> > > -                     hldt = 7;
> > > -     }
> > > -
> > > -     /* 400KHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > > -             sclfrq = 0;
> > > +     switch (bus_freq_hz) {
> > > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > > +             smb_timing = smb_timing_100khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_FREQ:
> > > +             smb_timing = smb_timing_400khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             if (src_clk_khz < 7500)
> > > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > > -                     return -EDOM;
> > > -
> > > -             else if (src_clk_khz >= 50000) {
> > > -                     k1 = 80;
> > > -                     k2 = 48;
> > > -                     hldt = 12;
> > > -                     dbnct = 7;
> > > -             }
> > > -
> > > -             /* Master or Slave with frequency > 25MHz */
> > > -             else if (src_clk_khz > 25000) {
> > > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > > -                     k1 = clk_coef(src_clk_khz, 1600);
> > > -                     k2 = clk_coef(src_clk_khz, 900);
> > > -             }
> > > -     }
> > > -
> > > -     /* 1MHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > > -             sclfrq = 0;
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > > +             smb_timing = smb_timing_1000khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > > -             if (src_clk_khz < 24000)
> > > -                     return -EDOM;
> > > -
> > > -             k1 = clk_coef(src_clk_khz, 620);
> > > -             k2 = clk_coef(src_clk_khz, 380);
> > > -
> > > -             /* Core clk > 40 MHz */
> > > -             if (src_clk_khz > 40000) {
> > > -                     /*
> > > -                      * Set HLDT:
> > > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > > -                      */
> > > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > > -             } else {
> > > -                     hldt = 7;
> > > -                     dbnct = 2;
> > > -             }
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> >
> > There is here a slight change of behaiour which is not mentioned
> > in the commit log. Before the user could set a bus_freq_hz which
> > had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> > precisely that.
> >
> > Do we want to check what the user has set in the DTS?
>
> The driver checks the bus frequency the user sets in the DTS.
>
> Please refer to the links:
> 1. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1995
> 2. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2002
>
> >
> > (Or am I missing something?)
> >
> > Thanks,
> > Andi
>
> Thank you.
>
> Regards,
> Tyrone

Thank you.

Regards,
Tyrone

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Andi Shyti]

Hi Tyrone,

Sorry for the late reply!

...

> > > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > >       }
> > >
> > >       npcm_i2c_init_params(bus);
> > > -     bus->dest_addr = slave_addr;
> >
> > We can now get rid of slave_addr. It's just used in
> > npcm_i2c_master_start_xmit(). Right?
> 
> Yes, slave_addr is just used as the link
> https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
> suggests with this patch.

What I mean is that slave_addr now is completely unused. We
declare it, we initialize it to msg0->addr and it stays like
this.

What I'm suggesting is to remove it completely.

Andi

Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Andi Shyti]

Hi Tyrone,

...

> > > -     /* 100KHz and below: */
> > > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > > -
> > > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > > -                     return -EDOM;
> > > -
> > > -             if (src_clk_khz >= 40000)
> > > -                     hldt = 17;
> > > -             else if (src_clk_khz >= 12500)
> > > -                     hldt = 15;
> > > -             else
> > > -                     hldt = 7;
> > > -     }
> > > -
> > > -     /* 400KHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > > -             sclfrq = 0;
> > > +     switch (bus_freq_hz) {
> > > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > > +             smb_timing = smb_timing_100khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_FREQ:
> > > +             smb_timing = smb_timing_400khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             if (src_clk_khz < 7500)
> > > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > > -                     return -EDOM;
> > > -
> > > -             else if (src_clk_khz >= 50000) {
> > > -                     k1 = 80;
> > > -                     k2 = 48;
> > > -                     hldt = 12;
> > > -                     dbnct = 7;
> > > -             }
> > > -
> > > -             /* Master or Slave with frequency > 25MHz */
> > > -             else if (src_clk_khz > 25000) {
> > > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > > -                     k1 = clk_coef(src_clk_khz, 1600);
> > > -                     k2 = clk_coef(src_clk_khz, 900);
> > > -             }
> > > -     }
> > > -
> > > -     /* 1MHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > > -             sclfrq = 0;
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > > +             smb_timing = smb_timing_1000khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > > -             if (src_clk_khz < 24000)
> > > -                     return -EDOM;
> > > -
> > > -             k1 = clk_coef(src_clk_khz, 620);
> > > -             k2 = clk_coef(src_clk_khz, 380);
> > > -
> > > -             /* Core clk > 40 MHz */
> > > -             if (src_clk_khz > 40000) {
> > > -                     /*
> > > -                      * Set HLDT:
> > > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > > -                      */
> > > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > > -             } else {
> > > -                     hldt = 7;
> > > -                     dbnct = 2;
> > > -             }
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> >
> > There is here a slight change of behaiour which is not mentioned
> > in the commit log. Before the user could set a bus_freq_hz which
> > had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> > precisely that.
> >
> > Do we want to check what the user has set in the DTS?
> 
> The driver checks the bus frequency the user sets in the DTS.

yes, but before it was checking the value within a range, while
now it's checking the exact value.

The difference is that now if you don't set the exact value you
get EINVAL, not before.

Andi

Re: [PATCH v7 2/4] i2c: npcm: Modify the client address assignment

[Tyrone Ting]

Hi Andi:

Thank you for your reply.

Andi Shyti <andi.shyti@kernel.org> 於 2024年11月20日 週三 上午6:22寫道：
>
> Hi Tyrone,
>
> Sorry for the late reply!
>
> ...
>
> > > > @@ -2172,7 +2191,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > > >       }
> > > >
> > > >       npcm_i2c_init_params(bus);
> > > > -     bus->dest_addr = slave_addr;
> > >
> > > We can now get rid of slave_addr. It's just used in
> > > npcm_i2c_master_start_xmit(). Right?
> >
> > Yes, slave_addr is just used as the link
> > https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
> > suggests with this patch.
>
> What I mean is that slave_addr now is completely unused. We
> declare it, we initialize it to msg0->addr and it stays like
> this.
>
> What I'm suggesting is to remove it completely.
>

Sorry that I might not describe it clearly in previous discussion
thread. slave_addr is passed to npcm_i2c_master_start_xmit() as
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2182
suggests. If slave_addr is removed, may I use "(bus->dest_addr &=
~I2C_M_RD)" to replace the slave_addr parameter to
npcm_i2c_master_start_xmit()?

> Andi

Thank you.

Regards,
Tyrone

Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

[Tali Perry]

Hi Andi,

>
> > > > -     /* 100KHz and below: */
> > > > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > > > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > > > -
> > > > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > > > -                     return -EDOM;
> > > > -
> > > > -             if (src_clk_khz >= 40000)
> > > > -                     hldt = 17;
> > > > -             else if (src_clk_khz >= 12500)
> > > > -                     hldt = 15;
> > > > -             else
> > > > -                     hldt = 7;
> > > > -     }
> > > > -
> > > > -     /* 400KHz: */
> > > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > > > -             sclfrq = 0;
> > > > +     switch (bus_freq_hz) {
> > > > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > > > +             smb_timing = smb_timing_100khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > > > +             break;
> > > > +     case I2C_MAX_FAST_MODE_FREQ:
> > > > +             smb_timing = smb_timing_400khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> > > >               fast_mode = I2CCTL3_400K_MODE;
> > > > -
> > > > -             if (src_clk_khz < 7500)
> > > > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > > > -                     return -EDOM;
> > > > -
> > > > -             else if (src_clk_khz >= 50000) {
> > > > -                     k1 = 80;
> > > > -                     k2 = 48;
> > > > -                     hldt = 12;
> > > > -                     dbnct = 7;
> > > > -             }
> > > > -
> > > > -             /* Master or Slave with frequency > 25MHz */
> > > > -             else if (src_clk_khz > 25000) {
> > > > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > > > -                     k1 = clk_coef(src_clk_khz, 1600);
> > > > -                     k2 = clk_coef(src_clk_khz, 900);
> > > > -             }
> > > > -     }
> > > > -
> > > > -     /* 1MHz: */
> > > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > > > -             sclfrq = 0;
> > > > +             break;
> > > > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > > > +             smb_timing = smb_timing_1000khz;
> > > > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> > > >               fast_mode = I2CCTL3_400K_MODE;
> > > > -
> > > > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > > > -             if (src_clk_khz < 24000)
> > > > -                     return -EDOM;
> > > > -
> > > > -             k1 = clk_coef(src_clk_khz, 620);
> > > > -             k2 = clk_coef(src_clk_khz, 380);
> > > > -
> > > > -             /* Core clk > 40 MHz */
> > > > -             if (src_clk_khz > 40000) {
> > > > -                     /*
> > > > -                      * Set HLDT:
> > > > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > > > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > > > -                      */
> > > > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > > > -             } else {
> > > > -                     hldt = 7;
> > > > -                     dbnct = 2;
> > > > -             }
> > > > +             break;
> > > > +     default:
> > > > +             return -EINVAL;
> > >
> > > There is here a slight change of behaiour which is not mentioned
> > > in the commit log. Before the user could set a bus_freq_hz which
> > > had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> > > precisely that.
> > >
> > > Do we want to check what the user has set in the DTS?
> >
> > The driver checks the bus frequency the user sets in the DTS.
>
> yes, but before it was checking the value within a range, while
> now it's checking the exact value.
>
> The difference is that now if you don't set the exact value you
> get EINVAL, not before.
>
> Andi

Previously the driver was rounding numbers down.
The driver has settings for 100, 400, 1000 KHz.
but what happens if the user asks for 200KHz?
Some of the coefficients were calculated according to the equations,
and some were hard-coded values per setting.
We don't want to support this mix.
We prefer the users to ask for numbers that are one of the three
supported values and block unknown input values.

Thanks ,
Tali