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@xxxxxxxxxxxx>
>
> 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;
> +}
