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@xxxxxxxxxxxxxxx wrote:
> > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    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