Messages in this thread | | | Subject | Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name. | From | Peter Rosin <> | Date | Mon, 10 Jul 2017 12:07:08 +0200 |
| |
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
| |