[PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Lakshmi Yadlapati]

Reintroduce per-client throttling of transfers for improved compatibility.

Some devices have experienced issues with small command turn-around times when using in-kernel device drivers. While a previous proposal was rejected due to concerns about error-prone open-coding of delays, recent upstream changes for similar problems in I2C devices (e.g., max15301 and ucd90320) and now max31785 make it sensible to reintroduce Andrew's generic solution. This change aims to improve compatibility for affected devices and may help avoid duplicating implementations of handlers for I2C and PMBus calls in driver code.

Reference to Andrew's previous proposal:
https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/

Lakshmi Yadlapati (2):
  i2c: smbus: Allow throttling of transfers to client devices
  hwmon: (pmbus/max31785) Add minimum delay between bus accesses

 drivers/hwmon/pmbus/max31785.c |   8 ++
 drivers/i2c/i2c-core-base.c    |   8 +-
 drivers/i2c/i2c-core-smbus.c   | 143 ++++++++++++++++++++++++++-------
 drivers/i2c/i2c-core.h         |  23 ++++++
 include/linux/i2c.h            |   2 +
 5 files changed, 153 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH v1 1/2] i2c: smbus: Allow throttling of transfers to client devices

[Lakshmi Yadlapati]

Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
---
 drivers/i2c/i2c-core-base.c  |   8 +-
 drivers/i2c/i2c-core-smbus.c | 143 ++++++++++++++++++++++++++++-------
 drivers/i2c/i2c-core.h       |  23 ++++++
 include/linux/i2c.h          |   2 +
 4 files changed, 145 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..c27cb054d462 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -931,13 +931,17 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
 struct i2c_client *
 i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 {
+	struct i2c_client_priv	*priv;
 	struct i2c_client	*client;
 	int			status;
 
-	client = kzalloc(sizeof *client, GFP_KERNEL);
-	if (!client)
+	priv = kzalloc(sizeof *priv, GFP_KERNEL);
+	if (!priv)
 		return ERR_PTR(-ENOMEM);
 
+	mutex_init(&priv->throttle_lock);
+	client = &priv->client;
+
 	client->adapter = adap;
 
 	client->dev.platform_data = info->platform_data;
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index e3b96fc53b5c..bcf1b15d6bd2 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -10,6 +10,7 @@
  * SMBus 2.0 support by Mark Studebaker <mdsxyz123@yahoo.com> and
  * Jean Delvare <jdelvare@suse.de>
  */
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -22,6 +23,9 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/smbus.h>
 
+static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client,
+				   char read_write, u8 command, int protocol,
+				   union i2c_smbus_data *data);
 
 /* The SMBus parts */
 
@@ -104,9 +108,8 @@ s32 i2c_smbus_read_byte(const struct i2c_client *client)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, 0,
-				I2C_SMBUS_BYTE, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, 0,
+					 I2C_SMBUS_BYTE, &data);
 	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte);
@@ -121,8 +124,8 @@ EXPORT_SYMBOL(i2c_smbus_read_byte);
  */
 s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value)
 {
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-	                      I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, value,
+				       I2C_SMBUS_BYTE, NULL);
 }
 EXPORT_SYMBOL(i2c_smbus_write_byte);
 
@@ -139,9 +142,8 @@ s32 i2c_smbus_read_byte_data(const struct i2c_client *client, u8 command)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_BYTE_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_BYTE_DATA, &data);
 	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte_data);
@@ -160,9 +162,8 @@ s32 i2c_smbus_write_byte_data(const struct i2c_client *client, u8 command,
 {
 	union i2c_smbus_data data;
 	data.byte = value;
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_BYTE_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_BYTE_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_byte_data);
 
@@ -179,9 +180,8 @@ s32 i2c_smbus_read_word_data(const struct i2c_client *client, u8 command)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_WORD_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_WORD_DATA, &data);
 	return (status < 0) ? status : data.word;
 }
 EXPORT_SYMBOL(i2c_smbus_read_word_data);
@@ -200,9 +200,8 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
 {
 	union i2c_smbus_data data;
 	data.word = value;
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_WORD_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_WORD_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 
@@ -227,9 +226,8 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_BLOCK_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_BLOCK_DATA, &data);
 	if (status)
 		return status;
 
@@ -257,9 +255,8 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
 	memcpy(&data.block[1], values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_BLOCK_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_BLOCK_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_block_data);
 
@@ -273,9 +270,8 @@ s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
 	if (length > I2C_SMBUS_BLOCK_MAX)
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_I2C_BLOCK_DATA, &data);
 	if (status < 0)
 		return status;
 
@@ -293,9 +289,8 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
 	memcpy(data.block + 1, values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_I2C_BLOCK_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
@@ -550,6 +545,71 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 }
 EXPORT_SYMBOL(i2c_smbus_xfer);
 
+static int i2c_smbus_throttle_enter(const struct i2c_client *client)
+		__acquires(&priv->throttle_lock)
+{
+	struct i2c_client_priv *priv;
+	ktime_t earliest;
+	int rc;
+
+	priv = to_i2c_client_priv(client);
+
+	if (i2c_in_atomic_xfer_mode()) {
+		if (!mutex_trylock(&priv->throttle_lock))
+			return -EAGAIN;
+	} else {
+		rc = mutex_lock_interruptible(&priv->throttle_lock);
+		if (rc)
+			return rc;
+	}
+	earliest = ktime_add_us(priv->last, priv->delay_us);
+
+	if (priv->delay_us && ktime_before(ktime_get(), earliest)) {
+		if (i2c_in_atomic_xfer_mode()) {
+			mutex_unlock(&priv->throttle_lock);
+			return -EAGAIN;
+		}
+
+		usleep_range(priv->delay_us, 2 * priv->delay_us);
+	}
+
+	return 0;
+}
+
+static void i2c_smbus_throttle_exit(const struct i2c_client *client)
+		__releases(&priv->throttle_lock)
+{
+	struct i2c_client_priv *priv;
+
+	priv = to_i2c_client_priv(client);
+
+	if (priv->delay_us)
+		priv->last = ktime_get();
+	mutex_unlock(&priv->throttle_lock);
+}
+
+static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client,
+				   char read_write, u8 command, int protocol,
+				   union i2c_smbus_data *data)
+{
+	s32 res;
+
+	res = i2c_smbus_throttle_enter(client);
+	if (res)
+		return res;
+
+	res = __i2c_lock_bus_helper(client->adapter);
+	if (!res)
+		res = __i2c_smbus_xfer(client->adapter, client->addr,
+				       client->flags, read_write, command,
+				       protocol, data);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	i2c_smbus_throttle_exit(client);
+
+	return res;
+}
+
 s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 		     unsigned short flags, char read_write,
 		     u8 command, int protocol, union i2c_smbus_data *data)
@@ -721,3 +781,28 @@ int i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 	return PTR_ERR_OR_ZERO(i2c_new_smbus_alert_device(adapter, NULL));
 }
 #endif
+
+
+int i2c_smbus_throttle_client(struct i2c_client *client,
+			       unsigned long delay_us)
+{
+	struct i2c_client_priv *priv;
+	int rc;
+
+	priv = to_i2c_client_priv(client);
+
+	if (i2c_in_atomic_xfer_mode()) {
+		if (!mutex_trylock(&priv->throttle_lock))
+			return -EAGAIN;
+	} else {
+		rc = mutex_lock_interruptible(&priv->throttle_lock);
+		if (rc)
+			return rc;
+	}
+	priv->delay_us = delay_us;
+	priv->last = ktime_get();
+	mutex_unlock(&priv->throttle_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_throttle_client);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 1247e6e6e975..4aebcd5a3dc3 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -4,6 +4,29 @@
  */
 
 #include <linux/rwsem.h>
+#include <linux/i2c.h>
+#include <linux/timekeeping.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+
+struct i2c_client_priv {
+	struct i2c_client client;
+
+	/*
+	 * Per-client access throttling, described in terms of microsecond
+	 * delay between the end of the nth transfer and the start of the
+	 * (n+1)th transfer. Used to manage devices which respond poorly to
+	 * rapid back-to-back commands.
+	 *
+	 * Do it in a wrapper struct to preserve const-ness of the i2c_smbus_*
+	 * interfaces.
+	 */
+	struct mutex throttle_lock;
+	unsigned long delay_us;
+	ktime_t last;
+};
+#define to_i2c_client_priv(c) container_of(c, struct i2c_client_priv, client)
+
 
 struct i2c_devinfo {
 	struct list_head	list;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0dae9db27538..b6a317e679ca 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -187,6 +187,8 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
 s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 					      u8 command, u8 length,
 					      u8 *values);
+int i2c_smbus_throttle_client(struct i2c_client *client,
+			      unsigned long delay_us);
 int i2c_get_device_id(const struct i2c_client *client,
 		      struct i2c_device_identity *id);
 const struct i2c_device_id *i2c_client_get_device_id(const struct i2c_client *client);
-- 
2.39.2

Re: [PATCH v1 1/2] i2c: smbus: Allow throttling of transfers to client devices

[Jean Delvare]

On Mon, 2023-10-09 at 16:14 -0500, Lakshmi Yadlapati wrote:
> Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
> ---
>  drivers/i2c/i2c-core-base.c  |   8 +-
>  drivers/i2c/i2c-core-smbus.c | 143 ++++++++++++++++++++++++++++-------
>  drivers/i2c/i2c-core.h       |  23 ++++++
>  include/linux/i2c.h          |   2 +
>  4 files changed, 145 insertions(+), 31 deletions(-)
> (...)

Non-trivial patch with no description -> not even looking, sorry. You
can't possibly propose a change to the core of a subsystem and not
bother explaining why this change is needed or what purpose it serves.

(And yes I know there's some information in patch 0/2, but that's not
going to make it into git, so it will be lost. Commits should be self-
sufficient, not only the implementation, but also the description.)

I would also suggest trimming the To and Cc lists. I can't really see
how linux-media and dri-devel are relevant here for example.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 1/2] i2c: smbus: Allow throttling of transfers to client devices

[Jani Nikula]

On Thu, 12 Oct 2023, Jean Delvare <jdelvare@suse.com> wrote:
> On Mon, 2023-10-09 at 16:14 -0500, Lakshmi Yadlapati wrote:
>> Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
>> ---
>>  drivers/i2c/i2c-core-base.c  |   8 +-
>>  drivers/i2c/i2c-core-smbus.c | 143 ++++++++++++++++++++++++++++-------
>>  drivers/i2c/i2c-core.h       |  23 ++++++
>>  include/linux/i2c.h          |   2 +
>>  4 files changed, 145 insertions(+), 31 deletions(-)
>> (...)
>
> Non-trivial patch with no description -> not even looking, sorry. You
> can't possibly propose a change to the core of a subsystem and not
> bother explaining why this change is needed or what purpose it serves.

We've even managed to write extensive documentation on this!

https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

>
> (And yes I know there's some information in patch 0/2, but that's not
> going to make it into git, so it will be lost. Commits should be self-
> sufficient, not only the implementation, but also the description.)
>
> I would also suggest trimming the To and Cc lists. I can't really see
> how linux-media and dri-devel are relevant here for example.

-- 
Jani Nikula, Intel

[PATCH v1 2/2] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Lakshmi Yadlapati]

The MAX31785 has shown erratic behaviour across multiple system
designs, unexpectedly clock stretching and NAKing transactions.
Inserting a small delay (250us) after register writes makes the
issue go away.

Signed-off-by: Lakshmi Yadlapati <lakshmiy@us.ibm.com>
---
 drivers/hwmon/pmbus/max31785.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index f9aa576495a5..532e1c8f632c 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -5,6 +5,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -24,6 +25,9 @@ enum max31785_regs {
 #define MAX31785_NR_PAGES		23
 #define MAX31785_NR_FAN_PAGES		6
 
+static unsigned long throttle_delay_us = 250;
+module_param(throttle_delay_us, ulong, 0664);
+
 static int max31785_read_byte_data(struct i2c_client *client, int page,
 				   int reg)
 {
@@ -337,6 +341,10 @@ static int max31785_probe(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
 
+	ret = i2c_smbus_throttle_client(client, throttle_delay_us);
+	if (ret)
+		return ret;
+
 	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
-- 
2.39.2

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Lakshmi Yadlapati]

Correct Link to Andrew's previous proposal:
https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/


On 10/9/23, 4:21 PM, "Lakshmi Yadlapati" <lakshmiy@us.ibm.com <mailto:lakshmiy@us.ibm.com>> wrote:


Reintroduce per-client throttling of transfers for improved compatibility.


Some devices have experienced issues with small command turn-around times when using in-kernel device drivers. While a previous proposal was rejected due to concerns about error-prone open-coding of delays, recent upstream changes for similar problems in I2C devices (e.g., max15301 and ucd90320) and now max31785 make it sensible to reintroduce Andrew's generic solution. This change aims to improve compatibility for affected devices and may help avoid duplicating implementations of handlers for I2C and PMBus calls in driver code.


Reference to Andrew's previous proposal:
https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au <mailto:20200914122811.3295678-1-andrew@aj.id.au>/


Lakshmi Yadlapati (2):
i2c: smbus: Allow throttling of transfers to client devices
hwmon: (pmbus/max31785) Add minimum delay between bus accesses


drivers/hwmon/pmbus/max31785.c | 8 ++
drivers/i2c/i2c-core-base.c | 8 +-
drivers/i2c/i2c-core-smbus.c | 143 ++++++++++++++++++++++++++-------
drivers/i2c/i2c-core.h | 23 ++++++
include/linux/i2c.h | 2 +
5 files changed, 153 insertions(+), 31 deletions(-)


-- 
2.39.2

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

Hi,

thanks for this series!

> Reference to Andrew's previous proposal:
> https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/

I do totally agree with Guenter's comment[1], though. This just affects
a few drivers and this patch is way too intrusive for the I2C core. The
later suggested prepare_device() callback[2] sounds better to me. I
still haven't fully understood why this all cannot be handled in the
driver's probe. Could someone give me a small summary about that?

All the best,

   Wolfram


[1] https://lore.kernel.org/all/e7a64983-fe1d-1ba2-b0c3-ae4a791f7a75@roeck-us.net/
[2] https://lore.kernel.org/all/120342ec-f44a-4550-8c54-45b97db41024@www.fastmail.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Guenter Roeck]

On Tue, Oct 10, 2023 at 11:31:56AM +0200, Wolfram Sang wrote:
> Hi,
> 
> thanks for this series!
> 
> > Reference to Andrew's previous proposal:
> > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/
> 
> I do totally agree with Guenter's comment[1], though. This just affects
> a few drivers and this patch is way too intrusive for the I2C core. The
> later suggested prepare_device() callback[2] sounds better to me. I
> still haven't fully understood why this all cannot be handled in the
> driver's probe. Could someone give me a small summary about that?
> 

Lots of PMBus devices have the same problem, we have always handled
it in PMBus drivers by implementing local wait code, and your references
point that out. What other summary are you looking for ?

On a side note, if anyone plans to implement the prepare_device() callback,
please make sure that it covers all requirements. It would be unfortunate
if such a callback was implemented if that would still require per-driver
code (besides the callback).

Thanks,
Guenter

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

Hi Guenter,

> > > Reference to Andrew's previous proposal:
> > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/
> > 
> > I do totally agree with Guenter's comment[1], though. This just affects
> > a few drivers and this patch is way too intrusive for the I2C core. The
> > later suggested prepare_device() callback[2] sounds better to me. I
> > still haven't fully understood why this all cannot be handled in the
> > driver's probe. Could someone give me a small summary about that?
> > 
> 
> Lots of PMBus devices have the same problem, we have always handled
> it in PMBus drivers by implementing local wait code, and your references
> point that out.

I am confused now. Reading your reply:

"I am not sure if an implementation in the i2c core is desirable. It
looks quite invasive to me, and it won't solve the problem for all
devices since it isn't always a simple "wait <n> microseconds between
accesses". For example, some devices may require a wait after a write
but not after a read, or a wait only after certain commands (such as
commands writing to an EEPROM)."

I get the impression you don't prefer to have a generic mechanism in the
I2C core. This I share. Your response now sounds like you do support
that idea now?

> What other summary are you looking for ?

What the actual problem is with these devices. The cover letter only
mentions "issues with small command turn-around times". More details
would be nice. Is it between transfers? Or even between messages within
one transfer? Has it been tried to lower the bus frequency? Stuff like
this.

> On a side note, if anyone plans to implement the prepare_device() callback,
> please make sure that it covers all requirements. It would be unfortunate
> if such a callback was implemented if that would still require per-driver
> code (besides the callback).

Is there a list of that somewhere? Or does it mean going through all the
drivers and see what they currently do?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Guenter Roeck]

On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote:
> Hi Guenter,
> 
> > > > Reference to Andrew's previous proposal:
> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/
> > > 
> > > I do totally agree with Guenter's comment[1], though. This just affects
> > > a few drivers and this patch is way too intrusive for the I2C core. The
> > > later suggested prepare_device() callback[2] sounds better to me. I
> > > still haven't fully understood why this all cannot be handled in the
> > > driver's probe. Could someone give me a small summary about that?
> > > 
> > 
> > Lots of PMBus devices have the same problem, we have always handled
> > it in PMBus drivers by implementing local wait code, and your references
> > point that out.
> 
> I am confused now. Reading your reply:
> 
> "I am not sure if an implementation in the i2c core is desirable. It
> looks quite invasive to me, and it won't solve the problem for all
> devices since it isn't always a simple "wait <n> microseconds between
> accesses". For example, some devices may require a wait after a write
> but not after a read, or a wait only after certain commands (such as
> commands writing to an EEPROM)."
> 
> I get the impression you don't prefer to have a generic mechanism in the
> I2C core. This I share. Your response now sounds like you do support
> that idea now?
> 

I didn't (want to) say that. I am perfectly happy with driver specific
code, and I would personally still very much prefer it. I only wanted to
suggest that _if_ a generic solution is implemented, it should cover all
existing use cases and not just this one. But, really, I'd rather leave
that alone and not risk introducing regressions to existing drivers.

> > What other summary are you looking for ?
> 
> What the actual problem is with these devices. The cover letter only
> mentions "issues with small command turn-around times". More details
> would be nice. Is it between transfers? Or even between messages within
> one transfer? Has it been tried to lower the bus frequency? Stuff like
> this.
> 

I don't know about this device, but in general the problem is that the
devices need some delay between some or all transfers or otherwise react
badly in one way or another. The problem is not always the same.
Lower bus frequencies don't help, at least not for the devices where
I have seen to problem myself. The issue is not bus speed, but time between
transfers. Typically the underlying problem is that there is some
microcontroller on the affected chips, and the microcode is less than
perfect. For example, the microcode may not poll its I2C interface
while it is busy writing into the chip's internal EEPROM or while it is
updating some internal parameters as result of a previous I2C transfer.

> > On a side note, if anyone plans to implement the prepare_device() callback,
> > please make sure that it covers all requirements. It would be unfortunate
> > if such a callback was implemented if that would still require per-driver
> > code (besides the callback).
> 
> Is there a list of that somewhere? Or does it mean going through all the
> drivers and see what they currently do?
> 

The latter. I never bothered trying to write up a list. Typically the behavior
is not documented and needs to be tweaked a couple of times, and it may be
different across chips supported by the same driver, or even across chip
revisions. Any list trying to keep track of the various details would
be difficult to maintain and notoriously be outdated.

Guenter

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Andrew Jeffery]

On Wed, 11 Oct 2023, at 09:29, Guenter Roeck wrote:
> On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote:
>> Hi Guenter,
>> 
>> > > > Reference to Andrew's previous proposal:
>> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/
>> > > 
>> > > I do totally agree with Guenter's comment[1], though. This just affects
>> > > a few drivers and this patch is way too intrusive for the I2C core. The
>> > > later suggested prepare_device() callback[2] sounds better to me. I
>> > > still haven't fully understood why this all cannot be handled in the
>> > > driver's probe. Could someone give me a small summary about that?
>> > > 
>> > 
>> > Lots of PMBus devices have the same problem, we have always handled
>> > it in PMBus drivers by implementing local wait code, and your references
>> > point that out.
>> 
>> I am confused now. Reading your reply:
>> 
>> "I am not sure if an implementation in the i2c core is desirable. It
>> looks quite invasive to me, and it won't solve the problem for all
>> devices since it isn't always a simple "wait <n> microseconds between
>> accesses". For example, some devices may require a wait after a write
>> but not after a read, or a wait only after certain commands (such as
>> commands writing to an EEPROM)."
>> 
>> I get the impression you don't prefer to have a generic mechanism in the
>> I2C core. This I share. Your response now sounds like you do support
>> that idea now?
>> 
>
> I didn't (want to) say that. I am perfectly happy with driver specific
> code, and I would personally still very much prefer it. I only wanted to
> suggest that _if_ a generic solution is implemented, it should cover all
> existing use cases and not just this one. But, really, I'd rather leave
> that alone and not risk introducing regressions to existing drivers.

We had an out-of-tree patch for the max31785[1] that I wrote a little 
after the initial discussion on this generic throttling and possibly 
somewhat before the other drivers had their delays added. Recently Joel 
pointed out the addition of the delays in the other drivers and I 
raised the idea that we could get rid of that out-of-tree patch by 
doing the same. Guenter's point about the work-arounds being very 
particular to the device is good justification for not trying to 
fix drivers that we can't immediately test - not that the series did 
that, but arguably if we're shooting for the generic solution then it 
should.

So I agree with Guenter that we probably want to do down the path of 
adding the delays directly into the max31785 driver and not trying to 
over-generalise.

Lakshmi: Apologies for misleading you in some way there - unfortunately 
I can't go back to understand exactly what I suggested as I've changed 
jobs in the mean time.

Andrew

[1]: https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be

RE: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Lakshmi Yadlapati]

Joel's suggestion to explore the previously proposed generic solution makes sense, especially considering the recent changes for PMBus devices that have added delays. Thank you for your review and input. 
I will modify the code to incorporate the necessary delay directly in the max31785 driver.

Thanks,
Lakshmi

On 10/10/23, 10:54 PM, "Andrew Jeffery" <andrew@aj.id.au <mailto:andrew@aj.id.au>> wrote:


On Wed, 11 Oct 2023, at 09:29, Guenter Roeck wrote:
> On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote:
>> Hi Guenter,
>> 
>> > > > Reference to Andrew's previous proposal:
>> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ <https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/> 
>> > > 
>> > > I do totally agree with Guenter's comment[1], though. This just affects
>> > > a few drivers and this patch is way too intrusive for the I2C core. The
>> > > later suggested prepare_device() callback[2] sounds better to me. I
>> > > still haven't fully understood why this all cannot be handled in the
>> > > driver's probe. Could someone give me a small summary about that?
>> > > 
>> > 
>> > Lots of PMBus devices have the same problem, we have always handled
>> > it in PMBus drivers by implementing local wait code, and your references
>> > point that out.
>> 
>> I am confused now. Reading your reply:
>> 
>> "I am not sure if an implementation in the i2c core is desirable. It
>> looks quite invasive to me, and it won't solve the problem for all
>> devices since it isn't always a simple "wait <n> microseconds between
>> accesses". For example, some devices may require a wait after a write
>> but not after a read, or a wait only after certain commands (such as
>> commands writing to an EEPROM)."
>> 
>> I get the impression you don't prefer to have a generic mechanism in the
>> I2C core. This I share. Your response now sounds like you do support
>> that idea now?
>> 
>
> I didn't (want to) say that. I am perfectly happy with driver specific
> code, and I would personally still very much prefer it. I only wanted to
> suggest that _if_ a generic solution is implemented, it should cover all
> existing use cases and not just this one. But, really, I'd rather leave
> that alone and not risk introducing regressions to existing drivers.


We had an out-of-tree patch for the max31785[1] that I wrote a little 
after the initial discussion on this generic throttling and possibly 
somewhat before the other drivers had their delays added. Recently Joel 
pointed out the addition of the delays in the other drivers and I 
raised the idea that we could get rid of that out-of-tree patch by 
doing the same. Guenter's point about the work-arounds being very 
particular to the device is good justification for not trying to 
fix drivers that we can't immediately test - not that the series did 
that, but arguably if we're shooting for the generic solution then it 
should.


So I agree with Guenter that we probably want to do down the path of 
adding the delays directly into the max31785 driver and not trying to 
over-generalise.


Lakshmi: Apologies for misleading you in some way there - unfortunately 
I can't go back to understand exactly what I suggested as I've changed 
jobs in the mean time.


Andrew


[1]: https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be <https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be> 

Re: [PATCH v1 0/2] [PATCH] hwmon: (pmbus/max31785) Add minimum delay between bus accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

Hi Guenter,

> I didn't (want to) say that. I am perfectly happy with driver specific
> code, and I would personally still very much prefer it. I only wanted to
> suggest that _if_ a generic solution is implemented, it should cover all
> existing use cases and not just this one. But, really, I'd rather leave
> that alone and not risk introducing regressions to existing drivers.

Okay, seems we are aligned again :)

> I don't know about this device, but in general the problem is that the
> devices need some delay between some or all transfers or otherwise react
> badly in one way or another. The problem is not always the same.

Ok, that again doesn't speak for a generic solution IMO.

> Lower bus frequencies don't help, at least not for the devices where
> I have seen to problem myself. The issue is not bus speed, but time between
> transfers. Typically the underlying problem is that there is some
> microcontroller on the affected chips, and the microcode is less than
> perfect. For example, the microcode may not poll its I2C interface
> while it is busy writing into the chip's internal EEPROM or while it is
> updating some internal parameters as result of a previous I2C transfer.

I see. Well, as you probably know, EEPROMs not reacting because they are
busy with an erase cycle is well-known in the I2C world. The bus driver
reports that the transfer couldn't get through, and then the EEPROM
driver knows the details and does something apropriate, probably waiting
a while. This assumes that the EEPROM can still play well on the I2C
bus. If a faulty device will lock up a bus because of bad microcode
while it is busy, then it surely needs handling of that :( And this
convinces me just more that it should be in the driver...

> The latter. I never bothered trying to write up a list. Typically the behavior
> is not documented and needs to be tweaked a couple of times, and it may be
> different across chips supported by the same driver, or even across chip
> revisions. Any list trying to keep track of the various details would
> be difficult to maintain and notoriously be outdated.

... especially because of that. If there is really some repeating
pattern for some of the devices, we could introduce helper functions
for the drivers to use maybe. But the I2C core functions are not the
place to handle it.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]