Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 00:11:46 +0200
From:   =?iso-8859-2?Q?Micha=B3_Miros=B3aw?= <mirq-linux@rere.qmqm.pl>
To:     Andi Shyti <andi.shyti@kernel.org>
Cc:     Svyatoslav Ryhel <clamor95@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate
Message-ID: <ZMbgIovV7lxlgd5T@qmqm.qmqm.pl>
References: <20230729160857.6332-1-clamor95@gmail.com>
 <20230729160857.6332-3-clamor95@gmail.com>
 <20230730202507.ojwinyjsx7ygyavp@intel.intel>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230730202507.ojwinyjsx7ygyavp@intel.intel>
Precedence: bulk

On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote:
> On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote:
> > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
[...]
> > +{
> > +	int ret;
> > +
> > +	if (priv->adap.algo_data)
> > +		return 0;
[...]
> > +	ret = i2c_add_adapter(&priv->adap);
> > +	if (!ret)
> > +		priv->adap.algo_data = (void *)1;
> 
> You want to set algo_data to "1" in order to keep the
> activate/deactivate ordering.
> 
> But if we fail to add the adapter, what's the point to keep it
> active?

The code above does "if we added the adapter, remember we did so".
IOW, if we failed to add the adapter we don't set the mark so that
the next interrupt edge can trigger another try. Also we prevent
trying to remove an adapter we didn't successfully add.

> > +static irqreturn_t i2c_hotplug_interrupt(int irq, void *dev_id)
> > +{
> > +	struct i2c_hotplug_priv *priv = dev_id;
> > +
> > +	/* debounce */
> > +	msleep(20);
> can you explain this waiting and why 20ms?

It's an arbitrary time long enough to avoid having to handle multiple
on/off events that could happen when the dock is being inserted (ringing)
and short enough to not have to worry about the user getting impatient.

Best Regards
Micha? Miros?aw