Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate

[Michał Mirosław]

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