Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Tue, 31 Oct 2023 12:43:39 +0100
From:   Andi Shyti <andi.shyti@kernel.org>
To:     mike.isely@cobaltdigital.com
Cc:     Florian Fainelli <florian.fainelli@broadcom.com>,
        Mike Isely <isely@pobox.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] [i2c-bcm2835] Fully clean up hardware state machine
 after a timeout
Message-ID: <20231031114339.nutx2q2vtwbaaw6h@zenone.zhora.eu>
References: <20231030162114.3603829-1-mike.isely@cobaltdigital.com>
 <20231030162114.3603829-2-mike.isely@cobaltdigital.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231030162114.3603829-2-mike.isely@cobaltdigital.com>
Precedence: bulk

Hi Mike,

> When the driver detects a timeout, there's no guarantee that the ISR
> would have fired.  Thus after a timeout, it's the foreground that
> becomes responsible to reset the hardware state machine.  The change
> here just duplicates what is already implemented in the ISR.

Is this a fix? What failing here?

Can we have a feedback from Florian, Ray or Scott here?

...

>  	if (!time_left) {
> +		/* Since we can't trust the ISR to have cleaned up, do the
> +		 * full cleanup here... */

Please use the

	/*
	 * comment
	 * comment
	 */

format

>  		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
>  				   BCM2835_I2C_C_CLEAR);
> +		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT |
> +				   BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE);

I'm not sure this is really making any difference though. How
have you tested this?

Have you tried reading those registers before and understand what
went wrong?

Andi