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

[PATCH v2 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.

---
Changes in v2:
- adjusted documentation
---

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 v2 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

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

[Krzysztof Kozlowski]

On 19/06/2023 17:37, Svyatoslav Ryhel wrote:
> Document device tree schema which describes hot-pluggable via GPIO
> i2c bus.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>

1. Don't send new version before discussion finishes. v2 after one hour
is definitely not enough. Usually one day.

2. Test the patches before sending...

What changed here?

> ---
>  .../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

"Driver" so SW? Bindings are for hardware, not for drivers.

> +  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
> +
> +  interrupts-extended:

I missed last time - this is wrong. Just interrupts.

> +    minItems: 1

maxItems

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

Discussion from v1 stands - this is a software construct, not a real device.

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

instead additionalProperties: false

Anyway, don't send v3, before the discussion about the entire concept
finishes. You create a software/virtual device, instead of adding these
properties to bindings for a real hardware.

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

I don't think you can have both interrupt and GPIO on the same line.

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

Best regards,
Krzysztof

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

[Michał Mirosław]

On Mon, Jun 19, 2023 at 05:53:24PM +0200, Krzysztof Kozlowski wrote:
> On 19/06/2023 17:37, Svyatoslav Ryhel wrote:
> > Document device tree schema which describes hot-pluggable via GPIO
> > i2c bus.
> > 
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
[...]
> > --- /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
> 
> "Driver" so SW? Bindings are for hardware, not for drivers.
[...]
> > +  detect-gpios:
> > +    maxItems: 1
> > +
> > +  i2c-parent:
> > +    maxItems: 1
> 
> Discussion from v1 stands - this is a software construct, not a real device.
[...]
> Anyway, don't send v3, before the discussion about the entire concept
> finishes. You create a software/virtual device, instead of adding these
> properties to bindings for a real hardware.

Hi,

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

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
2-port mux with enable joining two I2C busses (one "closer" to the
CPU -- parent).

> > +
> > +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>;
> 
> I don't think you can have both interrupt and GPIO on the same line.

This actually works as expected. There are multiple devices (and
drivers) that depend on this, e.g. matrix-keypad and gpio-keys.

Best Regards
Michał Mirosław

[PATCH v2 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.

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 v2 2/2] i2c: Add GPIO-based hotplug gate

[Andi Shyti]

Hi,

On Mon, Jun 19, 2023 at 06:37:32PM +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.
> 
> 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

without going through the code I am missing the big picture here.

What is this actually doing? Is this a new bus driver support? Is
this a feature to existing drivers? Is the GPIO an irq line for
signalling hoplugging and can be used by any driver or just this
one?

Without further discussing technicalities, can you please explain
better and more in detail what is the scope of this patch, why
there is a need for such a patch, how this new driver/feature
has been implemented and finally how it can be used.

This would help a lot so that I know already beforehand what I am
going to read without figuring it out.

Thanks,
Andi

PS Please notice that my set of questions is even longer than
your commit log :)

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

[Svyatoslav Ryhel]

ср, 21 черв. 2023 р. о 13:32 Andi Shyti <andi.shyti@kernel.org> пише:
>
> Hi,
>
> On Mon, Jun 19, 2023 at 06:37:32PM +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.
> >
> > 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
>
> without going through the code I am missing the big picture here.
>
> 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 detach signal all hot-pluggable devices are unprobed and
bus removed.

> Is this a new bus driver support?

Most likely not.

> Is this a feature to existing drivers?

Yes, it is more like i2c mux

> 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.

> Without further discussing technicalities, can you please explain
> better and more in detail what is the scope of this patch, why
> there is a need for such a patch, how this new driver/feature
> has been implemented and finally how it can be used.

This patch is a predecessor of a possible larger patchset which
should bring support for a asus-ec, a i2c mfd device programmed by
Asus for their Transformers tablet line.

This is Michał Mirosław, original author quote about this driver:
"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
2-port mux with enable joining two I2C busses (one "closer" to the
CPU -- parent)."

Similar approach is used in Microsoft Surface RT for attachable
Type Cover.

> This would help a lot so that I know already beforehand what I am
> going to read without figuring it out.
>
> Thanks,
> Andi
>
> PS Please notice that my set of questions is even longer than
> your commit log :)

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

[Krzysztof Kozlowski]

On 19/06/2023 17:37, 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.
> 
> ---
> Changes in v2:
> - adjusted documentation

This is too generic. Everything can be adjustment. Be precise what you
did here. What changed?

Best regards,
Krzysztof

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

[Svyatoslav Ryhel]

пн, 19 черв. 2023 р. о 18:49 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 19/06/2023 17:37, 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.
> >
> > ---
> > Changes in v2:
> > - adjusted documentation
>
> This is too generic. Everything can be adjustment. Be precise what you
> did here. What changed?
>

Everything that you asked for.

> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 19/06/2023 17:52, Svyatoslav Ryhel wrote:
> пн, 19 черв. 2023 р. о 18:49 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 19/06/2023 17:37, 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.
>>>
>>> ---
>>> Changes in v2:
>>> - adjusted documentation
>>
>> This is too generic. Everything can be adjustment. Be precise what you
>> did here. What changed?
>>
> 
> Everything that you asked for.

And how other people can know it? That's not a proper changelog.

What changed? If you are going to ignore writing proper changelogs, I am
going to ignore patches.

Best regards,
Krzysztof

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

[Andi Shyti]

Hi Svyatoslav,

On Mon, Jun 19, 2023 at 06:52:50PM +0300, Svyatoslav Ryhel wrote:
> пн, 19 черв. 2023 р. о 18:49 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
> >
> > On 19/06/2023 17:37, 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.
> > >
> > > ---
> > > Changes in v2:
> > > - adjusted documentation
> >
> > This is too generic. Everything can be adjustment. Be precise what you
> > did here. What changed?
> >
> 
> Everything that you asked for.

please list all the changes you made trying to be as more
specific as you can.

You make life to reviewers easier.

Thanks,
Andi