On 2017-07-08 00:03, [email protected] wrote: > From: Kuppuswamy Sathyanarayanan <[email protected]> > > 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 > <[email protected]> > --- > 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 */ >
