Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name.

[Peter Rosin]

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.

Cheers,
peda