Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Jul 27, 2016 at 06:23:19PM -0700, Cameron Gutman wrote:
> On 07/27/2016 05:29 PM, Dmitry Torokhov wrote:
> > On Wed, Jul 27, 2016 at 05:10:41PM -0700, Cameron Gutman wrote:
> >> On 07/27/2016 05:04 PM, Dmitry Torokhov wrote:
> >>> On Wed, Jul 27, 2016 at 04:24:06PM -0700, Cameron Gutman wrote:
> >>>> On 07/27/2016 02:39 PM, Dmitry Torokhov wrote:
> >>>>> On Wed, Jul 27, 2016 at 02:32:22PM -0700, Dmitry Torokhov wrote:
> >>>>>> On Mon, Jul 25, 2016 at 10:35:08PM -0700, Cameron Gutman wrote:
> >>>>>>> When the USB wireless adapter is suspended, the controllers
> >>>>>>> lose their connection. This causes them to start flashing
> >>>>>>> their LED rings and searching for the wireless adapter
> >>>>>>> again, wasting the controller's battery power.
> >>>>>>>
> >>>>>>> Instead, we will tell the controllers to power down when
> >>>>>>> we suspend. This mirrors the behavior of the controllers
> >>>>>>> when connected to the console itself and how the official
> >>>>>>> Xbox One wireless adapter behaves on Windows.
> >>>>>>>
> >>>>>>> Signed-off-by: Cameron Gutman <aicommander@gmail.com>
> >>>>>>> ---
> >>>>>>> This patch is independent of the other xpad patch [0] that I
> >>>>>>> submitted (and decided to wait on). It applies against
> >>>>>>> unmodified xpad.c in master.
> >>>>>>>
> >>>>>>> [0] http://www.spinics.net/lists/linux-input/msg46062.html
> >>>>>>> ---
> >>>>>>>  drivers/input/joystick/xpad.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>  1 file changed, 43 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> >>>>>>> index a529a45..3408019 100644
> >>>>>>> --- a/drivers/input/joystick/xpad.c
> >>>>>>> +++ b/drivers/input/joystick/xpad.c
> >>>>>>> @@ -115,6 +115,10 @@ static bool sticks_to_null;
> >>>>>>>  module_param(sticks_to_null, bool, S_IRUGO);
> >>>>>>>  MODULE_PARM_DESC(sticks_to_null, "Do not map sticks at all for unknown pads");
> >>>>>>>  
> >>>>>>> +static bool disable_auto_poweroff;
> >>>>>>> +module_param(disable_auto_poweroff, bool, S_IRUGO);
> >>>>>>> +MODULE_PARM_DESC(disable_auto_poweroff, "Do not power off wireless controllers on suspend");
> >>>>>>
> >>>>>> Why negating? Why not do
> >>>>>>
> >>>>>> static bool xpad_auto_poweroff = true;
> >>>>>>
> >>>>>> ?
> >>>>>>
> >>>>>> (No need to resubmit if agree/disagree, I can fix up on my side).
> >>>>
> >>>> Yeah, sounds fine to make it default Y and get rid of the negation.
> >>>>
> >>>> I'm fine with it if you want to make that change and apply it. See my
> >>>> comments below about the other issues.
> >>>>
> >>>>>
> >>>>> By the way, I think we can allow root writing to it, there is nothing
> >>>>> that stops us from changing behavior at runtime.
> >>>>
> >>>> Yep, I was following the convention of the existing module params. They
> >>>> can probably all be changed to allow root writing.
> >>>
> >>> My rule of thumb is we allow writing if behavior changes immediately.
> >>> The other parameters only affect devices that will be bound after the
> >>> parameter has been changed, which woudl be confusing. So they are RO and
> >>> you have to specify them at module load time.
> >>>
> >>> Thanks.
> >>>
> >>
> >> Sounds reasonable. Would you like to make the permission change also or
> >> do you prefer I resubmit the patch with those 2 modifications?
> > 
> > No, I made them. If you have a moment please take a look at my "next"
> > branch on kernel.org.
> > 
> 
> I reviewed and tested your changes. I think you forgot to undo one of the
> negations. With the patch below applied on the commit in next, it works
> correctly.

Oops, you are right. I'll fold it into the original patch.

> 
> ---
>  drivers/input/joystick/xpad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 0c8d5c3..83af17a 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -1631,7 +1631,7 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  		 * Unless explicitly disabled, power them down
>  		 * so they don't just sit there flashing.
>  		 */
> -		if (!auto_poweroff && xpad->pad_present)
> +		if (auto_poweroff && xpad->pad_present)
>  			xpad360w_poweroff_controller(xpad);
>  	} else {
>  		mutex_lock(&input->mutex);
> -- 
> 2.7.4
> 

-- 
Dmitry