| Message ID | 20170714214005.14967-2-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Generally looks like I imagined, but there are a few nits and some things that I'd like to do differently. Comments inline. Thanks! On 2017-07-14 23:40, Stephen Boyd wrote: > Sometimes drivers only use muxes under certain scenarios. For > example, the chipidea usb controller may be connected to a usb > switch on some platforms, and connected directly to a usb port on > others. The driver won't know one way or the other though, so add > a mux_control_get_optional() API that allows the driver to > differentiate errors getting the mux from there not being a mux > for the driver to use at all. > > Cc: Jonathan Cameron <jic23@kernel.org> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > Documentation/driver-model/devres.txt | 1 + > drivers/mux/mux-core.c | 98 ++++++++++++++++++++++++++++------- > include/linux/mux/consumer.h | 4 ++ > 3 files changed, 83 insertions(+), 20 deletions(-) > > diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt > index 30e04f7a690d..4fdd3e63ff8b 100644 > --- a/Documentation/driver-model/devres.txt > +++ b/Documentation/driver-model/devres.txt > @@ -342,6 +342,7 @@ MUX > devm_mux_chip_alloc() > devm_mux_chip_register() > devm_mux_control_get() > + devm_mux_control_get_optional() > > PER-CPU MEM > devm_alloc_percpu() > diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c > index 90b8995f07cb..a0e5bf16f02f 100644 > --- a/drivers/mux/mux-core.c > +++ b/drivers/mux/mux-core.c > @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register); > */ > unsigned int mux_control_states(struct mux_control *mux) > { > + if (!mux) > + return 0; > + I don't think this is appropriate. For this function, it might be ok, but... > return mux->states; > } > EXPORT_SYMBOL_GPL(mux_control_states); > @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state) > { > int ret; > > + if (!mux) > + return 0; > + ...here and for other cases below it's very odd to return "ok", when -EINVAL or something seems much more appropriate. And if -EINVAL is returned here, the benefit of returning fake values for anything pretty much falls apart. I simply don't like it, and prefer if the consumer code is arranged to not call the mux functions when the optional get() does not find the mux. > ret = down_killable(&mux->lock); > if (ret < 0) > return ret; > @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state) > { > int ret; > > + if (!mux) > + return 0; > + > if (down_trylock(&mux->lock)) > return -EBUSY; > > @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux) > { > int ret = 0; > > + if (!mux) > + return 0; > + > if (mux->idle_state != MUX_IDLE_AS_IS && > mux->idle_state != mux->cached_state) > ret = mux_control_set(mux, mux->idle_state); > @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np) > return dev ? to_mux_chip(dev) : NULL; > } > > -/** > - * mux_control_get() - Get the mux-control for a device. > - * @dev: The device that needs a mux-control. > - * @mux_name: The name identifying the mux-control. > - * > - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. > - */ > -struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > +struct mux_control * > +__mux_control_get(struct device *dev, const char *mux_name, bool optional) > { > struct device_node *np = dev->of_node; > struct of_phandle_args args; > @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > if (mux_name) { > index = of_property_match_string(np, "mux-control-names", > mux_name); > + if (index == -EINVAL && optional) > + return NULL; What about -ENODATA? And if an optional mux is found here, but lookup fails later in e.g. the of_parse_phandle_with_args call, then I think an error should be returned. Because that seems like an indication that DT specifies that there *should* be a mux, but then there isn't one. > if (index < 0) { > dev_err(dev, "mux controller '%s' not found\n", > mux_name); > @@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > ret = of_parse_phandle_with_args(np, > "mux-controls", "#mux-control-cells", > index, &args); > + if (ret == -ENOENT && optional) > + return NULL; > if (ret) { > dev_err(dev, "%s: failed to get mux-control %s(%i)\n", > np->full_name, mux_name ?: "", index); > @@ -482,9 +492,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > get_device(&mux_chip->dev); > return &mux_chip->mux[controller]; > } > + > +/** > + * mux_control_get() - Get the mux-control for a device. > + * @dev: The device that needs a mux-control. > + * @mux_name: The name identifying the mux-control. > + * > + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. > + */ > +struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > +{ > + return __mux_control_get(dev, mux_name, false); > +} > EXPORT_SYMBOL_GPL(mux_control_get); > > /** > + * mux_control_get_optional() - Get the optional mux-control for a device. > + * @dev: The device that needs a mux-control. > + * @mux_name: The name identifying the mux-control. > + * > + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. You don't mention that NULL may be returned, or when. > + */ > +struct mux_control * > +mux_control_get_optional(struct device *dev, const char *mux_name) > +{ > + return __mux_control_get(dev, mux_name, true); > +} > +EXPORT_SYMBOL_GPL(mux_control_get_optional); > + > +/** > * mux_control_put() - Put away the mux-control for good. > * @mux: The mux-control to put away. > * > @@ -492,7 +528,8 @@ EXPORT_SYMBOL_GPL(mux_control_get); > */ > void mux_control_put(struct mux_control *mux) > { > - put_device(&mux->chip->dev); > + if (mux) > + put_device(&mux->chip->dev); Don't put it if you don't have it. > } > EXPORT_SYMBOL_GPL(mux_control_put); > > @@ -503,16 +540,8 @@ static void devm_mux_control_release(struct device *dev, void *res) > mux_control_put(mux); > } > > -/** > - * devm_mux_control_get() - Get the mux-control for a device, with resource > - * management. > - * @dev: The device that needs a mux-control. > - * @mux_name: The name identifying the mux-control. > - * > - * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. > - */ > -struct mux_control *devm_mux_control_get(struct device *dev, > - const char *mux_name) > +static struct mux_control * > +__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional) > { > struct mux_control **ptr, *mux; > > @@ -520,7 +549,7 @@ struct mux_control *devm_mux_control_get(struct device *dev, > if (!ptr) > return ERR_PTR(-ENOMEM); > > - mux = mux_control_get(dev, mux_name); > + mux = __mux_control_get(dev, mux_name, optional); > if (IS_ERR(mux)) { > devres_free(ptr); > return mux; > @@ -531,8 +560,37 @@ struct mux_control *devm_mux_control_get(struct device *dev, > > return mux; > } > + > +/** > + * devm_mux_control_get() - Get the mux-control for a device, with resource > + * management. > + * @dev: The device that needs a mux-control. > + * @mux_name: The name identifying the mux-control. > + * > + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. > + */ > +struct mux_control * > +devm_mux_control_get(struct device *dev, const char *mux_name) > +{ > + return __devm_mux_control_get(dev, mux_name, false); > +} > EXPORT_SYMBOL_GPL(devm_mux_control_get); > > +/** > + * devm_mux_control_get_optional() - Get the optional mux-control for a device, > + * with resource management. > + * @dev: The device that needs a mux-control. > + * @mux_name: The name identifying the mux-control. > + * > + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. You don't mention that NULL may be returned, or when. Cheers, Peter > + */ > +struct mux_control * > +devm_mux_control_get_optional(struct device *dev, const char *mux_name) > +{ > + return __devm_mux_control_get(dev, mux_name, true); > +} > +EXPORT_SYMBOL_GPL(devm_mux_control_get_optional); > + > /* > * Using subsys_initcall instead of module_init here to try to ensure - for > * the non-modular case - that the subsystem is initialized when mux consumers > diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h > index 5577e1b773c4..5e2aa046f032 100644 > --- a/include/linux/mux/consumer.h > +++ b/include/linux/mux/consumer.h > @@ -24,9 +24,13 @@ int __must_check mux_control_try_select(struct mux_control *mux, > int mux_control_deselect(struct mux_control *mux); > > struct mux_control *mux_control_get(struct device *dev, const char *mux_name); > +struct mux_control *mux_control_get_optional(struct device *dev, > + const char *mux_name); > void mux_control_put(struct mux_control *mux); > > struct mux_control *devm_mux_control_get(struct device *dev, > const char *mux_name); > +struct mux_control *devm_mux_control_get_optional(struct device *dev, > + const char *mux_name); > > #endif /* _LINUX_MUX_CONSUMER_H */ >
Quoting Peter Rosin (2017-07-17 01:20:14) > On 2017-07-14 23:40, Stephen Boyd wrote: > > diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c > > index 90b8995f07cb..a0e5bf16f02f 100644 > > --- a/drivers/mux/mux-core.c > > +++ b/drivers/mux/mux-core.c > > @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register); > > */ > > unsigned int mux_control_states(struct mux_control *mux) > > { > > + if (!mux) > > + return 0; > > + > > I don't think this is appropriate. For this function, it might be ok, > but... > > > return mux->states; > > } > > EXPORT_SYMBOL_GPL(mux_control_states); > > @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state) > > { > > int ret; > > > > + if (!mux) > > + return 0; > > + > > ...here and for other cases below it's very odd to return "ok", when > -EINVAL or something seems much more appropriate. And if -EINVAL is > returned here, the benefit of returning fake values for anything > pretty much falls apart. > > I simply don't like it, and prefer if the consumer code is arranged > to not call the mux functions when the optional get() does not find > the mux. Do you want the callers of the mux APIs to know that an optional mux isn't there, and then have checks at all callsites on optional muxes to make sure consumers don't call the mux functions? Won't that duplicate lots of checks in drivers for something the core could treat as a don't care case? Sorry, I don't understand why the consumer cares that it was there or not when it is optional. > > > ret = down_killable(&mux->lock); > > if (ret < 0) > > return ret; > > @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state) > > { > > int ret; > > > > + if (!mux) > > + return 0; > > + > > if (down_trylock(&mux->lock)) > > return -EBUSY; > > > > @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux) > > { > > int ret = 0; > > > > + if (!mux) > > + return 0; > > + > > if (mux->idle_state != MUX_IDLE_AS_IS && > > mux->idle_state != mux->cached_state) > > ret = mux_control_set(mux, mux->idle_state); > > @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np) > > return dev ? to_mux_chip(dev) : NULL; > > } > > > > -/** > > - * mux_control_get() - Get the mux-control for a device. > > - * @dev: The device that needs a mux-control. > > - * @mux_name: The name identifying the mux-control. > > - * > > - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. > > - */ > > -struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > > +struct mux_control * > > +__mux_control_get(struct device *dev, const char *mux_name, bool optional) > > { > > struct device_node *np = dev->of_node; > > struct of_phandle_args args; > > @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > > if (mux_name) { > > index = of_property_match_string(np, "mux-control-names", > > mux_name); > > + if (index == -EINVAL && optional) > > + return NULL; > > What about -ENODATA? At this point in the code we're finding the index of a mux-control-names property so I was trying to handle the case where there isn't a mux-control-names property at all but we're looking for something optional anyway. If there is a property, then we would see some other error if something went wrong and then pass the error up. There is a hole where there isn't a mux-control-names property and we're looking for something that's optional, but there is a mux-control property. Do we care though? The DT seems broken then. > And if an optional mux is found here, but lookup > fails later in e.g. the of_parse_phandle_with_args call, then I think > an error should be returned. Because that seems like an indication that > DT specifies that there *should* be a mux, but then there isn't one. of_parse_phandle_with_args() would return ENOENT when there isn't a mux-control property in DT. So I've trapped that case and returned an "optional mux" pointer of NULL. I think we handle the case you mention, where some index is found but it returns an error, because that would return some error besides -ENOENT. Sorry, I'm not really following what you're suggesting. Maybe it got mixed up with the NULL vs. non-NULL return value from mux_control_get().
On 2017-07-19 04:08, Stephen Boyd wrote: > Quoting Peter Rosin (2017-07-17 01:20:14) >> On 2017-07-14 23:40, Stephen Boyd wrote: >>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c >>> index 90b8995f07cb..a0e5bf16f02f 100644 >>> --- a/drivers/mux/mux-core.c >>> +++ b/drivers/mux/mux-core.c >>> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register); >>> */ >>> unsigned int mux_control_states(struct mux_control *mux) >>> { >>> + if (!mux) >>> + return 0; >>> + >> >> I don't think this is appropriate. For this function, it might be ok, >> but... >> >>> return mux->states; >>> } >>> EXPORT_SYMBOL_GPL(mux_control_states); >>> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state) >>> { >>> int ret; >>> >>> + if (!mux) >>> + return 0; >>> + >> >> ...here and for other cases below it's very odd to return "ok", when >> -EINVAL or something seems much more appropriate. And if -EINVAL is >> returned here, the benefit of returning fake values for anything >> pretty much falls apart. >> >> I simply don't like it, and prefer if the consumer code is arranged >> to not call the mux functions when the optional get() does not find >> the mux. > > Do you want the callers of the mux APIs to know that an optional mux > isn't there, and then have checks at all callsites on optional muxes to > make sure consumers don't call the mux functions? Won't that duplicate > lots of checks in drivers for something the core could treat as a don't > care case? Sorry, I don't understand why the consumer cares that it was > there or not when it is optional. Ok, I had a look around to figure out how others handle this, and e.g. gpio has (ugly) macros (VALIDATE_DESC) to handle this. I guess you are right and I'm wrong. So, please keep all the if (!mux) checks. Thanks for insisting! >> >>> ret = down_killable(&mux->lock); >>> if (ret < 0) >>> return ret; >>> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state) >>> { >>> int ret; >>> >>> + if (!mux) >>> + return 0; >>> + >>> if (down_trylock(&mux->lock)) >>> return -EBUSY; >>> >>> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux) >>> { >>> int ret = 0; >>> >>> + if (!mux) >>> + return 0; >>> + >>> if (mux->idle_state != MUX_IDLE_AS_IS && >>> mux->idle_state != mux->cached_state) >>> ret = mux_control_set(mux, mux->idle_state); >>> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np) >>> return dev ? to_mux_chip(dev) : NULL; >>> } >>> >>> -/** >>> - * mux_control_get() - Get the mux-control for a device. >>> - * @dev: The device that needs a mux-control. >>> - * @mux_name: The name identifying the mux-control. >>> - * >>> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. >>> - */ >>> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name) >>> +struct mux_control * >>> +__mux_control_get(struct device *dev, const char *mux_name, bool optional) >>> { >>> struct device_node *np = dev->of_node; >>> struct of_phandle_args args; >>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) >>> if (mux_name) { >>> index = of_property_match_string(np, "mux-control-names", >>> mux_name); >>> + if (index == -EINVAL && optional) >>> + return NULL; >> >> What about -ENODATA? > > At this point in the code we're finding the index of a mux-control-names > property so I was trying to handle the case where there isn't a > mux-control-names property at all Yes, you indeed need to check for -EINVAL to catch that. No argument about that. > but we're looking for something > optional anyway. If there is a property, then we would see some other > error if something went wrong and then pass the error up. There is a > hole where there isn't a mux-control-names property and we're looking > for something that's optional, but there is a mux-control property. Do > we care though? The DT seems broken then. I was thinking about the case where mux-control-names names *other* muxes but not the one asked for in this call. That's not broken and should be handled. The way I read it, you get -ENODATA in that case? >> And if an optional mux is found here, but lookup >> fails later in e.g. the of_parse_phandle_with_args call, then I think >> an error should be returned. Because that seems like an indication that >> DT specifies that there *should* be a mux, but then there isn't one. > > of_parse_phandle_with_args() would return ENOENT when there isn't a > mux-control property in DT. So I've trapped that case and returned an > "optional mux" pointer of NULL. I think we handle the case you mention, > where some index is found but it returns an error, because that would > return some error besides -ENOENT. > > Sorry, I'm not really following what you're suggesting. Maybe it got > mixed up with the NULL vs. non-NULL return value from mux_control_get(). What I mean is that if you have passed a mux_name and the index of that name was indeed found in the of_property_match_string call, then any failure from of_parse_phandle_with_args indicates a bad DT and should IMO result in an error. I.e., when evaluating the result from of_parse_phandle_with_args, you should account for the optional param if and only if mux_name is NULL. You can do that by e.g. setting optional to false after looking up the mux_name index (because at that point the mux is no longer considered optional). E.g. as the last statement in the if (!mux_name) block. Cheers, peda
Quoting Peter Rosin (2017-07-19 00:15:38) > On 2017-07-19 04:08, Stephen Boyd wrote: > > Quoting Peter Rosin (2017-07-17 01:20:14) > >> On 2017-07-14 23:40, Stephen Boyd wrote: > >>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) > >>> if (mux_name) { > >>> index = of_property_match_string(np, "mux-control-names", > >>> mux_name); > >>> + if (index == -EINVAL && optional) > >>> + return NULL; > >> > >> What about -ENODATA? > > > > At this point in the code we're finding the index of a mux-control-names > > property so I was trying to handle the case where there isn't a > > mux-control-names property at all > > Yes, you indeed need to check for -EINVAL to catch that. No argument > about that. Ok. > > > but we're looking for something > > optional anyway. If there is a property, then we would see some other > > error if something went wrong and then pass the error up. There is a > > hole where there isn't a mux-control-names property and we're looking > > for something that's optional, but there is a mux-control property. Do > > we care though? The DT seems broken then. > > I was thinking about the case where mux-control-names names *other* muxes > but not the one asked for in this call. That's not broken and should be > handled. The way I read it, you get -ENODATA in that case? Yes that would return -ENODATA. Similarly, it would be returned if we had a boolean mux-control-names property (which is completely broken). > > >> And if an optional mux is found here, but lookup > >> fails later in e.g. the of_parse_phandle_with_args call, then I think > >> an error should be returned. Because that seems like an indication that > >> DT specifies that there *should* be a mux, but then there isn't one. > > > > of_parse_phandle_with_args() would return ENOENT when there isn't a > > mux-control property in DT. So I've trapped that case and returned an > > "optional mux" pointer of NULL. I think we handle the case you mention, > > where some index is found but it returns an error, because that would > > return some error besides -ENOENT. > > > > Sorry, I'm not really following what you're suggesting. Maybe it got > > mixed up with the NULL vs. non-NULL return value from mux_control_get(). > > What I mean is that if you have passed a mux_name and the index of that > name was indeed found in the of_property_match_string call, then any > failure from of_parse_phandle_with_args indicates a bad DT and should > IMO result in an error. I.e., when evaluating the result from > of_parse_phandle_with_args, you should account for the optional param > if and only if mux_name is NULL. > > You can do that by e.g. setting optional to false after looking up the > mux_name index (because at that point the mux is no longer considered > optional). E.g. as the last statement in the if (!mux_name) block. > Ok got it. I'll rework the logic.
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt index 30e04f7a690d..4fdd3e63ff8b 100644 --- a/Documentation/driver-model/devres.txt +++ b/Documentation/driver-model/devres.txt @@ -342,6 +342,7 @@ MUX devm_mux_chip_alloc() devm_mux_chip_register() devm_mux_control_get() + devm_mux_control_get_optional() PER-CPU MEM devm_alloc_percpu() diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c index 90b8995f07cb..a0e5bf16f02f 100644 --- a/drivers/mux/mux-core.c +++ b/drivers/mux/mux-core.c @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register); */ unsigned int mux_control_states(struct mux_control *mux) { + if (!mux) + return 0; + return mux->states; } EXPORT_SYMBOL_GPL(mux_control_states); @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state) { int ret; + if (!mux) + return 0; + ret = down_killable(&mux->lock); if (ret < 0) return ret; @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state) { int ret; + if (!mux) + return 0; + if (down_trylock(&mux->lock)) return -EBUSY; @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux) { int ret = 0; + if (!mux) + return 0; + if (mux->idle_state != MUX_IDLE_AS_IS && mux->idle_state != mux->cached_state) ret = mux_control_set(mux, mux->idle_state); @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np) return dev ? to_mux_chip(dev) : NULL; } -/** - * mux_control_get() - Get the mux-control for a device. - * @dev: The device that needs a mux-control. - * @mux_name: The name identifying the mux-control. - * - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. - */ -struct mux_control *mux_control_get(struct device *dev, const char *mux_name) +struct mux_control * +__mux_control_get(struct device *dev, const char *mux_name, bool optional) { struct device_node *np = dev->of_node; struct of_phandle_args args; @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) if (mux_name) { index = of_property_match_string(np, "mux-control-names", mux_name); + if (index == -EINVAL && optional) + return NULL; if (index < 0) { dev_err(dev, "mux controller '%s' not found\n", mux_name); @@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) ret = of_parse_phandle_with_args(np, "mux-controls", "#mux-control-cells", index, &args); + if (ret == -ENOENT && optional) + return NULL; if (ret) { dev_err(dev, "%s: failed to get mux-control %s(%i)\n", np->full_name, mux_name ?: "", index); @@ -482,9 +492,35 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name) get_device(&mux_chip->dev); return &mux_chip->mux[controller]; } + +/** + * mux_control_get() - Get the mux-control for a device. + * @dev: The device that needs a mux-control. + * @mux_name: The name identifying the mux-control. + * + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. + */ +struct mux_control *mux_control_get(struct device *dev, const char *mux_name) +{ + return __mux_control_get(dev, mux_name, false); +} EXPORT_SYMBOL_GPL(mux_control_get); /** + * mux_control_get_optional() - Get the optional mux-control for a device. + * @dev: The device that needs a mux-control. + * @mux_name: The name identifying the mux-control. + * + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno. + */ +struct mux_control * +mux_control_get_optional(struct device *dev, const char *mux_name) +{ + return __mux_control_get(dev, mux_name, true); +} +EXPORT_SYMBOL_GPL(mux_control_get_optional); + +/** * mux_control_put() - Put away the mux-control for good. * @mux: The mux-control to put away. * @@ -492,7 +528,8 @@ EXPORT_SYMBOL_GPL(mux_control_get); */ void mux_control_put(struct mux_control *mux) { - put_device(&mux->chip->dev); + if (mux) + put_device(&mux->chip->dev); } EXPORT_SYMBOL_GPL(mux_control_put); @@ -503,16 +540,8 @@ static void devm_mux_control_release(struct device *dev, void *res) mux_control_put(mux); } -/** - * devm_mux_control_get() - Get the mux-control for a device, with resource - * management. - * @dev: The device that needs a mux-control. - * @mux_name: The name identifying the mux-control. - * - * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. - */ -struct mux_control *devm_mux_control_get(struct device *dev, - const char *mux_name) +static struct mux_control * +__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional) { struct mux_control **ptr, *mux; @@ -520,7 +549,7 @@ struct mux_control *devm_mux_control_get(struct device *dev, if (!ptr) return ERR_PTR(-ENOMEM); - mux = mux_control_get(dev, mux_name); + mux = __mux_control_get(dev, mux_name, optional); if (IS_ERR(mux)) { devres_free(ptr); return mux; @@ -531,8 +560,37 @@ struct mux_control *devm_mux_control_get(struct device *dev, return mux; } + +/** + * devm_mux_control_get() - Get the mux-control for a device, with resource + * management. + * @dev: The device that needs a mux-control. + * @mux_name: The name identifying the mux-control. + * + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. + */ +struct mux_control * +devm_mux_control_get(struct device *dev, const char *mux_name) +{ + return __devm_mux_control_get(dev, mux_name, false); +} EXPORT_SYMBOL_GPL(devm_mux_control_get); +/** + * devm_mux_control_get_optional() - Get the optional mux-control for a device, + * with resource management. + * @dev: The device that needs a mux-control. + * @mux_name: The name identifying the mux-control. + * + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno. + */ +struct mux_control * +devm_mux_control_get_optional(struct device *dev, const char *mux_name) +{ + return __devm_mux_control_get(dev, mux_name, true); +} +EXPORT_SYMBOL_GPL(devm_mux_control_get_optional); + /* * Using subsys_initcall instead of module_init here to try to ensure - for * the non-modular case - that the subsystem is initialized when mux consumers diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h index 5577e1b773c4..5e2aa046f032 100644 --- a/include/linux/mux/consumer.h +++ b/include/linux/mux/consumer.h @@ -24,9 +24,13 @@ int __must_check mux_control_try_select(struct mux_control *mux, int mux_control_deselect(struct mux_control *mux); struct mux_control *mux_control_get(struct device *dev, const char *mux_name); +struct mux_control *mux_control_get_optional(struct device *dev, + const char *mux_name); void mux_control_put(struct mux_control *mux); struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name); +struct mux_control *devm_mux_control_get_optional(struct device *dev, + const char *mux_name); #endif /* _LINUX_MUX_CONSUMER_H */
Sometimes drivers only use muxes under certain scenarios. For example, the chipidea usb controller may be connected to a usb switch on some platforms, and connected directly to a usb port on others. The driver won't know one way or the other though, so add a mux_control_get_optional() API that allows the driver to differentiate errors getting the mux from there not being a mux for the driver to use at all. Cc: Jonathan Cameron <jic23@kernel.org> Cc: Philipp Zabel <p.zabel@pengutronix.de> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- Documentation/driver-model/devres.txt | 1 + drivers/mux/mux-core.c | 98 ++++++++++++++++++++++++++++------- include/linux/mux/consumer.h | 4 ++ 3 files changed, 83 insertions(+), 20 deletions(-)