Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[kbuild test robot]

Hi Kuppuswamy,

[auto build test WARNING on linus/master]
[also build test WARNING on next-20170707]
[cannot apply to v4.12]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/sathyanarayanan-kuppuswamy-linux-intel-com/mux-Add-new-API-to-get-mux_control-ref-by-device-name/20170709-011501


coccinelle warnings: (new ones prefixed by >>)

>> drivers/mux/mux-core.c:491:9-16: WARNING: ERR_CAST can be used with mux_chip

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[sathyanarayanan.kuppuswamy]

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.

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);
+}
+
+/**
+ * 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);
+
+	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);
 #endif /* _LINUX_MUX_CONSUMER_H */
-- 
2.7.4

[PATCH] mux: fix err_cast.cocci warnings

[kbuild test robot]

drivers/mux/mux-core.c:491:9-16: WARNING: ERR_CAST can be used with mux_chip


 Use ERR_CAST inlined function instead of ERR_PTR(PTR_ERR(...))

Generated by: scripts/coccinelle/api/err_cast.cocci

Fixes: 1203d8a531ed ("mux: Add new API to get mux_control ref by device name.")
CC: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 mux-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -488,7 +488,7 @@ struct mux_control *mux_control_get_by_i
 	mux_chip = mux_chip_get_by_index(devname, chip_index);
 
 	if (IS_ERR(mux_chip))
-		return ERR_PTR(PTR_ERR(mux_chip));
+		return ERR_CAST(mux_chip);
 
 	if (ctrl_index >= mux_chip->controllers) {
 		dev_err(&mux_chip->dev,

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Peter Rosin]

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

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Andy Shevchenko]

On Sun, Jul 9, 2017 at 12:12 AM, Peter Rosin <peda@axentia.se> wrote:
> 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?

Without Heikki's blessing, NAK for this activity.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Kuppuswamy, Sathyanarayanan]

Hi Andy,


On 7/8/2017 2:26 PM, Andy Shevchenko wrote:
> On Sun, Jul 9, 2017 at 12:12 AM, Peter Rosin <peda@axentia.se> wrote:
>> 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?
> Without Heikki's blessing, NAK for this activity.
I dropped the idea of Intel USB MUX driver after my last discussion with 
Hans. He is currently working on a solution for this issue. Once its 
merged, May be I can try improving it handle my use cases.

But I submitted these changes because it can be useful if any one wants 
to use MUX framework for non-dt case.
>

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Kuppuswamy, Sathyanarayanan]

Hi Peter,

On 7/8/2017 2:12 PM, Peter Rosin wrote:
> 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:
I am not planning to merge the Intel USB MUX driver any more. I agree 
with Hans comments
and decided not to proceed further on this approach.

But I created these helper functions to get my driver working with MUX 
framework. Since these
helper functions can be useful for any non-dt drivers who wants to use 
MUX framework, I thought
to submit these changes for review.
>
>     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, "."));
will fix it in next version.
>
>> +}
>> +
>> +/**
>> + * 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.
Good catch. I did not test the case with multiple chips. So I failed to 
notice this.
>
> 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.
Got it.
>
> Cheers,
> peda
>
>>   #endif /* _LINUX_MUX_CONSUMER_H */
>>

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Peter Rosin]

On 2017-07-09 01:24, Kuppuswamy, Sathyanarayanan wrote:
> Hi Peter,
> 
> On 7/8/2017 2:12 PM, Peter Rosin wrote:
>> 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:
> I am not planning to merge the Intel USB MUX driver any more. I agree 
> with Hans comments
> and decided not to proceed further on this approach.
> 
> But I created these helper functions to get my driver working with MUX 
> framework. Since these
> helper functions can be useful for any non-dt drivers who wants to use 
> MUX framework, I thought
> to submit these changes for review.
>>
>>     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, "."));
> will fix it in next version.
>>
>>> +}
>>> +
>>> +/**
>>> + * 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.
> Good catch. I did not test the case with multiple chips. So I failed to 
> notice this.
>>
>> Also, if you fix the loop, why is the ordering stable and something to rely
>> on?

You failed to comment on this very important point. Sorry for not putting
more emphasis on it. So, before you waste more time on the indexed approach,
have a look at e.g. the pwm core with its pwm_get (which takes a name) and
its *deprecated* pwm_request (which takes an index).

I think having a lookup table (like pwm) is closer to what the mux core
should do. Or something like that.

Cheers,
peda

Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[sathyanarayanan kuppuswamy]

Hi Peter,

Thanks for the comments.

On 07/10/2017 03:07 AM, Peter Rosin wrote:
> On 2017-07-09 01:24, Kuppuswamy, Sathyanarayanan wrote:
>> Hi Peter,
>>
>> On 7/8/2017 2:12 PM, Peter Rosin wrote:
>>> 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:
>> I am not planning to merge the Intel USB MUX driver any more. I agree
>> with Hans comments
>> and decided not to proceed further on this approach.
>>
>> But I created these helper functions to get my driver working with MUX
>> framework. Since these
>> helper functions can be useful for any non-dt drivers who wants to use
>> MUX framework, I thought
>> to submit these changes for review.
>>>      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, "."));
>> will fix it in next version.
>>>> +}
>>>> +
>>>> +/**
>>>> + * 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.
>> Good catch. I did not test the case with multiple chips. So I failed to
>> notice this.
>>> Also, if you fix the loop, why is the ordering stable and something to rely
>>> on?
> You failed to comment on this very important point. Sorry for not putting
> more emphasis on it. So, before you waste more time on the indexed approach,
> have a look at e.g. the pwm core with its pwm_get (which takes a name) and
> its *deprecated* pwm_request (which takes an index).
>
> I think having a lookup table (like pwm) is closer to what the mux core
> should do. Or something like that.
Let me go through it and get back to you.
>
> Cheers,
> peda
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer