Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1684653

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

Show all headers | View raw

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 <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.
Let me go through it and get back to you.
>
> Cheers,
> peda
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v1 1/1] mux: Add new API to get mux_control ref by device name. sathyanarayanan.kuppuswamy@linux.intel.com - 2017-07-08 00:10 +0200
  [PATCH] mux: fix err_cast.cocci warnings kbuild test robot <lkp@intel.com> - 2017-07-08 22:00 +0200
  Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device  name. kbuild test robot <lkp@intel.com> - 2017-07-08 22:00 +0200
  Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device name. Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-08 23:30 +0200
    Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device  name. "Kuppuswamy, Sathyanarayanan" <sathyaosid@gmail.com> - 2017-07-09 01:30 +0200
  Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device  name. "Kuppuswamy, Sathyanarayanan" <sathyaosid@gmail.com> - 2017-07-09 01:30 +0200
    Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device  name. sathyanarayanan kuppuswamy          <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-07-10 23:40 +0200

csiph-web