Re: [RFC 1/2] USB: OTG: Hold wakeupsource when VBUS present

[Felipe Balbi]

Hi,

On Fri, Aug 22, 2014 at 03:19:32PM +0530, Kiran Kumar Raparthy wrote:
> From: Todd Poynor <toddpoynor@xxxxxxxxxx>
> 
> USB: OTG: Hold wakeupsource when VBUS present
> 
> Enabled by default, can disable with:

I would turn this around and make it disabled by default so that it
doesn't change behavior for distro kernels.

>    echo N > /sys/module/otg_wakeupsource/parameters/enabled
> 
> This is one of the number of patches from the Android AOSP common.git tree,
> which is used on almost all Android devices. so I wanted to submit it for
> review to see if it should go upstream.

you never explain why this is needed and you have also added some
information to commit log which shouldn't be here.

> Cc: Felipe Balbi <balbi@xxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Cc: Android Kernel Team <kernel-team@xxxxxxxxxxx>
> Cc: John Stultz <john.stultz@xxxxxxxxxx>
> Cc: Arve Hjïnnevïg <arve@xxxxxxxxxxx>
> Cc: Benoit Goby <benoit@xxxxxxxxxxx>
> Signed-off-by: Todd Poynor <toddpoynor@xxxxxxxxxx>
> Signed-off-by: Kiran Raparthy <kiran.kumar@xxxxxxxxxx>
> [kiran: Added context to commit message
> 	Included build fix from Benoit Goby and Arve Arve Hjïnnevïg
> 	Removed lock->held field in driver as this mechanism is provided in wakeup.c
> 	wakelock(wl) terminology replaced with wakeup_source(ws)
> 	sys entry(module param) field modified to otg_wakeupsource
> 	__pm_stay_awake and __pm_relax called directly from the main
> 	code instead of calling them via otgws_grab,otgws_drop]
> ---
>  drivers/usb/phy/Kconfig            |   8 ++
>  drivers/usb/phy/Makefile           |   1 +
>  drivers/usb/phy/otg-wakeupsource.c | 171 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+)
>  create mode 100644 drivers/usb/phy/otg-wakeupsource.c
> 
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index e253fa0..9c747b2 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -6,6 +6,14 @@ menu "USB Physical Layer drivers"
>  config USB_PHY
>  	def_bool n
>  
> +config USB_OTG_WAKEUPSOURCE
> +	bool "Hold a wakeupsource when USB connected"
> +	depends on PM_SLEEP
> +	select USB_PHY
> +	help
> +	  Select this to automatically hold a wakeupsource when USB is
> +	  connected, preventing suspend.
> +
>  #
>  # USB Transceiver Drivers
>  #
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index 24a9133..ca2fbaf 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -3,6 +3,7 @@
>  #
>  obj-$(CONFIG_USB_PHY)			+= phy.o
>  obj-$(CONFIG_OF)			+= of.o
> +obj-$(CONFIG_USB_OTG_WAKEUPSOURCE)		+= otg-wakeupsource.o
>  
>  # transceiver drivers, keep the list sorted
>  
> diff --git a/drivers/usb/phy/otg-wakeupsource.c b/drivers/usb/phy/otg-wakeupsource.c
> new file mode 100644
> index 0000000..fa44e11
> --- /dev/null
> +++ b/drivers/usb/phy/otg-wakeupsource.c
> @@ -0,0 +1,171 @@
> +/*
> + * otg-wakeupsource.c
> + *
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/spinlock.h>
> +#include <linux/usb/otg.h>
> +
> +static bool enabled = true;
> +static struct usb_phy *otgws_xceiv;
> +static struct notifier_block otgws_nb;

this aches to be a per-PHY thing rather forcing it to be a one-for-all.

We have systems with more than one port and the PHY framework is also
starting to be used by the host side stack.

> +static DEFINE_SPINLOCK(otgws_spinlock);
> +
> +/*
> + * Only one lock, but since these 2 fields are associated with each other...
> + */

one line comment

> +
> +struct otgws_lock {
> +	char name[40];
> +	struct wakeup_source wsource;
> +};
> +
> +/*
> + * VBUS present lock.
> + */

one line comment

> +static struct otgws_lock vbus_lock;

should be per-PHY

-- 
balbi

Attachment: signature.asc
Description: Digital signature