| Message ID | 0d6a1cdb-39a1-4fad-a6e4-48953619f33b@gmail.com |
|---|---|
| Headers | show |
| Series | i2c: i801: collection of further improvements / refactorings | expand |
Hi Heiner, On Fri, Sep 22, 2023 at 09:34:13PM +0200, Heiner Kallweit wrote: > Replace magic number 10 with the appropriate constant. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
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
Hi Heiner, On Fri, Sep 22, 2023 at 09:37:35PM +0200, Heiner Kallweit wrote: > Avoid code duplication and factor out checking and clearing PEC error > bit to new helper i801_check_and_clear_pec_error(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Heiner, On Fri, Sep 22, 2023 at 09:41:41PM +0200, Heiner Kallweit wrote: > Avoid code duplication and factor out retrieving and checking the > block length value to new helper i801_get_block_len(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 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
On 22.09.2023 21:32, Heiner Kallweit wrote: > This series contains further improvements and refactorings. > > Heiner Kallweit (8): > i2c: i801: Replace magic value with constant in > dmi_check_onboard_devices > i2c: i801: Remove unused argument from tco functions > i2c: i801: Use i2c core default adapter timeout > i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4 > i2c: i801: Add helper i801_check_and_clear_pec_error > i2c: i801: Split i801_block_transaction > i2c: i801: Add SMBUS_LEN_SENTINEL > i2c: i801: Add helper i801_get_block_len > > drivers/i2c/busses/i2c-i801.c | 216 +++++++++++++++++----------------- > 1 file changed, 106 insertions(+), 110 deletions(-) > Thanks for the prompt review. Let's see what Jean thinks about patch 3. Then I can incorporate the feedback and provide a v2 of the series.
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")
Hi Heiner, On Fri, 22 Sep 2023 21:32:57 +0200, Heiner Kallweit wrote: > This series contains further improvements and refactorings. > > Heiner Kallweit (8): > i2c: i801: Replace magic value with constant in > dmi_check_onboard_devices > i2c: i801: Remove unused argument from tco functions > i2c: i801: Use i2c core default adapter timeout > i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4 > i2c: i801: Add helper i801_check_and_clear_pec_error > i2c: i801: Split i801_block_transaction > i2c: i801: Add SMBUS_LEN_SENTINEL > i2c: i801: Add helper i801_get_block_len > > [...] Applied the first to patches to i2c/i2c-host on git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git Thank you, Andi Patches applied =============== [1/8] i2c: i801: Replace magic value with constant in dmi_check_onboard_devices commit: 9f14f46a276521c92cdffb0fc36f907e868d3888 [2/8] i2c: i801: Remove unused argument from tco functions commit: 96b125361866d998471c1380f809f2a2b4db60c0
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
This series contains further improvements and refactorings. Heiner Kallweit (8): i2c: i801: Replace magic value with constant in dmi_check_onboard_devices i2c: i801: Remove unused argument from tco functions i2c: i801: Use i2c core default adapter timeout i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4 i2c: i801: Add helper i801_check_and_clear_pec_error i2c: i801: Split i801_block_transaction i2c: i801: Add SMBUS_LEN_SENTINEL i2c: i801: Add helper i801_get_block_len drivers/i2c/busses/i2c-i801.c | 216 +++++++++++++++++----------------- 1 file changed, 106 insertions(+), 110 deletions(-)