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;
Message-ID: <25858c22-ef92-2136-67ef-0d27364c1600@linaro.org>
Date:   Sun, 30 Jul 2023 22:30:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.1
Subject: Re: [PATCH v3 2/2] i2c: Add GPIO-based hotplug gate
To:     Svyatoslav Ryhel <clamor95@gmail.com>,
        Andi Shyti <andi.shyti@kernel.org>,
        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>
Cc:     linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230729160857.6332-1-clamor95@gmail.com>
 <20230729160857.6332-3-clamor95@gmail.com>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230729160857.6332-3-clamor95@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 29/07/2023 18:08, 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
> 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).
> 
> Co-developed-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

...

> +	ret = devm_add_action_or_reset(&pdev->dev, devm_i2c_put_adapter,
> +				       parent);
> +	if (ret)
> +		return ret;
> +
> +	priv->gpio = devm_gpiod_get(&pdev->dev, "detect", GPIOD_IN);
> +	if (IS_ERR(priv->gpio))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->gpio),
> +				     "failed to get detect GPIO\n");
> +
> +	is_i2c = parent->algo->master_xfer;
> +	is_smbus = parent->algo->smbus_xfer;
> +
> +	snprintf(priv->adap.name, sizeof(priv->adap.name),
> +		 "i2c-hotplug (master i2c-%d)", i2c_adapter_id(parent));
> +	priv->adap.owner = THIS_MODULE;
> +	priv->adap.algo = i2c_hotplug_algo[is_i2c][is_smbus];
> +	priv->adap.algo_data = NULL;
> +	priv->adap.lock_ops = &i2c_hotplug_lock_ops;
> +	priv->adap.class = parent->class;
> +	priv->adap.retries = parent->retries;
> +	priv->adap.timeout = parent->timeout;
> +	priv->adap.quirks = parent->quirks;
> +	if (parent->bus_recovery_info)
> +		priv->adap.bus_recovery_info = &i2c_hotplug_recovery_info;
> +
> +	if (!priv->adap.algo)
> +		return -EINVAL;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0)
> +		return dev_err_probe(&pdev->dev, priv->irq,
> +				     "failed to get IRQ %d\n", priv->irq);
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, priv->irq, NULL,
> +					i2c_hotplug_interrupt,
> +					IRQF_ONESHOT | IRQF_SHARED,

Shared IRQ with devm is a recipe for disaster. Are you sure this is a
shared one? You have a remove() function which also points that it is
not safe. You can:
1. investigate to be sure it is 100% safe (please document why do you
think it is safe)
2. drop devm
3. drop shared flag.


Best regards,
Krzysztof