Subject: Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device
 name.
To: sathyanarayanan.kuppuswamy@linux.intel.com
Cc: linux-kernel@vger.kernel.org, sathyaosid@gmail.com
References: <c5b64f76ed8e737ece843249c12b09032fc61cb6.1499464980.git.sathyanarayanan.kuppuswamy@linux.intel.com>
From: Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <08b3c179-6e58-73c7-5221-4b9b12d7ea9c@axentia.se>
Date: Sat, 8 Jul 2017 23:12:16 +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: <c5b64f76ed8e737ece843249c12b09032fc61cb6.1499464980.git.sathyanarayanan.kuppuswamy@linux.intel.com>
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: 08 Jul 2017 21:12:20.4886 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0202MB2558
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6298
Lines: 195

On 2017-07-08 00:03, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Currently this driver only provides a single API, mux_control_get() to
> get mux_control reference based on mux_name, and also this API has tight
> dependency on device tree node. For devices, that does not use device
> tree, it makes it difficult to use this API. This patch adds new
> API to access mux_control reference based on device name, chip index and
> controller index value.

I assume this is for the Intel USB Multiplexer that you sent a driver for
a month or so ago? If so, you still have not answered these questions:

   Is any other consumer in the charts at all? Can this existing consumer
   ever make use of some other mux? If the answer to both those questions
   are 'no', then I do not see much point in involving the mux subsystem at
   all. The Broxton USB PHY driver could just as well write to the register
   all by itself, no?

that I asked in https://lkml.org/lkml/2017/5/31/58

What is the point of that driver?

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/mux/mux-core.c       | 114 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mux/consumer.h |   6 ++-
>  2 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> index 90b8995..f8796b9 100644
> --- a/drivers/mux/mux-core.c
> +++ b/drivers/mux/mux-core.c
> @@ -422,6 +422,87 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>  	return dev ? to_mux_chip(dev) : NULL;
>  }
>  
> +static int dev_parent_name_match(struct device *dev, const void *data)
> +{
> +	const char *devname = dev_name(dev->parent);
> +	unsigned int i;
> +
> +	if (!devname || !data)
> +		return 0;
> +
> +	for (i = 0; i < strlen(devname); i++) {
> +		if (devname[i] == '.')
> +			break;
> +	}
> +
> +	return !strncmp(devname, data, i-1);

Ouch, strlen as a termination test is wasteful, you want to remove the loop
and do something like this

	return !strncmp(devname, data, strcspn(devname, "."));

> +}
> +
> +/**
> + * mux_chip_get_by_index() - Get the mux-chip associated with give device.
> + * @devname: Name of the device which registered the mux-chip.
> + * @index: Index of the mux chip.
> + *
> + * Return: A pointer to the mux-chip, or an ERR_PTR with a negative errno.
> + */
> +static struct mux_chip *mux_chip_get_by_index(const char *devname, int index)
> +{
> +	struct device *dev;
> +	int found = -1;
> +
> +	if (!devname)
> +		return ERR_PTR(-EINVAL);
> +
> +	do {
> +		dev = class_find_device(&mux_class, NULL, devname,
> +					dev_parent_name_match);
> +
> +		if (dev != NULL)
> +			found++;
> +
> +		if (found >= index)
> +			break;
> +	} while (dev != NULL);

This loop is broken. class_find_device will always return the same device.

Also, if you fix the loop, why is the ordering stable and something to rely
on?

> +
> +	if ((found == index) && dev)
> +		return to_mux_chip(dev);
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +/**
> + * mux_control_get_by_index() - Get the mux-control of given device based on
> + *				device name, chip and control index.
> + * @devname: Name of the device which registered the mux-chip.
> + * @chip_index: Index of the mux chip.
> + * @ctrl_index: Index of the mux controller.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get_by_index(const char *devname,
> +					     unsigned int chip_index,
> +					     unsigned int ctrl_index)
> +{
> +	struct mux_chip *mux_chip;
> +
> +	mux_chip = mux_chip_get_by_index(devname, chip_index);
> +
> +	if (IS_ERR(mux_chip))
> +		return ERR_PTR(PTR_ERR(mux_chip));
> +
> +	if (ctrl_index >= mux_chip->controllers) {
> +		dev_err(&mux_chip->dev,
> +				"Invalid controller index, maximum value is %d\n",
> +				mux_chip->controllers);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	get_device(&mux_chip->dev);
> +
> +	return &mux_chip->mux[ctrl_index];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get_by_index);
> +
>  /**
>   * mux_control_get() - Get the mux-control for a device.
>   * @dev: The device that needs a mux-control.
> @@ -533,6 +614,39 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_mux_control_get);
>  
> +/**
> + * devm_mux_control_get_by_index() - Get the mux-control for a device of given
> + *				     index value.
> + * @dev: The device that needs a mux-control.
> + * @devname: Name of the device which registered the mux-chip.
> + * @chip_index: Index of the mux chip.
> + * @ctrl_index: Index of the mux controller.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get_by_index(struct device *dev,
> +		const char *devname, unsigned int chip_index,
> +		unsigned int ctrl_index)
> +{
> +	struct mux_control **ptr, *mux;
> +
> +	ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mux = mux_control_get_by_index(devname, chip_index, ctrl_index);
> +	if (IS_ERR(mux)) {
> +		devres_free(ptr);
> +		return mux;
> +	}
> +
> +	*ptr = mux;
> +	devres_add(dev, ptr);
> +
> +	return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get_by_index);
> +
>  /*
>   * 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 5577e1b..e02485b 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -28,5 +28,9 @@ 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 *mux_control_get_by_index(const char *devname,
> +		unsigned int chip_index, unsigned int ctrl_index);
> +struct mux_control *devm_mux_control_get_by_index(struct device *dev,
> +		const char *devname, unsigned int chip_index,
> +		unsigned int ctrl_index);

I want an empty line here.

Cheers,
peda

>  #endif /* _LINUX_MUX_CONSUMER_H */
>