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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230730202507.ojwinyjsx7ygyavp@intel.intel>
Date:   Sun, 30 Jul 2023 22:25:07 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Svyatoslav Ryhel <clamor95@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
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@...e.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;
> +}