Messages in this thread | | | Date | Sun, 30 Jul 2023 22:25:07 +0200 | From | Andi Shyti <> | Subject | Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate |
| |
Hi Svyatoslav,
(I'm not going to comment at this stage on some coding issues)
On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > Implement driver for hot-plugged I2C busses, where some devices on > a bus are hot-pluggable and their presence is indicated by GPIO line. > > This feature is mainly used by the ASUS Transformers family. The
But not just Asus, right?
> Transformers have a connector that's used for USB, charging or for > attaching a dock-keyboard (which also has a battery and a touchpad). > This connector probably (can't be verified since no datasheets or > special equipment is available) has an I2C bus lines and a "detect" > line (pulled low on the dock side) among the pins. I guess there > is either no additional chip or a transparent bridge/buffer chip, > but nothing that could be controlled by software. For DT this setup > could be modelled like an I2C gate or 2-port mux with enable joining > two I2C buses (one "closer" to the CPU as a parent).
the description looks like it's hiding many doubts for a commit log :)
In the commit log we want to be sure on what we are doing.
[...]
> +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
there is no point for this to be "integer".
> +{ > + int ret; > + > + if (priv->adap.algo_data) > + return 0; > + > + /* > + * Store the dev data in adapter dev, since > + * previous i2c_del_adapter might have wiped it. > + */ > + priv->adap.dev.parent = priv->dev; > + priv->adap.dev.of_node = priv->dev->of_node; > + > + dev_dbg(priv->adap.dev.parent, "connection detected"); > + > + 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?
> + return ret; > +} > + > +static void i2c_hotplug_deactivate(struct i2c_hotplug_priv *priv) > +{ > + if (!priv->adap.algo_data) > + return; > + > + dev_dbg(priv->adap.dev.parent, "disconnection detected"); > + > + i2c_del_adapter(&priv->adap); > + priv->adap.algo_data = NULL; > +} > + > +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?
Andi
> + if (gpiod_get_value_cansleep(priv->gpio)) > + i2c_hotplug_activate(priv); > + else > + i2c_hotplug_deactivate(priv); > + > + return IRQ_HANDLED; > +}
| |