Message-ID: <56D8E0B9.3050608@linux.intel.com>
Date:	Fri, 4 Mar 2016 09:11:21 +0800
From:	Lu Baolu <baolu.lu@...ux.intel.com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Felipe Balbi <balbi@...nel.org>,
	Mathias Nyman <mathias.nyman@...el.com>,
	Lee Jones <lee.jones@...aro.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] usb: misc: add common code for Intel dual role port
 mux



On 03/04/2016 12:10 AM, Greg Kroah-Hartman wrote:
> On Thu, Mar 03, 2016 at 02:37:40PM +0800, Lu Baolu wrote:
>> Several Intel PCHs and SOCs have an internal mux that is used to
>> share one USB port between device controller and host controller.
>>
>> A usb port mux could be abstracted as the following elements:
>> 1) mux state: HOST or PERIPHERAL;
>> 2) an extcon cable which triggers the change of mux state between
>>    HOST and PERIPHERAL;
>> 3) The required action to do the real port switch.
>>
>> This patch adds the common code to handle usb port mux. With this
>> common code, the individual mux driver, which always is platform
>> dependent, could focus on the real operation of mux switch.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>> Reviewed-by: Felipe Balbi <balbi@...nel.org>
>> ---
>>  MAINTAINERS               |   7 ++
>>  drivers/usb/misc/Kconfig  |   4 ++
>>  drivers/usb/misc/Makefile |   2 +
>>  drivers/usb/misc/mux.c    | 172 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/mux.h   |  71 +++++++++++++++++++
>>  5 files changed, 256 insertions(+)
>>  create mode 100644 drivers/usb/misc/mux.c
>>  create mode 100644 include/linux/usb/mux.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d894ee2..45f1e1e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11389,6 +11389,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>>  S:	Maintained
>>  F:	drivers/usb/phy/
>>  
>> +USB PORT MUX DRIVER
>> +M:	Lu Baolu <baolu.lu@...ux.intel.com>
>> +L:	linux-usb@...r.kernel.org
>> +S:	Supported
>> +F:	drivers/usb/misc/mux.c
>> +F:	include/linux/usb/mux.h
>> +
>>  USB PRINTER DRIVER (usblp)
>>  M:	Pete Zaitcev <zaitcev@...hat.com>
>>  L:	linux-usb@...r.kernel.org
>> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
>> index f7a7fc2..6496d17 100644
>> --- a/drivers/usb/misc/Kconfig
>> +++ b/drivers/usb/misc/Kconfig
>> @@ -3,6 +3,10 @@
>>  #
>>  comment "USB Miscellaneous drivers"
>>  
>> +config USB_MUX
>> +	select EXTCON
>> +	def_bool n
>> +
>>  config USB_EMI62
>>  	tristate "EMI 6|2m USB Audio interface support"
>>  	---help---
>> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
>> index 45fd4ac..fd79dd5 100644
>> --- a/drivers/usb/misc/Makefile
>> +++ b/drivers/usb/misc/Makefile
>> @@ -29,3 +29,5 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>>  
>>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
>> +
>> +obj-$(CONFIG_USB_MUX)			+= mux.o
> "mux.ko" is a _VERY_ generic name of a kernel module.  Please make this
> much more specific to your chip as this will not work for any other
> platform that has this type of functionality.
>
> Same goes for all of your exported symbols.

Sure. I will change the module name to "intel-mux", and change
the exported symbols to "intel_usb_mux_*".

>
>
>> diff --git a/drivers/usb/misc/mux.c b/drivers/usb/misc/mux.c
>> new file mode 100644
>> index 0000000..e353fff
>> --- /dev/null
>> +++ b/drivers/usb/misc/mux.c
>> @@ -0,0 +1,172 @@
>> +/**
>> + * mux.c - USB Port Mux support
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@...ux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/notifier.h>
>> +#include <linux/usb/mux.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/err.h>
>> +
>> +static int usb_mux_change_state(struct usb_mux *mux, int state)
>> +{
>> +	int ret;
>> +	struct usb_mux_dev *umdev = mux->umdev;
>> +
>> +	dev_WARN_ONCE(umdev->dev, !mutex_is_locked(&mux->mux_mutex),
>> +			"mutex is unlocked\n");
>> +
>> +	mux->mux_state = state;
>> +
>> +	if (mux->mux_state)
>> +		ret = umdev->cable_set_cb(umdev);
>> +	else
>> +		ret = umdev->cable_unset_cb(umdev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int usb_mux_notifier(struct notifier_block *nb,
>> +		unsigned long event, void *ptr)
>> +{
>> +	struct usb_mux *mux;
>> +	int state;
>> +	int ret = NOTIFY_DONE;
>> +
>> +	mux = container_of(nb, struct usb_mux, nb);
>> +
>> +	state = extcon_get_cable_state(mux->obj.edev,
>> +			mux->umdev->cable_name);
>> +
>> +	if (mux->mux_state == -1 || mux->mux_state != state) {
>> +		mutex_lock(&mux->mux_mutex);
>> +		ret = usb_mux_change_state(mux, state);
>> +		mutex_unlock(&mux->mux_mutex);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t mux_debug_read(struct file *file, char __user *user_buf,
>> +		size_t len, loff_t *offset)
>> +{
>> +	struct usb_mux *mux = file->private_data;
>> +	char output_buf[16];
>> +
>> +	memset(output_buf, 0, sizeof(output_buf));
>> +	if (mux->mux_state)
>> +		strcpy(output_buf, "host\n");
>> +	else
>> +		strcpy(output_buf, "peripheral\n");
>> +
>> +	return simple_read_from_buffer(user_buf, len, offset,
>> +			output_buf, strlen(output_buf));
>> +}
>> +
>> +static ssize_t mux_debug_write(struct file *file, const char __user *user_buf,
>> +		size_t count, loff_t *offset)
>> +{
>> +	struct usb_mux *mux = file->private_data;
>> +	char input_buf[16];
>> +	int size, state;
>> +
>> +	size = min(count, sizeof(input_buf) - 1);
>> +	memset(input_buf, 0, sizeof(input_buf));
>> +	if (strncpy_from_user(input_buf, user_buf, size) < 0)
>> +		return -EFAULT;
>> +
>> +	if (!strncmp(input_buf, "host", 4))
>> +		state = 1;
>> +	else if (!strncmp(input_buf, "peripheral", 10))
>> +		state = 0;
>> +	else
>> +		state = -1;
>> +
>> +	if (state != -1) {
>> +		mutex_lock(&mux->mux_mutex);
>> +		usb_mux_change_state(mux, state);
>> +		mutex_unlock(&mux->mux_mutex);
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static const struct file_operations mux_debug_fops = {
>> +	.read =		mux_debug_read,
>> +	.write =	mux_debug_write,
>> +	.open =		simple_open,
>> +	.llseek =	default_llseek,
>> +};
>> +
>> +int usb_mux_register(struct usb_mux_dev *umdev)
>> +{
>> +	int ret;
>> +	struct device *dev = umdev->dev;
>> +	struct usb_mux *mux;
>> +
>> +	if (!umdev->cable_name)
>> +		return -ENODEV;
>> +
>> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +	if (!mux)
>> +		return -ENOMEM;
>> +
>> +	mux->umdev = umdev;
>> +	mux->nb.notifier_call = usb_mux_notifier;
>> +	mutex_init(&mux->mux_mutex);
>> +	mux->mux_state = -1;
>> +	dev_set_drvdata(dev, mux);
>> +
>> +	ret = extcon_register_interest(&mux->obj, umdev->extcon_name,
>> +			umdev->cable_name, &mux->nb);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register extcon notifier\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	usb_mux_notifier(&mux->nb, 0, NULL);
>> +
>> +	mux->debug_file = debugfs_create_file("usb_mux", 0600,
>> +			usb_debug_root, mux, &mux_debug_fops);
> So you control this through debugfs?  That's not a good idea, what if
> you have multiple ones of these?  What if debugfs is not enabled?

Agree. It's really a problem if we have multiple ones (although
currently we have only single one).

I will make it with sysfs instead.

> Who
> is in charge of controlling this from userspace?

Port mux is only handled in kernel. There is no requirement to
control it from user space. This is added for debugging and
information only.

>
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_mux_register);
>> +
>> +int usb_mux_unregister(struct device *dev)
>> +{
>> +	struct usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	debugfs_remove(mux->debug_file);
>> +	extcon_unregister_interest(&mux->obj);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_mux_unregister);
>> +
>> +struct usb_mux_dev *usb_mux_get_dev(struct device *dev)
>> +{
>> +	struct usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	if (mux)
>> +		return mux->umdev;
>> +
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_mux_get_dev);
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +void usb_mux_complete(struct device *dev)
>> +{
>> +	struct usb_mux *mux = dev_get_drvdata(dev);
>> +
>> +	usb_mux_notifier(&mux->nb, 0, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_mux_complete);
>> +#endif
>> diff --git a/include/linux/usb/mux.h b/include/linux/usb/mux.h
>> new file mode 100644
>> index 0000000..5dada48
>> --- /dev/null
>> +++ b/include/linux/usb/mux.h
>> @@ -0,0 +1,71 @@
>> +/**
>> + * mux.h - USB Port Mux defines
>> + *
>> + * Copyright (C) 2016 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@...ux.intel.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __LINUX_USB_MUX_H
>> +#define __LINUX_USB_MUX_H
>> +
>> +#include <linux/extcon.h>
>> +#include <linux/usb.h>
>> +
>> +struct usb_mux_dev {
>> +	struct device	*dev;
>> +	char		*extcon_name;
>> +	char		*cable_name;
>> +	int		(*cable_set_cb)(struct usb_mux_dev *mux);
>> +	int		(*cable_unset_cb)(struct usb_mux_dev *mux);
>> +};
>> +
>> +struct usb_mux {
>> +	struct usb_mux_dev		*umdev;
>> +	struct notifier_block		nb;
>> +	struct extcon_specific_cable_nb	obj;
>> +
>> +	/*
>> +	 * The state of the mux.
>> +	 * 0, 1 - mux switch state
>> +	 * -1   - uninitialized state
>> +	 *
>> +	 * mux_mutex is lock to protect mux_state
>> +	 */
>> +	int				mux_state;
>> +	struct mutex			mux_mutex;
>> +
>> +	struct dentry			*debug_file;
>> +};
>
> Why is this a public structure?
>
> And why isn't it properly reference counted if it is a public structure?

Agree. I will move it to .c file and make it private one.

>
>
>> +
>> +#if IS_ENABLED(CONFIG_USB_MUX)
>> +extern int usb_mux_register(struct usb_mux_dev *mux);
>> +extern int usb_mux_unregister(struct device *dev);
>> +extern struct usb_mux_dev *usb_mux_get_dev(struct device *dev);
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +extern void usb_mux_complete(struct device *dev);
>> +#endif
>> +
>> +#else /* CONFIG_USB_MUX */
>> +static inline int usb_mux_register(struct usb_mux_dev *mux)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>> +static inline int usb_mux_unregister(struct device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline struct usb_mux_dev *usb_mux_get_dev(struct device *dev)
>> +{
>> +	return NULL;
>> +}
>> +#endif /* CONFIG_USB_MUX */
>> +
>> +#endif /* __LINUX_USB_MUX_H */
>
> Why do you need this .h file at all?

File mux.c is a common file for all port mux drivers. The individual port
mux driver needs this .h file for structure and function interface definition.

>
> thanks,
>
> greg k-h
>

Thank you for your time.

Best Regards,
-Baolu

Powered by blists - more mailing lists