Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752012AbdGHXYU (ORCPT ); Sat, 8 Jul 2017 19:24:20 -0400 Received: from mail-pg0-f68.google.com ([74.125.83.68]:35892 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750883AbdGHXYT (ORCPT ); Sat, 8 Jul 2017 19:24:19 -0400 Subject: Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name. To: Peter Rosin , sathyanarayanan.kuppuswamy@linux.intel.com Cc: linux-kernel@vger.kernel.org References: <08b3c179-6e58-73c7-5221-4b9b12d7ea9c@axentia.se> From: "Kuppuswamy, Sathyanarayanan" Message-ID: <1e69f7d9-0973-71b7-86d3-534c046c5e0b@gmail.com> Date: Sat, 8 Jul 2017 16:24:17 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <08b3c179-6e58-73c7-5221-4b9b12d7ea9c@axentia.se> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7071 Lines: 207 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 >> >> 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 >> --- >> 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 */ >>