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:   Sun, 30 Jul 2023 22:25:07 +0200
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Svyatoslav Ryhel <clamor95@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Wolfram Sang <wsa@kernel.org>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <mirq-linux@rere.qmqm.pl>,
        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: <20230730202507.ojwinyjsx7ygyavp@intel.intel>
References: <20230729160857.6332-1-clamor95@gmail.com>
 <20230729160857.6332-3-clamor95@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230729160857.6332-3-clamor95@gmail.com>
Precedence: bulk

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