[PATCH 0/2] i2c: mux: pinctrl: remove platform_data and cleanup

[PATCH 0/2] i2c: mux: pinctrl: remove platform_data and cleanup

[Peter Rosin]

Hi!

As previously discussed [1], the platform_data interface of the i2c mux
pinctrl driver has never been used (upstream at least). Deleting code
is always nice, so here are two patches that gets rid of some lines...

Cheers,
peda

[1] https://lkml.org/lkml/2017/5/22/104

Peter Rosin (2):
  i2c: mux: pinctrl: remove platform_data
  i2c: mux: pinctrl: drop the idle_state member

 drivers/i2c/muxes/Kconfig           |   1 +
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 223 ++++++++++++------------------------
 include/linux/i2c-mux-pinctrl.h     |  41 -------
 3 files changed, 72 insertions(+), 193 deletions(-)
 delete mode 100644 include/linux/i2c-mux-pinctrl.h

-- 
2.11.0

[PATCH 1/2] i2c: mux: pinctrl: remove platform_data

[Peter Rosin]

No platform (at least no upstreamed platform) has ever used this
platform_data. Just drop it and simplify the code.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/Kconfig           |   1 +
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 219 ++++++++++++------------------------
 include/linux/i2c-mux-pinctrl.h     |  41 -------
 3 files changed, 72 insertions(+), 189 deletions(-)
 delete mode 100644 include/linux/i2c-mux-pinctrl.h

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 2c64d0e0740f..1bba95ecc7c0 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -76,6 +76,7 @@ config I2C_MUX_PCA954x
 config I2C_MUX_PINCTRL
 	tristate "pinctrl-based I2C multiplexer"
 	depends on PINCTRL
+	depends on OF || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for an I2C
 	  multiplexer that uses the pinctrl subsystem, i.e. pin multiplexing.
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index 7c0c264b07bc..aa4a3bf9507f 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -20,14 +20,12 @@
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
-#include <linux/i2c-mux-pinctrl.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include "../../pinctrl/core.h"
 
 struct i2c_mux_pinctrl {
-	struct i2c_mux_pinctrl_platform_data *pdata;
 	struct pinctrl *pinctrl;
 	struct pinctrl_state **states;
 	struct pinctrl_state *state_idle;
@@ -47,80 +45,6 @@ static int i2c_mux_pinctrl_deselect(struct i2c_mux_core *muxc, u32 chan)
 	return pinctrl_select_state(mux->pinctrl, mux->state_idle);
 }
 
-#ifdef CONFIG_OF
-static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
-				    struct platform_device *pdev)
-{
-	struct device_node *np = pdev->dev.of_node;
-	int num_names, i, ret;
-	struct device_node *adapter_np;
-	struct i2c_adapter *adapter;
-
-	if (!np)
-		return 0;
-
-	mux->pdata = devm_kzalloc(&pdev->dev, sizeof(*mux->pdata), GFP_KERNEL);
-	if (!mux->pdata)
-		return -ENOMEM;
-
-	num_names = of_property_count_strings(np, "pinctrl-names");
-	if (num_names < 0) {
-		dev_err(&pdev->dev, "Cannot parse pinctrl-names: %d\n",
-			num_names);
-		return num_names;
-	}
-
-	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
-		sizeof(*mux->pdata->pinctrl_states) * num_names,
-		GFP_KERNEL);
-	if (!mux->pdata->pinctrl_states)
-		return -ENOMEM;
-
-	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "Cannot parse pinctrl-names: %d\n",
-				ret);
-			return ret;
-		}
-		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
-			    "idle")) {
-			if (i != num_names - 1) {
-				dev_err(&pdev->dev,
-					"idle state must be last\n");
-				return -EINVAL;
-			}
-			mux->pdata->pinctrl_state_idle = "idle";
-		} else {
-			mux->pdata->bus_count++;
-		}
-	}
-
-	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
-	if (!adapter_np) {
-		dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
-		return -ENODEV;
-	}
-	adapter = of_find_i2c_adapter_by_node(adapter_np);
-	of_node_put(adapter_np);
-	if (!adapter) {
-		dev_err(&pdev->dev, "Cannot find parent bus\n");
-		return -EPROBE_DEFER;
-	}
-	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
-	put_device(&adapter->dev);
-
-	return 0;
-}
-#else
-static inline int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
-					   struct platform_device *pdev)
-{
-	return 0;
-}
-#endif
-
 static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
 	struct pinctrl_state *state)
 {
@@ -141,110 +65,109 @@ static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
 	return root;
 }
 
+static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *parent_np;
+	struct i2c_adapter *parent;
+
+	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	if (!parent_np) {
+		dev_err(dev, "Cannot parse i2c-parent\n");
+		return ERR_PTR(-ENODEV);
+	}
+	parent = of_find_i2c_adapter_by_node(parent_np);
+	of_node_put(parent_np);
+	if (!parent)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return parent;
+}
+
 static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct i2c_mux_core *muxc;
 	struct i2c_mux_pinctrl *mux;
+	struct i2c_adapter *parent;
 	struct i2c_adapter *root;
-	int i, ret;
-
-	mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
-	if (!mux) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	int num_names, i, ret;
+	const char *name;
 
-	mux->pdata = dev_get_platdata(&pdev->dev);
-	if (!mux->pdata) {
-		ret = i2c_mux_pinctrl_parse_dt(mux, pdev);
-		if (ret < 0)
-			goto err;
-	}
-	if (!mux->pdata) {
-		dev_err(&pdev->dev, "Missing platform data\n");
-		ret = -ENODEV;
-		goto err;
+	num_names = of_property_count_strings(np, "pinctrl-names");
+	if (num_names < 0) {
+		dev_err(dev, "Cannot parse pinctrl-names: %d\n",
+			num_names);
+		return num_names;
 	}
 
-	mux->states = devm_kzalloc(&pdev->dev,
-				   sizeof(*mux->states) * mux->pdata->bus_count,
-				   GFP_KERNEL);
-	if (!mux->states) {
-		dev_err(&pdev->dev, "Cannot allocate states\n");
-		ret = -ENOMEM;
-		goto err;
-	}
+	parent = i2c_mux_pinctrl_parent_adapter(dev);
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
 
-	muxc = i2c_mux_alloc(NULL, &pdev->dev, mux->pdata->bus_count, 0, 0,
-			     i2c_mux_pinctrl_select, NULL);
+	muxc = i2c_mux_alloc(parent, dev, num_names,
+			     sizeof(*mux) + num_names * sizeof(*mux->states),
+			     0, i2c_mux_pinctrl_select, NULL);
 	if (!muxc) {
 		ret = -ENOMEM;
-		goto err;
+		goto err_put_parent;
 	}
-	muxc->priv = mux;
+	mux = i2c_mux_priv(muxc);
+	mux->states = (struct pinctrl_state **)(mux + 1);
 
 	platform_set_drvdata(pdev, muxc);
 
-	mux->pinctrl = devm_pinctrl_get(&pdev->dev);
+	mux->pinctrl = devm_pinctrl_get(dev);
 	if (IS_ERR(mux->pinctrl)) {
 		ret = PTR_ERR(mux->pinctrl);
-		dev_err(&pdev->dev, "Cannot get pinctrl: %d\n", ret);
-		goto err;
+		dev_err(dev, "Cannot get pinctrl: %d\n", ret);
+		goto err_put_parent;
 	}
-	for (i = 0; i < mux->pdata->bus_count; i++) {
-		mux->states[i] = pinctrl_lookup_state(mux->pinctrl,
-						mux->pdata->pinctrl_states[i]);
+
+	for (i = 0; i < num_names; i++) {
+		ret = of_property_read_string_index(np, "pinctrl-names", i,
+						    &name);
+		if (ret < 0) {
+			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
+			goto err_put_parent;
+		}
+
+		mux->states[i] = pinctrl_lookup_state(mux->pinctrl, name);
 		if (IS_ERR(mux->states[i])) {
 			ret = PTR_ERR(mux->states[i]);
-			dev_err(&pdev->dev,
-				"Cannot look up pinctrl state %s: %d\n",
-				mux->pdata->pinctrl_states[i], ret);
-			goto err;
-		}
-	}
-	if (mux->pdata->pinctrl_state_idle) {
-		mux->state_idle = pinctrl_lookup_state(mux->pinctrl,
-						mux->pdata->pinctrl_state_idle);
-		if (IS_ERR(mux->state_idle)) {
-			ret = PTR_ERR(mux->state_idle);
-			dev_err(&pdev->dev,
-				"Cannot look up pinctrl state %s: %d\n",
-				mux->pdata->pinctrl_state_idle, ret);
-			goto err;
+			dev_err(dev, "Cannot look up pinctrl state %s: %d\n",
+				name, ret);
+			goto err_put_parent;
 		}
 
-		muxc->deselect = i2c_mux_pinctrl_deselect;
-	}
+		if (strcmp(name, "idle"))
+			continue;
 
-	muxc->parent = i2c_get_adapter(mux->pdata->parent_bus_num);
-	if (!muxc->parent) {
-		dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
-			mux->pdata->parent_bus_num);
-		ret = -EPROBE_DEFER;
-		goto err;
+		if (i != num_names - 1) {
+			dev_err(dev, "idle state must be last\n");
+			ret = -EINVAL;
+			goto err_put_parent;
+		}
+		mux->state_idle = mux->states[i];
+		muxc->deselect = i2c_mux_pinctrl_deselect;
 	}
 
 	root = i2c_root_adapter(&muxc->parent->dev);
 
 	muxc->mux_locked = true;
-	for (i = 0; i < mux->pdata->bus_count; i++) {
+	for (i = 0; i < num_names; i++) {
 		if (root != i2c_mux_pinctrl_root_adapter(mux->states[i])) {
 			muxc->mux_locked = false;
 			break;
 		}
 	}
-	if (muxc->mux_locked && mux->pdata->pinctrl_state_idle &&
-	    root != i2c_mux_pinctrl_root_adapter(mux->state_idle))
-		muxc->mux_locked = false;
-
 	if (muxc->mux_locked)
-		dev_info(&pdev->dev, "mux-locked i2c mux\n");
+		dev_info(dev, "mux-locked i2c mux\n");
 
-	for (i = 0; i < mux->pdata->bus_count; i++) {
-		u32 bus = mux->pdata->base_bus_num ?
-				(mux->pdata->base_bus_num + i) : 0;
-
-		ret = i2c_mux_add_adapter(muxc, bus, i, 0);
+	/* Do not add any adapter for the idle state (if it's there at all). */
+	for (i = 0; i < num_names - !!mux->state_idle; i++) {
+		ret = i2c_mux_add_adapter(muxc, 0, i, 0);
 		if (ret)
 			goto err_del_adapter;
 	}
@@ -253,8 +176,9 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 
 err_del_adapter:
 	i2c_mux_del_adapters(muxc);
+err_put_parent:
 	i2c_put_adapter(muxc->parent);
-err:
+
 	return ret;
 }
 
@@ -264,16 +188,15 @@ static int i2c_mux_pinctrl_remove(struct platform_device *pdev)
 
 	i2c_mux_del_adapters(muxc);
 	i2c_put_adapter(muxc->parent);
+
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id i2c_mux_pinctrl_of_match[] = {
 	{ .compatible = "i2c-mux-pinctrl", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_mux_pinctrl_of_match);
-#endif
 
 static struct platform_driver i2c_mux_pinctrl_driver = {
 	.driver	= {
diff --git a/include/linux/i2c-mux-pinctrl.h b/include/linux/i2c-mux-pinctrl.h
deleted file mode 100644
index a65c86429e84..000000000000
--- a/include/linux/i2c-mux-pinctrl.h
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * i2c-mux-pinctrl platform data
- *
- * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef _LINUX_I2C_MUX_PINCTRL_H
-#define _LINUX_I2C_MUX_PINCTRL_H
-
-/**
- * struct i2c_mux_pinctrl_platform_data - Platform data for i2c-mux-pinctrl
- * @parent_bus_num: Parent I2C bus number
- * @base_bus_num: Base I2C bus number for the child busses. 0 for dynamic.
- * @bus_count: Number of child busses. Also the number of elements in
- *	@pinctrl_states
- * @pinctrl_states: The names of the pinctrl state to select for each child bus
- * @pinctrl_state_idle: The pinctrl state to select when no child bus is being
- *	accessed. If NULL, the most recently used pinctrl state will be left
- *	selected.
- */
-struct i2c_mux_pinctrl_platform_data {
-	int parent_bus_num;
-	int base_bus_num;
-	int bus_count;
-	const char **pinctrl_states;
-	const char *pinctrl_state_idle;
-};
-
-#endif
-- 
2.11.0

Re: [PATCH 1/2] i2c: mux: pinctrl: remove platform_data

[Stephen Warren]

On 08/02/2017 01:27 AM, Peter Rosin wrote:
> No platform (at least no upstreamed platform) has ever used this
> platform_data. Just drop it and simplify the code.

> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c

>   static int i2c_mux_pinctrl_probe(struct platform_device *pdev)

(eliding some - lines for brevity in the following):

> +	for (i = 0; i < num_names; i++) {
> +		ret = of_property_read_string_index(np, "pinctrl-names", i,
> +						    &name);
> +		if (ret < 0) {
> +			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
> +			goto err_put_parent;
> +		}
> +
> +		mux->states[i] = pinctrl_lookup_state(mux->pinctrl, name);
>   		if (IS_ERR(mux->states[i])) {
>   			ret = PTR_ERR(mux->states[i]);
> +			dev_err(dev, "Cannot look up pinctrl state %s: %d\n",
> +				name, ret);
> +			goto err_put_parent;

This error path doesn't undo pinctrl_lookup_state. Is that OK? I think 
so, but wanted to check.

> +	muxc = i2c_mux_alloc(parent, dev, num_names,
> +			     sizeof(*mux) + num_names * sizeof(*mux->states),
> +			     0, i2c_mux_pinctrl_select, NULL);
...
> +	/* Do not add any adapter for the idle state (if it's there at all). */
> +	for (i = 0; i < num_names - !!mux->state_idle; i++) {
> +		ret = i2c_mux_add_adapter(muxc, 0, i, 0);

Is it OK to potentially add one fewer adapter here than the child bus 
count passed to i2c_mux_alloc() earlier? The old code specifically 
excluded the idle state (if present) from the child bus count passed to 
i2c_mux_alloc(), which was aided by the fact that it parsed the DT 
before calling i2c_mux_alloc().

If those two things are OK, then:
Reviewed-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH 1/2] i2c: mux: pinctrl: remove platform_data

[Peter Rosin]

On 2017-08-02 21:05, Stephen Warren wrote:
> On 08/02/2017 01:27 AM, Peter Rosin wrote:
>> No platform (at least no upstreamed platform) has ever used this
>> platform_data. Just drop it and simplify the code.
> 
>> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> 
>>   static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
> 
> (eliding some - lines for brevity in the following):
> 
>> +	for (i = 0; i < num_names; i++) {
>> +		ret = of_property_read_string_index(np, "pinctrl-names", i,
>> +						    &name);
>> +		if (ret < 0) {
>> +			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
>> +			goto err_put_parent;
>> +		}
>> +
>> +		mux->states[i] = pinctrl_lookup_state(mux->pinctrl, name);
>>   		if (IS_ERR(mux->states[i])) {
>>   			ret = PTR_ERR(mux->states[i]);
>> +			dev_err(dev, "Cannot look up pinctrl state %s: %d\n",
>> +				name, ret);
>> +			goto err_put_parent;
> 
> This error path doesn't undo pinctrl_lookup_state. Is that OK? I think 
> so, but wanted to check.

I also think so, looking at pinctrl_lookup_state, it seems to just match
strings and return a pointer. No refcounts or other state change involved
that I can see. Either way, the preexisting code would have the same issue
so it would be orthogonal and fodder for another patch...

>> +	muxc = i2c_mux_alloc(parent, dev, num_names,
>> +			     sizeof(*mux) + num_names * sizeof(*mux->states),
>> +			     0, i2c_mux_pinctrl_select, NULL);
> ...
>> +	/* Do not add any adapter for the idle state (if it's there at all). */
>> +	for (i = 0; i < num_names - !!mux->state_idle; i++) {
>> +		ret = i2c_mux_add_adapter(muxc, 0, i, 0);
> 
> Is it OK to potentially add one fewer adapter here than the child bus 
> count passed to i2c_mux_alloc() earlier? The old code specifically 
> excluded the idle state (if present) from the child bus count passed to 
> i2c_mux_alloc(), which was aided by the fact that it parsed the DT 
> before calling i2c_mux_alloc().

Yes, that is perfectly fine. The only issue is wasting space for one extra
pointer.

> If those two things are OK, then:
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Thanks!

Cheers,
Peter

Re: [PATCH 1/2] i2c: mux: pinctrl: remove platform_data

[Wolfram Sang]

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

On Wed, Aug 02, 2017 at 09:27:27AM +0200, Peter Rosin wrote:
> No platform (at least no upstreamed platform) has ever used this
> platform_data. Just drop it and simplify the code.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Very nice!

Acked-by: Wolfram Sang <wsa@the-dreams.de>


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

[PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Peter Rosin]

The information is available elsewhere.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index aa4a3bf9507f..20b90a7a1e61 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -28,7 +28,6 @@
 struct i2c_mux_pinctrl {
 	struct pinctrl *pinctrl;
 	struct pinctrl_state **states;
-	struct pinctrl_state *state_idle;
 };
 
 static int i2c_mux_pinctrl_select(struct i2c_mux_core *muxc, u32 chan)
@@ -40,9 +39,7 @@ static int i2c_mux_pinctrl_select(struct i2c_mux_core *muxc, u32 chan)
 
 static int i2c_mux_pinctrl_deselect(struct i2c_mux_core *muxc, u32 chan)
 {
-	struct i2c_mux_pinctrl *mux = i2c_mux_priv(muxc);
-
-	return pinctrl_select_state(mux->pinctrl, mux->state_idle);
+	return i2c_mux_pinctrl_select(muxc, muxc->num_adapters);
 }
 
 static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
@@ -149,7 +146,6 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 			ret = -EINVAL;
 			goto err_put_parent;
 		}
-		mux->state_idle = mux->states[i];
 		muxc->deselect = i2c_mux_pinctrl_deselect;
 	}
 
@@ -166,7 +162,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 		dev_info(dev, "mux-locked i2c mux\n");
 
 	/* Do not add any adapter for the idle state (if it's there at all). */
-	for (i = 0; i < num_names - !!mux->state_idle; i++) {
+	for (i = 0; i < num_names - !!muxc->deselect; i++) {
 		ret = i2c_mux_add_adapter(muxc, 0, i, 0);
 		if (ret)
 			goto err_del_adapter;
-- 
2.11.0

Re: [PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Stephen Warren]

On 08/02/2017 01:27 AM, Peter Rosin wrote:
> The information is available elsewhere.

> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c

>   static int i2c_mux_pinctrl_deselect(struct i2c_mux_core *muxc, u32 chan)
>   {
> +	return i2c_mux_pinctrl_select(muxc, muxc->num_adapters);
>   }

> @@ -166,7 +162,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)

>   	/* Do not add any adapter for the idle state (if it's there at all). */
> -	for (i = 0; i < num_names - !!mux->state_idle; i++) {
> +	for (i = 0; i < num_names - !!muxc->deselect; i++) {

I think that "num_names - !!muxc->deselect" could just be 
muxc->num_adapters? Otherwise,

Reviewed-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Peter Rosin]

On 2017-08-02 21:06, Stephen Warren wrote:
> On 08/02/2017 01:27 AM, Peter Rosin wrote:
>> The information is available elsewhere.
> 
>> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> 
>>   static int i2c_mux_pinctrl_deselect(struct i2c_mux_core *muxc, u32 chan)
>>   {
>> +	return i2c_mux_pinctrl_select(muxc, muxc->num_adapters);
>>   }
> 
>> @@ -166,7 +162,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
> 
>>   	/* Do not add any adapter for the idle state (if it's there at all). */
>> -	for (i = 0; i < num_names - !!mux->state_idle; i++) {
>> +	for (i = 0; i < num_names - !!muxc->deselect; i++) {
> 
> I think that "num_names - !!muxc->deselect" could just be 
> muxc->num_adapters? 

Not really, it's the i2c_mux_add_adapter call in the loop that bumps
muxc->num_adapters, so the loop would not be entered. Not desirable :-)

(and muxc->max_adapters == num_names)

>                     Otherwise,
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Thanks!

Cheers,
Peter

Re: [PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Stephen Warren]

On 08/02/2017 03:25 PM, Peter Rosin wrote:
> On 2017-08-02 21:06, Stephen Warren wrote:
>> On 08/02/2017 01:27 AM, Peter Rosin wrote:
>>> The information is available elsewhere.
>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
>>
>>>    static int i2c_mux_pinctrl_deselect(struct i2c_mux_core *muxc, u32 chan)
>>>    {
>>> +	return i2c_mux_pinctrl_select(muxc, muxc->num_adapters);
>>>    }
>>
>>> @@ -166,7 +162,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
>>
>>>    	/* Do not add any adapter for the idle state (if it's there at all). */
>>> -	for (i = 0; i < num_names - !!mux->state_idle; i++) {
>>> +	for (i = 0; i < num_names - !!muxc->deselect; i++) {
>>
>> I think that "num_names - !!muxc->deselect" could just be
>> muxc->num_adapters?
> 
> Not really, it's the i2c_mux_add_adapter call in the loop that bumps
> muxc->num_adapters, so the loop would not be entered. Not desirable :-)

Ok, that makes sense.

> (and muxc->max_adapters == num_names)

Well, unless muxc->deselect is true...

Re: [PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Peter Rosin]

On 2017-08-03 00:50, Stephen Warren wrote:
> On 08/02/2017 03:25 PM, Peter Rosin wrote:
>> (and muxc->max_adapters == num_names)
> 
> Well, unless muxc->deselect is true...

No, deselect does not affect neither max_adapters nor num_names. They
are always equal.

Cheers,
Peter

Re: [PATCH 2/2] i2c: mux: pinctrl: drop the idle_state member

[Stephen Warren]

On 08/02/2017 11:19 PM, Peter Rosin wrote:
> On 2017-08-03 00:50, Stephen Warren wrote:
>> On 08/02/2017 03:25 PM, Peter Rosin wrote:
>>> (and muxc->max_adapters == num_names)
>>
>> Well, unless muxc->deselect is true...
> 
> No, deselect does not affect neither max_adapters nor num_names. They
> are always equal.

Ah yes, I was confusing max_adapters with num_adapters.

Re: [PATCH 0/2] i2c: mux: pinctrl: remove platform_data and cleanup

[Peter Rosin]

On 2017-08-02 09:27, Peter Rosin wrote:
> Hi!
> 
> As previously discussed [1], the platform_data interface of the i2c mux
> pinctrl driver has never been used (upstream at least). Deleting code
> is always nice, so here are two patches that gets rid of some lines...

Both patches applied (with added tags) to my i2c-mux/for-next branch.

Cheers,
peda