2017-08-02 07:27:51

by Peter Rosin

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

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


2017-08-02 07:27:59

by Peter Rosin

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

The information is available elsewhere.

Signed-off-by: Peter Rosin <[email protected]>
---
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

2017-08-02 07:28:18

by Peter Rosin

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

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 <[email protected]>
---
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

2017-08-02 19:12:26

by Stephen Warren

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

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 <[email protected]>

2017-08-02 19:12:24

by Stephen Warren

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

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 <[email protected]>

2017-08-02 21:20:06

by Peter Rosin

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

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 <[email protected]>

Thanks!

Cheers,
Peter

2017-08-02 21:25:12

by Peter Rosin

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

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 <[email protected]>

Thanks!

Cheers,
Peter

2017-08-02 22:50:42

by Stephen Warren

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

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

2017-08-03 05:19:27

by Peter Rosin

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

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

2017-08-03 21:41:41

by Stephen Warren

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

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.

2017-08-12 14:12:16

by Wolfram Sang

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

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 <[email protected]>

Very nice!

Acked-by: Wolfram Sang <[email protected]>


Attachments:
(No filename) (288.00 B)
signature.asc (833.00 B)
Download all attachments

2017-08-15 07:00:22

by Peter Rosin

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

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