Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Tue, Mar 08, 2016 at 03:53:44PM +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@xxxxxxxxxxxxxxx>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Reviewed-by: Felipe Balbi <balbi@xxxxxxxxxx>
> ---
>  Documentation/ABI/testing/sysfs-bus-platform |  15 +++
>  MAINTAINERS                                  |   7 ++
>  drivers/usb/Kconfig                          |   2 +
>  drivers/usb/Makefile                         |   1 +
>  drivers/usb/mux/Kconfig                      |   9 ++
>  drivers/usb/mux/Makefile                     |   4 +
>  drivers/usb/mux/intel-mux.c                  | 166 +++++++++++++++++++++++++++
>  include/linux/usb/intel-mux.h                |  47 ++++++++
>  8 files changed, 251 insertions(+)
>  create mode 100644 drivers/usb/mux/Kconfig
>  create mode 100644 drivers/usb/mux/Makefile
>  create mode 100644 drivers/usb/mux/intel-mux.c
>  create mode 100644 include/linux/usb/intel-mux.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-platform b/Documentation/ABI/testing/sysfs-bus-platform
> index 5172a61..a2261cb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-platform
> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> @@ -18,3 +18,18 @@ Description:
>  		devices to opt-out of driver binding using a driver_override
>  		name such as "none".  Only a single driver may be specified in
>  		the override, there is no support for parsing delimiters.
> +
> +What:		/sys/bus/platform/devices/.../intel_mux
> +Date:		Febuary 2016
> +Contact:	Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> +Description:
> +		In some platforms, a single USB port is shared between a USB host
> +		controller and a device controller. A USB mux driver is needed to
> +		handle the port mux. intel_mux attribute shows and stores the mux
> +		state.
> +		For read:
> +		'peripheral' - mux switched to PERIPHERAL controller;
> +		'host'       - mux switched to HOST controller.
> +		For write:
> +		'peripheral' - mux will be switched to PERIPHERAL controller;
> +		'host'       - mux will be switched to HOST controller.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c55b37e..a93d26a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11400,6 +11400,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@xxxxxxxxxxxxxxx>
> +L:	linux-usb@xxxxxxxxxxxxxxx
> +S:	Supported
> +F:	drivers/usb/mux/intel-mux.c
> +F:	include/linux/usb/intel-mux.h
> +
>  USB PRINTER DRIVER (usblp)
>  M:	Pete Zaitcev <zaitcev@xxxxxxxxxx>
>  L:	linux-usb@xxxxxxxxxxxxxxx
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8ed451d..dbd6620 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -149,6 +149,8 @@ endif # USB
>  
>  source "drivers/usb/phy/Kconfig"
>  
> +source "drivers/usb/mux/Kconfig"
> +
>  source "drivers/usb/gadget/Kconfig"
>  
>  config USB_LED_TRIG
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index dca7856..9a92338 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -6,6 +6,7 @@
>  
>  obj-$(CONFIG_USB)		+= core/
>  obj-$(CONFIG_USB_SUPPORT)	+= phy/
> +obj-$(CONFIG_USB_SUPPORT)	+= mux/
>  
>  obj-$(CONFIG_USB_DWC3)		+= dwc3/
>  obj-$(CONFIG_USB_DWC2)		+= dwc2/
> diff --git a/drivers/usb/mux/Kconfig b/drivers/usb/mux/Kconfig
> new file mode 100644
> index 0000000..8fedc4f
> --- /dev/null
> +++ b/drivers/usb/mux/Kconfig
> @@ -0,0 +1,9 @@
> +#
> +# USB port mux driver configuration
> +#
> +menu "USB Port MUX drivers"
> +config INTEL_USB_MUX
> +	select EXTCON
> +	def_bool n

No help text?

> +
> +endmenu
> diff --git a/drivers/usb/mux/Makefile b/drivers/usb/mux/Makefile
> new file mode 100644
> index 0000000..84f0ae8
> --- /dev/null
> +++ b/drivers/usb/mux/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for USB port mux drivers
> +#
> +obj-$(CONFIG_INTEL_USB_MUX)		+= intel-mux.o
> diff --git a/drivers/usb/mux/intel-mux.c b/drivers/usb/mux/intel-mux.c
> new file mode 100644
> index 0000000..b243758
> --- /dev/null
> +++ b/drivers/usb/mux/intel-mux.c
> @@ -0,0 +1,166 @@
> +/**
> + * mux.c - USB Port Mux support
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> + *
> + * 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/extcon.h>
> +#include <linux/usb/intel-mux.h>
> +#include <linux/err.h>
> +
> +struct intel_usb_mux {
> +	struct intel_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;
> +};
> +
> +static int usb_mux_change_state(struct intel_usb_mux *mux, int state)
> +{
> +	int ret;
> +	struct intel_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 intel_usb_mux *mux;
> +	int state;
> +	int ret = NOTIFY_DONE;
> +
> +	mux = container_of(nb, struct intel_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 intel_mux_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
> +		return 0;
> +
> +	return sprintf(buf, "%s\n", mux->mux_state ? "host" : "peripheral");
> +}
> +
> +static ssize_t intel_mux_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +	int state;
> +
> +	if (dev_WARN_ONCE(dev, !mux, "mux without data structure\n"))
> +		return -EINVAL;
> +
> +	if (sysfs_streq(buf, "peripheral"))
> +		state = 0;
> +	else if (sysfs_streq(buf, "host"))
> +		state = 1;
> +	else
> +		return -EINVAL;
> +
> +	mutex_lock(&mux->mux_mutex);
> +	usb_mux_change_state(mux, state);
> +	mutex_unlock(&mux->mux_mutex);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(intel_mux);
> +
> +int intel_usb_mux_register(struct intel_mux_dev *umdev)
> +{
> +	int ret;
> +	struct device *dev = umdev->dev;
> +	struct intel_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);
> +
> +	/* register the sysfs interface */
> +	ret = device_create_file(dev, &dev_attr_intel_mux);
> +	if (ret) {
> +		dev_err(dev, "failed to create sysfs attribute\n");
> +		if (umdev->cable_name)
> +			extcon_unregister_interest(&mux->obj);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_register);
> +
> +int intel_usb_mux_unregister(struct device *dev)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	device_remove_file(dev, &dev_attr_intel_mux);
> +	extcon_unregister_interest(&mux->obj);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_unregister);
> +
> +#ifdef CONFIG_PM_SLEEP
> +void intel_usb_mux_complete(struct device *dev)
> +{
> +	struct intel_usb_mux *mux = dev_get_drvdata(dev);
> +
> +	usb_mux_notifier(&mux->nb, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(intel_usb_mux_complete);
> +#endif
> diff --git a/include/linux/usb/intel-mux.h b/include/linux/usb/intel-mux.h
> new file mode 100644
> index 0000000..6990174
> --- /dev/null
> +++ b/include/linux/usb/intel-mux.h
> @@ -0,0 +1,47 @@
> +/**
> + * mux.h - USB Port Mux defines
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> + *
> + * 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_INTEL_MUX_H
> +#define __LINUX_USB_INTEL_MUX_H
> +
> +#include <linux/usb.h>
> +
> +struct intel_mux_dev {
> +	struct device	*dev;
> +	char		*extcon_name;
> +	char		*cable_name;
> +	int		(*cable_set_cb)(struct intel_mux_dev *mux);
> +	int		(*cable_unset_cb)(struct intel_mux_dev *mux);
> +};

This is a device, why not make it one?  Don't just hold a reference.
And do you really even hold that reference?

> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX)
> +extern int intel_usb_mux_register(struct intel_mux_dev *mux);
> +extern int intel_usb_mux_unregister(struct device *dev);

It's obvious you didn't run this through checkpatch.pl, please do so...

And your api is horrid, think about what you want the "core" to do here,
it should be the one creating the device and returning it to the caller,
not forcing the caller to somehow create it first and then pass it in.

And why is it not symmetrical, you are passing one thing into register
and another into unregister.

Come on, I expect better work from Intel kernel developers, I shouldn't
have to be complaining about basic crap like this...

greg k-h