[PATCH v3 0/2] GPIO-based hotplug i2c bus

[PATCH v3 0/2] GPIO-based hotplug i2c bus

[Svyatoslav Ryhel]

ASUS Transformers require this driver for proper work with their dock.
Dock is controlled by EC and its presence is detected by a GPIO.

The Transformers have a connector that's used for USB, charging or
for attaching a keyboard (called a dock; it also has a battery and
a touchpad). This connector probably (I don't have the means to verify
that) 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
a 2-port mux with enable joining two I2C buses (one "closer" to the
CPU as a parent).

In this case it's hard to tell the difference if this is real or virtual
hardware.

This patchset is a predecessor of a possible larger patchset which
should bring support for a asus-ec, an i2c mfd device programmed by
Asus for their Transformers tablet line. Similar approach is used in
Microsoft Surface RT for attachable Type Cover.

> What is this actually doing?
Basically it duplicates the parent i2c bus once detection GPIO triggers
and probes all hot-pluggable devices which are connected to it. Once
GPIO triggers a detach signal all hot-pluggable devices are unprobed and
bus removed.

> Is the GPIO an irq line for signalling hoplugging and can be used by
> any driver or just this one?
It can be shared if necessary but usually all hot-pluggable devices
are gathered in one container and are plugged simultaneously.

---
Changes from v2:
- expanded descryption of driver implementation commit
- expanded descryption in patchset cover
- no changes to code or yaml from v2

Changes from v1:
- documentation changes:
  - dropped | from description
  - dropped nodename
  - unified use of quotes
  - used GPIO_ACTIVE_LOW define
  - used phandle instead of path
---

Michał Mirosław (1):
  i2c: Add GPIO-based hotplug gate

Svyatoslav Ryhel (1):
  dt-bindings: i2c: add binding for i2c-hotplug-gpio

 .../bindings/i2c/i2c-hotplug-gpio.yaml        |  65 +++++
 drivers/i2c/Kconfig                           |  11 +
 drivers/i2c/Makefile                          |   1 +
 drivers/i2c/i2c-hotplug-gpio.c                | 266 ++++++++++++++++++
 4 files changed, 343 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
 create mode 100644 drivers/i2c/i2c-hotplug-gpio.c

-- 
2.39.2

[PATCH v3 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

[Svyatoslav Ryhel]

Document device tree schema which describes hot-pluggable via GPIO
i2c bus.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/i2c/i2c-hotplug-gpio.yaml        | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml

diff --git a/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
new file mode 100644
index 000000000000..21f2b74ca6c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-hotplug-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO detected hot-plugged I2C bus
+
+maintainers:
+  - Michał Mirosław <mirq-linux@rere.qmqm.pl>
+
+description:
+  Driver for hot-plugged I2C busses, where some devices on a bus
+  are hot-pluggable and their presence is indicated by GPIO line.
+
+properties:
+  compatible:
+    items:
+      - const: i2c-hotplug-gpio
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  interrupts-extended:
+    minItems: 1
+
+  detect-gpios:
+    maxItems: 1
+
+  i2c-parent:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#address-cells'
+  - '#size-cells'
+  - interrupts-extended
+  - detect-gpios
+  - i2c-parent
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /*
+     * Asus Transformers use I2C hotplug for attachable dock keyboard
+     */
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c-dock {
+        compatible = "i2c-hotplug-gpio";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts-extended = <&gpio 164 IRQ_TYPE_EDGE_BOTH>;
+        detect-gpios = <&gpio 164 GPIO_ACTIVE_LOW>;
+
+        i2c-parent = <&gen2_i2c>;
+    };
+...
-- 
2.39.2

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

[Svyatoslav Ryhel]

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>
---
 drivers/i2c/Kconfig            |  11 ++
 drivers/i2c/Makefile           |   1 +
 drivers/i2c/i2c-hotplug-gpio.c | 266 +++++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/i2c/i2c-hotplug-gpio.c

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 438905e2a1d0..3e3f7675ea4a 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -98,6 +98,17 @@ config I2C_SMBUS
 source "drivers/i2c/algos/Kconfig"
 source "drivers/i2c/busses/Kconfig"
 
+config I2C_HOTPLUG_GPIO
+	tristate "Hot-plugged I2C bus detected by GPIO"
+	depends on GPIOLIB
+	depends on OF
+	help
+	  If you say yes to this option, support will be included for
+	  hot-plugging I2C devices with presence detected by GPIO pin value.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-hotplug-gpio.
+
 config I2C_STUB
 	tristate "I2C/SMBus Test Stub"
 	depends on m
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index c1d493dc9bac..9fd44310835a 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_I2C_SMBUS)		+= i2c-smbus.o
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-y				+= algos/ busses/ muxes/
+obj-$(CONFIG_I2C_HOTPLUG_GPIO)	+= i2c-hotplug-gpio.o
 obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
 obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
 obj-$(CONFIG_I2C_SLAVE_TESTUNIT)	+= i2c-slave-testunit.o
diff --git a/drivers/i2c/i2c-hotplug-gpio.c b/drivers/i2c/i2c-hotplug-gpio.c
new file mode 100644
index 000000000000..18c2d7f44d29
--- /dev/null
+++ b/drivers/i2c/i2c-hotplug-gpio.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * I2C hotplug gate controlled by GPIO
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct i2c_hotplug_priv {
+	struct i2c_adapter	 adap;
+	struct i2c_adapter	*parent;
+	struct device		*dev;
+	struct gpio_desc	*gpio;
+	int			 irq;
+};
+
+static inline struct i2c_adapter *i2c_hotplug_parent(struct i2c_adapter *adap)
+{
+	struct i2c_hotplug_priv *priv = container_of(adap, struct i2c_hotplug_priv, adap);
+
+	return priv->parent;
+}
+
+static int i2c_hotplug_master_xfer(struct i2c_adapter *adap,
+				   struct i2c_msg msgs[], int num)
+{
+	struct i2c_adapter *parent = i2c_hotplug_parent(adap);
+
+	return parent->algo->master_xfer(parent, msgs, num);
+}
+
+static int i2c_hotplug_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+				  unsigned short flags, char read_write,
+				  u8 command, int protocol,
+				  union i2c_smbus_data *data)
+{
+	struct i2c_adapter *parent = i2c_hotplug_parent(adap);
+
+	return parent->algo->smbus_xfer(parent, addr, flags, read_write,
+					command, protocol, data);
+}
+
+static u32 i2c_hotplug_functionality(struct i2c_adapter *adap)
+{
+	u32 parent_func = i2c_get_functionality(i2c_hotplug_parent(adap));
+
+	return parent_func & ~I2C_FUNC_SLAVE;
+}
+
+static const struct i2c_algorithm i2c_hotplug_algo_i2c = {
+	.master_xfer = i2c_hotplug_master_xfer,
+	.functionality = i2c_hotplug_functionality,
+};
+
+static const struct i2c_algorithm i2c_hotplug_algo_smbus = {
+	.smbus_xfer = i2c_hotplug_smbus_xfer,
+	.functionality = i2c_hotplug_functionality,
+};
+
+static const struct i2c_algorithm i2c_hotplug_algo_both = {
+	.master_xfer = i2c_hotplug_master_xfer,
+	.smbus_xfer = i2c_hotplug_smbus_xfer,
+	.functionality = i2c_hotplug_functionality,
+};
+
+static const struct i2c_algorithm *const i2c_hotplug_algo[2][2] = {
+	/* non-I2C */
+	{ NULL, &i2c_hotplug_algo_smbus },
+	/* I2C */
+	{ &i2c_hotplug_algo_i2c, &i2c_hotplug_algo_both }
+};
+
+static void i2c_hotplug_lock_bus(struct i2c_adapter *adap, unsigned int flags)
+{
+	i2c_lock_bus(i2c_hotplug_parent(adap), flags);
+}
+
+static int i2c_hotplug_trylock_bus(struct i2c_adapter *adap,
+				   unsigned int flags)
+{
+	return i2c_trylock_bus(i2c_hotplug_parent(adap), flags);
+}
+
+static void i2c_hotplug_unlock_bus(struct i2c_adapter *adap,
+				   unsigned int flags)
+{
+	i2c_unlock_bus(i2c_hotplug_parent(adap), flags);
+}
+
+static const struct i2c_lock_operations i2c_hotplug_lock_ops = {
+	.lock_bus =    i2c_hotplug_lock_bus,
+	.trylock_bus = i2c_hotplug_trylock_bus,
+	.unlock_bus =  i2c_hotplug_unlock_bus,
+};
+
+static int i2c_hotplug_recover_bus(struct i2c_adapter *adap)
+{
+	return i2c_recover_bus(i2c_hotplug_parent(adap));
+}
+
+static struct i2c_bus_recovery_info i2c_hotplug_recovery_info = {
+	.recover_bus = i2c_hotplug_recover_bus,
+};
+
+static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
+{
+	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;
+
+	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);
+
+	if (gpiod_get_value_cansleep(priv->gpio))
+		i2c_hotplug_activate(priv);
+	else
+		i2c_hotplug_deactivate(priv);
+
+	return IRQ_HANDLED;
+}
+
+static void devm_i2c_put_adapter(void *adapter)
+{
+	i2c_put_adapter(adapter);
+}
+
+static int i2c_hotplug_gpio_probe(struct platform_device *pdev)
+{
+	struct device_node *parent_np;
+	struct i2c_adapter *parent;
+	struct i2c_hotplug_priv *priv;
+	bool is_i2c, is_smbus;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+	priv->dev = &pdev->dev;
+
+	parent_np = of_parse_phandle(pdev->dev.of_node, "i2c-parent", 0);
+	if (IS_ERR(parent_np))
+		return dev_err_probe(&pdev->dev, PTR_ERR(parent_np),
+				     "cannot parse i2c-parent\n");
+
+	parent = of_find_i2c_adapter_by_node(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(parent))
+		return dev_err_probe(&pdev->dev, PTR_ERR(parent),
+				     "failed to get parent I2C adapter\n");
+	priv->parent = parent;
+
+	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,
+					"i2c-hotplug", priv);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to register IRQ %d\n", priv->irq);
+
+	irq_wake_thread(priv->irq, priv);
+
+	return 0;
+}
+
+static int i2c_hotplug_gpio_remove(struct platform_device *pdev)
+{
+	struct i2c_hotplug_priv *priv = platform_get_drvdata(pdev);
+
+	i2c_hotplug_deactivate(priv);
+
+	return 0;
+}
+
+static const struct of_device_id i2c_hotplug_gpio_of_match[] = {
+	{ .compatible = "i2c-hotplug-gpio" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_hotplug_gpio_of_match);
+
+static struct platform_driver i2c_hotplug_gpio_driver = {
+	.probe	= i2c_hotplug_gpio_probe,
+	.remove	= i2c_hotplug_gpio_remove,
+	.driver	= {
+		.name	= "i2c-hotplug-gpio",
+		.of_match_table = i2c_hotplug_gpio_of_match,
+	},
+};
+
+module_platform_driver(i2c_hotplug_gpio_driver);
+
+MODULE_DESCRIPTION("Hot-plugged I2C bus detected by GPIO");
+MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v3 0/2] GPIO-based hotplug i2c bus

[Andi Shyti]

Hi Svyatoslav,

On Sat, Jul 29, 2023 at 07:08:55PM +0300, Svyatoslav Ryhel wrote:
> ASUS Transformers require this driver for proper work with their dock.
> Dock is controlled by EC and its presence is detected by a GPIO.
> 
> The Transformers have a connector that's used for USB, charging or
> for attaching a keyboard (called a dock; it also has a battery and
> a touchpad). This connector probably (I don't have the means to verify
> that) 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
> a 2-port mux with enable joining two I2C buses (one "closer" to the
> CPU as a parent).
> 
> In this case it's hard to tell the difference if this is real or virtual
> hardware.

How did you test this device?

> This patchset is a predecessor of a possible larger patchset which
> should bring support for a asus-ec, an i2c mfd device programmed by
> Asus for their Transformers tablet line. Similar approach is used in
> Microsoft Surface RT for attachable Type Cover.

Would be nice to have a driver using this support in the series,
otherwise it looks like thrown there without any use. Do you have
any use of it already? Even in your private repository just to
take a look.

Thanks,
Andi

Re: [PATCH v3 0/2] GPIO-based hotplug i2c bus

[Svyatoslav Ryhel]

нд, 30 лип. 2023 р. о 20:49 Andi Shyti <andi.shyti@kernel.org> пише:
>
> Hi Svyatoslav,
>
> On Sat, Jul 29, 2023 at 07:08:55PM +0300, Svyatoslav Ryhel wrote:
> > ASUS Transformers require this driver for proper work with their dock.
> > Dock is controlled by EC and its presence is detected by a GPIO.
> >
> > The Transformers have a connector that's used for USB, charging or
> > for attaching a keyboard (called a dock; it also has a battery and
> > a touchpad). This connector probably (I don't have the means to verify
> > that) 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
> > a 2-port mux with enable joining two I2C buses (one "closer" to the
> > CPU as a parent).
> >
> > In this case it's hard to tell the difference if this is real or virtual
> > hardware.
>
> How did you test this device?
>
Using devices, which relay on this patch, here is a list of those:
- ASUS Eee Pad Transformer TF101 (mainlined)
- ASUS Transformer Prime TF201 (mainlined)
- ASUS Transformer Pad TF300T/TF300TG/TF300TL (mainlined)
- ASUS Transformer Infinity TF700T (mainlined)
- ASUS VivoTab RT TF600T (WIP)
- ASUS Transformer Pad TF701T (mainlined)

Non ASUS device is Microsoft Surface RT

Tested by many owners and users for more than a year iirc.

> > This patchset is a predecessor of a possible larger patchset which
> > should bring support for a asus-ec, an i2c mfd device programmed by
> > Asus for their Transformers tablet line. Similar approach is used in
> > Microsoft Surface RT for attachable Type Cover.
>
> Would be nice to have a driver using this support in the series,
> otherwise it looks like thrown there without any use. Do you have
> any use of it already? Even in your private repository just to
> take a look.
>

Bindings which call gpio hotplug i2c bus:
ASUS TF https://github.com/clamor-s/linux/commit/360f62f706670ab13101ef15b7f2bc8880da7a48
ASUS TF600T/TF701T
https://github.com/clamor-s/linux/blob/transformer/arch/arm/boot/dts/tegra30-asus-tf600t.dts#L1050-L1089
Surface RT https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra30-microsoft-surface-rt.dts#L35-L53

> Thanks,
> Andi

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

[Andi Shyti]

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

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

[Krzysztof Kozlowski]

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

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

[Michał Mirosław]

On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
> 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.
[...] 
> > +	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)

Could you elaborate on what is unsafe in using devm with shared
interrupts (as compared to non-shared or not devm-managed)?

The remove function is indeed reversing the order of cleanup. The
shutdown path can be fixed by removing `remove()` and adding
`devm_add_action_or_reset(...deactivate)` before the IRQ is registered.

Best Regards
Michał Mirosław

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

[Michał Mirosław]

On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote:
> On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote:
> > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
[...]
> > +{
> > +	int ret;
> > +
> > +	if (priv->adap.algo_data)
> > +		return 0;
[...]
> > +	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?

The code above does "if we added the adapter, remember we did so".
IOW, if we failed to add the adapter we don't set the mark so that
the next interrupt edge can trigger another try. Also we prevent
trying to remove an adapter we didn't successfully add.

> > +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?

It's an arbitrary time long enough to avoid having to handle multiple
on/off events that could happen when the dock is being inserted (ringing)
and short enough to not have to worry about the user getting impatient.

Best Regards
Michał Mirosław

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

[Krzysztof Kozlowski]

On 30/07/2023 23:55, Michał Mirosław wrote:
> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
>> 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.
> [...] 
>>> +	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)
> 
> Could you elaborate on what is unsafe in using devm with shared
> interrupts (as compared to non-shared or not devm-managed)?
> 
> The remove function is indeed reversing the order of cleanup. The
> shutdown path can be fixed by removing `remove()` and adding
> `devm_add_action_or_reset(...deactivate)` before the IRQ is registered.

Shared interrupt might be triggered easily by other device between
remove() and irq release function (devm_free_irq() or whatever it is
called).

Best regards,
Krzysztof

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

[Michał Mirosław]

On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote:
> On 30/07/2023 23:55, Michał Mirosław wrote:
> > On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
> >> 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.
> > [...] 
> >>> +	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)
> > 
> > Could you elaborate on what is unsafe in using devm with shared
> > interrupts (as compared to non-shared or not devm-managed)?
> > 
> > The remove function is indeed reversing the order of cleanup. The
> > shutdown path can be fixed by removing `remove()` and adding
> > `devm_add_action_or_reset(...deactivate)` before the IRQ is registered.
> Shared interrupt might be triggered easily by other device between
> remove() and irq release function (devm_free_irq() or whatever it is
> called).

This is no different tham a non-shared interrupt that can be triggered
by the device being removed. Since devres will release the IRQ first,
before freeing the driver data, the interrupt hander will see consistent
driver-internal state. (The difference between remove() and devres
release phase is that for the latter sysfs files are already removed.)

Best Regards
Michał Mirosław

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

[Krzysztof Kozlowski]

On 31/07/2023 10:49, Michał Mirosław wrote:
> On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote:
>> On 30/07/2023 23:55, Michał Mirosław wrote:
>>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
>>>> 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.
>>> [...] 
>>>>> +	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)
>>>
>>> Could you elaborate on what is unsafe in using devm with shared
>>> interrupts (as compared to non-shared or not devm-managed)?
>>>
>>> The remove function is indeed reversing the order of cleanup. The
>>> shutdown path can be fixed by removing `remove()` and adding
>>> `devm_add_action_or_reset(...deactivate)` before the IRQ is registered.
>> Shared interrupt might be triggered easily by other device between
>> remove() and irq release function (devm_free_irq() or whatever it is
>> called).
> 
> This is no different tham a non-shared interrupt that can be triggered
> by the device being removed. Since devres will release the IRQ first,
> before freeing the driver data, the interrupt hander will see consistent
> driver-internal state. (The difference between remove() and devres
> release phase is that for the latter sysfs files are already removed.)

True, therefore non-devm interrupts are recommended also in such case.
Maybe one of my solutions is actually not recommended.

However if done right, driver with non-shared interrupts, is expected to
disable interrupts in remove(), thus there is no risk. We have big
discussions in the past about it, so feel free to dig through LKML to
read more about. Anyway shared and devm is a clear no go.

Best regards,
Krzysztof

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

[Michał Mirosław]

On Mon, Jul 31, 2023 at 02:59:41PM +0200, Krzysztof Kozlowski wrote:
> On 31/07/2023 10:49, Michał Mirosław wrote:
> > On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote:
> >> On 30/07/2023 23:55, Michał Mirosław wrote:
> >>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
> >>>> 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.
> >>> [...] 
> >>>>> +	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)
> >>>
> >>> Could you elaborate on what is unsafe in using devm with shared
> >>> interrupts (as compared to non-shared or not devm-managed)?
> >>>
> >>> The remove function is indeed reversing the order of cleanup. The
> >>> shutdown path can be fixed by removing `remove()` and adding
> >>> `devm_add_action_or_reset(...deactivate)` before the IRQ is registered.
> >> Shared interrupt might be triggered easily by other device between
> >> remove() and irq release function (devm_free_irq() or whatever it is
> >> called).
> > 
> > This is no different tham a non-shared interrupt that can be triggered
> > by the device being removed. Since devres will release the IRQ first,
> > before freeing the driver data, the interrupt hander will see consistent
> > driver-internal state. (The difference between remove() and devres
> > release phase is that for the latter sysfs files are already removed.)
> 
> True, therefore non-devm interrupts are recommended also in such case.
> Maybe one of my solutions is actually not recommended.
> 
> However if done right, driver with non-shared interrupts, is expected to
> disable interrupts in remove(), thus there is no risk. We have big
> discussions in the past about it, so feel free to dig through LKML to
> read more about. Anyway shared and devm is a clear no go.

Can you share pointers to some of those discussions? Quick search
about devm_request_irq() and friends found only a thread from 2013
about conversions of RTC drivers to use devres. [1] IIRC the issue was
then that the drivers requested IRQs before fully initializing the state
(as many still do). Back to the original question: what is the risk
in using devres with shared interrupts? (Let's assume the probe() is already
fixed and remove() removed.)

BTW, We have devres doc [2] in the kernel tree that, among other things,
lists IRQs as a managed resource and mentions no warnings nor restictions
for driver authors. I'd expect that if devm_request_threaded_irq() for
shared iterrupts was indeed deprecated, it should be documented in a way
easy to refer to.

[1] https://groups.google.com/g/linux.kernel/c/yi2ueo-sNJs
[2] Documentation/udriver-api/driver-model/devres.rst

Best Regards
Michał Mirosław

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

[Michał Mirosław]

On Mon, Jul 31, 2023 at 12:11:47AM +0200, Michał Mirosław wrote:
> On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote:
> > On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote:
> > > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
> [...]
> > > +{
> > > +	int ret;
> > > +
> > > +	if (priv->adap.algo_data)
> > > +		return 0;
> [...]
> > > +	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?
> 
> The code above does "if we added the adapter, remember we did so".
> IOW, if we failed to add the adapter we don't set the mark so that
> the next interrupt edge can trigger another try. Also we prevent
> trying to remove an adapter we didn't successfully add.

Maybe the function's name is misleading? We could find a better one.
Activation/deactivation in this driver means "initialize/shutdown the
hotplugged bus" and is done in response to an edge (triggering an
interrupt) of the hotplug-detect signal.

Best Regards
Michał Mirosław

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

[Andi Shyti]

On Tue, Aug 01, 2023 at 01:01:43AM +0200, Michał Mirosław wrote:
> On Mon, Jul 31, 2023 at 12:11:47AM +0200, Michał Mirosław wrote:
> > On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote:
> > > On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote:
> > > > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
> > [...]
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (priv->adap.algo_data)
> > > > +		return 0;
> > [...]
> > > > +	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?
> > 
> > The code above does "if we added the adapter, remember we did so".
> > IOW, if we failed to add the adapter we don't set the mark so that
> > the next interrupt edge can trigger another try. Also we prevent
> > trying to remove an adapter we didn't successfully add.
> 
> Maybe the function's name is misleading? We could find a better one.
> Activation/deactivation in this driver means "initialize/shutdown the
> hotplugged bus" and is done in response to an edge (triggering an
> interrupt) of the hotplug-detect signal.

So that algo_data is randomly chosen as a boolean value given the
fact that this particular driver doesn't have its own algorithms
but it's using the ones from the parent. Right?

If so, can we have a different and more meaningful boolean value
for this?

And... thinking aloud... are there race conditions here? I
mean... you can't attach two docking stations, but are there
other scenarios?

Thanks,
Andi

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

[Krzysztof Kozlowski]

On 01/08/2023 00:50, Michał Mirosław wrote:
> On Mon, Jul 31, 2023 at 02:59:41PM +0200, Krzysztof Kozlowski wrote:
>> On 31/07/2023 10:49, Michał Mirosław wrote:
>>> On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote:
>>>> On 30/07/2023 23:55, Michał Mirosław wrote:
>>>>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
>>>>>> 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.
>>>>> [...] 
>>>>>>> +	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)
>>>>>
>>>>> Could you elaborate on what is unsafe in using devm with shared
>>>>> interrupts (as compared to non-shared or not devm-managed)?
>>>>>
>>>>> The remove function is indeed reversing the order of cleanup. The
>>>>> shutdown path can be fixed by removing `remove()` and adding
>>>>> `devm_add_action_or_reset(...deactivate)` before the IRQ is registered.
>>>> Shared interrupt might be triggered easily by other device between
>>>> remove() and irq release function (devm_free_irq() or whatever it is
>>>> called).
>>>
>>> This is no different tham a non-shared interrupt that can be triggered
>>> by the device being removed. Since devres will release the IRQ first,
>>> before freeing the driver data, the interrupt hander will see consistent
>>> driver-internal state. (The difference between remove() and devres
>>> release phase is that for the latter sysfs files are already removed.)
>>
>> True, therefore non-devm interrupts are recommended also in such case.
>> Maybe one of my solutions is actually not recommended.
>>
>> However if done right, driver with non-shared interrupts, is expected to
>> disable interrupts in remove(), thus there is no risk. We have big
>> discussions in the past about it, so feel free to dig through LKML to
>> read more about. Anyway shared and devm is a clear no go.
> 
> Can you share pointers to some of those discussions? Quick search
> about devm_request_irq() and friends found only a thread from 2013

Just look at CONFIG_DEBUG_SHIRQ. Some things lore points:
https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/
https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/

I think pretty clear:
https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/
https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/

Also:
https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/
https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/

> about conversions of RTC drivers to use devres. [1] IIRC the issue was
> then that the drivers requested IRQs before fully initializing the state
> (as many still do). Back to the original question: what is the risk
> in using devres with shared interrupts? (Let's assume the probe() is already
> fixed and remove() removed.)



> 
> BTW, We have devres doc [2] in the kernel tree that, among other things,
> lists IRQs as a managed resource and mentions no warnings nor restictions
> for driver authors. I'd expect that if devm_request_threaded_irq() for
> shared iterrupts was indeed deprecated, it should be documented in a way
> easy to refer to.
> 
> [1] https://groups.google.com/g/linux.kernel/c/yi2ueo-sNJs
> [2] Documentation/udriver-api/driver-model/devres.rst

That's not really an argument. For some reason we have
CONFIG_DEBUG_SHIRQ, right? If you think documentation is missing,
everyone is encouraged to fix it, but lack of documentation is not a
proof of some correct code pattern.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

[Krzysztof Kozlowski]

On 29/07/2023 18:08, Svyatoslav Ryhel wrote:
> Document device tree schema which describes hot-pluggable via GPIO
> i2c bus.
> 

A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.

> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/i2c/i2c-hotplug-gpio.yaml        | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> new file mode 100644
> index 000000000000..21f2b74ca6c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-hotplug-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO detected hot-plugged I2C bus
> +
> +maintainers:
> +  - Michał Mirosław <mirq-linux@rere.qmqm.pl>
> +
> +description:
> +  Driver for hot-plugged I2C busses, where some devices on a bus
> +  are hot-pluggable and their presence is indicated by GPIO line.
> +
> +properties:
> +  compatible:
> +    items:

Drop items.

> +      - const: i2c-hotplug-gpio
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0

Why do you need these two? You do not have any children.
> +
> +  interrupts-extended:

Just interrupts.

> +    minItems: 1

maxItems instead

> +
> +  detect-gpios:
> +    maxItems: 1
> +
> +  i2c-parent:
> +    maxItems: 1

Where is the type defined? Why this has maxItems? Is it an array?

> +
> +required:
> +  - compatible
> +  - '#address-cells'
> +  - '#size-cells'
> +  - interrupts-extended
> +  - detect-gpios
> +  - i2c-parent
> +
> +unevaluatedProperties: false

Without any $ref, this should be additionalProperties: false.

> +
> +examples:
> +  - |
> +    /*
> +     * Asus Transformers use I2C hotplug for attachable dock keyboard
> +     */
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c-dock {
> +        compatible = "i2c-hotplug-gpio";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        interrupts-extended = <&gpio 164 IRQ_TYPE_EDGE_BOTH>;
> +        detect-gpios = <&gpio 164 GPIO_ACTIVE_LOW>;
> +
> +        i2c-parent = <&gen2_i2c>;

So do you expect here any children or not?

> +    };
> +...

Best regards,
Krzysztof

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

[Michał Mirosław]

On Sat, Aug 05, 2023 at 09:17:50PM +0200, Krzysztof Kozlowski wrote:
> On 01/08/2023 00:50, Michał Mirosław wrote:
> > On Mon, Jul 31, 2023 at 02:59:41PM +0200, Krzysztof Kozlowski wrote:
> >> On 31/07/2023 10:49, Michał Mirosław wrote:
> >>> On Mon, Jul 31, 2023 at 08:58:14AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 30/07/2023 23:55, Michał Mirosław wrote:
> >>>>> On Sun, Jul 30, 2023 at 10:30:56PM +0200, Krzysztof Kozlowski wrote:
> >>>>>> 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.
> >>>>> [...] 
> >>>>>>> +	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)
[...]
> >> True, therefore non-devm interrupts are recommended also in such case.
> >> Maybe one of my solutions is actually not recommended.
> >>
> >> However if done right, driver with non-shared interrupts, is expected to
> >> disable interrupts in remove(), thus there is no risk. We have big
> >> discussions in the past about it, so feel free to dig through LKML to
> >> read more about. Anyway shared and devm is a clear no go.
> > 
> > Can you share pointers to some of those discussions? Quick search
> > about devm_request_irq() and friends found only a thread from 2013
> 
> Just look at CONFIG_DEBUG_SHIRQ. Some things lore points:
> https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/
> https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/
> 
> I think pretty clear:
> https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/
> https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/
> 
> Also:
> https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/
> https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/
[...]

Thanks! It all looks like a proof by example [1]: a broken driver [2]
was converted to devres [3] and allowed a shared interrupt [4] and now is
used to back an argument that devres and/or shared IRQs are bad. I have
a hard time accepting this line of reasoning.

So: sure, if you disable device's clock, you should first disable the
interrupt handler one way or another, and if you request a shared interrupt
then you have to write the handler expecting spurious invocations anytime
between entry to register_irq() and return from free_irq() (BTW, DEBUG_SHIRQ
is here to help test exactly this). And, when used correctly, devres can
release you from having to write remove() and error paths (but I guess it
might be a challenge to find a single driver that is a complete, good and
complex-enough example).

Coming back from the digression: I gathered following items from the
review of the i2c-hotplug-gpio driver:

  1. TODO: register i2c_hotplug_deactivate(priv) using
     devm_add_action_or_reset() before registering the IRQ handler
     and remove remove();

  2. shared IRQ: it is expected to be an edge-triggered, rarely
     signalled interrupt and the handler will work fine if called
     spuriously; it is not required to be shared for my Transformer,
     but I can't say much about other hardware. Would a comment help?

  3. TODO: DT-binding needs an expanded example and fixing some schema issues;

  4. question from Andi in another thread: I'll answer shortly.

Please correct me if I missed something.

Best Regards
Michał Mirosław

[1] https://en.wikipedia.org/wiki/Proof_by_example
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa11e38ce6fe8846fec046a95cecd5d4690c48cd
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f8a3e7fd5bd08e3fd9847c04a5a445e2994f6b3
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df0a2fdab0068f7452bf0a97ea9ba0ad69d49a1f

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

[Michał Mirosław]

On Sat, Aug 05, 2023 at 01:45:53AM +0200, Andi Shyti wrote:
> On Tue, Aug 01, 2023 at 01:01:43AM +0200, Michał Mirosław wrote:
> > On Mon, Jul 31, 2023 at 12:11:47AM +0200, Michał Mirosław wrote:
> > > On Sun, Jul 30, 2023 at 10:25:07PM +0200, Andi Shyti wrote:
> > > > On Sat, Jul 29, 2023 at 07:08:57PM +0300, Svyatoslav Ryhel wrote:
> > > > > +static int i2c_hotplug_activate(struct i2c_hotplug_priv *priv)
> > > [...]
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	if (priv->adap.algo_data)
> > > > > +		return 0;
> > > [...]
> > > > > +	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?
> > > 
> > > The code above does "if we added the adapter, remember we did so".
> > > IOW, if we failed to add the adapter we don't set the mark so that
> > > the next interrupt edge can trigger another try. Also we prevent
> > > trying to remove an adapter we didn't successfully add.
> > 
> > Maybe the function's name is misleading? We could find a better one.
> > Activation/deactivation in this driver means "initialize/shutdown the
> > hotplugged bus" and is done in response to an edge (triggering an
> > interrupt) of the hotplug-detect signal.
> 
> So that algo_data is randomly chosen as a boolean value given the
> fact that this particular driver doesn't have its own algorithms
> but it's using the ones from the parent. Right?
[...]

Not exactly.  There is an 'algorithm for this driver just forwards the calls to
the parent bus and has no use of the algo_data field.  The field is thus
used to store a flag noting whether the child bus was registered or not.

> And... thinking aloud... are there race conditions here? I
> mean... you can't attach two docking stations, but are there
> other scenarios?

The driver depends on I2C core code synchronization (e.g. i2c_del_adapter()
waiting for ongoing transfers). Outside of probe/remove there is only
a single thread used by the driver: the interrupt handler.

While reading to answer your question I noticed that IRQF_ONESHOT can be
removed: if the thread picks up the signal then it atomically clears the
trigger flag; if another signal arrives before the handler is done,
handler will be called again.

Best Regards,
Michał Mirosław

Re: [PATCH v3 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

[Rob Herring]

On Sat, Jul 29, 2023 at 07:08:56PM +0300, Svyatoslav Ryhel wrote:
> Document device tree schema which describes hot-pluggable via GPIO
> i2c bus.

What's that? 'hot-pluggable via GPIO i2c bus' is not coherent.

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../bindings/i2c/i2c-hotplug-gpio.yaml        | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> new file mode 100644
> index 000000000000..21f2b74ca6c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-hotplug-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO detected hot-plugged I2C bus
> +
> +maintainers:
> +  - Michał Mirosław <mirq-linux@rere.qmqm.pl>
> +
> +description:
> +  Driver for hot-plugged I2C busses, where some devices on a bus

Bindings are for h/w, not a driver.

> +  are hot-pluggable and their presence is indicated by GPIO line.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: i2c-hotplug-gpio
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0

What are these for? You don't have any child nodes.

> +
> +  interrupts-extended:
> +    minItems: 1
> +
> +  detect-gpios:
> +    maxItems: 1
> +
> +  i2c-parent:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - '#address-cells'
> +  - '#size-cells'
> +  - interrupts-extended
> +  - detect-gpios
> +  - i2c-parent
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    /*
> +     * Asus Transformers use I2C hotplug for attachable dock keyboard
> +     */
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c-dock {
> +        compatible = "i2c-hotplug-gpio";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        interrupts-extended = <&gpio 164 IRQ_TYPE_EDGE_BOTH>;
> +        detect-gpios = <&gpio 164 GPIO_ACTIVE_LOW>;

Looks like the same signal. You should only need one of them. Probably 
'detect-gpios' as I guess you need to read the state of the line.

> +
> +        i2c-parent = <&gen2_i2c>;

Couldn't you just add 'detect-gpios' to the existing bus node? It's 
really part of that bus and there's not a separate bus. That would be a 
lot simpler. I suppose you want to instantiate a driver, but that's not 
DT's problem.

Rob

Re: [PATCH v3 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

[Michał Mirosław]

On Fri, Aug 11, 2023 at 11:37:52AM -0600, Rob Herring wrote:
> On Sat, Jul 29, 2023 at 07:08:56PM +0300, Svyatoslav Ryhel wrote:
[...]
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-hotplug-gpio.yaml
[...]
> > +  - |
> > +    /*
> > +     * Asus Transformers use I2C hotplug for attachable dock keyboard
> > +     */
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c-dock {
> > +        compatible = "i2c-hotplug-gpio";
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
[...]
> > +        i2c-parent = <&gen2_i2c>;
> 
> Couldn't you just add 'detect-gpios' to the existing bus node? It's 
> really part of that bus and there's not a separate bus. That would be a 
> lot simpler. I suppose you want to instantiate a driver, but that's not 
> DT's problem.

Not a driver but a group of devices (possibly discovered dynamically)
behind a passive gate (like e.g. PCA9517A with EN tied to a connector pin).
It's not much different to a I2C gate or mux with a single child bus
(i2c-mux-gpio that has only a single child). For ASUS Transformers with
only the dock plugged-in it could work with a 'detect-gpios' extension
(I'll take a look at how this way would work).  I think there were also
different attachments made you could connect instead of the dock.

Best Regards
Michał Mirosław

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

[Krzysztof Kozlowski]

On 10/08/2023 23:52, Michał Mirosław wrote:
>>>>>>>> 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)
> [...]
>>>> True, therefore non-devm interrupts are recommended also in such case.
>>>> Maybe one of my solutions is actually not recommended.
>>>>
>>>> However if done right, driver with non-shared interrupts, is expected to
>>>> disable interrupts in remove(), thus there is no risk. We have big
>>>> discussions in the past about it, so feel free to dig through LKML to
>>>> read more about. Anyway shared and devm is a clear no go.
>>>
>>> Can you share pointers to some of those discussions? Quick search
>>> about devm_request_irq() and friends found only a thread from 2013
>>
>> Just look at CONFIG_DEBUG_SHIRQ. Some things lore points:
>> https://lore.kernel.org/all/1592130544-19759-2-git-send-email-krzk@kernel.org/
>> https://lore.kernel.org/all/20200616103956.GL4447@sirena.org.uk/
>>
>> I think pretty clear:
>> https://lore.kernel.org/all/87mu52ca4b.fsf@nanos.tec.linutronix.de/
>> https://lore.kernel.org/all/CA+h21hrxQ1fRahyQGFS42Xuop_Q2petE=No1dft4nVb-ijUu2g@mail.gmail.com/
>>
>> Also:
>> https://lore.kernel.org/all/651c9a33-71e6-c042-58e2-6ad501e984cd@pengutronix.de/
>> https://lore.kernel.org/all/36AC4067-78C6-4986-8B97-591F93E266D8@gmail.com/
> [...]
> 
> Thanks! It all looks like a proof by example [1]: a broken driver [2]
> was converted to devres [3] and allowed a shared interrupt [4] and now is
> used to back an argument that devres and/or shared IRQs are bad. I have
> a hard time accepting this line of reasoning.
> 
> So: sure, if you disable device's clock, you should first disable the
> interrupt handler one way or another, and if you request a shared interrupt
> then you have to write the handler expecting spurious invocations anytime
> between entry to register_irq() and return from free_irq() (BTW, DEBUG_SHIRQ
> is here to help test exactly this). And, when used correctly, devres can
> release you from having to write remove() and error paths (but I guess it
> might be a challenge to find a single driver that is a complete, good and
> complex-enough example).
> 
> Coming back from the digression: I gathered following items from the
> review of the i2c-hotplug-gpio driver:
> 
>   1. TODO: register i2c_hotplug_deactivate(priv) using
>      devm_add_action_or_reset() before registering the IRQ handler
>      and remove remove();
> 
>   2. shared IRQ: it is expected to be an edge-triggered, rarely
>      signalled interrupt and the handler will work fine if called
>      spuriously; it is not required to be shared for my Transformer,
>      but I can't say much about other hardware. Would a comment help?

We have way too lengthy discussion and now we are circling back. Can you
refer to the first email I wrote?

"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

Re: [PATCH v3 1/2] dt-bindings: i2c: add binding for i2c-hotplug-gpio

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]


> Not a driver but a group of devices (possibly discovered dynamically)
> behind a passive gate (like e.g. PCA9517A with EN tied to a connector pin).
> It's not much different to a I2C gate or mux with a single child bus
> (i2c-mux-gpio that has only a single child).

I agree. To prevent the the bus from spikes when connecting /
disconnecting something like PCA9517A should be in place. And this one
can be considered a 1:1 mux. Just with the exception that Linux cannot
control the gate, but can only react to changes to the enable pin.

Can't we have a driver for the PCA9517 which gets interrupts when the
enable pin changes and then adds / removes the child i2c adapter?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]