LKML Archive on lore.kernel.org
 help / color / Atom feed
From: <Codrin.Ciubotariu@microchip.com>
To: <linux@armlinux.org.uk>
Cc: <wsa@kernel.org>, <linux-i2c@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <robh+dt@kernel.org>,
	<Ludovic.Desroches@microchip.com>, <Nicolas.Ferre@microchip.com>,
	<alexandre.belloni@bootlin.com>, <kamel.bouhara@bootlin.com>
Subject: Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
Date: Mon, 3 Aug 2020 16:42:59 +0000
Message-ID: <765384b9-20dc-1a90-2a2c-89721d6ce5e8@microchip.com> (raw)
In-Reply-To: <20200803141613.GN1551@shell.armlinux.org.uk>

On 03.08.2020 17:16, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jul 30, 2020 at 09:00:36AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>>>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>>>
>>>>>>>> +- pinctrl
>>>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>>>
>>>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>>>
>>>>> Fully converted to what?  The generic handling where the i2c core layer
>>>>> handles everything to do with recovery, including the switch between
>>>>> modes?
>>>>>
>>>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>>>> any additional glitches on the bus.  Given the use case that this recovery
>>>>> is targetted at, avoiding glitches is very important to keep.
>>>>
>>>> Why is it not possbile to handle glitches in a generic way? I guess it
>>>> depends on the pinctl, but we could treat a worst-case scenario to
>>>> assure the switch between states is done properly.
>>>
>>> Please look at how i2c-pxa switches between the two, and decide whether
>>> the generic implementation can do the same.
>>
>> The handling of glitches from initialization looks generic to me. I see
>> that there are specific clear/reset routines that are in the
>> (un)prepare_recovery() callbacks, but these callbacks are not replaced
>> by the generic i2c recovery and will still be used if given by the
>> driver. The only thing the generic recovery does is to switch the pinmux
>> state. We can discuss whether we want to change the pinmux state first
>> or call the (un)preapre_recovery().
> 
> Right, the key point i2c-pxa does is that on prepare:
> - read the current state of the SCL and SDA lines and set the GPIO to
>    reflect those values.
> - then switch the pinmux state.
> 
> That must be preserved, otherwise if SCL is being held low by the I2C
> master, and we switch to GPIO mode, SCL will be released.  So the
> driver needs to be involved before the pinmux state is changed.

I understand, and I admit that I didn't see this case. In my mind, the 
master would always be in (almost) a reset state before calling for SDA 
recovery, so it won't hold any lines.
These steps can't be generic, of course. Also, not all I2C masters have 
a way to show the state of its lines. For these masters, one idea would 
be to reset them before calling i2c_recover_bus()

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 


  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic " Codrin Ciubotariu
2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
2020-07-05 21:19   ` Wolfram Sang
2020-07-24 19:39     ` Wolfram Sang
2020-07-24 20:52       ` Russell King - ARM Linux admin
2020-07-27 10:44         ` Codrin.Ciubotariu
2020-07-27 10:50           ` Russell King - ARM Linux admin
2020-07-30  9:00             ` Codrin.Ciubotariu
2020-08-03 14:16               ` Russell King - ARM Linux admin
2020-08-03 16:42                 ` Codrin.Ciubotariu [this message]
2020-07-15 19:21   ` Rob Herring
2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu
2020-08-02 16:54   ` Wolfram Sang
2020-08-03 13:27     ` Codrin.Ciubotariu
2020-08-03 16:49       ` wsa
2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu
2020-08-02 17:05   ` Wolfram Sang
2020-08-03 15:33     ` Codrin.Ciubotariu
2020-08-03 16:59       ` wsa
2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu
2020-08-02 17:08   ` Wolfram Sang
2020-08-03 15:42     ` Codrin.Ciubotariu
2020-08-03 16:59       ` wsa
2020-08-26  6:14       ` Wolfram Sang
2020-09-04  8:55         ` Codrin.Ciubotariu
2020-09-04  9:20           ` Wolfram Sang
2020-07-05 21:09 ` [RFC PATCH 0/4] i2c: core: add " 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=765384b9-20dc-1a90-2a2c-89721d6ce5e8@microchip.com \
    --to=codrin.ciubotariu@microchip.com \
    --cc=Ludovic.Desroches@microchip.com \
    --cc=Nicolas.Ferre@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=devicetree@vger.kernel.org \
    --cc=kamel.bouhara@bootlin.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=robh+dt@kernel.org \
    --cc=wsa@kernel.org \
    /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: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git