From: Wolfram Sang <wsa@kernel.org> To: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Cc: linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com, ludovic.desroches@microchip.com, andrew@sanpeople.com, mhoffman@lightlink.com, khali@linux-fr.org Subject: Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of at91_do_twi_transfer() Date: Mon, 29 Nov 2021 11:39:56 +0100 [thread overview] Message-ID: <YaSt/CXWmZGmt+te@kunai> (raw) In-Reply-To: <20210727111554.1338832-2-codrin.ciubotariu@microchip.com> [-- Attachment #1: Type: text/plain, Size: 1416 bytes --] Hi Codrin, sorry for the super-long delay. There is an issue here with regard to bus recovery which affetcs more drivers and I can't make up my mind how to handle it... > Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery") Sidenote: I don't think this is a fix. > + if (ret < 0) { > + /* > + * some faulty I2C slave devices might hold SDA down; > + * we can send a bus clear command, hoping that the pins will be > + * released > + */ > + i2c_recover_bus(&dev->adapter); > + } else { > + ret = num; > + } So, one issue is more straightforward. Bus recovery is applied on all errors. It should only be called when SDA is stuck. The other issue is that bus recovery is applied after a transfer. The I2C specs mention bus recovery only at the beginning of a transfer when SDA is detected low. I think it also makes more sense because the bus may also be stuck because of a misbehaving bootloader etc. This will be caught when the check is done at the beginning. However, moving the detection to the beginning leaves room for a regression, because your driver already does it at the end of a transfer. However, I'd think all regressions coming up need seperate fixing anyhow. Unless I overlooked something, of course. So, I think it should be moved to the beginning of a transfer, but I am open for discussion, so we get the best possible bus recovery in Linux. Happy hacking, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Wolfram Sang <wsa@kernel.org> To: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Cc: alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org, mhoffman@lightlink.com, ludovic.desroches@microchip.com, linux-i2c@vger.kernel.org, khali@linux-fr.org, andrew@sanpeople.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of at91_do_twi_transfer() Date: Mon, 29 Nov 2021 11:39:56 +0100 [thread overview] Message-ID: <YaSt/CXWmZGmt+te@kunai> (raw) In-Reply-To: <20210727111554.1338832-2-codrin.ciubotariu@microchip.com> [-- Attachment #1.1: Type: text/plain, Size: 1416 bytes --] Hi Codrin, sorry for the super-long delay. There is an issue here with regard to bus recovery which affetcs more drivers and I can't make up my mind how to handle it... > Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery") Sidenote: I don't think this is a fix. > + if (ret < 0) { > + /* > + * some faulty I2C slave devices might hold SDA down; > + * we can send a bus clear command, hoping that the pins will be > + * released > + */ > + i2c_recover_bus(&dev->adapter); > + } else { > + ret = num; > + } So, one issue is more straightforward. Bus recovery is applied on all errors. It should only be called when SDA is stuck. The other issue is that bus recovery is applied after a transfer. The I2C specs mention bus recovery only at the beginning of a transfer when SDA is detected low. I think it also makes more sense because the bus may also be stuck because of a misbehaving bootloader etc. This will be caught when the check is done at the beginning. However, moving the detection to the beginning leaves room for a regression, because your driver already does it at the end of a transfer. However, I'd think all regressions coming up need seperate fixing anyhow. Unless I overlooked something, of course. So, I think it should be moved to the beginning of a transfer, but I am open for discussion, so we get the best possible bus recovery in Linux. Happy hacking, Wolfram [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-11-29 10:42 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-27 11:15 [PATCH 0/3] i2c: at91: Fixes and updates Codrin Ciubotariu 2021-07-27 11:15 ` Codrin Ciubotariu 2021-07-27 11:15 ` [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of at91_do_twi_transfer() Codrin Ciubotariu 2021-07-27 11:15 ` Codrin Ciubotariu 2021-11-29 10:39 ` Wolfram Sang [this message] 2021-11-29 10:39 ` Wolfram Sang 2021-07-27 11:15 ` [PATCH 2/3] i2c: at91: keep the controller disabled when it is not used Codrin Ciubotariu 2021-07-27 11:15 ` Codrin Ciubotariu 2021-07-27 11:15 ` [PATCH 3/3] i2c: at91: add advanced digital filtering support for SAMA5D4 Codrin Ciubotariu 2021-07-27 11:15 ` Codrin Ciubotariu 2021-11-29 10:41 ` Wolfram Sang 2021-11-29 10:41 ` Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YaSt/CXWmZGmt+te@kunai \ --to=wsa@kernel.org \ --cc=alexandre.belloni@bootlin.com \ --cc=andrew@sanpeople.com \ --cc=codrin.ciubotariu@microchip.com \ --cc=khali@linux-fr.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ludovic.desroches@microchip.com \ --cc=mhoffman@lightlink.com \ --cc=nicolas.ferre@microchip.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.