| Message ID | 380b592a-cb28-47ef-b110-e2ee6e8550fb@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: collection of further improvements / refactorings | expand |
Hi Heiner, On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: > I see no need for a driver-specific timeout value, therefore let's go > with the i2c core default timeout of 1s set by i2c_register_adapter(). Why is the timeout value not needed in your opinion? Is the datasheet specifying anything here? Jean any opinion here? Thanks, Andi
On 30.01.2024 00:46, Andi Shyti wrote: > Hi Heiner, > > On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: >> I see no need for a driver-specific timeout value, therefore let's go >> with the i2c core default timeout of 1s set by i2c_register_adapter(). > > Why is the timeout value not needed in your opinion? Is the > datasheet specifying anything here? > I2C core sets a timeout of 1s if driver doesn't set a timeout value. So for me the question is: Is there an actual need or benefit of setting a lower timeout value? And that's something I don't see. > Jean any opinion here? > > Thanks, > Andi Heiner
Hi Heiner, > > On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: > >> I see no need for a driver-specific timeout value, therefore let's go > >> with the i2c core default timeout of 1s set by i2c_register_adapter(). > > > > Why is the timeout value not needed in your opinion? Is the > > datasheet specifying anything here? > > > I2C core sets a timeout of 1s if driver doesn't set a timeout value. > So for me the question is: Is there an actual need or benefit of > setting a lower timeout value? And that's something I don't see. yes, that's why I am asking and I would like to have an opinion from Jean. I will try to get hold of the datasheets, as well and see if there is any constraint on the timeout. Thanks, Andi
On 30.01.2024 11:00, Andi Shyti wrote: > Hi Heiner, > >>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: >>>> I see no need for a driver-specific timeout value, therefore let's go >>>> with the i2c core default timeout of 1s set by i2c_register_adapter(). >>> >>> Why is the timeout value not needed in your opinion? Is the >>> datasheet specifying anything here? >>> >> I2C core sets a timeout of 1s if driver doesn't set a timeout value. >> So for me the question is: Is there an actual need or benefit of >> setting a lower timeout value? And that's something I don't see. > > yes, that's why I am asking and I would like to have an opinion > from Jean. I will try to get hold of the datasheets, as well and > see if there is any constraint on the timeout. > The datasheet for the 7-series (doc# 326776-003) states: 5.21.3.2 Bus Time Out (The PCH as SMBus Master) If there is an error in the transaction, such that an SMBus device does not signal an acknowledge, or holds the clock lower than the allowed time-out time, the transaction will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start after the last bit of data is transferred by the PCH and it is waiting for a response. The 25-ms time-out counter will not count under the following conditions: 1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set 2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that the system has not locked up). It's my understanding that the chip will signal timeout after 25ms. So we should never have the case that the host timeout kicks in (as long as it's >25ms). So the host timeout value doesn't really matter. > Thanks, > Andi Heiner
Hi Heiner, On Tue, Jan 30, 2024 at 11:25:33AM +0100, Heiner Kallweit wrote: > On 30.01.2024 11:00, Andi Shyti wrote: > >>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: > >>>> I see no need for a driver-specific timeout value, therefore let's go > >>>> with the i2c core default timeout of 1s set by i2c_register_adapter(). > >>> > >>> Why is the timeout value not needed in your opinion? Is the > >>> datasheet specifying anything here? > >>> > >> I2C core sets a timeout of 1s if driver doesn't set a timeout value. > >> So for me the question is: Is there an actual need or benefit of > >> setting a lower timeout value? And that's something I don't see. > > > > yes, that's why I am asking and I would like to have an opinion > > from Jean. I will try to get hold of the datasheets, as well and > > see if there is any constraint on the timeout. > > > The datasheet for the 7-series (doc# 326776-003) states: > > 5.21.3.2 Bus Time Out (The PCH as SMBus Master) > If there is an error in the transaction, such that an SMBus device does not signal an > acknowledge, or holds the clock lower than the allowed time-out time, the transaction > will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out > minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start > after the last bit of data is transferred by the PCH and it is waiting for a response. > The 25-ms time-out counter will not count under the following conditions: > 1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set > 2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that > the system has not locked up). > > It's my understanding that the chip will signal timeout after 25ms. So we should never > have the case that the host timeout kicks in (as long as it's >25ms). > So the host timeout value doesn't really matter. This driver is used by many platforms. I scrolled through the datasheets of few of them and they differ from each other. This was set by Jean[*] that's why I need to hear from him from where this 200 ms comes from. As this change doesn't change much to the economy of the driver, I would take it out from this series or place it at the end. As of now, I'm going to take patch 1 and 2 in and you can resubmit a v2 without the first two patches. Thanks, Andi [*] b3b8df97723d ("i2c: i801: Use wait_event_timeout to wait for interrupts")
On 30.01.2024 22:58, Andi Shyti wrote: > Hi Heiner, > > On Tue, Jan 30, 2024 at 11:25:33AM +0100, Heiner Kallweit wrote: >> On 30.01.2024 11:00, Andi Shyti wrote: >>>>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote: >>>>>> I see no need for a driver-specific timeout value, therefore let's go >>>>>> with the i2c core default timeout of 1s set by i2c_register_adapter(). >>>>> >>>>> Why is the timeout value not needed in your opinion? Is the >>>>> datasheet specifying anything here? >>>>> >>>> I2C core sets a timeout of 1s if driver doesn't set a timeout value. >>>> So for me the question is: Is there an actual need or benefit of >>>> setting a lower timeout value? And that's something I don't see. >>> >>> yes, that's why I am asking and I would like to have an opinion >>> from Jean. I will try to get hold of the datasheets, as well and >>> see if there is any constraint on the timeout. >>> >> The datasheet for the 7-series (doc# 326776-003) states: >> >> 5.21.3.2 Bus Time Out (The PCH as SMBus Master) >> If there is an error in the transaction, such that an SMBus device does not signal an >> acknowledge, or holds the clock lower than the allowed time-out time, the transaction >> will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out >> minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start >> after the last bit of data is transferred by the PCH and it is waiting for a response. >> The 25-ms time-out counter will not count under the following conditions: >> 1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set >> 2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that >> the system has not locked up). >> >> It's my understanding that the chip will signal timeout after 25ms. So we should never >> have the case that the host timeout kicks in (as long as it's >25ms). >> So the host timeout value doesn't really matter. > > This driver is used by many platforms. I scrolled through the > datasheets of few of them and they differ from each other. > > This was set by Jean[*] that's why I need to hear from him from > where this 200 ms comes from. > > As this change doesn't change much to the economy of the driver, > I would take it out from this series or place it at the end. > > As of now, I'm going to take patch 1 and 2 in and you can > resubmit a v2 without the first two patches. > Sounds good. Thanks! > Thanks, > Andi > > [*] b3b8df97723d ("i2c: i801: Use wait_event_timeout to wait for interrupts") Heiner
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index cea8aaba0..344544d1f 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1707,9 +1707,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) outb_p(inb_p(SMBAUXCTL(priv)) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv)); - /* Default timeout in interrupt mode: 200 ms */ - priv->adapter.timeout = HZ / 5; - if (dev->irq == IRQ_NOTCONNECTED) priv->features &= ~FEATURE_IRQ;
I see no need for a driver-specific timeout value, therefore let's go with the i2c core default timeout of 1s set by i2c_register_adapter(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 3 --- 1 file changed, 3 deletions(-)