* [PATCH 0/8] i2c: i801: Series with minor improvements
@ 2022-04-15 16:53 Heiner Kallweit
2022-04-15 16:54 ` [PATCH 1/8] i2c: i801: improve interrupt handler Heiner Kallweit
` (7 more replies)
0 siblings, 8 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:53 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
This series includes a number of minor improvements, partially it's
a re-send of patches submitted in December last year already.
Heiner Kallweit (8):
i2c: i801: improve interrupt handler
i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ
i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER
i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip
versions
i2c: i801: add helper i801_set_hstadd
i2c: i801: add i801_single_transaction(), complementing
i801_block_transaction()
i2c: i801: call i801_check_pre() from i801_access()
i2c: i801: call i801_check_post() from i801_access()
drivers/i2c/busses/i2c-i801.c | 339 ++++++++++++++++++----------------
1 file changed, 175 insertions(+), 164 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/8] i2c: i801: improve interrupt handler
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
@ 2022-04-15 16:54 ` Heiner Kallweit
2022-06-07 12:34 ` Jean Delvare
2022-04-15 16:55 ` [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ Heiner Kallweit
` (6 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:54 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
and an error flag is set, then don't trust the result and skip calling
i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
in the main interrupt handler, this allows to simplify the code a
little.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ff706349b..c481f121d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -556,9 +556,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
/* Write next byte, except for IRQ after last byte */
outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
}
-
- /* Clear BYTE_DONE to continue with next byte */
- outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
}
static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
@@ -588,7 +585,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
* BUS_ERR - SMI# transaction collision
* FAILED - transaction was canceled due to a KILL request
* When any of these occur, update ->status and signal completion.
- * ->status must be cleared before kicking off the next transaction.
*
* 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
* occurs for each byte of a byte-by-byte to prepare the next byte.
@@ -613,23 +609,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
}
status = inb_p(SMBHSTSTS(priv));
- if (status & SMBHSTSTS_BYTE_DONE)
+ if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))
i801_isr_byte_done(priv);
/*
- * Clear remaining IRQ sources: Completion of last command, errors
- * and the SMB_ALERT signal. SMB_ALERT status is set after signal
- * assertion independently of the interrupt generation being blocked
- * or not so clear it always when the status is set.
- */
- status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
- if (status)
- outb_p(status, SMBHSTSTS(priv));
- status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
- /*
- * Report transaction result.
- * ->status must be cleared before the next transaction is started.
+ * Clear IRQ sources: SMB_ALERT status is set after signal assertion
+ * independently of the interrupt generation being blocked or not
+ * so clear it always when the status is set.
*/
+ status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
+ outb_p(status, SMBHSTSTS(priv));
+
+ status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
if (status) {
priv->status = status;
complete(&priv->done);
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
2022-04-15 16:54 ` [PATCH 1/8] i2c: i801: improve interrupt handler Heiner Kallweit
@ 2022-04-15 16:55 ` Heiner Kallweit
2022-06-07 12:48 ` Jean Delvare
2022-04-15 16:55 ` [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER Heiner Kallweit
` (5 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:55 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
Host notification uses an interrupt, therefore it makes sense only if
interrupts are enabled. Make this dependency explicit by removing
FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c481f121d..eccdc7035 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
dev_info(&dev->dev, "SMBus using %s\n",
priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
+ if (!(priv->features & FEATURE_IRQ))
+ priv->features &= ~FEATURE_HOST_NOTIFY;
+
i801_add_tco(priv);
snprintf(priv->adapter.name, sizeof(priv->adapter.name),
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
2022-04-15 16:54 ` [PATCH 1/8] i2c: i801: improve interrupt handler Heiner Kallweit
2022-04-15 16:55 ` [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ Heiner Kallweit
@ 2022-04-15 16:55 ` Heiner Kallweit
2022-06-07 14:13 ` Jean Delvare
2022-04-15 16:56 ` [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions Heiner Kallweit
` (4 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:55 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
According to the datasheet the block process call requires block
buffer mode. The user may disable block buffer mode by module
parameter disable_features, in such a case we have to clear
FEATURE_BLOCK_PROC.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eccdc7035..1d8182901 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
}
priv->features &= ~disable_features;
+ if (!(priv->features & FEATURE_BLOCK_BUFFER))
+ priv->features &= ~FEATURE_BLOCK_PROC;
+
err = pcim_enable_device(dev);
if (err) {
dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
` (2 preceding siblings ...)
2022-04-15 16:55 ` [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER Heiner Kallweit
@ 2022-04-15 16:56 ` Heiner Kallweit
2022-06-07 14:24 ` Jean Delvare
2022-04-15 16:57 ` [PATCH 5/8] i2c: i801: add helper i801_set_hstadd() Heiner Kallweit
` (3 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:56 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
According to the datasheets interrupt mode and i2c block read are
supported on all chip versions. Therefore set both feature flags for
all chip versions.
Note: Don't remove the two feature flags as such (at least for now),
so that in case of a problem users can use the disable_features
module parameter to disable a problematic feature.
Patch is based solely on the datasheets. I don't have old enough
test hw, therefore the patch is compile-tested only.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 140 +++++++++++++++++-----------------
1 file changed, 69 insertions(+), 71 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1d8182901..a9737f14d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -12,70 +12,70 @@
/*
* Supports the following Intel I/O Controller Hubs (ICH):
*
- * I/O Block I2C
- * region SMBus Block proc. block
- * Chip name PCI ID size PEC buffer call read
- * ---------------------------------------------------------------------------
- * 82801AA (ICH) 0x2413 16 no no no no
- * 82801AB (ICH0) 0x2423 16 no no no no
- * 82801BA (ICH2) 0x2443 16 no no no no
- * 82801CA (ICH3) 0x2483 32 soft no no no
- * 82801DB (ICH4) 0x24c3 32 hard yes no no
- * 82801E (ICH5) 0x24d3 32 hard yes yes yes
- * 6300ESB 0x25a4 32 hard yes yes yes
- * 82801F (ICH6) 0x266a 32 hard yes yes yes
- * 6310ESB/6320ESB 0x269b 32 hard yes yes yes
- * 82801G (ICH7) 0x27da 32 hard yes yes yes
- * 82801H (ICH8) 0x283e 32 hard yes yes yes
- * 82801I (ICH9) 0x2930 32 hard yes yes yes
- * EP80579 (Tolapai) 0x5032 32 hard yes yes yes
- * ICH10 0x3a30 32 hard yes yes yes
- * ICH10 0x3a60 32 hard yes yes yes
- * 5/3400 Series (PCH) 0x3b30 32 hard yes yes yes
- * 6 Series (PCH) 0x1c22 32 hard yes yes yes
- * Patsburg (PCH) 0x1d22 32 hard yes yes yes
- * Patsburg (PCH) IDF 0x1d70 32 hard yes yes yes
- * Patsburg (PCH) IDF 0x1d71 32 hard yes yes yes
- * Patsburg (PCH) IDF 0x1d72 32 hard yes yes yes
- * DH89xxCC (PCH) 0x2330 32 hard yes yes yes
- * Panther Point (PCH) 0x1e22 32 hard yes yes yes
- * Lynx Point (PCH) 0x8c22 32 hard yes yes yes
- * Lynx Point-LP (PCH) 0x9c22 32 hard yes yes yes
- * Avoton (SOC) 0x1f3c 32 hard yes yes yes
- * Wellsburg (PCH) 0x8d22 32 hard yes yes yes
- * Wellsburg (PCH) MS 0x8d7d 32 hard yes yes yes
- * Wellsburg (PCH) MS 0x8d7e 32 hard yes yes yes
- * Wellsburg (PCH) MS 0x8d7f 32 hard yes yes yes
- * Coleto Creek (PCH) 0x23b0 32 hard yes yes yes
- * Wildcat Point (PCH) 0x8ca2 32 hard yes yes yes
- * Wildcat Point-LP (PCH) 0x9ca2 32 hard yes yes yes
- * BayTrail (SOC) 0x0f12 32 hard yes yes yes
- * Braswell (SOC) 0x2292 32 hard yes yes yes
- * Sunrise Point-H (PCH) 0xa123 32 hard yes yes yes
- * Sunrise Point-LP (PCH) 0x9d23 32 hard yes yes yes
- * DNV (SOC) 0x19df 32 hard yes yes yes
- * Emmitsburg (PCH) 0x1bc9 32 hard yes yes yes
- * Broxton (SOC) 0x5ad4 32 hard yes yes yes
- * Lewisburg (PCH) 0xa1a3 32 hard yes yes yes
- * Lewisburg Supersku (PCH) 0xa223 32 hard yes yes yes
- * Kaby Lake PCH-H (PCH) 0xa2a3 32 hard yes yes yes
- * Gemini Lake (SOC) 0x31d4 32 hard yes yes yes
- * Cannon Lake-H (PCH) 0xa323 32 hard yes yes yes
- * Cannon Lake-LP (PCH) 0x9da3 32 hard yes yes yes
- * Cedar Fork (PCH) 0x18df 32 hard yes yes yes
- * Ice Lake-LP (PCH) 0x34a3 32 hard yes yes yes
- * Ice Lake-N (PCH) 0x38a3 32 hard yes yes yes
- * Comet Lake (PCH) 0x02a3 32 hard yes yes yes
- * Comet Lake-H (PCH) 0x06a3 32 hard yes yes yes
- * Elkhart Lake (PCH) 0x4b23 32 hard yes yes yes
- * Tiger Lake-LP (PCH) 0xa0a3 32 hard yes yes yes
- * Tiger Lake-H (PCH) 0x43a3 32 hard yes yes yes
- * Jasper Lake (SOC) 0x4da3 32 hard yes yes yes
- * Comet Lake-V (PCH) 0xa3a3 32 hard yes yes yes
- * Alder Lake-S (PCH) 0x7aa3 32 hard yes yes yes
- * Alder Lake-P (PCH) 0x51a3 32 hard yes yes yes
- * Alder Lake-M (PCH) 0x54a3 32 hard yes yes yes
- * Raptor Lake-S (PCH) 0x7a23 32 hard yes yes yes
+ * I/O Block
+ * region SMBus Block proc.
+ * Chip name PCI ID size PEC buffer call
+ * -------------------------------------------------------------------
+ * 82801AA (ICH) 0x2413 16 no no no
+ * 82801AB (ICH0) 0x2423 16 no no no
+ * 82801BA (ICH2) 0x2443 16 no no no
+ * 82801CA (ICH3) 0x2483 32 soft no no
+ * 82801DB (ICH4) 0x24c3 32 hard yes no
+ * 82801E (ICH5) 0x24d3 32 hard yes yes
+ * 6300ESB 0x25a4 32 hard yes yes
+ * 82801F (ICH6) 0x266a 32 hard yes yes
+ * 6310ESB/6320ESB 0x269b 32 hard yes yes
+ * 82801G (ICH7) 0x27da 32 hard yes yes
+ * 82801H (ICH8) 0x283e 32 hard yes yes
+ * 82801I (ICH9) 0x2930 32 hard yes yes
+ * EP80579 (Tolapai) 0x5032 32 hard yes yes
+ * ICH10 0x3a30 32 hard yes yes
+ * ICH10 0x3a60 32 hard yes yes
+ * 5/3400 Series (PCH) 0x3b30 32 hard yes yes
+ * 6 Series (PCH) 0x1c22 32 hard yes yes
+ * Patsburg (PCH) 0x1d22 32 hard yes yes
+ * Patsburg (PCH) IDF 0x1d70 32 hard yes yes
+ * Patsburg (PCH) IDF 0x1d71 32 hard yes yes
+ * Patsburg (PCH) IDF 0x1d72 32 hard yes yes
+ * DH89xxCC (PCH) 0x2330 32 hard yes yes
+ * Panther Point (PCH) 0x1e22 32 hard yes yes
+ * Lynx Point (PCH) 0x8c22 32 hard yes yes
+ * Lynx Point-LP (PCH) 0x9c22 32 hard yes yes
+ * Avoton (SOC) 0x1f3c 32 hard yes yes
+ * Wellsburg (PCH) 0x8d22 32 hard yes yes
+ * Wellsburg (PCH) MS 0x8d7d 32 hard yes yes
+ * Wellsburg (PCH) MS 0x8d7e 32 hard yes yes
+ * Wellsburg (PCH) MS 0x8d7f 32 hard yes yes
+ * Coleto Creek (PCH) 0x23b0 32 hard yes yes
+ * Wildcat Point (PCH) 0x8ca2 32 hard yes yes
+ * Wildcat Point-LP (PCH) 0x9ca2 32 hard yes yes
+ * BayTrail (SOC) 0x0f12 32 hard yes yes
+ * Braswell (SOC) 0x2292 32 hard yes yes
+ * Sunrise Point-H (PCH) 0xa123 32 hard yes yes
+ * Sunrise Point-LP (PCH) 0x9d23 32 hard yes yes
+ * DNV (SOC) 0x19df 32 hard yes yes
+ * Emmitsburg (PCH) 0x1bc9 32 hard yes yes
+ * Broxton (SOC) 0x5ad4 32 hard yes yes
+ * Lewisburg (PCH) 0xa1a3 32 hard yes yes
+ * Lewisburg Supersku (PCH) 0xa223 32 hard yes yes
+ * Kaby Lake PCH-H (PCH) 0xa2a3 32 hard yes yes
+ * Gemini Lake (SOC) 0x31d4 32 hard yes yes
+ * Cannon Lake-H (PCH) 0xa323 32 hard yes yes
+ * Cannon Lake-LP (PCH) 0x9da3 32 hard yes yes
+ * Cedar Fork (PCH) 0x18df 32 hard yes yes
+ * Ice Lake-LP (PCH) 0x34a3 32 hard yes yes
+ * Ice Lake-N (PCH) 0x38a3 32 hard yes yes
+ * Comet Lake (PCH) 0x02a3 32 hard yes yes
+ * Comet Lake-H (PCH) 0x06a3 32 hard yes yes
+ * Elkhart Lake (PCH) 0x4b23 32 hard yes yes
+ * Tiger Lake-LP (PCH) 0xa0a3 32 hard yes yes
+ * Tiger Lake-H (PCH) 0x43a3 32 hard yes yes
+ * Jasper Lake (SOC) 0x4da3 32 hard yes yes
+ * Comet Lake-V (PCH) 0xa3a3 32 hard yes yes
+ * Alder Lake-S (PCH) 0x7aa3 32 hard yes yes
+ * Alder Lake-P (PCH) 0x51a3 32 hard yes yes
+ * Alder Lake-M (PCH) 0x54a3 32 hard yes yes
+ * Raptor Lake-S (PCH) 0x7a23 32 hard yes yes
*
* Features supported by this driver:
* Software PEC no
@@ -168,7 +168,7 @@
#define I801_WORD_DATA 0x0C
#define I801_PROC_CALL 0x10
#define I801_BLOCK_DATA 0x14
-#define I801_I2C_BLOCK_DATA 0x18 /* ICH5 and later */
+#define I801_I2C_BLOCK_DATA 0x18
#define I801_BLOCK_PROC_CALL 0x1C
/* I801 Host Control register bits */
@@ -973,11 +973,8 @@ static const struct i2c_algorithm smbus_algorithm = {
.functionality = i801_func,
};
-#define FEATURES_ICH5 (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \
- FEATURE_IRQ | FEATURE_SMBUS_PEC | \
- FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
-#define FEATURES_ICH4 (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \
- FEATURE_HOST_NOTIFY)
+#define FEATURES_ICH4 (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
+#define FEATURES_ICH5 (FEATURES_ICH4 | FEATURE_BLOCK_PROC)
static const struct pci_device_id i801_ids[] = {
{ PCI_DEVICE_DATA(INTEL, 82801AA_3, 0) },
@@ -1665,7 +1662,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
mutex_init(&priv->acpi_lock);
priv->pci_dev = dev;
- priv->features = id->driver_data;
+ priv->features = FEATURE_IRQ | FEATURE_I2C_BLOCK_READ;
+ priv->features |= id->driver_data;
/* Disable features on user request */
for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) {
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
` (3 preceding siblings ...)
2022-04-15 16:56 ` [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions Heiner Kallweit
@ 2022-04-15 16:57 ` Heiner Kallweit
2022-06-09 13:53 ` Jean Delvare
2022-04-15 16:58 ` [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction() Heiner Kallweit
` (2 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:57 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
Factor out setting SMBHSTADD to a helper. The current code makes the
assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
Therefore let the new helper explicitly check for I2C_SMBUS_READ.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9737f14d..bf77f8640 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
return result;
}
+static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
+{
+ addr <<= 1;
+ if (read_write == I2C_SMBUS_READ)
+ addr |= 0x01;
+ outb_p(addr, SMBHSTADD(priv));
+}
+
/* Return negative errno on error. */
static s32 i801_access(struct i2c_adapter *adap, u16 addr,
unsigned short flags, char read_write, u8 command,
@@ -795,28 +803,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
switch (size) {
case I2C_SMBUS_QUICK:
- outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
- SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, read_write);
xact = I801_QUICK;
break;
case I2C_SMBUS_BYTE:
- outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
- SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, read_write);
if (read_write == I2C_SMBUS_WRITE)
outb_p(command, SMBHSTCMD(priv));
xact = I801_BYTE;
break;
case I2C_SMBUS_BYTE_DATA:
- outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
- SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, read_write);
outb_p(command, SMBHSTCMD(priv));
if (read_write == I2C_SMBUS_WRITE)
outb_p(data->byte, SMBHSTDAT0(priv));
xact = I801_BYTE_DATA;
break;
case I2C_SMBUS_WORD_DATA:
- outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
- SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, read_write);
outb_p(command, SMBHSTCMD(priv));
if (read_write == I2C_SMBUS_WRITE) {
outb_p(data->word & 0xff, SMBHSTDAT0(priv));
@@ -825,7 +829,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
xact = I801_WORD_DATA;
break;
case I2C_SMBUS_PROC_CALL:
- outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
outb_p(command, SMBHSTCMD(priv));
outb_p(data->word & 0xff, SMBHSTDAT0(priv));
outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
@@ -833,8 +837,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
read_write = I2C_SMBUS_READ;
break;
case I2C_SMBUS_BLOCK_DATA:
- outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
- SMBHSTADD(priv));
+ i801_set_hstadd(priv, addr, read_write);
outb_p(command, SMBHSTCMD(priv));
block = 1;
break;
@@ -845,10 +848,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
* However if SPD Write Disable is set (Lynx Point and later),
* the read will fail if we don't set the R/#W bit.
*/
- outb_p(((addr & 0x7f) << 1) |
- ((priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
- (read_write & 0x01) : 0),
- SMBHSTADD(priv));
+ if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
+ i801_set_hstadd(priv, addr, read_write);
+ else
+ i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
+
if (read_write == I2C_SMBUS_READ) {
/* NB: page 240 of ICH5 datasheet also shows
* that DATA1 is the cmd field when reading */
@@ -858,11 +862,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
block = 1;
break;
case I2C_SMBUS_BLOCK_PROC_CALL:
- /*
- * Bit 0 of the slave address register always indicate a write
- * command.
- */
- outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
+ /* Needs to be flagged as write transaction */
+ i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
outb_p(command, SMBHSTCMD(priv));
block = 1;
break;
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
` (4 preceding siblings ...)
2022-04-15 16:57 ` [PATCH 5/8] i2c: i801: add helper i801_set_hstadd() Heiner Kallweit
@ 2022-04-15 16:58 ` Heiner Kallweit
2022-06-10 11:03 ` Jean Delvare
2022-04-15 16:58 ` [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access() Heiner Kallweit
2022-04-15 16:59 ` [PATCH 8/8] i2c: i801: call i801_check_post() " Heiner Kallweit
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:58 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
This patch factors out non-block pre/post processing to a new function
i801_single_transaction(), complementing existing function
i801_block_transaction(). This makes i801_access() better readable.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 95 +++++++++++++++++++++--------------
1 file changed, 58 insertions(+), 37 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index bf77f8640..8c2245f38 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
return result;
}
+/* Single transaction function */
+static int i801_single_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
+ char read_write, int command)
+{
+ int xact, ret;
+
+ switch (command) {
+ case I2C_SMBUS_QUICK:
+ xact = I801_QUICK;
+ break;
+ case I2C_SMBUS_BYTE:
+ xact = I801_BYTE;
+ break;
+ case I2C_SMBUS_BYTE_DATA:
+ if (read_write == I2C_SMBUS_WRITE)
+ outb_p(data->byte, SMBHSTDAT0(priv));
+ xact = I801_BYTE_DATA;
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ if (read_write == I2C_SMBUS_WRITE) {
+ outb_p(data->word & 0xff, SMBHSTDAT0(priv));
+ outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+ }
+ xact = I801_WORD_DATA;
+ break;
+ case I2C_SMBUS_PROC_CALL:
+ outb_p(data->word & 0xff, SMBHSTDAT0(priv));
+ outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+ xact = I801_PROC_CALL;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ ret = i801_transaction(priv, xact);
+
+ if (ret || read_write == I2C_SMBUS_WRITE)
+ return ret;
+
+ switch (command) {
+ case I2C_SMBUS_BYTE:
+ case I2C_SMBUS_BYTE_DATA:
+ data->byte = inb_p(SMBHSTDAT0(priv));
+ break;
+ case I2C_SMBUS_WORD_DATA:
+ case I2C_SMBUS_PROC_CALL:
+ data->word = inb_p(SMBHSTDAT0(priv)) +
+ (inb_p(SMBHSTDAT1(priv)) << 8);
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
{
addr <<= 1;
@@ -784,9 +840,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
unsigned short flags, char read_write, u8 command,
int size, union i2c_smbus_data *data)
{
- int hwpec;
- int block = 0;
- int ret, xact;
+ int hwpec, ret, block = 0;
struct i801_priv *priv = i2c_get_adapdata(adap);
mutex_lock(&priv->acpi_lock);
@@ -804,36 +858,23 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
switch (size) {
case I2C_SMBUS_QUICK:
i801_set_hstadd(priv, addr, read_write);
- xact = I801_QUICK;
break;
case I2C_SMBUS_BYTE:
i801_set_hstadd(priv, addr, read_write);
if (read_write == I2C_SMBUS_WRITE)
outb_p(command, SMBHSTCMD(priv));
- xact = I801_BYTE;
break;
case I2C_SMBUS_BYTE_DATA:
i801_set_hstadd(priv, addr, read_write);
outb_p(command, SMBHSTCMD(priv));
- if (read_write == I2C_SMBUS_WRITE)
- outb_p(data->byte, SMBHSTDAT0(priv));
- xact = I801_BYTE_DATA;
break;
case I2C_SMBUS_WORD_DATA:
i801_set_hstadd(priv, addr, read_write);
outb_p(command, SMBHSTCMD(priv));
- if (read_write == I2C_SMBUS_WRITE) {
- outb_p(data->word & 0xff, SMBHSTDAT0(priv));
- outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
- }
- xact = I801_WORD_DATA;
break;
case I2C_SMBUS_PROC_CALL:
i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
outb_p(command, SMBHSTCMD(priv));
- outb_p(data->word & 0xff, SMBHSTDAT0(priv));
- outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
- xact = I801_PROC_CALL;
read_write = I2C_SMBUS_READ;
break;
case I2C_SMBUS_BLOCK_DATA:
@@ -883,7 +924,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
if (block)
ret = i801_block_transaction(priv, data, read_write, size);
else
- ret = i801_transaction(priv, xact);
+ ret = i801_single_transaction(priv, data, read_write, size);
/* Some BIOSes don't like it when PEC is enabled at reboot or resume
time, so we forcibly disable it after every transaction. Turn off
@@ -891,26 +932,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
if (hwpec || block)
outb_p(inb_p(SMBAUXCTL(priv)) &
~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
-
- if (block)
- goto out;
- if (ret)
- goto out;
- if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
- goto out;
-
- switch (xact) {
- case I801_BYTE: /* Result put in SMBHSTDAT0 */
- case I801_BYTE_DATA:
- data->byte = inb_p(SMBHSTDAT0(priv));
- break;
- case I801_WORD_DATA:
- case I801_PROC_CALL:
- data->word = inb_p(SMBHSTDAT0(priv)) +
- (inb_p(SMBHSTDAT1(priv)) << 8);
- break;
- }
-
out:
/*
* Unlock the SMBus device for use by BIOS/ACPI,
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access()
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
` (5 preceding siblings ...)
2022-04-15 16:58 ` [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction() Heiner Kallweit
@ 2022-04-15 16:58 ` Heiner Kallweit
2022-06-10 13:52 ` Jean Delvare
2022-04-15 16:59 ` [PATCH 8/8] i2c: i801: call i801_check_post() " Heiner Kallweit
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:58 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
This avoids code duplication, in a next step we'll call
i801_check_post() from i801_access() as well.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8c2245f38..9061333f2 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -460,10 +460,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
unsigned long result;
const struct i2c_adapter *adap = &priv->adapter;
- status = i801_check_pre(priv);
- if (status < 0)
- return status;
-
if (priv->features & FEATURE_IRQ) {
reinit_completion(&priv->done);
outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
@@ -647,10 +643,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
if (command == I2C_SMBUS_BLOCK_PROC_CALL)
return -EOPNOTSUPP;
- status = i801_check_pre(priv);
- if (status < 0)
- return status;
-
len = data->block[0];
if (read_write == I2C_SMBUS_WRITE) {
@@ -851,6 +843,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
pm_runtime_get_sync(&priv->pci_dev->dev);
+ ret = i801_check_pre(priv);
+ if (ret)
+ goto out;
+
hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
&& size != I2C_SMBUS_QUICK
&& size != I2C_SMBUS_I2C_BLOCK_DATA;
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 8/8] i2c: i801: call i801_check_post() from i801_access()
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
` (6 preceding siblings ...)
2022-04-15 16:58 ` [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access() Heiner Kallweit
@ 2022-04-15 16:59 ` Heiner Kallweit
2022-06-10 14:31 ` Jean Delvare
7 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-15 16:59 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
Avoid code duplication by calling i801_check_post() from i801_access().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/i2c/busses/i2c-i801.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 9061333f2..ecec7a3a8 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv)
busy = status & SMBHSTSTS_HOST_BUSY;
status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
if (!busy && status)
- return status;
+ return status & STATUS_ERROR_FLAGS;
} while (time_is_after_eq_jiffies(timeout));
return -ETIMEDOUT;
@@ -456,7 +456,6 @@ static int i801_wait_byte_done(struct i801_priv *priv)
static int i801_transaction(struct i801_priv *priv, int xact)
{
- int status;
unsigned long result;
const struct i2c_adapter *adap = &priv->adapter;
@@ -465,13 +464,12 @@ static int i801_transaction(struct i801_priv *priv, int xact)
outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
SMBHSTCNT(priv));
result = wait_for_completion_timeout(&priv->done, adap->timeout);
- return i801_check_post(priv, result ? priv->status : -ETIMEDOUT);
+ return result ? priv->status : -ETIMEDOUT;
}
outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
- status = i801_wait_intr(priv);
- return i801_check_post(priv, status);
+ return i801_wait_intr(priv);
}
static int i801_block_transaction_by_block(struct i801_priv *priv,
@@ -618,7 +616,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
if (status) {
- priv->status = status;
+ priv->status = status & STATUS_ERROR_FLAGS;
complete(&priv->done);
}
@@ -668,7 +666,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
reinit_completion(&priv->done);
outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
result = wait_for_completion_timeout(&priv->done, adap->timeout);
- return i801_check_post(priv, result ? priv->status : -ETIMEDOUT);
+ return result ? priv->status : -ETIMEDOUT;
}
for (i = 1; i <= len; i++) {
@@ -682,7 +680,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
status = i801_wait_byte_done(priv);
if (status)
- goto exit;
+ return status;
if (i == 1 && read_write == I2C_SMBUS_READ
&& command != I2C_SMBUS_I2C_BLOCK_DATA) {
@@ -712,9 +710,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
}
- status = i801_wait_intr(priv);
-exit:
- return i801_check_post(priv, status);
+ return i801_wait_intr(priv);
}
/* Block transaction function */
@@ -922,6 +918,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
else
ret = i801_single_transaction(priv, data, read_write, size);
+ ret = i801_check_post(priv, ret);
+
/* Some BIOSes don't like it when PEC is enabled at reboot or resume
time, so we forcibly disable it after every transaction. Turn off
E32B for the same reason. */
--
2.35.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/8] i2c: i801: improve interrupt handler
2022-04-15 16:54 ` [PATCH 1/8] i2c: i801: improve interrupt handler Heiner Kallweit
@ 2022-06-07 12:34 ` Jean Delvare
2022-12-15 22:15 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-07 12:34 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:54:32 +0200, Heiner Kallweit wrote:
> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
> and an error flag is set, then don't trust the result and skip calling
> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
> in the main interrupt handler, this allows to simplify the code a
> little.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ff706349b..c481f121d 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -556,9 +556,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
> /* Write next byte, except for IRQ after last byte */
> outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
> }
> -
> - /* Clear BYTE_DONE to continue with next byte */
> - outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> }
>
> static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
> @@ -588,7 +585,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
> * BUS_ERR - SMI# transaction collision
> * FAILED - transaction was canceled due to a KILL request
> * When any of these occur, update ->status and signal completion.
> - * ->status must be cleared before kicking off the next transaction.
> *
> * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
> * occurs for each byte of a byte-by-byte to prepare the next byte.
> @@ -613,23 +609,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
> }
>
> status = inb_p(SMBHSTSTS(priv));
> - if (status & SMBHSTSTS_BYTE_DONE)
> + if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))
Isn't this better written
if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)
? At least my compiler generates smaller binary code.
> i801_isr_byte_done(priv);
>
> /*
> - * Clear remaining IRQ sources: Completion of last command, errors
> - * and the SMB_ALERT signal. SMB_ALERT status is set after signal
> - * assertion independently of the interrupt generation being blocked
> - * or not so clear it always when the status is set.
> - */
> - status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
> - if (status)
> - outb_p(status, SMBHSTSTS(priv));
> - status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
> - /*
> - * Report transaction result.
> - * ->status must be cleared before the next transaction is started.
> + * Clear IRQ sources: SMB_ALERT status is set after signal assertion
> + * independently of the interrupt generation being blocked or not
> + * so clear it always when the status is set.
> */
> + status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
> + outb_p(status, SMBHSTSTS(priv));
You are making the call to outb_p() unconditional. Is this under the
assumption that at least one of the bits must be set, so the condition
was always met?
> +
> + status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
> if (status) {
> priv->status = status;
> complete(&priv->done);
Tested OK on my system, looks good overall, nice simplification.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ
2022-04-15 16:55 ` [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ Heiner Kallweit
@ 2022-06-07 12:48 ` Jean Delvare
2022-12-16 20:23 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-07 12:48 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote:
> Host notification uses an interrupt, therefore it makes sense only if
> interrupts are enabled.
It would be nice to have this comment in the code itself.
> Make this dependency explicit by removing
> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c481f121d..eccdc7035 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> dev_info(&dev->dev, "SMBus using %s\n",
> priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
>
> + if (!(priv->features & FEATURE_IRQ))
> + priv->features &= ~FEATURE_HOST_NOTIFY;
Earlier in this function, there's an action which depends on the
FEATURE_HOST_NOTIFY flag being set. While this will only result in a
useless action and wouldn't cause a bug as far as I can see, wouldn't
it be cleaner to move that piece of code *after* the check you're
adding?
> +
> i801_add_tco(priv);
>
> snprintf(priv->adapter.name, sizeof(priv->adapter.name),
Looks good, tested OK on my system (non-regression only, I'm not using
the Host Notify feature).
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER
2022-04-15 16:55 ` [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER Heiner Kallweit
@ 2022-06-07 14:13 ` Jean Delvare
2022-12-16 20:57 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-07 14:13 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:55:46 +0200, Heiner Kallweit wrote:
> According to the datasheet the block process call requires block
> buffer mode. The user may disable block buffer mode by module
> parameter disable_features, in such a case we have to clear
> FEATURE_BLOCK_PROC.
In which datasheet are you seeing this? Can you point me to the
specific section and/or quote the statement? I can't find it in the
datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
huge and I may just be missing it.
Also, same request as previous patch, I'd like a comment in the code,
so that developers don't have to read the git log to figure out why this
piece of code is there.
Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
affects the value returned by i801_func(). i801_access() does not
verify whether this flag is set before processing a command where size
== I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
only partial (will work if the device driver calls .functionality as it
is supposed to, will fail with - I suppose - unpredictable results if
the device driver calls .smbus_xfer directly).
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eccdc7035..1d8182901 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> }
> priv->features &= ~disable_features;
>
> + if (!(priv->features & FEATURE_BLOCK_BUFFER))
> + priv->features &= ~FEATURE_BLOCK_PROC;
> +
> err = pcim_enable_device(dev);
> if (err) {
> dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions
2022-04-15 16:56 ` [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions Heiner Kallweit
@ 2022-06-07 14:24 ` Jean Delvare
2022-06-13 17:08 ` Jean Delvare
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-07 14:24 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:56:30 +0200, Heiner Kallweit wrote:
> According to the datasheets interrupt mode and i2c block read are
> supported on all chip versions. Therefore set both feature flags for
> all chip versions.
While the datasheets do match your claims (I checked the 82801CAM aka
ICH3-M datasheet), I have a hard time believing we would have made the
feature device-dependent without a good reason (and I have vague
memories that there was a problem, although I can't find any proof of
that).
So I'll try to resurrect my old ICH3-M-based laptop and test these
changes on it. If I manage to get a Linux distribution to install on
that 20-year-old system...
> Note: Don't remove the two feature flags as such (at least for now),
> so that in case of a problem users can use the disable_features
> module parameter to disable a problematic feature.
Agreed.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()
2022-04-15 16:57 ` [PATCH 5/8] i2c: i801: add helper i801_set_hstadd() Heiner Kallweit
@ 2022-06-09 13:53 ` Jean Delvare
2022-12-16 21:37 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-09 13:53 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:57:21 +0200, Heiner Kallweit wrote:
> Factor out setting SMBHSTADD to a helper. The current code makes the
> assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
This isn't an "assumption". The values of I2C_SMBUS_WRITE and
I2C_SMBUS_READ were chosen to match the bit position and values in the
I2C protocol. Maybe it should have been made clearer by defining them
as hexadecimal values instead of decimal values. Nevertheless, I find
it unfortunate to not use this fact to optimize the code, see below.
> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a9737f14d..bf77f8640 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
> return result;
> }
>
> +static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
> +{
> + addr <<= 1;
> + if (read_write == I2C_SMBUS_READ)
> + addr |= 0x01;
> + outb_p(addr, SMBHSTADD(priv));
This can be written:
outb_p((addr << 1) | read_write, SMBHSTADD(priv));
Net result -48 bytes of (x86_64) binary code. That's basically what the
original code was doing, minus the useless masking.
> +}
> +
> /* Return negative errno on error. */
> static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> unsigned short flags, char read_write, u8 command,
> @@ -795,28 +803,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>
> switch (size) {
> case I2C_SMBUS_QUICK:
> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> - SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, read_write);
> xact = I801_QUICK;
> break;
> case I2C_SMBUS_BYTE:
> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> - SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, read_write);
> if (read_write == I2C_SMBUS_WRITE)
> outb_p(command, SMBHSTCMD(priv));
> xact = I801_BYTE;
> break;
> case I2C_SMBUS_BYTE_DATA:
> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> - SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, read_write);
> outb_p(command, SMBHSTCMD(priv));
> if (read_write == I2C_SMBUS_WRITE)
> outb_p(data->byte, SMBHSTDAT0(priv));
> xact = I801_BYTE_DATA;
> break;
> case I2C_SMBUS_WORD_DATA:
> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> - SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, read_write);
> outb_p(command, SMBHSTCMD(priv));
> if (read_write == I2C_SMBUS_WRITE) {
> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> @@ -825,7 +829,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> xact = I801_WORD_DATA;
> break;
> case I2C_SMBUS_PROC_CALL:
> - outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> outb_p(command, SMBHSTCMD(priv));
> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> @@ -833,8 +837,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> read_write = I2C_SMBUS_READ;
> break;
> case I2C_SMBUS_BLOCK_DATA:
> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> - SMBHSTADD(priv));
> + i801_set_hstadd(priv, addr, read_write);
> outb_p(command, SMBHSTCMD(priv));
> block = 1;
> break;
> @@ -845,10 +848,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> * However if SPD Write Disable is set (Lynx Point and later),
> * the read will fail if we don't set the R/#W bit.
> */
> - outb_p(((addr & 0x7f) << 1) |
> - ((priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
> - (read_write & 0x01) : 0),
> - SMBHSTADD(priv));
> + if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
> + i801_set_hstadd(priv, addr, read_write);
> + else
> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
Preserving the use of the ternary operator makes the generated binary
smaller once again:
i801_set_hstadd(priv, addr,
(priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
read_write : I2C_SMBUS_WRITE);
Net result -11 bytes of (x86_64) binary code.
> +
> if (read_write == I2C_SMBUS_READ) {
> /* NB: page 240 of ICH5 datasheet also shows
> * that DATA1 is the cmd field when reading */
> @@ -858,11 +862,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> block = 1;
> break;
> case I2C_SMBUS_BLOCK_PROC_CALL:
> - /*
> - * Bit 0 of the slave address register always indicate a write
> - * command.
> - */
> - outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
> + /* Needs to be flagged as write transaction */
> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> outb_p(command, SMBHSTCMD(priv));
> block = 1;
> break;
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()
2022-04-15 16:58 ` [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction() Heiner Kallweit
@ 2022-06-10 11:03 ` Jean Delvare
2022-12-17 17:07 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-10 11:03 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:58:03 +0200, Heiner Kallweit wrote:
> This patch factors out non-block pre/post processing to a new function
> i801_single_transaction(), complementing existing function
> i801_block_transaction(). This makes i801_access() better readable.
I like the idea, but I have objections about some implementation
details, see below.
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 95 +++++++++++++++++++++--------------
> 1 file changed, 58 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index bf77f8640..8c2245f38 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
> return result;
> }
>
> +/* Single transaction function */
The term "single transaction" is a bit misleading. Block transactions
are also single transactions, in the sense that there's one start
condition at the beginning and one stop condition at the end. I'd
rather call non-block transactions "single value transactions" or
"simple transactions".
> +static int i801_single_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> + char read_write, int command)
> +{
> + int xact, ret;
> +
> + switch (command) {
> + case I2C_SMBUS_QUICK:
> + xact = I801_QUICK;
> + break;
> + case I2C_SMBUS_BYTE:
> + xact = I801_BYTE;
> + break;
Previous 2 lines are indented with spaces instead of tabs.
> + case I2C_SMBUS_BYTE_DATA:
> + if (read_write == I2C_SMBUS_WRITE)
> + outb_p(data->byte, SMBHSTDAT0(priv));
> + xact = I801_BYTE_DATA;
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + if (read_write == I2C_SMBUS_WRITE) {
> + outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> + }
> + xact = I801_WORD_DATA;
> + break;
> + case I2C_SMBUS_PROC_CALL:
> + outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> + xact = I801_PROC_CALL;
> + break;
> + default:
> + return -EOPNOTSUPP;
That's never going to happen.
Generally speaking, I'm worried about having the same switch/case
construct here that we already have in i801_access. Looks to me like we
are doing half of the work here and the other half there and I fail to
see the rationale for splitting the work like that. I mean, I see how
it solves the asymmetry between the block and non-block code paths, but
the result doesn't look appealing. From a performance perspective it's
questionable too.
What prevents us from doing all the work on either side? Maybe we
should move more code into i801_single_transaction (possibly in a
subsequent patch)?
> + }
> +
> + ret = i801_transaction(priv, xact);
> +
Traditionally no blank line here.
> + if (ret || read_write == I2C_SMBUS_WRITE)
> + return ret;
> +
> + switch (command) {
> + case I2C_SMBUS_BYTE:
> + case I2C_SMBUS_BYTE_DATA:
> + data->byte = inb_p(SMBHSTDAT0(priv));
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + case I2C_SMBUS_PROC_CALL:
> + data->word = inb_p(SMBHSTDAT0(priv)) +
> + (inb_p(SMBHSTDAT1(priv)) << 8);
> + break;
> + default:
> + break;
Default case is not needed.
> + }
> +
> + return 0;
> +}
> +
> static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
> {
> addr <<= 1;
> @@ -784,9 +840,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> unsigned short flags, char read_write, u8 command,
> int size, union i2c_smbus_data *data)
> {
> - int hwpec;
> - int block = 0;
> - int ret, xact;
> + int hwpec, ret, block = 0;
> struct i801_priv *priv = i2c_get_adapdata(adap);
>
> mutex_lock(&priv->acpi_lock);
> @@ -804,36 +858,23 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> switch (size) {
> case I2C_SMBUS_QUICK:
> i801_set_hstadd(priv, addr, read_write);
> - xact = I801_QUICK;
> break;
> case I2C_SMBUS_BYTE:
> i801_set_hstadd(priv, addr, read_write);
> if (read_write == I2C_SMBUS_WRITE)
> outb_p(command, SMBHSTCMD(priv));
> - xact = I801_BYTE;
> break;
> case I2C_SMBUS_BYTE_DATA:
> i801_set_hstadd(priv, addr, read_write);
> outb_p(command, SMBHSTCMD(priv));
> - if (read_write == I2C_SMBUS_WRITE)
> - outb_p(data->byte, SMBHSTDAT0(priv));
> - xact = I801_BYTE_DATA;
> break;
> case I2C_SMBUS_WORD_DATA:
> i801_set_hstadd(priv, addr, read_write);
> outb_p(command, SMBHSTCMD(priv));
> - if (read_write == I2C_SMBUS_WRITE) {
> - outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> - outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> - }
> - xact = I801_WORD_DATA;
> break;
> case I2C_SMBUS_PROC_CALL:
> i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> outb_p(command, SMBHSTCMD(priv));
> - outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> - outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> - xact = I801_PROC_CALL;
> read_write = I2C_SMBUS_READ;
> break;
> case I2C_SMBUS_BLOCK_DATA:
> @@ -883,7 +924,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> if (block)
> ret = i801_block_transaction(priv, data, read_write, size);
> else
> - ret = i801_transaction(priv, xact);
> + ret = i801_single_transaction(priv, data, read_write, size);
>
> /* Some BIOSes don't like it when PEC is enabled at reboot or resume
> time, so we forcibly disable it after every transaction. Turn off
> @@ -891,26 +932,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> if (hwpec || block)
> outb_p(inb_p(SMBAUXCTL(priv)) &
> ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
> -
> - if (block)
> - goto out;
> - if (ret)
> - goto out;
> - if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
> - goto out;
> -
> - switch (xact) {
> - case I801_BYTE: /* Result put in SMBHSTDAT0 */
> - case I801_BYTE_DATA:
> - data->byte = inb_p(SMBHSTDAT0(priv));
> - break;
> - case I801_WORD_DATA:
> - case I801_PROC_CALL:
> - data->word = inb_p(SMBHSTDAT0(priv)) +
> - (inb_p(SMBHSTDAT1(priv)) << 8);
> - break;
> - }
> -
> out:
> /*
> * Unlock the SMBus device for use by BIOS/ACPI,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access()
2022-04-15 16:58 ` [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access() Heiner Kallweit
@ 2022-06-10 13:52 ` Jean Delvare
0 siblings, 0 replies; 26+ messages in thread
From: Jean Delvare @ 2022-06-10 13:52 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:58:40 +0200, Heiner Kallweit wrote:
> This avoids code duplication, in a next step we'll call
> i801_check_post() from i801_access() as well.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8c2245f38..9061333f2 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -460,10 +460,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
> unsigned long result;
> const struct i2c_adapter *adap = &priv->adapter;
>
> - status = i801_check_pre(priv);
> - if (status < 0)
> - return status;
> -
> if (priv->features & FEATURE_IRQ) {
> reinit_completion(&priv->done);
> outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
> @@ -647,10 +643,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
> if (command == I2C_SMBUS_BLOCK_PROC_CALL)
> return -EOPNOTSUPP;
>
> - status = i801_check_pre(priv);
> - if (status < 0)
> - return status;
> -
> len = data->block[0];
>
> if (read_write == I2C_SMBUS_WRITE) {
> @@ -851,6 +843,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>
> pm_runtime_get_sync(&priv->pci_dev->dev);
>
> + ret = i801_check_pre(priv);
> + if (ret)
> + goto out;
> +
> hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
> && size != I2C_SMBUS_QUICK
> && size != I2C_SMBUS_I2C_BLOCK_DATA;
Makes sense, thanks for the clean-up.
Reviewed-by: Jean Delvare <jdelvare@suse.de>
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 8/8] i2c: i801: call i801_check_post() from i801_access()
2022-04-15 16:59 ` [PATCH 8/8] i2c: i801: call i801_check_post() " Heiner Kallweit
@ 2022-06-10 14:31 ` Jean Delvare
2022-12-17 17:21 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-10 14:31 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Fri, 15 Apr 2022 18:59:46 +0200, Heiner Kallweit wrote:
> Avoid code duplication by calling i801_check_post() from i801_access().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
Overall I like the idea. I only have one question to make sure I'm not
missing something.
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 9061333f2..ecec7a3a8 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv)
> busy = status & SMBHSTSTS_HOST_BUSY;
> status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
> if (!busy && status)
> - return status;
> + return status & STATUS_ERROR_FLAGS;
> } while (time_is_after_eq_jiffies(timeout));
Do I understand correctly that this change isn't really related to the
rest of the patch, and could have been done independently?
You are filtering out SMBHSTSTS_INTR simply because i801_check_post()
will never check it anyway, right? If so, I wonder if that's really
something we want to do, as ultimately this adds code with no
functional benefit just to be "cleaner". But please correct me if I'm
wrong.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions
2022-06-07 14:24 ` Jean Delvare
@ 2022-06-13 17:08 ` Jean Delvare
2022-06-14 12:59 ` Jean Delvare
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-13 17:08 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
work. I get the following error in the kernel log:
[13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
[13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
[13565.434728] Pid: 321, comm: udevd Tainted: G O 3.4.63-2.44-default #1
[13565.434734] Call Trace:
[13565.434773] [<c0205349>] try_stack_unwind+0x199/0x1b0
[13565.434793] [<c02041c7>] dump_trace+0x47/0xf0
[13565.434808] [<c02053ab>] show_trace_log_lvl+0x4b/0x60
[13565.434820] [<c02053d8>] show_trace+0x18/0x20
[13565.434837] [<c0682e81>] dump_stack+0x6d/0x72
[13565.434855] [<c02adad1>] __report_bad_irq+0x21/0xc0
[13565.434870] [<c02adef1>] note_interrupt+0x181/0x1d0
[13565.434887] [<c02abe9e>] handle_irq_event_percpu+0x9e/0x1d0
[13565.434901] [<c02abffe>] handle_irq_event+0x2e/0x50
[13565.434915] [<c02ae3e6>] handle_level_irq+0x56/0x90
[13565.434928] [<c0204168>] handle_irq+0x88/0xa0
[13565.434939] handlers:
[13565.434949] [<c04bbf6c>] acpi_irq
[13565.435027] [<e0baad00>] usb_hcd_irq
[13565.435054] [<e0baad00>] usb_hcd_irq
[13565.435079] [<e0baad00>] usb_hcd_irq
[13565.435193] [<e0d6fb80>] radeon_driver_irq_handler_kms
[13565.435206] [<e0af5c60>] yenta_interrupt
[13565.435214] [<e0af5c60>] yenta_interrupt
[13565.435227] [<e0b514c0>] irq_handler
[13565.435238] [<e0aeec90>] snd_intel8x0m_interrupt
[13565.435251] [<e0c514a0>] snd_intel8x0_interrupt
[13565.435264] [<e0ab4060>] e100_intr
[13565.435278] [<e09465d0>] i801_isr
[13565.435283] Disabling IRQ #9
[13565.437114] i801_smbus 0000:00:1f.3: Transaction timeout
[13565.437125] i801_smbus 0000:00:1f.3: Terminating the current operation
[13565.439189] i801_smbus 0000:00:1f.3: Failed terminating the transaction
If it matters, and as seen in the list of handlers above, IRQ9 is
heavily shared on this laptop (ACPI, USB, graphics, PCMCIA, sound,
network and SMBus).
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions
2022-06-13 17:08 ` Jean Delvare
@ 2022-06-14 12:59 ` Jean Delvare
2022-12-16 21:36 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Jean Delvare @ 2022-06-14 12:59 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c
Hi Heiner,
On Mon, 13 Jun 2022 19:08:41 +0200, Jean Delvare wrote:
> I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
> on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
> work. I get the following error in the kernel log:
>
> [13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
> [13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
And now it's all coming back to me. The reason why we did not enable
interrupts on chipsets older than the ICH5 is because they lack bit
INTS in PCI register PCISTS. When the IRQ line is shared, we don't know
whether the interrupt was caused by our device or by another device.
Specifically, the following piece of code fails (it returns IRQ_NONE
unconditionally):
static irqreturn_t i801_isr(int irq, void *dev_id)
{
(...)
/* Confirm this is our interrupt */
pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
if (!(pcists & PCI_STATUS_INTERRUPT))
return IRQ_NONE;
I tried replacing the code above by a check on SMBHSTSTS instead:
status = inb_p(SMBHSTSTS(priv));
if (!(status & STATUS_FLAGS))
return IRQ_NONE;
It seems to work, however my testing is limited and I don't know how
reliable that would be if the other devices sharing the interrupt line
were used heavily at the same time (the laptop is idling in text mode
at the moment so the fact that the interrupt line is heavily shared
does not really get exercised).
Then I loaded the driver with interrupts disabled
(disable_features=0x10) to see if I2C block read was working. It is NOT
working, as can be seen from the following dumps from the memory SPD
EEPROM:
linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 b
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 80 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 ??????@.?uT.??.?
10: 8f 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 ?????.??`..???,
20: 15 08 15 08 00 00 00 00 00 00 00 00 00 00 00 00 ????............
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 b9 ..............??
40: 7f da 00 00 00 00 00 00 53 53 50 31 33 33 2d 30 ??......SSP133-0
50: 36 34 33 32 33 4a 00 00 00 00 00 00 00 01 02 00 64323J.......??.
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 64 cd ..............d?
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 i
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
10: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
20: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
30: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
40: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
50: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
60: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
70: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
80: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
90: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
a0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
b0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
c0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
d0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
e0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
f0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
As you can see, the requested offset of the I2C block read (0x00, then
0x20, then 0x40 etc.) is being ignored, and instead every I2C block
read starts at EEPROM offset 0x01.
If you compare the datasheets of the ICH5 (Intel doc 252516-001) and
ICH3-M (Intel doc 290716-001), section "I2C Read", you will see that
the description of the command is different. The format described for
the ICH5 (table 114 in the datasheet) does match what the Linux i2c
stack calls an I2C block read, while the format described for the
ICH3-M (table 5-87 in the datasheet) does NOT. Apparently the original
implementation was aimed at specific devices using 10-bit I2C
addressing. As no other SMBus host device implemented that, we did not
even allocate an SMBus command constant to it (and the fact that Intel
changed the format in later hardware iterations proves that this was the
right thing to do).
Looking at the ICH3-M implementation of the "I2C Block Read" transfer,
I feel very lucky that I did not trash my memory SPD EEPROM while
running the command. Because the format really starts with a WRITE of 2
bytes to the EEPROM before reading from it.
So at least the I2C block read part of the patch is never going to
work. The interrupt part could work if we change the test as described
above, however I would question the relevance of making that change
considering that it is no longer the obvious clean-up you originally
proposed, and would then impact recent hardware too. I would hate to
introduce a regression for the sole purpose of enabling interrupts on
20-year old hardware which I doubt many people are still using.
I am available to perform any test you want me to on this old laptop.
As long as it runs...
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/8] i2c: i801: improve interrupt handler
2022-06-07 12:34 ` Jean Delvare
@ 2022-12-15 22:15 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-15 22:15 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 07.06.2022 14:34, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:54:32 +0200, Heiner Kallweit wrote:
>> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
>> and an error flag is set, then don't trust the result and skip calling
>> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
>> in the main interrupt handler, this allows to simplify the code a
>> little.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
>> 1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index ff706349b..c481f121d 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -556,9 +556,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>> /* Write next byte, except for IRQ after last byte */
>> outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
>> }
>> -
>> - /* Clear BYTE_DONE to continue with next byte */
>> - outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>> }
>>
>> static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>> @@ -588,7 +585,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>> * BUS_ERR - SMI# transaction collision
>> * FAILED - transaction was canceled due to a KILL request
>> * When any of these occur, update ->status and signal completion.
>> - * ->status must be cleared before kicking off the next transaction.
>> *
>> * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
>> * occurs for each byte of a byte-by-byte to prepare the next byte.
>> @@ -613,23 +609,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>> }
>>
>> status = inb_p(SMBHSTSTS(priv));
>> - if (status & SMBHSTSTS_BYTE_DONE)
>> + if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))
>
> Isn't this better written
>
> if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)
>
> ? At least my compiler generates smaller binary code.
>
OK. Meanwhile I prefer readability over saving few bytes, but here IMO
the readability doesn't suffer.
>> i801_isr_byte_done(priv);
>>
>> /*
>> - * Clear remaining IRQ sources: Completion of last command, errors
>> - * and the SMB_ALERT signal. SMB_ALERT status is set after signal
>> - * assertion independently of the interrupt generation being blocked
>> - * or not so clear it always when the status is set.
>> - */
>> - status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
>> - if (status)
>> - outb_p(status, SMBHSTSTS(priv));
>> - status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
>> - /*
>> - * Report transaction result.
>> - * ->status must be cleared before the next transaction is started.
>> + * Clear IRQ sources: SMB_ALERT status is set after signal assertion
>> + * independently of the interrupt generation being blocked or not
>> + * so clear it always when the status is set.
>> */
>> + status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
>> + outb_p(status, SMBHSTSTS(priv));
>
> You are making the call to outb_p() unconditional. Is this under the
> assumption that at least one of the bits must be set, so the condition
> was always met?
>
So far we exclude here the case where SMBHSTSTS is written in
i801_isr_byte_done(). The patch allows to write SMBHSTSTS here only,
so we don't need the condition any longer.
>> +
>> + status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>> if (status) {
>> priv->status = status;
>> complete(&priv->done);
>
> Tested OK on my system, looks good overall, nice simplification.
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ
2022-06-07 12:48 ` Jean Delvare
@ 2022-12-16 20:23 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-16 20:23 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 07.06.2022 14:48, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote:
>> Host notification uses an interrupt, therefore it makes sense only if
>> interrupts are enabled.
>
> It would be nice to have this comment in the code itself.
>
OK
>> Make this dependency explicit by removing
>> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index c481f121d..eccdc7035 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1756,6 +1756,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> dev_info(&dev->dev, "SMBus using %s\n",
>> priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
>>
>> + if (!(priv->features & FEATURE_IRQ))
>> + priv->features &= ~FEATURE_HOST_NOTIFY;
>
> Earlier in this function, there's an action which depends on the
> FEATURE_HOST_NOTIFY flag being set. While this will only result in a
> useless action and wouldn't cause a bug as far as I can see, wouldn't
> it be cleaner to move that piece of code *after* the check you're
> adding?
>
Yes, this would be better. Will be part of v2.
>> +
>> i801_add_tco(priv);
>>
>> snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>
> Looks good, tested OK on my system (non-regression only, I'm not using
> the Host Notify feature).
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER
2022-06-07 14:13 ` Jean Delvare
@ 2022-12-16 20:57 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-16 20:57 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 07.06.2022 16:13, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:55:46 +0200, Heiner Kallweit wrote:
>> According to the datasheet the block process call requires block
>> buffer mode. The user may disable block buffer mode by module
>> parameter disable_features, in such a case we have to clear
>> FEATURE_BLOCK_PROC.
>
> In which datasheet are you seeing this? Can you point me to the
> specific section and/or quote the statement? I can't find it in the
> datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
> huge and I may just be missing it.
>
I used the following datasheet:
Intel 9 Series Chipset Family PCH
330550-002
On page 211 the block process call is described.
There's a note: E32B bit in the Auxiliary Control register must be set when using this protocol.
The same note can be found on page 663.
> Also, same request as previous patch, I'd like a comment in the code,
> so that developers don't have to read the git log to figure out why this
> piece of code is there.
>
OK
> Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
> affects the value returned by i801_func(). i801_access() does not
> verify whether this flag is set before processing a command where size
> == I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
> only partial (will work if the device driver calls .functionality as it
> is supposed to, will fail with - I suppose - unpredictable results if
> the device driver calls .smbus_xfer directly).
>
If FEATURE_BLOCK_PROC isn't set then we would call
i801_block_transaction_byte_by_byte() according to the following code
in i801_block_transaction().
if ((priv->features & FEATURE_BLOCK_BUFFER) &&
command != I2C_SMBUS_I2C_BLOCK_DATA)
result = i801_block_transaction_by_block(priv, data,
read_write,
command);
else
result = i801_block_transaction_byte_by_byte(priv, data,
read_write,
command);
And i801_block_transaction_byte_by_byte() immediately returns
-EOPNOTSUPP in case of command == I2C_SMBUS_BLOCK_PROC_CALL.
Having said that the requested check is there, it's just executed
a little bit later.
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index eccdc7035..1d8182901 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> }
>> priv->features &= ~disable_features;
>>
>> + if (!(priv->features & FEATURE_BLOCK_BUFFER))
>> + priv->features &= ~FEATURE_BLOCK_PROC;
>> +
>> err = pcim_enable_device(dev);
>> if (err) {
>> dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
>
> Thanks,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions
2022-06-14 12:59 ` Jean Delvare
@ 2022-12-16 21:36 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-16 21:36 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 14.06.2022 14:59, Jean Delvare wrote:
> Hi Heiner,
>
> On Mon, 13 Jun 2022 19:08:41 +0200, Jean Delvare wrote:
>> I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
>> on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
>> work. I get the following error in the kernel log:
>>
>> [13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
>> [13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
>
> And now it's all coming back to me. The reason why we did not enable
> interrupts on chipsets older than the ICH5 is because they lack bit
> INTS in PCI register PCISTS. When the IRQ line is shared, we don't know
> whether the interrupt was caused by our device or by another device.
> Specifically, the following piece of code fails (it returns IRQ_NONE
> unconditionally):
>
> static irqreturn_t i801_isr(int irq, void *dev_id)
> {
> (...)
> /* Confirm this is our interrupt */
> pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
> if (!(pcists & PCI_STATUS_INTERRUPT))
> return IRQ_NONE;
>
> I tried replacing the code above by a check on SMBHSTSTS instead:
>
> status = inb_p(SMBHSTSTS(priv));
> if (!(status & STATUS_FLAGS))
> return IRQ_NONE;
>
> It seems to work, however my testing is limited and I don't know how
> reliable that would be if the other devices sharing the interrupt line
> were used heavily at the same time (the laptop is idling in text mode
> at the moment so the fact that the interrupt line is heavily shared
> does not really get exercised).
>
> Then I loaded the driver with interrupts disabled
> (disable_features=0x10) to see if I2C block read was working. It is NOT
> working, as can be seen from the following dumps from the memory SPD
> EEPROM:
>
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 b
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 80 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 ??????@.?uT.??.?
> 10: 8f 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 ?????.??`..???,
> 20: 15 08 15 08 00 00 00 00 00 00 00 00 00 00 00 00 ????............
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 b9 ..............??
> 40: 7f da 00 00 00 00 00 00 53 53 50 31 33 33 2d 30 ??......SSP133-0
> 50: 36 34 33 32 33 4a 00 00 00 00 00 00 00 01 02 00 64323J.......??.
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 64 cd ..............d?
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
>
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 i
> 0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
> 00: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> 10: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> 20: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> 30: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> 40: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> 50: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> 60: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> 70: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> 80: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> 90: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> a0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> b0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> c0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> d0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
> e0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f ?????@.?uT.??.??
> f0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15 ????.??`..???, ?
>
> As you can see, the requested offset of the I2C block read (0x00, then
> 0x20, then 0x40 etc.) is being ignored, and instead every I2C block
> read starts at EEPROM offset 0x01.
>
> If you compare the datasheets of the ICH5 (Intel doc 252516-001) and
> ICH3-M (Intel doc 290716-001), section "I2C Read", you will see that
> the description of the command is different. The format described for
> the ICH5 (table 114 in the datasheet) does match what the Linux i2c
> stack calls an I2C block read, while the format described for the
> ICH3-M (table 5-87 in the datasheet) does NOT. Apparently the original
> implementation was aimed at specific devices using 10-bit I2C
> addressing. As no other SMBus host device implemented that, we did not
> even allocate an SMBus command constant to it (and the fact that Intel
> changed the format in later hardware iterations proves that this was the
> right thing to do).
>
> Looking at the ICH3-M implementation of the "I2C Block Read" transfer,
> I feel very lucky that I did not trash my memory SPD EEPROM while
> running the command. Because the format really starts with a WRITE of 2
> bytes to the EEPROM before reading from it.
>
> So at least the I2C block read part of the patch is never going to
> work. The interrupt part could work if we change the test as described
> above, however I would question the relevance of making that change
> considering that it is no longer the obvious clean-up you originally
> proposed, and would then impact recent hardware too. I would hate to
> introduce a regression for the sole purpose of enabling interrupts on
> 20-year old hardware which I doubt many people are still using.
>
> I am available to perform any test you want me to on this old laptop.
> As long as it runs...
>
Thanks for testing! So I'll drop this patch from the series.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()
2022-06-09 13:53 ` Jean Delvare
@ 2022-12-16 21:37 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-16 21:37 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 09.06.2022 15:53, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:57:21 +0200, Heiner Kallweit wrote:
>> Factor out setting SMBHSTADD to a helper. The current code makes the
>> assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
>
> This isn't an "assumption". The values of I2C_SMBUS_WRITE and
> I2C_SMBUS_READ were chosen to match the bit position and values in the
> I2C protocol. Maybe it should have been made clearer by defining them
> as hexadecimal values instead of decimal values. Nevertheless, I find
> it unfortunate to not use this fact to optimize the code, see below.
>
>> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
>> 1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a9737f14d..bf77f8640 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>> return result;
>> }
>>
>> +static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>> +{
>> + addr <<= 1;
>> + if (read_write == I2C_SMBUS_READ)
>> + addr |= 0x01;
>> + outb_p(addr, SMBHSTADD(priv));
>
> This can be written:
>
> outb_p((addr << 1) | read_write, SMBHSTADD(priv));
>
> Net result -48 bytes of (x86_64) binary code. That's basically what the
> original code was doing, minus the useless masking.
>
OK
>> +}
>> +
>> /* Return negative errno on error. */
>> static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> unsigned short flags, char read_write, u8 command,
>> @@ -795,28 +803,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>
>> switch (size) {
>> case I2C_SMBUS_QUICK:
>> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> - SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, read_write);
>> xact = I801_QUICK;
>> break;
>> case I2C_SMBUS_BYTE:
>> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> - SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, read_write);
>> if (read_write == I2C_SMBUS_WRITE)
>> outb_p(command, SMBHSTCMD(priv));
>> xact = I801_BYTE;
>> break;
>> case I2C_SMBUS_BYTE_DATA:
>> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> - SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, read_write);
>> outb_p(command, SMBHSTCMD(priv));
>> if (read_write == I2C_SMBUS_WRITE)
>> outb_p(data->byte, SMBHSTDAT0(priv));
>> xact = I801_BYTE_DATA;
>> break;
>> case I2C_SMBUS_WORD_DATA:
>> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> - SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, read_write);
>> outb_p(command, SMBHSTCMD(priv));
>> if (read_write == I2C_SMBUS_WRITE) {
>> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> @@ -825,7 +829,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> xact = I801_WORD_DATA;
>> break;
>> case I2C_SMBUS_PROC_CALL:
>> - outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>> outb_p(command, SMBHSTCMD(priv));
>> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> @@ -833,8 +837,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> read_write = I2C_SMBUS_READ;
>> break;
>> case I2C_SMBUS_BLOCK_DATA:
>> - outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> - SMBHSTADD(priv));
>> + i801_set_hstadd(priv, addr, read_write);
>> outb_p(command, SMBHSTCMD(priv));
>> block = 1;
>> break;
>> @@ -845,10 +848,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> * However if SPD Write Disable is set (Lynx Point and later),
>> * the read will fail if we don't set the R/#W bit.
>> */
>> - outb_p(((addr & 0x7f) << 1) |
>> - ((priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
>> - (read_write & 0x01) : 0),
>> - SMBHSTADD(priv));
>> + if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
>> + i801_set_hstadd(priv, addr, read_write);
>> + else
>> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>
> Preserving the use of the ternary operator makes the generated binary
> smaller once again:
>
> i801_set_hstadd(priv, addr,
> (priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
> read_write : I2C_SMBUS_WRITE);
>
> Net result -11 bytes of (x86_64) binary code.
>
OK
>> +
>> if (read_write == I2C_SMBUS_READ) {
>> /* NB: page 240 of ICH5 datasheet also shows
>> * that DATA1 is the cmd field when reading */
>> @@ -858,11 +862,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> block = 1;
>> break;
>> case I2C_SMBUS_BLOCK_PROC_CALL:
>> - /*
>> - * Bit 0 of the slave address register always indicate a write
>> - * command.
>> - */
>> - outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>> + /* Needs to be flagged as write transaction */
>> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>> outb_p(command, SMBHSTCMD(priv));
>> block = 1;
>> break;
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()
2022-06-10 11:03 ` Jean Delvare
@ 2022-12-17 17:07 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-17 17:07 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 10.06.2022 13:03, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:58:03 +0200, Heiner Kallweit wrote:
>> This patch factors out non-block pre/post processing to a new function
>> i801_single_transaction(), complementing existing function
>> i801_block_transaction(). This makes i801_access() better readable.
>
> I like the idea, but I have objections about some implementation
> details, see below.
>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 95 +++++++++++++++++++++--------------
>> 1 file changed, 58 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index bf77f8640..8c2245f38 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>> return result;
>> }
>>
>> +/* Single transaction function */
>
> The term "single transaction" is a bit misleading. Block transactions
> are also single transactions, in the sense that there's one start
> condition at the beginning and one stop condition at the end. I'd
> rather call non-block transactions "single value transactions" or
> "simple transactions".
>
OK
>> +static int i801_single_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>> + char read_write, int command)
>> +{
>> + int xact, ret;
>> +
>> + switch (command) {
>> + case I2C_SMBUS_QUICK:
>> + xact = I801_QUICK;
>> + break;
>> + case I2C_SMBUS_BYTE:
>> + xact = I801_BYTE;
>> + break;
>
> Previous 2 lines are indented with spaces instead of tabs.
>
OK
>> + case I2C_SMBUS_BYTE_DATA:
>> + if (read_write == I2C_SMBUS_WRITE)
>> + outb_p(data->byte, SMBHSTDAT0(priv));
>> + xact = I801_BYTE_DATA;
>> + break;
>> + case I2C_SMBUS_WORD_DATA:
>> + if (read_write == I2C_SMBUS_WRITE) {
>> + outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> + }
>> + xact = I801_WORD_DATA;
>> + break;
>> + case I2C_SMBUS_PROC_CALL:
>> + outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> + outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> + xact = I801_PROC_CALL;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>
> That's never going to happen.
>
> Generally speaking, I'm worried about having the same switch/case
> construct here that we already have in i801_access. Looks to me like we
> are doing half of the work here and the other half there and I fail to
> see the rationale for splitting the work like that. I mean, I see how
> it solves the asymmetry between the block and non-block code paths, but
> the result doesn't look appealing. From a performance perspective it's
> questionable too.
>
> What prevents us from doing all the work on either side? Maybe we
> should move more code into i801_single_transaction (possibly in a
> subsequent patch)?
>
Makes sense. Ill add this in v2.
>> + }
>> +
>> + ret = i801_transaction(priv, xact);
>> +
>
> Traditionally no blank line here.
>
OK
>> + if (ret || read_write == I2C_SMBUS_WRITE)
>> + return ret;
>> +
>> + switch (command) {
>> + case I2C_SMBUS_BYTE:
>> + case I2C_SMBUS_BYTE_DATA:
>> + data->byte = inb_p(SMBHSTDAT0(priv));
>> + break;
>> + case I2C_SMBUS_WORD_DATA:
>> + case I2C_SMBUS_PROC_CALL:
>> + data->word = inb_p(SMBHSTDAT0(priv)) +
>> + (inb_p(SMBHSTDAT1(priv)) << 8);
>> + break;
>> + default:
>> + break;
>
> Default case is not needed.
>
OK
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>> {
>> addr <<= 1;
>> @@ -784,9 +840,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> unsigned short flags, char read_write, u8 command,
>> int size, union i2c_smbus_data *data)
>> {
>> - int hwpec;
>> - int block = 0;
>> - int ret, xact;
>> + int hwpec, ret, block = 0;
>> struct i801_priv *priv = i2c_get_adapdata(adap);
>>
>> mutex_lock(&priv->acpi_lock);
>> @@ -804,36 +858,23 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> switch (size) {
>> case I2C_SMBUS_QUICK:
>> i801_set_hstadd(priv, addr, read_write);
>> - xact = I801_QUICK;
>> break;
>> case I2C_SMBUS_BYTE:
>> i801_set_hstadd(priv, addr, read_write);
>> if (read_write == I2C_SMBUS_WRITE)
>> outb_p(command, SMBHSTCMD(priv));
>> - xact = I801_BYTE;
>> break;
>> case I2C_SMBUS_BYTE_DATA:
>> i801_set_hstadd(priv, addr, read_write);
>> outb_p(command, SMBHSTCMD(priv));
>> - if (read_write == I2C_SMBUS_WRITE)
>> - outb_p(data->byte, SMBHSTDAT0(priv));
>> - xact = I801_BYTE_DATA;
>> break;
>> case I2C_SMBUS_WORD_DATA:
>> i801_set_hstadd(priv, addr, read_write);
>> outb_p(command, SMBHSTCMD(priv));
>> - if (read_write == I2C_SMBUS_WRITE) {
>> - outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> - outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> - }
>> - xact = I801_WORD_DATA;
>> break;
>> case I2C_SMBUS_PROC_CALL:
>> i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>> outb_p(command, SMBHSTCMD(priv));
>> - outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> - outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> - xact = I801_PROC_CALL;
>> read_write = I2C_SMBUS_READ;
>> break;
>> case I2C_SMBUS_BLOCK_DATA:
>> @@ -883,7 +924,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> if (block)
>> ret = i801_block_transaction(priv, data, read_write, size);
>> else
>> - ret = i801_transaction(priv, xact);
>> + ret = i801_single_transaction(priv, data, read_write, size);
>>
>> /* Some BIOSes don't like it when PEC is enabled at reboot or resume
>> time, so we forcibly disable it after every transaction. Turn off
>> @@ -891,26 +932,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>> if (hwpec || block)
>> outb_p(inb_p(SMBAUXCTL(priv)) &
>> ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
>> -
>> - if (block)
>> - goto out;
>> - if (ret)
>> - goto out;
>> - if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
>> - goto out;
>> -
>> - switch (xact) {
>> - case I801_BYTE: /* Result put in SMBHSTDAT0 */
>> - case I801_BYTE_DATA:
>> - data->byte = inb_p(SMBHSTDAT0(priv));
>> - break;
>> - case I801_WORD_DATA:
>> - case I801_PROC_CALL:
>> - data->word = inb_p(SMBHSTDAT0(priv)) +
>> - (inb_p(SMBHSTDAT1(priv)) << 8);
>> - break;
>> - }
>> -
>> out:
>> /*
>> * Unlock the SMBus device for use by BIOS/ACPI,
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 8/8] i2c: i801: call i801_check_post() from i801_access()
2022-06-10 14:31 ` Jean Delvare
@ 2022-12-17 17:21 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-12-17 17:21 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-i2c
On 10.06.2022 16:31, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:59:46 +0200, Heiner Kallweit wrote:
>> Avoid code duplication by calling i801_check_post() from i801_access().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 20 +++++++++-----------
>> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> Overall I like the idea. I only have one question to make sure I'm not
> missing something.
>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 9061333f2..ecec7a3a8 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv)
>> busy = status & SMBHSTSTS_HOST_BUSY;
>> status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>> if (!busy && status)
>> - return status;
>> + return status & STATUS_ERROR_FLAGS;
>> } while (time_is_after_eq_jiffies(timeout));
>
> Do I understand correctly that this change isn't really related to the
> rest of the patch, and could have been done independently?
>
> You are filtering out SMBHSTSTS_INTR simply because i801_check_post()
> will never check it anyway, right? If so, I wonder if that's really
> something we want to do, as ultimately this adds code with no
> functional benefit just to be "cleaner". But please correct me if I'm
> wrong.
>
Reason is that in few places we check whether return value of
i801_wait_intr() is zero, this would fail if not filtering out SMBHSTSTS_INTR.
Example:
i801_transaction() returns the return value of i801_wait_intr() now.
And in i801_block_transaction_by_block() we check whether return value of
i801_transaction() is zero.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-12-17 17:22 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 16:53 [PATCH 0/8] i2c: i801: Series with minor improvements Heiner Kallweit
2022-04-15 16:54 ` [PATCH 1/8] i2c: i801: improve interrupt handler Heiner Kallweit
2022-06-07 12:34 ` Jean Delvare
2022-12-15 22:15 ` Heiner Kallweit
2022-04-15 16:55 ` [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ Heiner Kallweit
2022-06-07 12:48 ` Jean Delvare
2022-12-16 20:23 ` Heiner Kallweit
2022-04-15 16:55 ` [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER Heiner Kallweit
2022-06-07 14:13 ` Jean Delvare
2022-12-16 20:57 ` Heiner Kallweit
2022-04-15 16:56 ` [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions Heiner Kallweit
2022-06-07 14:24 ` Jean Delvare
2022-06-13 17:08 ` Jean Delvare
2022-06-14 12:59 ` Jean Delvare
2022-12-16 21:36 ` Heiner Kallweit
2022-04-15 16:57 ` [PATCH 5/8] i2c: i801: add helper i801_set_hstadd() Heiner Kallweit
2022-06-09 13:53 ` Jean Delvare
2022-12-16 21:37 ` Heiner Kallweit
2022-04-15 16:58 ` [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction() Heiner Kallweit
2022-06-10 11:03 ` Jean Delvare
2022-12-17 17:07 ` Heiner Kallweit
2022-04-15 16:58 ` [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access() Heiner Kallweit
2022-06-10 13:52 ` Jean Delvare
2022-04-15 16:59 ` [PATCH 8/8] i2c: i801: call i801_check_post() " Heiner Kallweit
2022-06-10 14:31 ` Jean Delvare
2022-12-17 17:21 ` Heiner Kallweit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.