[PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

The main idea of this patchset was to add the dt-based GPIOs support
in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
driver implementation didn't provide this ability.

On the way of adding the full dt-GPIO flags support a small set of
refactorings has been done in order to keep the platform_data-based
systems support, make the code more readable and the alterations - clearer.
In general the whole changes might be considered as the plat- and dt-
specific code split up. In first patch we unpinned the platform-specific
method of GPIO-chip probing. The second patch makes the driver to return
an error if of-based (last resort path) failed to retrieve the driver
private data. The next three patches is the sequence of initial channel
info retrieval, platform_data-specific code isolation and finally full
dt-based GPIOs request method introduction. The last patch does what
we inteded this patchset for in the first place - adds the full dt-GPIO
specifiers support.


Serge Semin (5):
  i2c-mux-gpio: Unpin a platform-based device initialization
  i2c-mux-gpio: Return an error if no config data found
  i2c-mux-gpio: Save initial channel number to the idle data field
  i2c-mux-gpio: Unpin the platform-specific GPIOs request code
  i2c-mux-gpio: Create of-based GPIOs request method

 drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
 1 file changed, 146 insertions(+), 78 deletions(-)

-- 
2.21.0

[PATCH 1/5] i2c-mux-gpio: Unpin a platform-based device initialization

[Serge Semin]

We can unpin a code specific for i2c-mux-gpio device declared
as platform device. In this case the platform data just needs to be
copied to the private storage and if GPIO chip pointer is referring to
a valid GPIO chip descriptor save it' base number for further GPIOs
request and initialization. The rest of the code is common for both
platform and OF-based setups.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 67 ++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 13882a2a4f60..24cf6ec02e75 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -136,44 +136,51 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 }
 #endif
 
+static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
+	struct gpio_chip *gpio;
+
+	/*
+	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
+	 * relative to its base GPIO number. Otherwise they are absolute.
+	 */
+	if (data->gpio_chip) {
+		gpio = gpiochip_find(data->gpio_chip,
+				     match_gpio_chip_by_label);
+		if (!gpio)
+			return -EPROBE_DEFER;
+
+		mux->gpio_base = gpio->base;
+	} else {
+		mux->gpio_base = 0;
+	}
+
+	memcpy(&mux->data, data, sizeof(mux->data));
+
+	return 0;
+}
+
 static int i2c_mux_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc;
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
 	struct i2c_adapter *root;
-	unsigned initial_state, gpio_base;
+	unsigned initial_state;
 	int i, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
 	if (!mux)
 		return -ENOMEM;
 
-	if (!dev_get_platdata(&pdev->dev)) {
+	if (!dev_get_platdata(&pdev->dev))
 		ret = i2c_mux_gpio_probe_dt(mux, pdev);
-		if (ret < 0)
-			return ret;
-	} else {
-		memcpy(&mux->data, dev_get_platdata(&pdev->dev),
-			sizeof(mux->data));
-	}
-
-	/*
-	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
-	 * relative to its base GPIO number. Otherwise they are absolute.
-	 */
-	if (mux->data.gpio_chip) {
-		struct gpio_chip *gpio;
-
-		gpio = gpiochip_find(mux->data.gpio_chip,
-				     match_gpio_chip_by_label);
-		if (!gpio)
-			return -EPROBE_DEFER;
-
-		gpio_base = gpio->base;
-	} else {
-		gpio_base = 0;
-	}
+	else
+		ret = i2c_mux_gpio_probe_plat(mux, pdev);
+	if (ret < 0)
+		return ret;
 
 	parent = i2c_get_adapter(mux->data.parent);
 	if (!parent)
@@ -194,7 +201,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	root = i2c_root_adapter(&parent->dev);
 
 	muxc->mux_locked = true;
-	mux->gpio_base = gpio_base;
 
 	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
 		initial_state = mux->data.idle;
@@ -207,14 +213,15 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		struct device *gpio_dev;
 		struct gpio_desc *gpio_desc;
 
-		ret = gpio_request(gpio_base + mux->data.gpios[i], "i2c-mux-gpio");
+		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
+				   "i2c-mux-gpio");
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
 				mux->data.gpios[i]);
 			goto err_request_gpio;
 		}
 
-		ret = gpio_direction_output(gpio_base + mux->data.gpios[i],
+		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
 					    initial_state & (1 << i));
 		if (ret) {
 			dev_err(&pdev->dev,
@@ -224,7 +231,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 			goto err_request_gpio;
 		}
 
-		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
+		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
 		mux->gpios[i] = gpio_desc;
 
 		if (!muxc->mux_locked)
@@ -256,7 +263,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	i = mux->data.n_gpios;
 err_request_gpio:
 	for (; i > 0; i--)
-		gpio_free(gpio_base + mux->data.gpios[i - 1]);
+		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
 alloc_failed:
 	i2c_put_adapter(parent);
 
-- 
2.21.0

[PATCH 2/5] i2c-mux-gpio: Return an error if no config data found

[Serge Semin]

It's pointless and might be even errors prone to proceed with further
initialization if neither of- no platform-based settings were discovered.
Just return an error in this case.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 24cf6ec02e75..a14fe132b0c3 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -132,7 +132,7 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 					struct platform_device *pdev)
 {
-	return 0;
+	return -EINVAL;
 }
 #endif
 
@@ -142,6 +142,9 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
 	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
 	struct gpio_chip *gpio;
 
+	if (!data)
+		return -EINVAL;
+
 	/*
 	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
 	 * relative to its base GPIO number. Otherwise they are absolute.
@@ -175,11 +178,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	if (!mux)
 		return -ENOMEM;
 
-	if (!dev_get_platdata(&pdev->dev))
+	ret = i2c_mux_gpio_probe_plat(mux, pdev);
+	if (ret)
 		ret = i2c_mux_gpio_probe_dt(mux, pdev);
-	else
-		ret = i2c_mux_gpio_probe_plat(mux, pdev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	parent = i2c_get_adapter(mux->data.parent);
-- 
2.21.0

[PATCH 3/5] i2c-mux-gpio: Save initial channel number to the idle data field

[Serge Semin]

In case if the idle state has been specified in the data structure,
the idle variable is left untouched as before, so to keep a default
channel number enabled in the mux idle state. But if a platform doesn't
specify which channel is going to be enabled by default, we as before
don't setup the deselect callback, but the initial state is saved in the
idle variable for further initialization. We can safely do this here
since that variable is used for initial state setting only, when no
idling lane is specified.

The reason of this change is to prepare the code for future GPIOs request
path being split up into of- and plat- based methods. The idle variable
here is used as a container of the initial state for both of the paths in
case of idle-channel isn't specified.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index a14fe132b0c3..535c83c43371 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -171,7 +171,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
 	struct i2c_adapter *root;
-	unsigned initial_state;
 	int i, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
@@ -204,12 +203,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 	muxc->mux_locked = true;
 
-	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
-		initial_state = mux->data.idle;
+	/*
+	 * Set descelect callback if idle state has been setup otherwise just
+	 * use the idle variable to store the initial muxer value.
+	 */
+	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE)
 		muxc->deselect = i2c_mux_gpio_deselect;
-	} else {
-		initial_state = mux->data.values[0];
-	}
+	else
+		mux->data.idle = mux->data.values[0];
 
 	for (i = 0; i < mux->data.n_gpios; i++) {
 		struct device *gpio_dev;
@@ -224,7 +225,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		}
 
 		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
-					    initial_state & (1 << i));
+					    mux->data.idle & (1 << i));
 		if (ret) {
 			dev_err(&pdev->dev,
 				"Failed to set direction of GPIO %d to output\n",
-- 
2.21.0

[PATCH 4/5] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Serge Semin]

The GPIOs request loop can be safely moved to a separate function.
First of all it shall improve the code readability. Secondly the
initialization loop at this point is used for both of- and
platform_data-based initialization paths, but it will be changed in
the next patch, so by isolating the code we'll simplify the future
work.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 105 +++++++++++++++++++------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 535c83c43371..317c019e1415 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -165,12 +165,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
 	return 0;
 }
 
+static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+	struct gpio_desc *gpio_desc;
+	struct i2c_adapter *root;
+	struct device *gpio_dev;
+	int i, ret;
+
+	root = i2c_root_adapter(&muxc->parent->dev);
+
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
+				   "i2c-mux-gpio");
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
+				mux->data.gpios[i]);
+			goto err_request_gpio;
+		}
+
+		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
+					    mux->data.idle & (1 << i));
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to set direction of GPIO %d to output\n",
+				mux->data.gpios[i]);
+			i++;	/* gpio_request above succeeded, so must free */
+			goto err_request_gpio;
+		}
+
+		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
+		mux->gpios[i] = gpio_desc;
+
+		if (!muxc->mux_locked)
+			continue;
+
+		gpio_dev = &gpio_desc->gdev->dev;
+		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
+	}
+
+	return 0;
+
+err_request_gpio:
+	for (; i > 0; i--)
+		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
+
+	return ret;
+}
+
+static void i2c_mux_gpio_free(struct gpiomux *mux)
+{
+	int i;
+
+	for (i = 0; i < mux->data.n_gpios; i++)
+		gpiod_free(mux->gpios[i]);
+}
+
 static int i2c_mux_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc;
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
-	struct i2c_adapter *root;
 	int i, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
@@ -199,8 +255,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, muxc);
 
-	root = i2c_root_adapter(&parent->dev);
-
 	muxc->mux_locked = true;
 
 	/*
@@ -212,37 +266,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	else
 		mux->data.idle = mux->data.values[0];
 
-	for (i = 0; i < mux->data.n_gpios; i++) {
-		struct device *gpio_dev;
-		struct gpio_desc *gpio_desc;
-
-		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
-				   "i2c-mux-gpio");
-		if (ret) {
-			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
-				mux->data.gpios[i]);
-			goto err_request_gpio;
-		}
-
-		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
-					    mux->data.idle & (1 << i));
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to set direction of GPIO %d to output\n",
-				mux->data.gpios[i]);
-			i++;	/* gpio_request above succeeded, so must free */
-			goto err_request_gpio;
-		}
-
-		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
-		mux->gpios[i] = gpio_desc;
-
-		if (!muxc->mux_locked)
-			continue;
-
-		gpio_dev = &gpio_desc->gdev->dev;
-		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
-	}
+	ret = i2c_mux_gpio_request_plat(mux, pdev);
+	if (ret)
+		goto alloc_failed;
 
 	if (muxc->mux_locked)
 		dev_info(&pdev->dev, "mux-locked i2c mux\n");
@@ -263,10 +289,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 add_adapter_failed:
 	i2c_mux_del_adapters(muxc);
-	i = mux->data.n_gpios;
-err_request_gpio:
-	for (; i > 0; i--)
-		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
+
+	i2c_mux_gpio_free(mux);
+
 alloc_failed:
 	i2c_put_adapter(parent);
 
@@ -277,12 +302,10 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
 	struct gpiomux *mux = i2c_mux_priv(muxc);
-	int i;
 
 	i2c_mux_del_adapters(muxc);
 
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_free(mux->gpio_base + mux->data.gpios[i]);
+	i2c_mux_gpio_free(mux);
 
 	i2c_put_adapter(muxc->parent);
 
-- 
2.21.0

[PATCH 5/5] i2c-mux-gpio: Create of-based GPIOs request method

[Serge Semin]

Most modern platforms provide a dts with description of the devices
available in the system. It may also include i2c-gpio-mux'es.
Up until now the i2c-mux-gpio driver supported it' dts nodes, but
performed the GPIOs request by means of legacy GPIO API, which by design
and due to being legacy doesn't know anything about of/dtb/fdt/dts stuff.
It means even though the i2c-gpio-mux dts nodes are successfully mapped
to the kernel i2c-mux devices, the GPIOs used for initialization are
requested without OF_GPIO_* flags setup. It causes problems on the
platforms which fully rely on dts and reside, for instance,
i2c-gpio-muxes with active low or open drain GPIOs connected.

It is fixed by implementing a dedicated method for full dts-based
GPIOs requests. It is mostly similar to the platform one, but
utilizes the gpiod_get_from_of_node() method to request the GPIOs.

Finally the platform code i2c-gpio-mux devices are also supported.
So the fallback to dtb is performed only if array with GPIOs isn't
detected.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 65 ++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 317c019e1415..e5e10ba35ad9 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -65,8 +65,8 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *adapter_np, *child;
 	struct i2c_adapter *adapter;
-	unsigned *values, *gpios;
-	int i = 0, ret;
+	unsigned int *values;
+	int i = 0;
 
 	if (!np)
 		return -ENODEV;
@@ -109,24 +109,48 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 		return -EINVAL;
 	}
 
-	gpios = devm_kcalloc(&pdev->dev,
-			     mux->data.n_gpios, sizeof(*mux->data.gpios),
-			     GFP_KERNEL);
-	if (!gpios) {
-		dev_err(&pdev->dev, "Cannot allocate gpios array");
-		return -ENOMEM;
-	}
+	return 0;
+}
+
+static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	struct i2c_adapter *root;
+	struct device *gpio_dev;
+	enum gpiod_flags dflags;
+	int i, ret;
+
+	root = i2c_root_adapter(&muxc->parent->dev);
 
 	for (i = 0; i < mux->data.n_gpios; i++) {
-		ret = of_get_named_gpio(np, "mux-gpios", i);
-		if (ret < 0)
-			return ret;
-		gpios[i] = ret;
-	}
+		if (mux->data.idle & (1 << i))
+			dflags = GPIOD_OUT_HIGH;
+		else
+			dflags = GPIOD_OUT_LOW;
+
+		mux->gpios[i] = gpiod_get_from_of_node(np, "mux-gpios", i,
+						       dflags, "i2c-mux-gpio");
+		if (IS_ERR(mux->gpios[i])) {
+			ret = PTR_ERR(mux->gpios[i]);
+			goto err_request_gpio;
+		}
 
-	mux->data.gpios = gpios;
+		if (!muxc->mux_locked)
+			continue;
+
+		gpio_dev = &mux->gpios[i]->gdev->dev;
+		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
+	}
 
 	return 0;
+
+err_request_gpio:
+	for (i--; i >= 0; i--)
+		gpiod_free(mux->gpios[i]);
+
+	return ret;
 }
 #else
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
@@ -134,6 +158,12 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 {
 	return -EINVAL;
 }
+
+static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	return -EINVAL;
+}
 #endif
 
 static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
@@ -174,6 +204,9 @@ static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
 	struct device *gpio_dev;
 	int i, ret;
 
+	if (!mux->data.gpios)
+		return -EINVAL;
+
 	root = i2c_root_adapter(&muxc->parent->dev);
 
 	for (i = 0; i < mux->data.n_gpios; i++) {
@@ -267,6 +300,8 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		mux->data.idle = mux->data.values[0];
 
 	ret = i2c_mux_gpio_request_plat(mux, pdev);
+	if (ret)
+		ret = i2c_mux_gpio_request_dt(mux, pdev);
 	if (ret)
 		goto alloc_failed;
 
-- 
2.21.0

Re: [PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Peter Rosin]

On 2019-04-24 14:34, Serge Semin wrote:
> The main idea of this patchset was to add the dt-based GPIOs support
> in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
> specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
> GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
> driver implementation didn't provide this ability.

I'm curious why active low/high is of any importance? That will only affect
the state numbering, but I fail to see any relevance in that numbering. It's
just numbers, no?

If all the pins are inverted (anything else seems very strange), just
reverse the order. I.e. for a 4-way mux, use 3, 2, 1, 0 instead of
0, 1, 2, 3.

Why not?

Cheers,
Peter

> On the way of adding the full dt-GPIO flags support a small set of
> refactorings has been done in order to keep the platform_data-based
> systems support, make the code more readable and the alterations - clearer.
> In general the whole changes might be considered as the plat- and dt-
> specific code split up. In first patch we unpinned the platform-specific
> method of GPIO-chip probing. The second patch makes the driver to return
> an error if of-based (last resort path) failed to retrieve the driver
> private data. The next three patches is the sequence of initial channel
> info retrieval, platform_data-specific code isolation and finally full
> dt-based GPIOs request method introduction. The last patch does what
> we inteded this patchset for in the first place - adds the full dt-GPIO
> specifiers support.
> 
> 
> Serge Semin (5):
>   i2c-mux-gpio: Unpin a platform-based device initialization
>   i2c-mux-gpio: Return an error if no config data found
>   i2c-mux-gpio: Save initial channel number to the idle data field
>   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>   i2c-mux-gpio: Create of-based GPIOs request method
> 
>  drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
>  1 file changed, 146 insertions(+), 78 deletions(-)
> 

Re: [PATCH 2/5] i2c-mux-gpio: Return an error if no config data found

[Peter Rosin]

On 2019-04-24 14:34, Serge Semin wrote:
> It's pointless and might be even errors prone to proceed with further
> initialization if neither of- no platform-based settings were discovered.
> Just return an error in this case.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 24cf6ec02e75..a14fe132b0c3 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -132,7 +132,7 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  					struct platform_device *pdev)
>  {
> -	return 0;
> +	return -EINVAL;
>  }
>  #endif
>  
> @@ -142,6 +142,9 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
>  	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
>  	struct gpio_chip *gpio;
>  
> +	if (!data)
> +		return -EINVAL;
> +
>  	/*
>  	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
>  	 * relative to its base GPIO number. Otherwise they are absolute.
> @@ -175,11 +178,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	if (!mux)
>  		return -ENOMEM;
>  
> -	if (!dev_get_platdata(&pdev->dev))
> +	ret = i2c_mux_gpio_probe_plat(mux, pdev);
> +	if (ret)
>  		ret = i2c_mux_gpio_probe_dt(mux, pdev);
> -	else
> -		ret = i2c_mux_gpio_probe_plat(mux, pdev);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;

I notice that after this patch, all probe failures from non-dt configs
will return -EINVAL from the dummy i2c_mux_gpio_probe_dt that gets
called on i2c_mux_gpio_probe_plat failure.

So, any -EPROBE_DEFER is now lost. That probably doesn't fly.

Cheers,
Peter

>  
>  	parent = i2c_get_adapter(mux->data.parent);
> 

Re: [PATCH 3/5] i2c-mux-gpio: Save initial channel number to the idle data field

[Peter Rosin]

On 2019-04-24 14:34, Serge Semin wrote:
> In case if the idle state has been specified in the data structure,
> the idle variable is left untouched as before, so to keep a default
> channel number enabled in the mux idle state. But if a platform doesn't
> specify which channel is going to be enabled by default, we as before
> don't setup the deselect callback, but the initial state is saved in the
> idle variable for further initialization. We can safely do this here
> since that variable is used for initial state setting only, when no
> idling lane is specified.

While this subtlety is *maybe* ok, a comment about it belongs in the
*code* where it will be seen when the next person makes changes.

But why not extend the struct with the initial state? How many of
these muxes do you expect to exist in a system? Multiplied by a
couple of bytes. Who cares?

Cheers,
Peter

> 
> The reason of this change is to prepare the code for future GPIOs request
> path being split up into of- and plat- based methods. The idle variable
> here is used as a container of the initial state for both of the paths in
> case of idle-channel isn't specified.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index a14fe132b0c3..535c83c43371 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -171,7 +171,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	struct gpiomux *mux;
>  	struct i2c_adapter *parent;
>  	struct i2c_adapter *root;
> -	unsigned initial_state;
>  	int i, ret;
>  
>  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> @@ -204,12 +203,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  
>  	muxc->mux_locked = true;
>  
> -	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> -		initial_state = mux->data.idle;
> +	/*
> +	 * Set descelect callback if idle state has been setup otherwise just
> +	 * use the idle variable to store the initial muxer value.
> +	 */
> +	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE)
>  		muxc->deselect = i2c_mux_gpio_deselect;
> -	} else {
> -		initial_state = mux->data.values[0];
> -	}
> +	else
> +		mux->data.idle = mux->data.values[0];
>  
>  	for (i = 0; i < mux->data.n_gpios; i++) {
>  		struct device *gpio_dev;
> @@ -224,7 +225,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  		}
>  
>  		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> -					    initial_state & (1 << i));
> +					    mux->data.idle & (1 << i));
>  		if (ret) {
>  			dev_err(&pdev->dev,
>  				"Failed to set direction of GPIO %d to output\n",
> 

Re: [PATCH 5/5] i2c-mux-gpio: Create of-based GPIOs request method

[Peter Rosin]

On 2019-04-24 14:34, Serge Semin wrote:
> Most modern platforms provide a dts with description of the devices
> available in the system. It may also include i2c-gpio-mux'es.
> Up until now the i2c-mux-gpio driver supported it' dts nodes, but
> performed the GPIOs request by means of legacy GPIO API, which by design
> and due to being legacy doesn't know anything about of/dtb/fdt/dts stuff.
> It means even though the i2c-gpio-mux dts nodes are successfully mapped
> to the kernel i2c-mux devices, the GPIOs used for initialization are
> requested without OF_GPIO_* flags setup. It causes problems on the
> platforms which fully rely on dts and reside, for instance,
> i2c-gpio-muxes with active low or open drain GPIOs connected.
> 
> It is fixed by implementing a dedicated method for full dts-based
> GPIOs requests. It is mostly similar to the platform one, but
> utilizes the gpiod_get_from_of_node() method to request the GPIOs.
> 
> Finally the platform code i2c-gpio-mux devices are also supported.
> So the fallback to dtb is performed only if array with GPIOs isn't
> detected.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 65 ++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 317c019e1415..e5e10ba35ad9 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -65,8 +65,8 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  	struct device_node *np = pdev->dev.of_node;
>  	struct device_node *adapter_np, *child;
>  	struct i2c_adapter *adapter;
> -	unsigned *values, *gpios;
> -	int i = 0, ret;
> +	unsigned int *values;
> +	int i = 0;
>  
>  	if (!np)
>  		return -ENODEV;
> @@ -109,24 +109,48 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  		return -EINVAL;
>  	}
>  
> -	gpios = devm_kcalloc(&pdev->dev,
> -			     mux->data.n_gpios, sizeof(*mux->data.gpios),
> -			     GFP_KERNEL);
> -	if (!gpios) {
> -		dev_err(&pdev->dev, "Cannot allocate gpios array");
> -		return -ENOMEM;
> -	}
> +	return 0;
> +}
> +
> +static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
> +					struct platform_device *pdev)
> +{
> +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct i2c_adapter *root;
> +	struct device *gpio_dev;
> +	enum gpiod_flags dflags;
> +	int i, ret;
> +
> +	root = i2c_root_adapter(&muxc->parent->dev);
>  
>  	for (i = 0; i < mux->data.n_gpios; i++) {
> -		ret = of_get_named_gpio(np, "mux-gpios", i);
> -		if (ret < 0)
> -			return ret;
> -		gpios[i] = ret;
> -	}
> +		if (mux->data.idle & (1 << i))
> +			dflags = GPIOD_OUT_HIGH;
> +		else
> +			dflags = GPIOD_OUT_LOW;
> +
> +		mux->gpios[i] = gpiod_get_from_of_node(np, "mux-gpios", i,
> +						       dflags, "i2c-mux-gpio");
> +		if (IS_ERR(mux->gpios[i])) {
> +			ret = PTR_ERR(mux->gpios[i]);
> +			goto err_request_gpio;
> +		}
>  
> -	mux->data.gpios = gpios;
> +		if (!muxc->mux_locked)
> +			continue;
> +
> +		gpio_dev = &mux->gpios[i]->gdev->dev;
> +		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> +	}
>  
>  	return 0;
> +
> +err_request_gpio:
> +	for (i--; i >= 0; i--)
> +		gpiod_free(mux->gpios[i]);
> +
> +	return ret;
>  }
>  #else
>  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> @@ -134,6 +158,12 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  {
>  	return -EINVAL;
>  }
> +
> +static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
> +					struct platform_device *pdev)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
>  static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> @@ -174,6 +204,9 @@ static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
>  	struct device *gpio_dev;
>  	int i, ret;
>  
> +	if (!mux->data.gpios)
> +		return -EINVAL;
> +
>  	root = i2c_root_adapter(&muxc->parent->dev);
>  
>  	for (i = 0; i < mux->data.n_gpios; i++) {
> @@ -267,6 +300,8 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  		mux->data.idle = mux->data.values[0];
>  
>  	ret = i2c_mux_gpio_request_plat(mux, pdev);
> +	if (ret)
> +		ret = i2c_mux_gpio_request_dt(mux, pdev);
>  	if (ret)
>  		goto alloc_failed;

Same pattern as for 2/5; any and all errors from i2c_mux_gpio_request_plat
are now lost.

Cheers,
Peter

>  
> 

Re: [PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

On Wed, Apr 24, 2019 at 09:25:24PM +0000, Peter Rosin wrote:

Hello Peter,

> On 2019-04-24 14:34, Serge Semin wrote:
> > The main idea of this patchset was to add the dt-based GPIOs support
> > in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
> > specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
> > GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
> > driver implementation didn't provide this ability.
> 
> I'm curious why active low/high is of any importance? That will only affect
> the state numbering, but I fail to see any relevance in that numbering. It's
> just numbers, no?
> 
> If all the pins are inverted (anything else seems very strange), just
> reverse the order. I.e. for a 4-way mux, use 3, 2, 1, 0 instead of
> 0, 1, 2, 3.
> 
> Why not?

I may misunderstood you, but active low/high flag has nothing to do with
pins ordering. It is relevant to an individual pin setting, most likely
related with hardware setup.

Here is a simple example:
i2cmux {
                compatible = "i2c-mux-gpio";
                mux-gpios = <&gpioa 0 GPIO_ACTIVE_LOW
                             &control 2 GPIO_ACTIVE_HIGH
                             &last 5 GPIO_ACTIVE_LOW>;
};

In this setup we've got some i2c-mux with GPIOs-driven channel selector.
First channel is selected by GPIO#0 of controller &gpioa, second one -
by GPIO#2 of controller &control and third - by GPIO#3 of controller
&last. In accordance with the i2c_mux_gpio_set() method of i2c-mux-gpio
driver a GPIO from this set will be driven high in case of a corresponding
mux channel being enabled. But as you can see from the "mux-gpios" property
these GPIOs aren't identical. First of all they belong to different
controller and most importantly they've got different active-attribute.
This attribute actually means the straight or inverted activation policy.
So in case of ACTIVE_HIGH flag you get a straight policy. If you set GPIO'
value the hardware pin will be driven high, and if you clear it GPIO'
value the hardware pin will be pushed to ground. In case ACTIVE_LOW flag
is specified, the GPIO and actual hardware pin values are inverted.
So if you set GPIO to one, the hardware pin will be driven to zero,
and vise-versa. All this logic is implemented in the gpiod subsystem
of the kernel and can be defined in dts scripts, while legacy GPIO
subsystem relied on the drivers to handle this.

Yeah, it might be confusing, but some hardware is designed this way, so
the ordinary GPIO outputs are inverted on the way to the i2c-mux channel
activators. For instance in case if some level-shifter is used as a single
channel i2c-mux and we don't want i2c-bus being always connected to a bus
behind it. Such level-shifters are usually activated by ACTIVE_LOW signals.

In addition there are other than ACTIVE_LOW/ACTIVE_HIGH flags available for
GPIOs in dts, like GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE, which are
also specific not only to the GPIO functionality but to the target port and
hardware design in general. So the support of dt- GPIO-specifiers is very
important to properly describe the hardware setup.

-Sergey

> 
> Cheers,
> Peter
> 
> > On the way of adding the full dt-GPIO flags support a small set of
> > refactorings has been done in order to keep the platform_data-based
> > systems support, make the code more readable and the alterations - clearer.
> > In general the whole changes might be considered as the plat- and dt-
> > specific code split up. In first patch we unpinned the platform-specific
> > method of GPIO-chip probing. The second patch makes the driver to return
> > an error if of-based (last resort path) failed to retrieve the driver
> > private data. The next three patches is the sequence of initial channel
> > info retrieval, platform_data-specific code isolation and finally full
> > dt-based GPIOs request method introduction. The last patch does what
> > we inteded this patchset for in the first place - adds the full dt-GPIO
> > specifiers support.
> > 
> > 
> > Serge Semin (5):
> >   i2c-mux-gpio: Unpin a platform-based device initialization
> >   i2c-mux-gpio: Return an error if no config data found
> >   i2c-mux-gpio: Save initial channel number to the idle data field
> >   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
> >   i2c-mux-gpio: Create of-based GPIOs request method
> > 
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
> >  1 file changed, 146 insertions(+), 78 deletions(-)
> > 
> 

Re: [PATCH 2/5] i2c-mux-gpio: Return an error if no config data found

[Serge Semin]

On Wed, Apr 24, 2019 at 09:25:50PM +0000, Peter Rosin wrote:
> On 2019-04-24 14:34, Serge Semin wrote:
> > It's pointless and might be even errors prone to proceed with further
> > initialization if neither of- no platform-based settings were discovered.
> > Just return an error in this case.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 24cf6ec02e75..a14fe132b0c3 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -132,7 +132,7 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> >  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> >  					struct platform_device *pdev)
> >  {
> > -	return 0;
> > +	return -EINVAL;
> >  }
> >  #endif
> >  
> > @@ -142,6 +142,9 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> >  	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
> >  	struct gpio_chip *gpio;
> >  
> > +	if (!data)
> > +		return -EINVAL;
> > +
> >  	/*
> >  	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
> >  	 * relative to its base GPIO number. Otherwise they are absolute.
> > @@ -175,11 +178,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  	if (!mux)
> >  		return -ENOMEM;
> >  
> > -	if (!dev_get_platdata(&pdev->dev))
> > +	ret = i2c_mux_gpio_probe_plat(mux, pdev);
> > +	if (ret)
> >  		ret = i2c_mux_gpio_probe_dt(mux, pdev);
> > -	else
> > -		ret = i2c_mux_gpio_probe_plat(mux, pdev);
> > -	if (ret < 0)
> > +	if (ret)
> >  		return ret;
> 
> I notice that after this patch, all probe failures from non-dt configs
> will return -EINVAL from the dummy i2c_mux_gpio_probe_dt that gets
> called on i2c_mux_gpio_probe_plat failure.
> 
> So, any -EPROBE_DEFER is now lost. That probably doesn't fly.
> 

So what do you suggest then? We can return to something like:
if (dev_get_platdata(&pdev->dev))
    ret = i2c_mux_gpio_probe_plat(mux, pdev);
else
    ret = i2c_mux_gpio_probe_dt(mux, pdev);

In this case there is no falling back to dt. Just either plat- or of-based
initialization. The same can be done for i2c_mux_gpio_request_*() methods.

-Sergey

> Cheers,
> Peter
> 
> >  
> >  	parent = i2c_get_adapter(mux->data.parent);
> > 
> 

Re: [PATCH 3/5] i2c-mux-gpio: Save initial channel number to the idle data field

[Serge Semin]

On Wed, Apr 24, 2019 at 09:26:22PM +0000, Peter Rosin wrote:
> On 2019-04-24 14:34, Serge Semin wrote:
> > In case if the idle state has been specified in the data structure,
> > the idle variable is left untouched as before, so to keep a default
> > channel number enabled in the mux idle state. But if a platform doesn't
> > specify which channel is going to be enabled by default, we as before
> > don't setup the deselect callback, but the initial state is saved in the
> > idle variable for further initialization. We can safely do this here
> > since that variable is used for initial state setting only, when no
> > idling lane is specified.
> 
> While this subtlety is *maybe* ok, a comment about it belongs in the
> *code* where it will be seen when the next person makes changes.
> 
> But why not extend the struct with the initial state? How many of
> these muxes do you expect to exist in a system? Multiplied by a
> couple of bytes. Who cares?
> 

I actually thought about this when started working on the patchset.
That time saving the initial value in the idle variable seemed like a
good idea. I even put a small comment in the code about this.

Anyway lets add a new field to "struct gpiomux" structure and use
it as a container for initial value. It is also a good alternative.
I'll do this in a v2 patchset.

-Sergey

> Cheers,
> Peter
> 
> > 
> > The reason of this change is to prepare the code for future GPIOs request
> > path being split up into of- and plat- based methods. The idle variable
> > here is used as a container of the initial state for both of the paths in
> > case of idle-channel isn't specified.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index a14fe132b0c3..535c83c43371 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -171,7 +171,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  	struct gpiomux *mux;
> >  	struct i2c_adapter *parent;
> >  	struct i2c_adapter *root;
> > -	unsigned initial_state;
> >  	int i, ret;
> >  
> >  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> > @@ -204,12 +203,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  
> >  	muxc->mux_locked = true;
> >  
> > -	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> > -		initial_state = mux->data.idle;
> > +	/*
> > +	 * Set descelect callback if idle state has been setup otherwise just
> > +	 * use the idle variable to store the initial muxer value.
> > +	 */
> > +	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE)
> >  		muxc->deselect = i2c_mux_gpio_deselect;
> > -	} else {
> > -		initial_state = mux->data.values[0];
> > -	}
> > +	else
> > +		mux->data.idle = mux->data.values[0];
> >  
> >  	for (i = 0; i < mux->data.n_gpios; i++) {
> >  		struct device *gpio_dev;
> > @@ -224,7 +225,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  		}
> >  
> >  		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> > -					    initial_state & (1 << i));
> > +					    mux->data.idle & (1 << i));
> >  		if (ret) {
> >  			dev_err(&pdev->dev,
> >  				"Failed to set direction of GPIO %d to output\n",
> > 
> 

Re: [PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Peter Rosin]

On 2019-04-25 16:37, Serge Semin wrote:
> On Wed, Apr 24, 2019 at 09:25:24PM +0000, Peter Rosin wrote:
> 
> Hello Peter,
> 
>> On 2019-04-24 14:34, Serge Semin wrote:
>>> The main idea of this patchset was to add the dt-based GPIOs support
>>> in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
>>> specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
>>> GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
>>> driver implementation didn't provide this ability.
>>
>> I'm curious why active low/high is of any importance? That will only affect
>> the state numbering, but I fail to see any relevance in that numbering. It's
>> just numbers, no?
>>
>> If all the pins are inverted (anything else seems very strange), just
>> reverse the order. I.e. for a 4-way mux, use 3, 2, 1, 0 instead of
>> 0, 1, 2, 3.
>>
>> Why not?
> 
> I may misunderstood you, but active low/high flag has nothing to do with
> pins ordering. It is relevant to an individual pin setting, most likely
> related with hardware setup.

I was not talking about pin order. I was obviously referring to the
state order.

> Here is a simple example:
> i2cmux {
>                 compatible = "i2c-mux-gpio";
>                 mux-gpios = <&gpioa 0 GPIO_ACTIVE_LOW
>                              &control 2 GPIO_ACTIVE_HIGH
>                              &last 5 GPIO_ACTIVE_LOW>;
> };

So, with this, instead of having two pins active-low and using state 3,
you could use state 6 with all pins active-high. Same thing. I.e., use
"state ^ 5" instead of the "direct" state (whatever that is, the state
numbers have no real meaning, they are just numbers).

> In this setup we've got some i2c-mux with GPIOs-driven channel selector.
> First channel is selected by GPIO#0 of controller &gpioa, second one -
> by GPIO#2 of controller &control and third - by GPIO#3 of controller
> &last. In accordance with the i2c_mux_gpio_set() method of i2c-mux-gpio
> driver a GPIO from this set will be driven high in case of a corresponding
> mux channel being enabled. But as you can see from the "mux-gpios" property
> these GPIOs aren't identical. First of all they belong to different
> controller and most importantly they've got different active-attribute.
> This attribute actually means the straight or inverted activation policy.
> So in case of ACTIVE_HIGH flag you get a straight policy. If you set GPIO'
> value the hardware pin will be driven high, and if you clear it GPIO'
> value the hardware pin will be pushed to ground. In case ACTIVE_LOW flag
> is specified, the GPIO and actual hardware pin values are inverted.
> So if you set GPIO to one, the hardware pin will be driven to zero,
> and vise-versa. All this logic is implemented in the gpiod subsystem
> of the kernel and can be defined in dts scripts, while legacy GPIO
> subsystem relied on the drivers to handle this.
> 
> Yeah, it might be confusing, but some hardware is designed this way, so
> the ordinary GPIO outputs are inverted on the way to the i2c-mux channel
> activators. For instance in case if some level-shifter is used as a single
> channel i2c-mux and we don't want i2c-bus being always connected to a bus
> behind it. Such level-shifters are usually activated by ACTIVE_LOW signals.

See above, you could just adjust the state numbers instead.

> In addition there are other than ACTIVE_LOW/ACTIVE_HIGH flags available for
> GPIOs in dts, like GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE, which are
> also specific not only to the GPIO functionality but to the target port and
> hardware design in general. So the support of dt- GPIO-specifiers is very
> important to properly describe the hardware setup.

These things are another matter. I thought this was supposed to be
controlled with pinctrl drivers, but now that I look I see that gpio
drivers can do this directly without help from pinctrl. That's just
not how it has been done on the platforms I have been using...

So, ok, but open-drain/open-source/push-pull should perhaps have been
mentioned more prominently. (I now see that I managed to read past a
mention of open-drain in the commit message for 5/5)

Cheers,
Peter

> -Sergey
> 
>>
>> Cheers,
>> Peter
>>
>>> On the way of adding the full dt-GPIO flags support a small set of
>>> refactorings has been done in order to keep the platform_data-based
>>> systems support, make the code more readable and the alterations - clearer.
>>> In general the whole changes might be considered as the plat- and dt-
>>> specific code split up. In first patch we unpinned the platform-specific
>>> method of GPIO-chip probing. The second patch makes the driver to return
>>> an error if of-based (last resort path) failed to retrieve the driver
>>> private data. The next three patches is the sequence of initial channel
>>> info retrieval, platform_data-specific code isolation and finally full
>>> dt-based GPIOs request method introduction. The last patch does what
>>> we inteded this patchset for in the first place - adds the full dt-GPIO
>>> specifiers support.
>>>
>>>
>>> Serge Semin (5):
>>>   i2c-mux-gpio: Unpin a platform-based device initialization
>>>   i2c-mux-gpio: Return an error if no config data found
>>>   i2c-mux-gpio: Save initial channel number to the idle data field
>>>   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>>>   i2c-mux-gpio: Create of-based GPIOs request method
>>>
>>>  drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
>>>  1 file changed, 146 insertions(+), 78 deletions(-)
>>>
>>

Re: [PATCH 2/5] i2c-mux-gpio: Return an error if no config data found

[Peter Rosin]

On 2019-04-25 17:47, Serge Semin wrote:
> On Wed, Apr 24, 2019 at 09:25:50PM +0000, Peter Rosin wrote:
>> On 2019-04-24 14:34, Serge Semin wrote:
>>> It's pointless and might be even errors prone to proceed with further
>>> initialization if neither of- no platform-based settings were discovered.
>>> Just return an error in this case.
>>>
>>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>>> ---
>>>  drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> index 24cf6ec02e75..a14fe132b0c3 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> @@ -132,7 +132,7 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>>>  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>>>  					struct platform_device *pdev)
>>>  {
>>> -	return 0;
>>> +	return -EINVAL;
>>>  }
>>>  #endif
>>>  
>>> @@ -142,6 +142,9 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
>>>  	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
>>>  	struct gpio_chip *gpio;
>>>  
>>> +	if (!data)
>>> +		return -EINVAL;
>>> +
>>>  	/*
>>>  	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
>>>  	 * relative to its base GPIO number. Otherwise they are absolute.
>>> @@ -175,11 +178,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>>  	if (!mux)
>>>  		return -ENOMEM;
>>>  
>>> -	if (!dev_get_platdata(&pdev->dev))
>>> +	ret = i2c_mux_gpio_probe_plat(mux, pdev);
>>> +	if (ret)
>>>  		ret = i2c_mux_gpio_probe_dt(mux, pdev);
>>> -	else
>>> -		ret = i2c_mux_gpio_probe_plat(mux, pdev);
>>> -	if (ret < 0)
>>> +	if (ret)
>>>  		return ret;
>>
>> I notice that after this patch, all probe failures from non-dt configs
>> will return -EINVAL from the dummy i2c_mux_gpio_probe_dt that gets
>> called on i2c_mux_gpio_probe_plat failure.
>>
>> So, any -EPROBE_DEFER is now lost. That probably doesn't fly.
>>
> 
> So what do you suggest then?

I don't know, I'm just pointing out that you are breaking probe-defer.

>                              We can return to something like:
> if (dev_get_platdata(&pdev->dev))
>     ret = i2c_mux_gpio_probe_plat(mux, pdev);
> else
>     ret = i2c_mux_gpio_probe_dt(mux, pdev);
> 
> In this case there is no falling back to dt. Just either plat- or of-based
> initialization. The same can be done for i2c_mux_gpio_request_*() methods.

Works for me, I fail to see why it is interesting with a fallback
anyway? If you supply platform data, that is supposed to take
precedence. No?

If the platform data fails, I'd rather not have the code run into the
weeds attempting stuff that's not even supposed to work...

Cheers,
Peter

> -Sergey
> 
>> Cheers,
>> Peter
>>
>>>  
>>>  	parent = i2c_get_adapter(mux->data.parent);
>>>
>>

Re: [PATCH 3/5] i2c-mux-gpio: Save initial channel number to the idle data field

[Peter Rosin]

On 2019-04-25 17:53, Serge Semin wrote:
> On Wed, Apr 24, 2019 at 09:26:22PM +0000, Peter Rosin wrote:
>> On 2019-04-24 14:34, Serge Semin wrote:
>>> In case if the idle state has been specified in the data structure,
>>> the idle variable is left untouched as before, so to keep a default
>>> channel number enabled in the mux idle state. But if a platform doesn't
>>> specify which channel is going to be enabled by default, we as before
>>> don't setup the deselect callback, but the initial state is saved in the
>>> idle variable for further initialization. We can safely do this here
>>> since that variable is used for initial state setting only, when no
>>> idling lane is specified.
>>
>> While this subtlety is *maybe* ok, a comment about it belongs in the
>> *code* where it will be seen when the next person makes changes.
>>
>> But why not extend the struct with the initial state? How many of
>> these muxes do you expect to exist in a system? Multiplied by a
>> couple of bytes. Who cares?
>>
> 
> I actually thought about this when started working on the patchset.
> That time saving the initial value in the idle variable seemed like a
> good idea. I even put a small comment in the code about this.

Oh, I missed that. Crap, and sorry!

> Anyway lets add a new field to "struct gpiomux" structure and use
> it as a container for initial value. It is also a good alternative.
> I'll do this in a v2 patchset.

Good. Thanks!

Cheers,
Peter

> -Sergey
> 
>> Cheers,
>> Peter
>>
>>>
>>> The reason of this change is to prepare the code for future GPIOs request
>>> path being split up into of- and plat- based methods. The idle variable
>>> here is used as a container of the initial state for both of the paths in
>>> case of idle-channel isn't specified.
>>>
>>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>>> ---
>>>  drivers/i2c/muxes/i2c-mux-gpio.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> index a14fe132b0c3..535c83c43371 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> @@ -171,7 +171,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>>  	struct gpiomux *mux;
>>>  	struct i2c_adapter *parent;
>>>  	struct i2c_adapter *root;
>>> -	unsigned initial_state;
>>>  	int i, ret;
>>>  
>>>  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
>>> @@ -204,12 +203,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>>  
>>>  	muxc->mux_locked = true;
>>>  
>>> -	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
>>> -		initial_state = mux->data.idle;
>>> +	/*
>>> +	 * Set descelect callback if idle state has been setup otherwise just

Spelling "deselect"

>>> +	 * use the idle variable to store the initial muxer value.
>>> +	 */
>>> +	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE)
>>>  		muxc->deselect = i2c_mux_gpio_deselect;
>>> -	} else {
>>> -		initial_state = mux->data.values[0];
>>> -	}
>>> +	else
>>> +		mux->data.idle = mux->data.values[0];
>>>  
>>>  	for (i = 0; i < mux->data.n_gpios; i++) {
>>>  		struct device *gpio_dev;
>>> @@ -224,7 +225,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>>  		}
>>>  
>>>  		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
>>> -					    initial_state & (1 << i));
>>> +					    mux->data.idle & (1 << i));
>>>  		if (ret) {
>>>  			dev_err(&pdev->dev,
>>>  				"Failed to set direction of GPIO %d to output\n",
>>>
>>

Re: [PATCH 0/5] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

On Thu, Apr 25, 2019 at 07:21:02PM +0000, Peter Rosin wrote:
> On 2019-04-25 16:37, Serge Semin wrote:
> > On Wed, Apr 24, 2019 at 09:25:24PM +0000, Peter Rosin wrote:
> > 
> > Hello Peter,
> > 
> >> On 2019-04-24 14:34, Serge Semin wrote:
> >>> The main idea of this patchset was to add the dt-based GPIOs support
> >>> in i2c-mux-gpio driver. In particular we needed to have the full GPIOs
> >>> specifier being handled including the dt-flags like GPIO_ACTIVE_HIGH,
> >>> GPIO_ACTIVE_LOW, etc. Due to using a legacy GPIO interface the former
> >>> driver implementation didn't provide this ability.
> >>
> >> I'm curious why active low/high is of any importance? That will only affect
> >> the state numbering, but I fail to see any relevance in that numbering. It's
> >> just numbers, no?
> >>
> >> If all the pins are inverted (anything else seems very strange), just
> >> reverse the order. I.e. for a 4-way mux, use 3, 2, 1, 0 instead of
> >> 0, 1, 2, 3.
> >>
> >> Why not?
> > 
> > I may misunderstood you, but active low/high flag has nothing to do with
> > pins ordering. It is relevant to an individual pin setting, most likely
> > related with hardware setup.
> 
> I was not talking about pin order. I was obviously referring to the
> state order.
> 
> > Here is a simple example:
> > i2cmux {
> >                 compatible = "i2c-mux-gpio";
> >                 mux-gpios = <&gpioa 0 GPIO_ACTIVE_LOW
> >                              &control 2 GPIO_ACTIVE_HIGH
> >                              &last 5 GPIO_ACTIVE_LOW>;
> > };
> 
> So, with this, instead of having two pins active-low and using state 3,
> you could use state 6 with all pins active-high. Same thing. I.e., use
> "state ^ 5" instead of the "direct" state (whatever that is, the state
> numbers have no real meaning, they are just numbers).
> 
> > In this setup we've got some i2c-mux with GPIOs-driven channel selector.
> > First channel is selected by GPIO#0 of controller &gpioa, second one -
> > by GPIO#2 of controller &control and third - by GPIO#3 of controller
> > &last. In accordance with the i2c_mux_gpio_set() method of i2c-mux-gpio
> > driver a GPIO from this set will be driven high in case of a corresponding
> > mux channel being enabled. But as you can see from the "mux-gpios" property
> > these GPIOs aren't identical. First of all they belong to different
> > controller and most importantly they've got different active-attribute.
> > This attribute actually means the straight or inverted activation policy.
> > So in case of ACTIVE_HIGH flag you get a straight policy. If you set GPIO'
> > value the hardware pin will be driven high, and if you clear it GPIO'
> > value the hardware pin will be pushed to ground. In case ACTIVE_LOW flag
> > is specified, the GPIO and actual hardware pin values are inverted.
> > So if you set GPIO to one, the hardware pin will be driven to zero,
> > and vise-versa. All this logic is implemented in the gpiod subsystem
> > of the kernel and can be defined in dts scripts, while legacy GPIO
> > subsystem relied on the drivers to handle this.
> > 
> > Yeah, it might be confusing, but some hardware is designed this way, so
> > the ordinary GPIO outputs are inverted on the way to the i2c-mux channel
> > activators. For instance in case if some level-shifter is used as a single
> > channel i2c-mux and we don't want i2c-bus being always connected to a bus
> > behind it. Such level-shifters are usually activated by ACTIVE_LOW signals.
> 
> See above, you could just adjust the state numbers instead.
> 

Ahh, now I see what you meant. Sorry for explaining the obvious.)

It is definitely a solution in case if active-low pins used for channel
selection, but it seems more like a hack than a best choice. The main
problem is that the hardware programmer needs to take into account the
active-{low,high} flags when assigning the reg-values of subnodes. While in
case if these flags are supported by GPIO subsystem itself, the reg
properties can be the same as if all GPIOs were active-high. As for me
this is a good simplification, which also makes the i2c-mux-gpio nodes
more readable. 

> > In addition there are other than ACTIVE_LOW/ACTIVE_HIGH flags available for
> > GPIOs in dts, like GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE, which are
> > also specific not only to the GPIO functionality but to the target port and
> > hardware design in general. So the support of dt- GPIO-specifiers is very
> > important to properly describe the hardware setup.
> 
> These things are another matter. I thought this was supposed to be
> controlled with pinctrl drivers, but now that I look I see that gpio
> drivers can do this directly without help from pinctrl. That's just
> not how it has been done on the platforms I have been using...
> 
> So, ok, but open-drain/open-source/push-pull should perhaps have been
> mentioned more prominently. (I now see that I managed to read past a
> mention of open-drain in the commit message for 5/5)
> 

Yeah, these flags is another positive side of supporting the full dt GPIO
specifiers.

-Sergey

> Cheers,
> Peter
> 
> > -Sergey
> > 
> >>
> >> Cheers,
> >> Peter
> >>
> >>> On the way of adding the full dt-GPIO flags support a small set of
> >>> refactorings has been done in order to keep the platform_data-based
> >>> systems support, make the code more readable and the alterations - clearer.
> >>> In general the whole changes might be considered as the plat- and dt-
> >>> specific code split up. In first patch we unpinned the platform-specific
> >>> method of GPIO-chip probing. The second patch makes the driver to return
> >>> an error if of-based (last resort path) failed to retrieve the driver
> >>> private data. The next three patches is the sequence of initial channel
> >>> info retrieval, platform_data-specific code isolation and finally full
> >>> dt-based GPIOs request method introduction. The last patch does what
> >>> we inteded this patchset for in the first place - adds the full dt-GPIO
> >>> specifiers support.
> >>>
> >>>
> >>> Serge Semin (5):
> >>>   i2c-mux-gpio: Unpin a platform-based device initialization
> >>>   i2c-mux-gpio: Return an error if no config data found
> >>>   i2c-mux-gpio: Save initial channel number to the idle data field
> >>>   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
> >>>   i2c-mux-gpio: Create of-based GPIOs request method
> >>>
> >>>  drivers/i2c/muxes/i2c-mux-gpio.c | 224 ++++++++++++++++++++-----------
> >>>  1 file changed, 146 insertions(+), 78 deletions(-)
> >>>
> >>
> 

Re: [PATCH 2/5] i2c-mux-gpio: Return an error if no config data found

[Serge Semin]

On Thu, Apr 25, 2019 at 07:28:52PM +0000, Peter Rosin wrote:
> On 2019-04-25 17:47, Serge Semin wrote:
> > On Wed, Apr 24, 2019 at 09:25:50PM +0000, Peter Rosin wrote:
> >> On 2019-04-24 14:34, Serge Semin wrote:
> >>> It's pointless and might be even errors prone to proceed with further
> >>> initialization if neither of- no platform-based settings were discovered.
> >>> Just return an error in this case.
> >>>
> >>> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> >>> ---
> >>>  drivers/i2c/muxes/i2c-mux-gpio.c | 12 +++++++-----
> >>>  1 file changed, 7 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> >>> index 24cf6ec02e75..a14fe132b0c3 100644
> >>> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> >>> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> >>> @@ -132,7 +132,7 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> >>>  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> >>>  					struct platform_device *pdev)
> >>>  {
> >>> -	return 0;
> >>> +	return -EINVAL;
> >>>  }
> >>>  #endif
> >>>  
> >>> @@ -142,6 +142,9 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> >>>  	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
> >>>  	struct gpio_chip *gpio;
> >>>  
> >>> +	if (!data)
> >>> +		return -EINVAL;
> >>> +
> >>>  	/*
> >>>  	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
> >>>  	 * relative to its base GPIO number. Otherwise they are absolute.
> >>> @@ -175,11 +178,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >>>  	if (!mux)
> >>>  		return -ENOMEM;
> >>>  
> >>> -	if (!dev_get_platdata(&pdev->dev))
> >>> +	ret = i2c_mux_gpio_probe_plat(mux, pdev);
> >>> +	if (ret)
> >>>  		ret = i2c_mux_gpio_probe_dt(mux, pdev);
> >>> -	else
> >>> -		ret = i2c_mux_gpio_probe_plat(mux, pdev);
> >>> -	if (ret < 0)
> >>> +	if (ret)
> >>>  		return ret;
> >>
> >> I notice that after this patch, all probe failures from non-dt configs
> >> will return -EINVAL from the dummy i2c_mux_gpio_probe_dt that gets
> >> called on i2c_mux_gpio_probe_plat failure.
> >>
> >> So, any -EPROBE_DEFER is now lost. That probably doesn't fly.
> >>
> > 
> > So what do you suggest then?
> 
> I don't know, I'm just pointing out that you are breaking probe-defer.
> 
> >                              We can return to something like:
> > if (dev_get_platdata(&pdev->dev))
> >     ret = i2c_mux_gpio_probe_plat(mux, pdev);
> > else
> >     ret = i2c_mux_gpio_probe_dt(mux, pdev);
> > 
> > In this case there is no falling back to dt. Just either plat- or of-based
> > initialization. The same can be done for i2c_mux_gpio_request_*() methods.
> 
> Works for me, I fail to see why it is interesting with a fallback
> anyway? If you supply platform data, that is supposed to take
> precedence. No?
> 
> If the platform data fails, I'd rather not have the code run into the
> weeds attempting stuff that's not even supposed to work...
> 

Yeah, you are right. Adding fallback pattern here was a bad idea.
I'll fix it in the next patchset version.

-Sergey

> Cheers,
> Peter
> 
> > -Sergey
> > 
> >> Cheers,
> >> Peter
> >>
> >>>  
> >>>  	parent = i2c_get_adapter(mux->data.parent);
> >>>
> >>
> 

[PATCH v2 0/3] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

The main idea of this patchset was to add the full dt GPIOs specifier
support in i2c-mux-gpio driver. In particular we needed to have the
full GPIOs specifier being handled including the flags like GPIO_ACTIVE_HIGH,
GPIO_ACTIVE_LOW, GPIO_PUSH_PULL, GPIO_OPEN_DRAIN or GPIO_OPEN_SOURCE.
Due to using a legacy GPIO interface the former driver implementation
didn't provide this ability.

On the way of adding the full dt-GPIO flags support a small set of
refactorings has been done in order to keep the platform_data-based
systems support, make the code more readable and the alterations - clearer.
In general the whole changes might be considered as the plat- and dt-
specific code split up. In the first patch we unpinned the platform-specific
method of GPIO-chip probing. The second patch introduces a new initial_state
value field into the "gpiomux" structure. The third one is responsible for
GPIO request loop isoltaing into a dedicated function. At this stage common
it is a common function for both dt- and plat- code paths. Finally last
patch introduces a full dt-based GPIOs request method, which uses
gpiod_get_from_of_node() method in order to parse the corresponding dt GPIO
specifiers with their falgs. The last patch does what we inteded this patchset
for in the first place - adds the full dt-GPIO specifiers support.

Changelog v2
- Remove fallback pattern when selecting the dt- or plat-based code paths.
  (Cause the patch "i2c-mux-gpio: Return an error if no onfig data found"
   removal.)
- Use a dedicated initial_state variable to keep the initial channels selector
  state. (Causes the patch "i2c-mux-gpio: Save initial channel number to the
  idle" removal.)
- Mention open-drain, open-source flags in the patchset descriptions.


Serge Semin (3):
  i2c-mux-gpio: Unpin a platform-based device initialization
  i2c-mux-gpio: Unpin the platform-specific GPIOs request code
  i2c-mux-gpio: Create of-based GPIOs request method

 drivers/i2c/muxes/i2c-mux-gpio.c | 226 ++++++++++++++++++++-----------
 1 file changed, 146 insertions(+), 80 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] i2c-mux-gpio: Unpin a platform-based device initialization

[Serge Semin]

We can unpin a code specific for i2c-mux-gpio device declared
as platform device. In this case the platform data just needs to be
copied to the private storage and if GPIO chip pointer is referring to
a valid GPIO chip descriptor save it' base number for further GPIOs
request and initialization. The rest of the code is common for both
platform and OF-based setups.

It's also pointless and might be even errors prone to proceed with
further initialization if OF kernel config is disabled and plat-based
initialization isn't defined. Just return an error in this case.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Return an error if OF kconfig is disabled while dt-based GPIOs probe
  is called.
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 69 ++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 13882a2a4f60..54158b825acd 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -132,48 +132,55 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 					struct platform_device *pdev)
 {
-	return 0;
+	return -EINVAL;
 }
 #endif
 
+static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
+	struct gpio_chip *gpio;
+
+	/*
+	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
+	 * relative to its base GPIO number. Otherwise they are absolute.
+	 */
+	if (data->gpio_chip) {
+		gpio = gpiochip_find(data->gpio_chip,
+				     match_gpio_chip_by_label);
+		if (!gpio)
+			return -EPROBE_DEFER;
+
+		mux->gpio_base = gpio->base;
+	} else {
+		mux->gpio_base = 0;
+	}
+
+	memcpy(&mux->data, data, sizeof(mux->data));
+
+	return 0;
+}
+
 static int i2c_mux_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc;
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
 	struct i2c_adapter *root;
-	unsigned initial_state, gpio_base;
+	unsigned initial_state;
 	int i, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
 	if (!mux)
 		return -ENOMEM;
 
-	if (!dev_get_platdata(&pdev->dev)) {
+	if (!dev_get_platdata(&pdev->dev))
 		ret = i2c_mux_gpio_probe_dt(mux, pdev);
-		if (ret < 0)
-			return ret;
-	} else {
-		memcpy(&mux->data, dev_get_platdata(&pdev->dev),
-			sizeof(mux->data));
-	}
-
-	/*
-	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
-	 * relative to its base GPIO number. Otherwise they are absolute.
-	 */
-	if (mux->data.gpio_chip) {
-		struct gpio_chip *gpio;
-
-		gpio = gpiochip_find(mux->data.gpio_chip,
-				     match_gpio_chip_by_label);
-		if (!gpio)
-			return -EPROBE_DEFER;
-
-		gpio_base = gpio->base;
-	} else {
-		gpio_base = 0;
-	}
+	else
+		ret = i2c_mux_gpio_probe_plat(mux, pdev);
+	if (ret)
+		return ret;
 
 	parent = i2c_get_adapter(mux->data.parent);
 	if (!parent)
@@ -194,7 +201,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	root = i2c_root_adapter(&parent->dev);
 
 	muxc->mux_locked = true;
-	mux->gpio_base = gpio_base;
 
 	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
 		initial_state = mux->data.idle;
@@ -207,14 +213,15 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		struct device *gpio_dev;
 		struct gpio_desc *gpio_desc;
 
-		ret = gpio_request(gpio_base + mux->data.gpios[i], "i2c-mux-gpio");
+		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
+				   "i2c-mux-gpio");
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
 				mux->data.gpios[i]);
 			goto err_request_gpio;
 		}
 
-		ret = gpio_direction_output(gpio_base + mux->data.gpios[i],
+		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
 					    initial_state & (1 << i));
 		if (ret) {
 			dev_err(&pdev->dev,
@@ -224,7 +231,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 			goto err_request_gpio;
 		}
 
-		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
+		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
 		mux->gpios[i] = gpio_desc;
 
 		if (!muxc->mux_locked)
@@ -256,7 +263,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	i = mux->data.n_gpios;
 err_request_gpio:
 	for (; i > 0; i--)
-		gpio_free(gpio_base + mux->data.gpios[i - 1]);
+		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
 alloc_failed:
 	i2c_put_adapter(parent);
 
-- 
2.21.0

[PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Serge Semin]

The GPIOs request loop can be safely moved to a separate function.
First of all it shall improve the code readability. Secondly the
initialization loop at this point is used for both of- and
platform_data-based initialization paths, but it will be changed in
the next patch, so by isolating the code we'll simplify the future
work.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Create a dedicated initial_state field in the "gpiomux" structure to
  keep an initial channel selector state.
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 113 +++++++++++++++++++------------
 1 file changed, 68 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 54158b825acd..e10f72706b99 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -20,7 +20,8 @@
 
 struct gpiomux {
 	struct i2c_mux_gpio_platform_data data;
-	unsigned gpio_base;
+	unsigned int gpio_base;
+	unsigned int initial_state;
 	struct gpio_desc **gpios;
 };
 
@@ -162,13 +163,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
 	return 0;
 }
 
+static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+	struct gpio_desc *gpio_desc;
+	struct i2c_adapter *root;
+	struct device *gpio_dev;
+	int i, ret;
+
+	root = i2c_root_adapter(&muxc->parent->dev);
+
+	for (i = 0; i < mux->data.n_gpios; i++) {
+		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
+				   "i2c-mux-gpio");
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
+				mux->data.gpios[i]);
+			goto err_request_gpio;
+		}
+
+		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
+					    mux->initial_state & (1 << i));
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to set direction of GPIO %d to output\n",
+				mux->data.gpios[i]);
+			i++;	/* gpio_request above succeeded, so must free */
+			goto err_request_gpio;
+		}
+
+		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
+		mux->gpios[i] = gpio_desc;
+
+		if (!muxc->mux_locked)
+			continue;
+
+		gpio_dev = &gpio_desc->gdev->dev;
+		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
+	}
+
+	return 0;
+
+err_request_gpio:
+	for (; i > 0; i--)
+		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
+
+	return ret;
+}
+
+static void i2c_mux_gpio_free(struct gpiomux *mux)
+{
+	int i;
+
+	for (i = 0; i < mux->data.n_gpios; i++)
+		gpiod_free(mux->gpios[i]);
+}
+
 static int i2c_mux_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc;
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
-	struct i2c_adapter *root;
-	unsigned initial_state;
 	int i, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
@@ -198,48 +254,18 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, muxc);
 
-	root = i2c_root_adapter(&parent->dev);
-
 	muxc->mux_locked = true;
 
 	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
-		initial_state = mux->data.idle;
+		mux->initial_state = mux->data.idle;
 		muxc->deselect = i2c_mux_gpio_deselect;
 	} else {
-		initial_state = mux->data.values[0];
+		mux->initial_state = mux->data.values[0];
 	}
 
-	for (i = 0; i < mux->data.n_gpios; i++) {
-		struct device *gpio_dev;
-		struct gpio_desc *gpio_desc;
-
-		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
-				   "i2c-mux-gpio");
-		if (ret) {
-			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
-				mux->data.gpios[i]);
-			goto err_request_gpio;
-		}
-
-		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
-					    initial_state & (1 << i));
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to set direction of GPIO %d to output\n",
-				mux->data.gpios[i]);
-			i++;	/* gpio_request above succeeded, so must free */
-			goto err_request_gpio;
-		}
-
-		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
-		mux->gpios[i] = gpio_desc;
-
-		if (!muxc->mux_locked)
-			continue;
-
-		gpio_dev = &gpio_desc->gdev->dev;
-		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
-	}
+	ret = i2c_mux_gpio_request_plat(mux, pdev);
+	if (ret)
+		goto alloc_failed;
 
 	if (muxc->mux_locked)
 		dev_info(&pdev->dev, "mux-locked i2c mux\n");
@@ -260,10 +286,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 add_adapter_failed:
 	i2c_mux_del_adapters(muxc);
-	i = mux->data.n_gpios;
-err_request_gpio:
-	for (; i > 0; i--)
-		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
+
+	i2c_mux_gpio_free(mux);
+
 alloc_failed:
 	i2c_put_adapter(parent);
 
@@ -274,12 +299,10 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
 	struct gpiomux *mux = i2c_mux_priv(muxc);
-	int i;
 
 	i2c_mux_del_adapters(muxc);
 
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_free(mux->gpio_base + mux->data.gpios[i]);
+	i2c_mux_gpio_free(mux);
 
 	i2c_put_adapter(muxc->parent);
 
-- 
2.21.0

[PATCH v2 3/3] i2c-mux-gpio: Create of-based GPIOs request method

[Serge Semin]

Most modern platforms provide a dts with description of the devices
available in the system. It may also include i2c-gpio-mux'es.
Up until now the i2c-mux-gpio driver supported it' dts nodes, but
performed the GPIOs request by means of legacy GPIO API, which by design
and due to being legacy doesn't know anything about of/dtb/fdt/dts stuff.
It means even though the i2c-gpio-mux dts nodes are successfully mapped
to the kernel i2c-mux devices, the GPIOs used for initialization are
requested without OF_GPIO_* flags setup. It causes problems on the
platforms which fully rely on dts and reside, for instance,
i2c-gpio-muxes with active low, push-pull, open drain or open source
GPIOs connected.

It is fixed by implementing a dedicated method for full dts-based
GPIOs requests. It is mostly similar to the platform one, but
utilizes the gpiod_get_from_of_node() method to request the GPIOs.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---
Changelog v2
- Remove fallback pattern when selecting the dt- or plat-based GPIOs
  request methods.
- Use a dedicated initial_state field in the "gpiomux" structure to
  select a proper channel initially.
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 68 ++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index e10f72706b99..d1a9c56fa1ec 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -66,8 +66,8 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *adapter_np, *child;
 	struct i2c_adapter *adapter;
-	unsigned *values, *gpios;
-	int i = 0, ret;
+	unsigned int *values;
+	int i = 0;
 
 	if (!np)
 		return -ENODEV;
@@ -110,24 +110,48 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 		return -EINVAL;
 	}
 
-	gpios = devm_kcalloc(&pdev->dev,
-			     mux->data.n_gpios, sizeof(*mux->data.gpios),
-			     GFP_KERNEL);
-	if (!gpios) {
-		dev_err(&pdev->dev, "Cannot allocate gpios array");
-		return -ENOMEM;
-	}
+	return 0;
+}
+
+static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	struct i2c_adapter *root;
+	struct device *gpio_dev;
+	enum gpiod_flags dflags;
+	int i, ret;
+
+	root = i2c_root_adapter(&muxc->parent->dev);
 
 	for (i = 0; i < mux->data.n_gpios; i++) {
-		ret = of_get_named_gpio(np, "mux-gpios", i);
-		if (ret < 0)
-			return ret;
-		gpios[i] = ret;
-	}
+		if (mux->initial_state & (1 << i))
+			dflags = GPIOD_OUT_HIGH;
+		else
+			dflags = GPIOD_OUT_LOW;
+
+		mux->gpios[i] = gpiod_get_from_of_node(np, "mux-gpios", i,
+						       dflags, "i2c-mux-gpio");
+		if (IS_ERR(mux->gpios[i])) {
+			ret = PTR_ERR(mux->gpios[i]);
+			goto err_request_gpio;
+		}
+
+		if (!muxc->mux_locked)
+			continue;
 
-	mux->data.gpios = gpios;
+		gpio_dev = &mux->gpios[i]->gdev->dev;
+		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
+	}
 
 	return 0;
+
+err_request_gpio:
+	for (i--; i >= 0; i--)
+		gpiod_free(mux->gpios[i]);
+
+	return ret;
 }
 #else
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
@@ -135,6 +159,12 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 {
 	return -EINVAL;
 }
+
+static int i2c_mux_gpio_request_dt(struct gpiomux *mux,
+					struct platform_device *pdev)
+{
+	return -EINVAL;
+}
 #endif
 
 static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
@@ -172,6 +202,9 @@ static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
 	struct device *gpio_dev;
 	int i, ret;
 
+	if (!mux->data.gpios)
+		return -EINVAL;
+
 	root = i2c_root_adapter(&muxc->parent->dev);
 
 	for (i = 0; i < mux->data.n_gpios; i++) {
@@ -263,7 +296,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		mux->initial_state = mux->data.values[0];
 	}
 
-	ret = i2c_mux_gpio_request_plat(mux, pdev);
+	if (!dev_get_platdata(&pdev->dev))
+		ret = i2c_mux_gpio_request_dt(mux, pdev);
+	else
+		ret = i2c_mux_gpio_request_plat(mux, pdev);
 	if (ret)
 		goto alloc_failed;
 
-- 
2.21.0

Re: [PATCH v2 0/3] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

Hello folks,

Any updates on this patchset status? I haven't got any comment on v2, but
instead a notification about the status change was sent to me:

> * linux-i2c: [v2,1/3] i2c-mux-gpio: Unpin a platform-based device initialization
>     - http://patchwork.ozlabs.org/patch/1091120/
>     - for: Linux I2C development
>    was: New
>    now: Superseded
>
> * linux-i2c: [v2,2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>     - http://patchwork.ozlabs.org/patch/1091122/
>     - for: Linux I2C development
>    was: New
>    now: Superseded
>
> * linux-i2c: [v2,3/3] i2c-mux-gpio: Create of-based GPIOs request method
>     - http://patchwork.ozlabs.org/patch/1091121/
>     - for: Linux I2C development
>    was: New
>    now: Superseded

I may misunderstand something, but how come the v2 patchset switched to be superseded
while it is the last patchset version I've sent?

-Sergey

On Fri, Apr 26, 2019 at 02:20:25AM +0300, Serge Semin wrote:
> The main idea of this patchset was to add the full dt GPIOs specifier
> support in i2c-mux-gpio driver. In particular we needed to have the
> full GPIOs specifier being handled including the flags like GPIO_ACTIVE_HIGH,
> GPIO_ACTIVE_LOW, GPIO_PUSH_PULL, GPIO_OPEN_DRAIN or GPIO_OPEN_SOURCE.
> Due to using a legacy GPIO interface the former driver implementation
> didn't provide this ability.
> 
> On the way of adding the full dt-GPIO flags support a small set of
> refactorings has been done in order to keep the platform_data-based
> systems support, make the code more readable and the alterations - clearer.
> In general the whole changes might be considered as the plat- and dt-
> specific code split up. In the first patch we unpinned the platform-specific
> method of GPIO-chip probing. The second patch introduces a new initial_state
> value field into the "gpiomux" structure. The third one is responsible for
> GPIO request loop isoltaing into a dedicated function. At this stage common
> it is a common function for both dt- and plat- code paths. Finally last
> patch introduces a full dt-based GPIOs request method, which uses
> gpiod_get_from_of_node() method in order to parse the corresponding dt GPIO
> specifiers with their falgs. The last patch does what we inteded this patchset
> for in the first place - adds the full dt-GPIO specifiers support.
> 
> Changelog v2
> - Remove fallback pattern when selecting the dt- or plat-based code paths.
>   (Cause the patch "i2c-mux-gpio: Return an error if no onfig data found"
>    removal.)
> - Use a dedicated initial_state variable to keep the initial channels selector
>   state. (Causes the patch "i2c-mux-gpio: Save initial channel number to the
>   idle" removal.)
> - Mention open-drain, open-source flags in the patchset descriptions.
> 
> 
> Serge Semin (3):
>   i2c-mux-gpio: Unpin a platform-based device initialization
>   i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>   i2c-mux-gpio: Create of-based GPIOs request method
> 
>  drivers/i2c/muxes/i2c-mux-gpio.c | 226 ++++++++++++++++++++-----------
>  1 file changed, 146 insertions(+), 80 deletions(-)
> 
> -- 
> 2.21.0
> 

Re: [PATCH v2 0/3] i2c-mux-gpio: Split plat- and dt-specific code up

[Peter Rosin]

On 2019-05-07 11:02, Serge Semin wrote:
> Hello folks,
> 
> Any updates on this patchset status? I haven't got any comment on v2, but
> instead a notification about the status change was sent to me:
> 
>> * linux-i2c: [v2,1/3] i2c-mux-gpio: Unpin a platform-based device initialization
>>     - http://patchwork.ozlabs.org/patch/1091120/
>>     - for: Linux I2C development
>>    was: New
>>    now: Superseded
>>
>> * linux-i2c: [v2,2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code
>>     - http://patchwork.ozlabs.org/patch/1091122/
>>     - for: Linux I2C development
>>    was: New
>>    now: Superseded
>>
>> * linux-i2c: [v2,3/3] i2c-mux-gpio: Create of-based GPIOs request method
>>     - http://patchwork.ozlabs.org/patch/1091121/
>>     - for: Linux I2C development
>>    was: New
>>    now: Superseded
> 
> I may misunderstand something, but how come the v2 patchset switched to be superseded
> while it is the last patchset version I've sent?

That was my mistake. Patchwork got confused when v2 was sent as a reply to
something in the v1 tree, and marked all 8 patches as "v2". Then I in turn
got confused by that, and changed status on the wrong set. Sorry!

So, thanks for the heads up, it should be fixed now.

As for comments on the patches, I'm personally buried in work and others
may have the merge window to focus on...

Cheers,
Peter

Re: [PATCH v2 0/3] i2c-mux-gpio: Split plat- and dt-specific code up

[Serge Semin]

Hello Peter

On Tue, May 07, 2019 at 09:17:38AM +0000, Peter Rosin wrote:
> On 2019-05-07 11:02, Serge Semin wrote:
> > Hello folks,
> > 
> > Any updates on this patchset status? I haven't got any comment on v2, but
> > instead a notification about the status change was sent to me:
> > 
> >> * linux-i2c: [v2,1/3] i2c-mux-gpio: Unpin a platform-based device initialization
> >>     - http://patchwork.ozlabs.org/patch/1091120/
> >>     - for: Linux I2C development
> >>    was: New
> >>    now: Superseded
> >>
> >> * linux-i2c: [v2,2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code
> >>     - http://patchwork.ozlabs.org/patch/1091122/
> >>     - for: Linux I2C development
> >>    was: New
> >>    now: Superseded
> >>
> >> * linux-i2c: [v2,3/3] i2c-mux-gpio: Create of-based GPIOs request method
> >>     - http://patchwork.ozlabs.org/patch/1091121/
> >>     - for: Linux I2C development
> >>    was: New
> >>    now: Superseded
> > 
> > I may misunderstand something, but how come the v2 patchset switched to be superseded
> > while it is the last patchset version I've sent?
> 
> That was my mistake. Patchwork got confused when v2 was sent as a reply to
> something in the v1 tree, and marked all 8 patches as "v2". Then I in turn
> got confused by that, and changed status on the wrong set. Sorry!
> 
> So, thanks for the heads up, it should be fixed now.
> 
> As for comments on the patches, I'm personally buried in work and others
> may have the merge window to focus on...
> 
> Cheers,
> Peter

No worries. Glad everything is clear now. Thanks for the quick response.

Regarding the patchset comments. Lets wait for the merge window being closed.
Then if no comments will have been received in one-two weeks after that I'll
ping this patchset mailing-list again .)

Cheers,
-Sergey

Re: [PATCH v2 1/3] i2c-mux-gpio: Unpin a platform-based device initialization

[Peter Rosin]

Thanks for your patches, and sorry for the slow review...

On 2019-04-26 01:20, Serge Semin wrote:
> We can unpin a code specific for i2c-mux-gpio device declared

Unpin? I think the common phrase is "factor out"? That unpin is also
present in the subject. BTW, I prefer the subject to start with
[PATCH ...] i2c: mux: gpio: factor out...

> as platform device. In this case the platform data just needs to be
> copied to the private storage and if GPIO chip pointer is referring to
> a valid GPIO chip descriptor save it' base number for further GPIOs
> request and initialization. The rest of the code is common for both
> platform and OF-based setups.
> 
> It's also pointless and might be even errors prone to proceed with
> further initialization if OF kernel config is disabled and plat-based
> initialization isn't defined. Just return an error in this case.

Hmm, there are a couple of other language issues, how about:

Subject: i2c: mux: gpio: factor out platform-based device init

We can factor out the probe code specific for i2c-mux-gpio when used as
a platform device. In this case the platform data just needs to be
copied to the private storage except if the GPIO chip pointer is
referring to a valid GPIO chip descriptor, in which case we save its
base number for further GPIO requests and init. The rest of the code
is common for both platform and OF-based setups.

It's also pointless and might even be error prone to proceed with
further initialization if neither OF nor platform-based parameters
are given. Just error out in this case.

> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> ---
> Changelog v2
> - Return an error if OF kconfig is disabled while dt-based GPIOs probe
>   is called.
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 69 ++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 13882a2a4f60..54158b825acd 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -132,48 +132,55 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
>  					struct platform_device *pdev)
>  {
> -	return 0;
> +	return -EINVAL;

This is unrelated and should be a separate patch, as is almost always the
case when there is an "also" like you have in the commit message.

>  }
>  #endif
>  
> +static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> +					struct platform_device *pdev)

I think you should spell out platform, and please align the arguments
vertically.

> +{
> +	struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
> +	struct gpio_chip *gpio;
> +
> +	/*
> +	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
> +	 * relative to its base GPIO number. Otherwise they are absolute.
> +	 */
> +	if (data->gpio_chip) {
> +		gpio = gpiochip_find(data->gpio_chip,
> +				     match_gpio_chip_by_label);
> +		if (!gpio)
> +			return -EPROBE_DEFER;
> +
> +		mux->gpio_base = gpio->base;
> +	} else {
> +		mux->gpio_base = 0;

This else-branch is pointless. I realize that you are just moving
code around, but mux->gpio_base is already zero here. Could be
simplified in a followup commit, I suppose.

Cheers,
Peter

> +	}
> +
> +	memcpy(&mux->data, data, sizeof(mux->data));
> +
> +	return 0;
> +}
> +
>  static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  {
>  	struct i2c_mux_core *muxc;
>  	struct gpiomux *mux;
>  	struct i2c_adapter *parent;
>  	struct i2c_adapter *root;
> -	unsigned initial_state, gpio_base;
> +	unsigned initial_state;
>  	int i, ret;
>  
>  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
>  	if (!mux)
>  		return -ENOMEM;
>  
> -	if (!dev_get_platdata(&pdev->dev)) {
> +	if (!dev_get_platdata(&pdev->dev))
>  		ret = i2c_mux_gpio_probe_dt(mux, pdev);
> -		if (ret < 0)
> -			return ret;
> -	} else {
> -		memcpy(&mux->data, dev_get_platdata(&pdev->dev),
> -			sizeof(mux->data));
> -	}
> -
> -	/*
> -	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
> -	 * relative to its base GPIO number. Otherwise they are absolute.
> -	 */
> -	if (mux->data.gpio_chip) {
> -		struct gpio_chip *gpio;
> -
> -		gpio = gpiochip_find(mux->data.gpio_chip,
> -				     match_gpio_chip_by_label);
> -		if (!gpio)
> -			return -EPROBE_DEFER;
> -
> -		gpio_base = gpio->base;
> -	} else {
> -		gpio_base = 0;
> -	}
> +	else
> +		ret = i2c_mux_gpio_probe_plat(mux, pdev);
> +	if (ret)
> +		return ret;
>  
>  	parent = i2c_get_adapter(mux->data.parent);
>  	if (!parent)
> @@ -194,7 +201,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	root = i2c_root_adapter(&parent->dev);
>  
>  	muxc->mux_locked = true;
> -	mux->gpio_base = gpio_base;
>  
>  	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
>  		initial_state = mux->data.idle;
> @@ -207,14 +213,15 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  		struct device *gpio_dev;
>  		struct gpio_desc *gpio_desc;
>  
> -		ret = gpio_request(gpio_base + mux->data.gpios[i], "i2c-mux-gpio");
> +		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> +				   "i2c-mux-gpio");
>  		if (ret) {
>  			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
>  				mux->data.gpios[i]);
>  			goto err_request_gpio;
>  		}
>  
> -		ret = gpio_direction_output(gpio_base + mux->data.gpios[i],
> +		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
>  					    initial_state & (1 << i));
>  		if (ret) {
>  			dev_err(&pdev->dev,
> @@ -224,7 +231,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  			goto err_request_gpio;
>  		}
>  
> -		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
> +		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
>  		mux->gpios[i] = gpio_desc;
>  
>  		if (!muxc->mux_locked)
> @@ -256,7 +263,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  	i = mux->data.n_gpios;
>  err_request_gpio:
>  	for (; i > 0; i--)
> -		gpio_free(gpio_base + mux->data.gpios[i - 1]);
> +		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
>  alloc_failed:
>  	i2c_put_adapter(parent);
>  
> 

Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Peter Rosin]

On 2019-04-26 01:20, Serge Semin wrote:
> The GPIOs request loop can be safely moved to a separate function.
> First of all it shall improve the code readability. Secondly the
> initialization loop at this point is used for both of- and
> platform_data-based initialization paths, but it will be changed in
> the next patch, so by isolating the code we'll simplify the future
> work.

This patch is just preparatory for patch 3/3, as I see it. And since
I'm not really fond of the end result after patch 3/3, I'm going to
sum up my issues here, instead of trying do it piecemeal in the two
patches.

Linus and Jean, for your convenience, link to this patch series [1].

While I agree with the goal (to use the more flexible gpiod functions
to get at the gpio descriptors), the cost is too high when the init
code for platform and OF is basically completely separated. I much
prefer the approach taken by Linus [2], which instead converts the
platform interface and its single user to use gpio descriptors instead
of the legacy gpio interface. The i2c-mux-gpio code then has the
potential to take a unified approach to the given gpio descriptors,
wherever they are originating from, which is much nicer than the
code-fork in this series.

I also think it is pretty pointless to first split the code into
platform and OF paths, just so that the next patch (from Linus) can
unify the two paths again. I'd like to skip the intermediate step.

So, I'm hoping for the following to happen.
1. Sergey sends a revised patch for patch 1/3.
2. I put the patch on the for-next branch.
3. Linus rebases his patch on top of that (while thinking about
   the questions raised by Sergey).
4. Sergey tests the result, I and Jean review it, then possibly
   go back to 3.
5. I put the patch on the for-next branch.

Is that ok? Or is someone insisting that we take a detour?

Cheers,
Peter

[1] https://patchwork.ozlabs.org/cover/1091119/ (and show related)
[2] https://patchwork.ozlabs.org/patch/1109521/

> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> ---
> Changelog v2
> - Create a dedicated initial_state field in the "gpiomux" structure to
>   keep an initial channel selector state.
> ---
>  drivers/i2c/muxes/i2c-mux-gpio.c | 113 +++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 54158b825acd..e10f72706b99 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -20,7 +20,8 @@
>  
>  struct gpiomux {
>  	struct i2c_mux_gpio_platform_data data;
> -	unsigned gpio_base;
> +	unsigned int gpio_base;
> +	unsigned int initial_state;
>  	struct gpio_desc **gpios;
>  };
>  
> @@ -162,13 +163,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
>  	return 0;
>  }
>  
> +static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
> +					struct platform_device *pdev)
> +{
> +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> +	struct gpio_desc *gpio_desc;
> +	struct i2c_adapter *root;
> +	struct device *gpio_dev;
> +	int i, ret;
> +
> +	root = i2c_root_adapter(&muxc->parent->dev);
> +
> +	for (i = 0; i < mux->data.n_gpios; i++) {
> +		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> +				   "i2c-mux-gpio");
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> +				mux->data.gpios[i]);
> +			goto err_request_gpio;
> +		}
> +
> +		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> +					    mux->initial_state & (1 << i));
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"Failed to set direction of GPIO %d to output\n",
> +				mux->data.gpios[i]);
> +			i++;	/* gpio_request above succeeded, so must free */
> +			goto err_request_gpio;
> +		}
> +
> +		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> +		mux->gpios[i] = gpio_desc;
> +
> +		if (!muxc->mux_locked)
> +			continue;
> +
> +		gpio_dev = &gpio_desc->gdev->dev;
> +		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> +	}
> +
> +	return 0;
> +
> +err_request_gpio:
> +	for (; i > 0; i--)
> +		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> +
> +	return ret;
> +}
> +
> +static void i2c_mux_gpio_free(struct gpiomux *mux)
> +{
> +	int i;
> +
> +	for (i = 0; i < mux->data.n_gpios; i++)
> +		gpiod_free(mux->gpios[i]);
> +}
> +
>  static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  {
>  	struct i2c_mux_core *muxc;
>  	struct gpiomux *mux;
>  	struct i2c_adapter *parent;
> -	struct i2c_adapter *root;
> -	unsigned initial_state;
>  	int i, ret;
>  
>  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> @@ -198,48 +254,18 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, muxc);
>  
> -	root = i2c_root_adapter(&parent->dev);
> -
>  	muxc->mux_locked = true;
>  
>  	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> -		initial_state = mux->data.idle;
> +		mux->initial_state = mux->data.idle;
>  		muxc->deselect = i2c_mux_gpio_deselect;
>  	} else {
> -		initial_state = mux->data.values[0];
> +		mux->initial_state = mux->data.values[0];
>  	}
>  
> -	for (i = 0; i < mux->data.n_gpios; i++) {
> -		struct device *gpio_dev;
> -		struct gpio_desc *gpio_desc;
> -
> -		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> -				   "i2c-mux-gpio");
> -		if (ret) {
> -			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> -				mux->data.gpios[i]);
> -			goto err_request_gpio;
> -		}
> -
> -		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> -					    initial_state & (1 << i));
> -		if (ret) {
> -			dev_err(&pdev->dev,
> -				"Failed to set direction of GPIO %d to output\n",
> -				mux->data.gpios[i]);
> -			i++;	/* gpio_request above succeeded, so must free */
> -			goto err_request_gpio;
> -		}
> -
> -		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> -		mux->gpios[i] = gpio_desc;
> -
> -		if (!muxc->mux_locked)
> -			continue;
> -
> -		gpio_dev = &gpio_desc->gdev->dev;
> -		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> -	}
> +	ret = i2c_mux_gpio_request_plat(mux, pdev);
> +	if (ret)
> +		goto alloc_failed;
>  
>  	if (muxc->mux_locked)
>  		dev_info(&pdev->dev, "mux-locked i2c mux\n");
> @@ -260,10 +286,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>  
>  add_adapter_failed:
>  	i2c_mux_del_adapters(muxc);
> -	i = mux->data.n_gpios;
> -err_request_gpio:
> -	for (; i > 0; i--)
> -		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> +
> +	i2c_mux_gpio_free(mux);
> +
>  alloc_failed:
>  	i2c_put_adapter(parent);
>  
> @@ -274,12 +299,10 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
>  {
>  	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
>  	struct gpiomux *mux = i2c_mux_priv(muxc);
> -	int i;
>  
>  	i2c_mux_del_adapters(muxc);
>  
> -	for (i = 0; i < mux->data.n_gpios; i++)
> -		gpio_free(mux->gpio_base + mux->data.gpios[i]);
> +	i2c_mux_gpio_free(mux);
>  
>  	i2c_put_adapter(muxc->parent);
>  
> 

Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Serge Semin]

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

Hello Peter,

On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote:
> On 2019-04-26 01:20, Serge Semin wrote:
> > The GPIOs request loop can be safely moved to a separate function.
> > First of all it shall improve the code readability. Secondly the
> > initialization loop at this point is used for both of- and
> > platform_data-based initialization paths, but it will be changed in
> > the next patch, so by isolating the code we'll simplify the future
> > work.
> 
> This patch is just preparatory for patch 3/3, as I see it. And since
> I'm not really fond of the end result after patch 3/3, I'm going to
> sum up my issues here, instead of trying do it piecemeal in the two
> patches.
> 
> Linus and Jean, for your convenience, link to this patch series [1].
> 
> While I agree with the goal (to use the more flexible gpiod functions
> to get at the gpio descriptors), the cost is too high when the init
> code for platform and OF is basically completely separated. I much
> prefer the approach taken by Linus [2], which instead converts the
> platform interface and its single user to use gpio descriptors instead
> of the legacy gpio interface. The i2c-mux-gpio code then has the
> potential to take a unified approach to the given gpio descriptors,
> wherever they are originating from, which is much nicer than the
> code-fork in this series.
> 
> I also think it is pretty pointless to first split the code into
> platform and OF paths, just so that the next patch (from Linus) can
> unify the two paths again. I'd like to skip the intermediate step.
> 
> So, I'm hoping for the following to happen.
> 1. Sergey sends a revised patch for patch 1/3.
> 2. I put the patch on the for-next branch.
> 3. Linus rebases his patch on top of that (while thinking about
>    the questions raised by Sergey).
> 4. Sergey tests the result, I and Jean review it, then possibly
>    go back to 3.
> 5. I put the patch on the for-next branch.
> 
> Is that ok? Or is someone insisting that we take a detour?
> 

The series was intended to add the gpiod support to the i2c-mux-gpio driver
(see the cover letter of the series). So the last patch is the most valuable
one. Without it the whole series is nothing but a small readability improvement.
So it is pointless to merge the first patch only.

Anyway since you refuse to add the last patch and the first patch is actually
pointless without the rest of the series, and I would have to spend my time to
resubmit the v3 of the first patch anyway, it was much easier to test the
current version of the Linus' patch and make it working for OF-based platforms.
Additionally the Linus' patch also reaches the main goal of this patchset.

I don't know what would be the appropriate way to send the updated version of
the Linus' patch. So I just attached the v4 of it to this email. Shall I better
send it in reply to the Linus' patch series?

Regards,
-Sergey


> Cheers,
> Peter
> 
> [1] https://patchwork.ozlabs.org/cover/1091119/ (and show related)
> [2] https://patchwork.ozlabs.org/patch/1109521/
> 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > ---
> > Changelog v2
> > - Create a dedicated initial_state field in the "gpiomux" structure to
> >   keep an initial channel selector state.
> > ---
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 113 +++++++++++++++++++------------
> >  1 file changed, 68 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 54158b825acd..e10f72706b99 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -20,7 +20,8 @@
> >  
> >  struct gpiomux {
> >  	struct i2c_mux_gpio_platform_data data;
> > -	unsigned gpio_base;
> > +	unsigned int gpio_base;
> > +	unsigned int initial_state;
> >  	struct gpio_desc **gpios;
> >  };
> >  
> > @@ -162,13 +163,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> >  	return 0;
> >  }
> >  
> > +static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
> > +					struct platform_device *pdev)
> > +{
> > +	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> > +	struct gpio_desc *gpio_desc;
> > +	struct i2c_adapter *root;
> > +	struct device *gpio_dev;
> > +	int i, ret;
> > +
> > +	root = i2c_root_adapter(&muxc->parent->dev);
> > +
> > +	for (i = 0; i < mux->data.n_gpios; i++) {
> > +		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> > +				   "i2c-mux-gpio");
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> > +				mux->data.gpios[i]);
> > +			goto err_request_gpio;
> > +		}
> > +
> > +		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> > +					    mux->initial_state & (1 << i));
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"Failed to set direction of GPIO %d to output\n",
> > +				mux->data.gpios[i]);
> > +			i++;	/* gpio_request above succeeded, so must free */
> > +			goto err_request_gpio;
> > +		}
> > +
> > +		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> > +		mux->gpios[i] = gpio_desc;
> > +
> > +		if (!muxc->mux_locked)
> > +			continue;
> > +
> > +		gpio_dev = &gpio_desc->gdev->dev;
> > +		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_request_gpio:
> > +	for (; i > 0; i--)
> > +		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> > +
> > +	return ret;
> > +}
> > +
> > +static void i2c_mux_gpio_free(struct gpiomux *mux)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < mux->data.n_gpios; i++)
> > +		gpiod_free(mux->gpios[i]);
> > +}
> > +
> >  static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  {
> >  	struct i2c_mux_core *muxc;
> >  	struct gpiomux *mux;
> >  	struct i2c_adapter *parent;
> > -	struct i2c_adapter *root;
> > -	unsigned initial_state;
> >  	int i, ret;
> >  
> >  	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> > @@ -198,48 +254,18 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, muxc);
> >  
> > -	root = i2c_root_adapter(&parent->dev);
> > -
> >  	muxc->mux_locked = true;
> >  
> >  	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> > -		initial_state = mux->data.idle;
> > +		mux->initial_state = mux->data.idle;
> >  		muxc->deselect = i2c_mux_gpio_deselect;
> >  	} else {
> > -		initial_state = mux->data.values[0];
> > +		mux->initial_state = mux->data.values[0];
> >  	}
> >  
> > -	for (i = 0; i < mux->data.n_gpios; i++) {
> > -		struct device *gpio_dev;
> > -		struct gpio_desc *gpio_desc;
> > -
> > -		ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> > -				   "i2c-mux-gpio");
> > -		if (ret) {
> > -			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> > -				mux->data.gpios[i]);
> > -			goto err_request_gpio;
> > -		}
> > -
> > -		ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> > -					    initial_state & (1 << i));
> > -		if (ret) {
> > -			dev_err(&pdev->dev,
> > -				"Failed to set direction of GPIO %d to output\n",
> > -				mux->data.gpios[i]);
> > -			i++;	/* gpio_request above succeeded, so must free */
> > -			goto err_request_gpio;
> > -		}
> > -
> > -		gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> > -		mux->gpios[i] = gpio_desc;
> > -
> > -		if (!muxc->mux_locked)
> > -			continue;
> > -
> > -		gpio_dev = &gpio_desc->gdev->dev;
> > -		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> > -	}
> > +	ret = i2c_mux_gpio_request_plat(mux, pdev);
> > +	if (ret)
> > +		goto alloc_failed;
> >  
> >  	if (muxc->mux_locked)
> >  		dev_info(&pdev->dev, "mux-locked i2c mux\n");
> > @@ -260,10 +286,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> >  
> >  add_adapter_failed:
> >  	i2c_mux_del_adapters(muxc);
> > -	i = mux->data.n_gpios;
> > -err_request_gpio:
> > -	for (; i > 0; i--)
> > -		gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> > +
> > +	i2c_mux_gpio_free(mux);
> > +
> >  alloc_failed:
> >  	i2c_put_adapter(parent);
> >  
> > @@ -274,12 +299,10 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
> >  {
> >  	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> >  	struct gpiomux *mux = i2c_mux_priv(muxc);
> > -	int i;
> >  
> >  	i2c_mux_del_adapters(muxc);
> >  
> > -	for (i = 0; i < mux->data.n_gpios; i++)
> > -		gpio_free(mux->gpio_base + mux->data.gpios[i]);
> > +	i2c_mux_gpio_free(mux);
> >  
> >  	i2c_put_adapter(muxc->parent);
> >  
> > 
> 

[-- Attachment #2: 0001-i2c-mux-i801-Switch-to-use-descriptor-passing.patch --]
[-- Type: text/x-patch, Size: 12446 bytes --]

From 660ff4062e4cb89601d8c27ff555f7c18ebfafcc Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Tue, 4 Jun 2019 00:08:19 +0200
Subject: [PATCH v4] i2c: mux/i801: Switch to use descriptor passing

This switches the i801 GPIO mux to use GPIO descriptors for
handling the GPIO lines. The previous hack which was reaching
inside the GPIO chips etc cannot live on. We pass descriptors
along with the GPIO mux device at creation instead.

The GPIO mux was only used by way of platform data with a
platform device from one place in the kernel: the i801 i2c bus
driver. Let's just associate the GPIO descriptor table with
the actual device like everyone else and dynamically create
a descriptor table passed along with the GPIO i2c mux.

This enables simplification of the GPIO i2c mux driver to
use only the descriptor API and the OF probe path gets
simplified in the process.

The i801 driver was registering the GPIO i2c mux with
PLATFORM_DEVID_AUTO which would make it hard to predict the
device name and assign the descriptor table properly, but
this seems to be a mistake to begin with: all of the
GPIO mux devices are hardcoded to look up GPIO lines from
the "gpio_ich" GPIO chip. If there are more than one mux,
there is certainly more than one gpio chip as well, and
then we have more serious problems. Switch to
PLATFORM_DEVID_NONE instead. There can be only one.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Peter Rosin <peda@axentia.se>
Cc: Jean Delvare <jdelvare@suse.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
Co-Developed-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

---
ChangeLog v3->v4:
- Use "mux" value as con_id of GPIOd methods.
- Check negative value of gpiod_count() method.

ChangeLog v2->v3:
- Reorder variable declarations to inverse christmas tree.
- Stash away GPIO lookup table reference and make sure to
  remove it on deletion and on error path.
- Insert some nasty FIXME comments about poking around
  in gpiolib internals.
ChangeLog v1->v2:
- Found some unused vars when compiling for DT, mea culpa.

Folks, you surely see what I am trying to do. Would
appreciate help fixing any bugs (it compiles).
---
 drivers/i2c/busses/i2c-i801.c              |  37 +++++--
 drivers/i2c/muxes/i2c-mux-gpio.c           | 115 ++++++---------------
 include/linux/platform_data/i2c-mux-gpio.h |   7 --
 3 files changed, 60 insertions(+), 99 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ac7f7817dc89..ec54b5b4f1a1 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -99,7 +99,7 @@
 #include <linux/pm_runtime.h>
 
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
-#include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/platform_data/i2c-mux-gpio.h>
 #endif
 
@@ -266,6 +266,7 @@ struct i801_priv {
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
 	const struct i801_mux_config *mux_drvdata;
 	struct platform_device *mux_pdev;
+	struct gpiod_lookup_table *lookup;
 #endif
 	struct platform_device *tco_pdev;
 
@@ -1250,7 +1251,8 @@ static int i801_add_mux(struct i801_priv *priv)
 	struct device *dev = &priv->adapter.dev;
 	const struct i801_mux_config *mux_config;
 	struct i2c_mux_gpio_platform_data gpio_data;
-	int err;
+	struct gpiod_lookup_table *lookup;
+	int err, i;
 
 	if (!priv->mux_drvdata)
 		return 0;
@@ -1262,17 +1264,36 @@ static int i801_add_mux(struct i801_priv *priv)
 	gpio_data.values = mux_config->values;
 	gpio_data.n_values = mux_config->n_values;
 	gpio_data.classes = mux_config->classes;
-	gpio_data.gpio_chip = mux_config->gpio_chip;
-	gpio_data.gpios = mux_config->gpios;
-	gpio_data.n_gpios = mux_config->n_gpios;
 	gpio_data.idle = I2C_MUX_GPIO_NO_IDLE;
 
-	/* Register the mux device */
+	/* Register GPIO descriptor lookup table */
+	lookup = devm_kzalloc(dev,
+			      struct_size(lookup, table, mux_config->n_gpios),
+			      GFP_KERNEL);
+	if (!lookup)
+		return -ENOMEM;
+	lookup->dev_id = "i2c-mux-gpio";
+	for (i = 0; i < mux_config->n_gpios; i++) {
+		lookup->table[i].chip_label = mux_config->gpio_chip;
+		lookup->table[i].chip_hwnum = mux_config->gpios[i];
+		lookup->table[i].con_id = "mux";
+	}
+	gpiod_add_lookup_table(lookup);
+	priv->lookup = lookup;
+
+	/*
+	 * Register the mux device, we use PLATFORM_DEVID_NONE here
+	 * because since we are referring to the GPIO chip by name we are
+	 * anyways in deep trouble if there is more than one of these
+	 * devices, and there should likely only be one platform controller
+	 * hub.
+	 */
 	priv->mux_pdev = platform_device_register_data(dev, "i2c-mux-gpio",
-				PLATFORM_DEVID_AUTO, &gpio_data,
+				PLATFORM_DEVID_NONE, &gpio_data,
 				sizeof(struct i2c_mux_gpio_platform_data));
 	if (IS_ERR(priv->mux_pdev)) {
 		err = PTR_ERR(priv->mux_pdev);
+		gpiod_remove_lookup_table(lookup);
 		priv->mux_pdev = NULL;
 		dev_err(dev, "Failed to register i2c-mux-gpio device\n");
 		return err;
@@ -1285,6 +1306,8 @@ static void i801_del_mux(struct i801_priv *priv)
 {
 	if (priv->mux_pdev)
 		platform_device_unregister(priv->mux_pdev);
+	if (priv->lookup)
+		gpiod_remove_lookup_table(priv->lookup);
 }
 
 static unsigned int i801_get_adapter_class(struct i801_priv *priv)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 13882a2a4f60..1ea097dc8d5d 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -14,13 +14,14 @@
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/bits.h>
+#include <linux/gpio/consumer.h>
+/* FIXME: stop poking around inside gpiolib */
 #include "../../gpio/gpiolib.h"
-#include <linux/of_gpio.h>
 
 struct gpiomux {
 	struct i2c_mux_gpio_platform_data data;
-	unsigned gpio_base;
+	int ngpios;
 	struct gpio_desc **gpios;
 };
 
@@ -30,7 +31,7 @@ static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
 
 	values[0] = val;
 
-	gpiod_set_array_value_cansleep(mux->data.n_gpios, mux->gpios, NULL,
+	gpiod_set_array_value_cansleep(mux->ngpios, mux->gpios, NULL,
 				       values);
 }
 
@@ -52,12 +53,6 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan)
 	return 0;
 }
 
-static int match_gpio_chip_by_label(struct gpio_chip *chip,
-					      void *data)
-{
-	return !strcmp(chip->label, data);
-}
-
 #ifdef CONFIG_OF
 static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 					struct platform_device *pdev)
@@ -65,8 +60,8 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *adapter_np, *child;
 	struct i2c_adapter *adapter;
-	unsigned *values, *gpios;
-	int i = 0, ret;
+	unsigned *values;
+	int i = 0;
 
 	if (!np)
 		return -ENODEV;
@@ -103,29 +98,6 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
 	if (of_property_read_u32(np, "idle-state", &mux->data.idle))
 		mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
 
-	mux->data.n_gpios = of_gpio_named_count(np, "mux-gpios");
-	if (mux->data.n_gpios < 0) {
-		dev_err(&pdev->dev, "Missing mux-gpios property in the DT.\n");
-		return -EINVAL;
-	}
-
-	gpios = devm_kcalloc(&pdev->dev,
-			     mux->data.n_gpios, sizeof(*mux->data.gpios),
-			     GFP_KERNEL);
-	if (!gpios) {
-		dev_err(&pdev->dev, "Cannot allocate gpios array");
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < mux->data.n_gpios; i++) {
-		ret = of_get_named_gpio(np, "mux-gpios", i);
-		if (ret < 0)
-			return ret;
-		gpios[i] = ret;
-	}
-
-	mux->data.gpios = gpios;
-
 	return 0;
 }
 #else
@@ -142,8 +114,8 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	struct gpiomux *mux;
 	struct i2c_adapter *parent;
 	struct i2c_adapter *root;
-	unsigned initial_state, gpio_base;
-	int i, ret;
+	unsigned initial_state;
+	int i, ngpios, ret;
 
 	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
 	if (!mux)
@@ -158,29 +130,19 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 			sizeof(mux->data));
 	}
 
-	/*
-	 * If a GPIO chip name is provided, the GPIO pin numbers provided are
-	 * relative to its base GPIO number. Otherwise they are absolute.
-	 */
-	if (mux->data.gpio_chip) {
-		struct gpio_chip *gpio;
-
-		gpio = gpiochip_find(mux->data.gpio_chip,
-				     match_gpio_chip_by_label);
-		if (!gpio)
-			return -EPROBE_DEFER;
-
-		gpio_base = gpio->base;
-	} else {
-		gpio_base = 0;
+	ngpios = gpiod_count(&pdev->dev, "mux");
+	if (ngpios <= 0) {
+		dev_err(&pdev->dev, "no valid gpios provided\n");
+		return ngpios ?: -EINVAL;
 	}
+	mux->ngpios = ngpios;
 
 	parent = i2c_get_adapter(mux->data.parent);
 	if (!parent)
 		return -EPROBE_DEFER;
 
 	muxc = i2c_mux_alloc(parent, &pdev->dev, mux->data.n_values,
-			     mux->data.n_gpios * sizeof(*mux->gpios), 0,
+			     ngpios * sizeof(*mux->gpios), 0,
 			     i2c_mux_gpio_select, NULL);
 	if (!muxc) {
 		ret = -ENOMEM;
@@ -194,7 +156,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 	root = i2c_root_adapter(&parent->dev);
 
 	muxc->mux_locked = true;
-	mux->gpio_base = gpio_base;
 
 	if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
 		initial_state = mux->data.idle;
@@ -203,34 +164,28 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		initial_state = mux->data.values[0];
 	}
 
-	for (i = 0; i < mux->data.n_gpios; i++) {
+	for (i = 0; i < ngpios; i++) {
 		struct device *gpio_dev;
-		struct gpio_desc *gpio_desc;
-
-		ret = gpio_request(gpio_base + mux->data.gpios[i], "i2c-mux-gpio");
-		if (ret) {
-			dev_err(&pdev->dev, "Failed to request GPIO %d\n",
-				mux->data.gpios[i]);
-			goto err_request_gpio;
+		struct gpio_desc *gpiod;
+		enum gpiod_flags flag;
+
+		if (initial_state & BIT(i))
+			flag = GPIOD_OUT_HIGH;
+		else
+			flag = GPIOD_OUT_LOW;
+		gpiod = devm_gpiod_get_index(&pdev->dev, "mux", i, flag);
+		if (IS_ERR(gpiod)) {
+			ret = PTR_ERR(gpiod);
+			goto alloc_failed;
 		}
 
-		ret = gpio_direction_output(gpio_base + mux->data.gpios[i],
-					    initial_state & (1 << i));
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to set direction of GPIO %d to output\n",
-				mux->data.gpios[i]);
-			i++;	/* gpio_request above succeeded, so must free */
-			goto err_request_gpio;
-		}
-
-		gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
-		mux->gpios[i] = gpio_desc;
+		mux->gpios[i] = gpiod;
 
 		if (!muxc->mux_locked)
 			continue;
 
-		gpio_dev = &gpio_desc->gdev->dev;
+		/* FIXME: find a proper way to access the GPIO device */
+		gpio_dev = &gpiod->gdev->dev;
 		muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
 	}
 
@@ -253,10 +208,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 
 add_adapter_failed:
 	i2c_mux_del_adapters(muxc);
-	i = mux->data.n_gpios;
-err_request_gpio:
-	for (; i > 0; i--)
-		gpio_free(gpio_base + mux->data.gpios[i - 1]);
 alloc_failed:
 	i2c_put_adapter(parent);
 
@@ -266,14 +217,8 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 static int i2c_mux_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
-	struct gpiomux *mux = i2c_mux_priv(muxc);
-	int i;
 
 	i2c_mux_del_adapters(muxc);
-
-	for (i = 0; i < mux->data.n_gpios; i++)
-		gpio_free(mux->gpio_base + mux->data.gpios[i]);
-
 	i2c_put_adapter(muxc->parent);
 
 	return 0;
diff --git a/include/linux/platform_data/i2c-mux-gpio.h b/include/linux/platform_data/i2c-mux-gpio.h
index 4406108201fe..28f288eed652 100644
--- a/include/linux/platform_data/i2c-mux-gpio.h
+++ b/include/linux/platform_data/i2c-mux-gpio.h
@@ -22,10 +22,6 @@
  *	position
  * @n_values: Number of multiplexer positions (busses to instantiate)
  * @classes: Optional I2C auto-detection classes
- * @gpio_chip: Optional GPIO chip name; if set, GPIO pin numbers are given
- *	relative to the base GPIO number of that chip
- * @gpios: Array of GPIO numbers used to control MUX
- * @n_gpios: Number of GPIOs used to control MUX
  * @idle: Bitmask to write to MUX when idle or GPIO_I2CMUX_NO_IDLE if not used
  */
 struct i2c_mux_gpio_platform_data {
@@ -34,9 +30,6 @@ struct i2c_mux_gpio_platform_data {
 	const unsigned *values;
 	int n_values;
 	const unsigned *classes;
-	char *gpio_chip;
-	const unsigned *gpios;
-	int n_gpios;
 	unsigned idle;
 };
 
-- 
2.21.0

Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Peter Rosin]

On 2019-06-14 18:31, Serge Semin wrote:
> Hello Peter,
> 
> On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote:
>> On 2019-04-26 01:20, Serge Semin wrote:
>>> The GPIOs request loop can be safely moved to a separate function.
>>> First of all it shall improve the code readability. Secondly the
>>> initialization loop at this point is used for both of- and
>>> platform_data-based initialization paths, but it will be changed in
>>> the next patch, so by isolating the code we'll simplify the future
>>> work.
>>
>> This patch is just preparatory for patch 3/3, as I see it. And since
>> I'm not really fond of the end result after patch 3/3, I'm going to
>> sum up my issues here, instead of trying do it piecemeal in the two
>> patches.
>>
>> Linus and Jean, for your convenience, link to this patch series [1].
>>
>> While I agree with the goal (to use the more flexible gpiod functions
>> to get at the gpio descriptors), the cost is too high when the init
>> code for platform and OF is basically completely separated. I much
>> prefer the approach taken by Linus [2], which instead converts the
>> platform interface and its single user to use gpio descriptors instead
>> of the legacy gpio interface. The i2c-mux-gpio code then has the
>> potential to take a unified approach to the given gpio descriptors,
>> wherever they are originating from, which is much nicer than the
>> code-fork in this series.
>>
>> I also think it is pretty pointless to first split the code into
>> platform and OF paths, just so that the next patch (from Linus) can
>> unify the two paths again. I'd like to skip the intermediate step.
>>
>> So, I'm hoping for the following to happen.
>> 1. Sergey sends a revised patch for patch 1/3.
>> 2. I put the patch on the for-next branch.
>> 3. Linus rebases his patch on top of that (while thinking about
>>    the questions raised by Sergey).
>> 4. Sergey tests the result, I and Jean review it, then possibly
>>    go back to 3.
>> 5. I put the patch on the for-next branch.
>>
>> Is that ok? Or is someone insisting that we take a detour?
>>
> 
> The series was intended to add the gpiod support to the i2c-mux-gpio driver
> (see the cover letter of the series). So the last patch is the most valuable
> one. Without it the whole series is nothing but a small readability improvement.
> So it is pointless to merge the first patch only.

Agreed on all points, except perhaps for the "refuse" part below and
that the readability improvement of patch 1/3 is perhaps not all that
pointless.

> Anyway since you refuse to add the last patch and the first patch is actually
> pointless without the rest of the series, and I would have to spend my time to
> resubmit the v3 of the first patch anyway, it was much easier to test the
> current version of the Linus' patch and make it working for OF-based platforms.
> Additionally the Linus' patch also reaches the main goal of this patchset.

I'm very pleased that you do not feel totally put off, and are willing
to help even if we end up storing your series in /dev/null. Kudos!

> I don't know what would be the appropriate way to send the updated version of
> the Linus' patch. So I just attached the v4 of it to this email. Shall I better
> send it in reply to the Linus' patch series?

I get the impression that you have already done the work? In that case,
how I would proceed would depend on how big the difference is. If it's
just a few one-liners here and there, I think I would make a detailed
review comment so that it is easy for Linus to incorporate the needed
changes. If it's anything even remotely complex I would post an
incremental patch. Of course, the former does not exclude the latter,
but I do think an incremental patch is better than a repost.

Thanks again!

Cheers,
Peter

Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code

[Serge Semin]

On Fri, Jun 14, 2019 at 06:04:33PM +0000, Peter Rosin wrote:
> On 2019-06-14 18:31, Serge Semin wrote:
> > Hello Peter,
> > 
> > On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote:
> >> On 2019-04-26 01:20, Serge Semin wrote:
> >>> The GPIOs request loop can be safely moved to a separate function.
> >>> First of all it shall improve the code readability. Secondly the
> >>> initialization loop at this point is used for both of- and
> >>> platform_data-based initialization paths, but it will be changed in
> >>> the next patch, so by isolating the code we'll simplify the future
> >>> work.
> >>
> >> This patch is just preparatory for patch 3/3, as I see it. And since
> >> I'm not really fond of the end result after patch 3/3, I'm going to
> >> sum up my issues here, instead of trying do it piecemeal in the two
> >> patches.
> >>
> >> Linus and Jean, for your convenience, link to this patch series [1].
> >>
> >> While I agree with the goal (to use the more flexible gpiod functions
> >> to get at the gpio descriptors), the cost is too high when the init
> >> code for platform and OF is basically completely separated. I much
> >> prefer the approach taken by Linus [2], which instead converts the
> >> platform interface and its single user to use gpio descriptors instead
> >> of the legacy gpio interface. The i2c-mux-gpio code then has the
> >> potential to take a unified approach to the given gpio descriptors,
> >> wherever they are originating from, which is much nicer than the
> >> code-fork in this series.
> >>
> >> I also think it is pretty pointless to first split the code into
> >> platform and OF paths, just so that the next patch (from Linus) can
> >> unify the two paths again. I'd like to skip the intermediate step.
> >>
> >> So, I'm hoping for the following to happen.
> >> 1. Sergey sends a revised patch for patch 1/3.
> >> 2. I put the patch on the for-next branch.
> >> 3. Linus rebases his patch on top of that (while thinking about
> >>    the questions raised by Sergey).
> >> 4. Sergey tests the result, I and Jean review it, then possibly
> >>    go back to 3.
> >> 5. I put the patch on the for-next branch.
> >>
> >> Is that ok? Or is someone insisting that we take a detour?
> >>
> > 
> > The series was intended to add the gpiod support to the i2c-mux-gpio driver
> > (see the cover letter of the series). So the last patch is the most valuable
> > one. Without it the whole series is nothing but a small readability improvement.
> > So it is pointless to merge the first patch only.
> 
> Agreed on all points, except perhaps for the "refuse" part below and
> that the readability improvement of patch 1/3 is perhaps not all that
> pointless.
> 
> > Anyway since you refuse to add the last patch and the first patch is actually
> > pointless without the rest of the series, and I would have to spend my time to
> > resubmit the v3 of the first patch anyway, it was much easier to test the
> > current version of the Linus' patch and make it working for OF-based platforms.
> > Additionally the Linus' patch also reaches the main goal of this patchset.
> 
> I'm very pleased that you do not feel totally put off, and are willing
> to help even if we end up storing your series in /dev/null. Kudos!
> 
> > I don't know what would be the appropriate way to send the updated version of
> > the Linus' patch. So I just attached the v4 of it to this email. Shall I better
> > send it in reply to the Linus' patch series?
> 
> I get the impression that you have already done the work? In that case,
> how I would proceed would depend on how big the difference is. If it's
> just a few one-liners here and there, I think I would make a detailed
> review comment so that it is easy for Linus to incorporate the needed
> changes. If it's anything even remotely complex I would post an
> incremental patch. Of course, the former does not exclude the latter,
> but I do think an incremental patch is better than a repost.
> 

Yes, I tested the Linus' patch on my OF-based platform (though had to port to
kernel 4.9) and found two problems. The incremental patch, which fixes them is
send to you with mailing lists and everyone involved being Cc'ed.

Regards,
-Sergey

> Thanks again!
> 
> Cheers,
> Peter