Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754934AbdGJVh5 (ORCPT ); Mon, 10 Jul 2017 17:37:57 -0400 Received: from mga09.intel.com ([134.134.136.24]:63385 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754696AbdGJVhz (ORCPT ); Mon, 10 Jul 2017 17:37:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,342,1496127600"; d="scan'208";a="123442500" Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name. To: Peter Rosin , "Kuppuswamy, Sathyanarayanan" Cc: linux-kernel@vger.kernel.org References: <08b3c179-6e58-73c7-5221-4b9b12d7ea9c@axentia.se> <1e69f7d9-0973-71b7-86d3-534c046c5e0b@gmail.com> <84f60a9d-c843-df7b-2c28-f5b9d17e776c@axentia.se> From: sathyanarayanan kuppuswamy Organization: Intel Message-ID: <3d633396-bfb6-c3b9-a670-b73f98fc6ce0@linux.intel.com> Date: Mon, 10 Jul 2017 14:37:38 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <84f60a9d-c843-df7b-2c28-f5b9d17e776c@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: 4524 Lines: 120 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 >>>> >>>> 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? > 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