Subject: Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API
To: Stephen Boyd <stephen.boyd@linaro.org>
Cc: linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, devicetree@vger.infradead.org,
        Rob Herring <robh+dt@kernel.org>, Rob Clark <robdclark@gmail.com>,
        Peter Chen <Peter.Chen@nxp.com>, Andy Gross <andy.gross@linaro.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>
References: <20170714214005.14967-1-stephen.boyd@linaro.org>
 <20170714214005.14967-2-stephen.boyd@linaro.org>
From: Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <aac61f46-f963-2631-00e0-2d62baac6bf4@axentia.se>
Date: Mon, 17 Jul 2017 10:20:14 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170714214005.14967-2-stephen.boyd@linaro.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2017 08:20:18.2160 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0202MB2557
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 9450
Lines: 275

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 */
>