On 2017-01-27 16:52, Rob Herring wrote:
> On Mon, Jan 23, 2017 at 11:24:18AM +0100, Peter Rosin wrote:
>> On 2017-01-22 14:30, Jonathan Cameron wrote:
>>> On 18/01/17 15:57, Peter Rosin wrote:
>>>> Allow bindings for a GPIO controlled mux to be specified in the
>>>> mux consumer node.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>>> Code is good as far as I am concerned. Only question is whether this
>>
>> Hmmm, now that I think some more about it, the code supporting the
>> simplified binding (patch 12/12) is a bit fishy in one respect.
>>
>> A driver that calls mux_control_get and gets a mux_control that happens
>> to be backed by an implicit mux chip (i.e. using the simplified binding)
>> will not be able to reverse the resource allocation with less than a
>> complete destruction of itself. Now, this is likely not a problem in
>> most cases, but I bet it will creep up at the most inopportune time. And
>> your remark that I'm the one that has to maintain this makes me dislike
>> this concept...
>>
>> I.e. mux_control_put *should* reverse mux_control_get, but this simply
>> does not happen for the implicit mux chips, as implicit mux chips are
>> not put away until the owning device is put away.
>
> I think this is because you aren't creating a device in this case. Nodes
> in DT are not the only way to create devices. Drivers can create a child
> device when they find mux-gpios property.
Yes, but even with such a child device, a flag is needed somewhere that
triggers cleanup when the mux_control is put away. And then it is possible
to cleanup w/o the help of a child device. I wrote some code for this when
I realized the problem, and it looks simple enough, but I haven't tested
it yet, so who knows... It is attached (patch to be applied on top of 12/12)
if anyone cares.
>> Every time I have tried to come up with a way to implement the simplified
>> bindings I seem to hit one of these subtleties.
>>
>>> is worth the hassle given the normal bindings don't give that high
>>> a burden in complexity!
>
> I was going to change my mind here, but we already have "mux-gpios" as a
> binding at least for i2c-gpio-mux. So really the question is do we want
> to support that here?
I think my preference is to drop the simplified binding, but I can also
live with it. But as there appears to be no strong feelings, let's just
drop it. It is always possible to add it later. Ok?
>> I am missing an ack from Rob though.
>>
>>> I don't really care either way:)
>>
>> But Rob seems to care, this series just has to find a way to get out of
>> his too-much-churn-will-look-at-it-later list. I sadly don't know how to
>> pull that trick...
>
> By complaining that I'm putting it off... :) I guess I'm okay with this
> series in general. I will reply on the specific patches today.
Great, it appears that I'm quite the magician. :-) Thanks!
Cheers,
peda
>From 5c448b8dfd831c7bc501d9543d48b2077ee1ba7b Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@xxxxxxxxxx>
Date: Tue, 24 Jan 2017 16:58:58 +0100
Subject: [PATCH] mux: fix cleanup for simplified bindings
---
drivers/mux/mux-core.c | 19 ++++++++++++++++++-
include/linux/mux.h | 2 ++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index 0caafd6f5a77..53954bd12709 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -321,9 +321,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
if (ret == -ENOENT && !mux_name) {
mux_chip = mux_gpio_alloc(dev);
if (!IS_ERR(mux_chip)) {
+ mux_chip->private = true;
ret = devm_mux_chip_register(dev, mux_chip);
- if (ret < 0)
+ if (ret < 0) {
+ devm_mux_chip_free(dev, mux_chip);
return ERR_PTR(ret);
+ }
get_device(&mux_chip->dev);
return mux_chip->mux;
}
@@ -344,6 +347,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
if (!mux_chip)
return ERR_PTR(-EPROBE_DEFER);
+ if (mux_chip->private) {
+ dev_err(dev, "%s: private mux chip specified in %s\n",
+ np->full_name, args.np->full_name);
+ return ERR_PTR(-EBUSY);
+ }
+
if (args.args_count > 1 ||
(!args.args_count && (mux_chip->controllers > 1))) {
dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
@@ -368,7 +377,15 @@ EXPORT_SYMBOL_GPL(mux_control_get);
void mux_control_put(struct mux_control *mux)
{
+ bool private = mux->chip->private;
+ struct device *parent = mux->chip->dev.parent;
+
put_device(&mux->chip->dev);
+
+ if (private) {
+ devm_mux_chip_unregister(parent, mux->chip);
+ devm_mux_chip_free(parent, mux->chip);
+ }
}
EXPORT_SYMBOL_GPL(mux_control_put);
diff --git a/include/linux/mux.h b/include/linux/mux.h
index ec9e605d8acf..3ad2e475c9dd 100644
--- a/include/linux/mux.h
+++ b/include/linux/mux.h
@@ -49,6 +49,7 @@ struct mux_control {
* @mux: Array of mux controllers that is handled.
* @dev: Device structure.
* @id: Used to identify the device internally.
+ * @private: The mux chip is implicitly allocated by a single user.
* @ops: Mux controller operations.
*/
struct mux_chip {
@@ -56,6 +57,7 @@ struct mux_chip {
struct mux_control *mux;
struct device dev;
int id;
+ bool private;
const struct mux_control_ops *ops;
};
--
2.1.4
