* [PATCH] Input: xpad - power off wireless 360 controllers on suspend @ 2016-07-26 5:35 Cameron Gutman 2016-07-27 21:32 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Cameron Gutman @ 2016-07-26 5:35 UTC (permalink / raw) To: dmitry.torokhov, rojtberg; +Cc: linux-input 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"); + static const struct xpad_device { u16 idVendor; u16 idProduct; @@ -1248,6 +1252,36 @@ static void xpad_stop_input(struct usb_xpad *xpad) usb_kill_urb(xpad->irq_in); } +static void xpad360w_poweroff_controller(struct usb_xpad *xpad) +{ + unsigned long flags; + struct xpad_output_packet *packet = + &xpad->out_packets[XPAD_OUT_CMD_IDX]; + + spin_lock_irqsave(&xpad->odata_lock, flags); + + packet->data[0] = 0x00; + packet->data[1] = 0x00; + packet->data[2] = 0x08; + packet->data[3] = 0xC0; + packet->data[4] = 0x00; + packet->data[5] = 0x00; + packet->data[6] = 0x00; + packet->data[7] = 0x00; + packet->data[8] = 0x00; + packet->data[9] = 0x00; + packet->data[10] = 0x00; + packet->data[11] = 0x00; + packet->len = 12; + packet->pending = true; + + /* Reset the sequence so we send out poweroff now */ + xpad->last_out_packet = -1; + xpad_try_sending_next_out_packet(xpad); + + spin_unlock_irqrestore(&xpad->odata_lock, flags); +} + static int xpad360w_start_input(struct usb_xpad *xpad) { int error; @@ -1590,6 +1624,15 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message) * or goes away. */ xpad360w_stop_input(xpad); + + /* + * The wireless adapter is going off now, so the + * gamepads are going to become disconnected. + * Unless explicitly disabled, power them down + * so they don't just sit there flashing. + */ + if (!disable_auto_poweroff && xpad->pad_present) + xpad360w_poweroff_controller(xpad); } else { mutex_lock(&input->mutex); if (input->users) -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-26 5:35 [PATCH] Input: xpad - power off wireless 360 controllers on suspend Cameron Gutman @ 2016-07-27 21:32 ` Dmitry Torokhov 2016-07-27 21:39 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-27 21:32 UTC (permalink / raw) To: Cameron Gutman; +Cc: rojtberg, linux-input 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). > + > static const struct xpad_device { > u16 idVendor; > u16 idProduct; > @@ -1248,6 +1252,36 @@ static void xpad_stop_input(struct usb_xpad *xpad) > usb_kill_urb(xpad->irq_in); > } > > +static void xpad360w_poweroff_controller(struct usb_xpad *xpad) > +{ > + unsigned long flags; > + struct xpad_output_packet *packet = > + &xpad->out_packets[XPAD_OUT_CMD_IDX]; > + > + spin_lock_irqsave(&xpad->odata_lock, flags); > + > + packet->data[0] = 0x00; > + packet->data[1] = 0x00; > + packet->data[2] = 0x08; > + packet->data[3] = 0xC0; > + packet->data[4] = 0x00; > + packet->data[5] = 0x00; > + packet->data[6] = 0x00; > + packet->data[7] = 0x00; > + packet->data[8] = 0x00; > + packet->data[9] = 0x00; > + packet->data[10] = 0x00; > + packet->data[11] = 0x00; > + packet->len = 12; > + packet->pending = true; I wonder of we don't want to convert commands to something like that: static const u8 power_off_cmd[] = { 0x00, 0x00, 0x08, 0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }; ... memcpy(packet->data, power_off_cmd, sizeof(power_off_cmd)); // if we need to change something // packet->data[3] += command; // packet->data[6] = id; packet->len = sizeof(power_off_cmd); ... This should be a separate patch though. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-27 21:32 ` Dmitry Torokhov @ 2016-07-27 21:39 ` Dmitry Torokhov 2016-07-27 23:24 ` Cameron Gutman 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-27 21:39 UTC (permalink / raw) To: Cameron Gutman; +Cc: rojtberg, linux-input 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). By the way, I think we can allow root writing to it, there is nothing that stops us from changing behavior at runtime. > > > + > > static const struct xpad_device { > > u16 idVendor; > > u16 idProduct; > > @@ -1248,6 +1252,36 @@ static void xpad_stop_input(struct usb_xpad *xpad) > > usb_kill_urb(xpad->irq_in); > > } > > > > +static void xpad360w_poweroff_controller(struct usb_xpad *xpad) > > +{ > > + unsigned long flags; > > + struct xpad_output_packet *packet = > > + &xpad->out_packets[XPAD_OUT_CMD_IDX]; > > + > > + spin_lock_irqsave(&xpad->odata_lock, flags); > > + > > + packet->data[0] = 0x00; > > + packet->data[1] = 0x00; > > + packet->data[2] = 0x08; > > + packet->data[3] = 0xC0; > > + packet->data[4] = 0x00; > > + packet->data[5] = 0x00; > > + packet->data[6] = 0x00; > > + packet->data[7] = 0x00; > > + packet->data[8] = 0x00; > > + packet->data[9] = 0x00; > > + packet->data[10] = 0x00; > > + packet->data[11] = 0x00; > > + packet->len = 12; > > + packet->pending = true; > > I wonder of we don't want to convert commands to something like that: > > static const u8 power_off_cmd[] = { > 0x00, 0x00, 0x08, 0xc0, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, > }; > > ... > > memcpy(packet->data, power_off_cmd, sizeof(power_off_cmd)); > // if we need to change something > // packet->data[3] += command; > // packet->data[6] = id; > packet->len = sizeof(power_off_cmd); > ... > > This should be a separate patch though. > > Thanks. > > -- > Dmitry -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-27 21:39 ` Dmitry Torokhov @ 2016-07-27 23:24 ` Cameron Gutman 2016-07-28 0:04 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Cameron Gutman @ 2016-07-27 23:24 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: rojtberg, linux-input 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. >> >>> + >>> static const struct xpad_device { >>> u16 idVendor; >>> u16 idProduct; >>> @@ -1248,6 +1252,36 @@ static void xpad_stop_input(struct usb_xpad *xpad) >>> usb_kill_urb(xpad->irq_in); >>> } >>> >>> +static void xpad360w_poweroff_controller(struct usb_xpad *xpad) >>> +{ >>> + unsigned long flags; >>> + struct xpad_output_packet *packet = >>> + &xpad->out_packets[XPAD_OUT_CMD_IDX]; >>> + >>> + spin_lock_irqsave(&xpad->odata_lock, flags); >>> + >>> + packet->data[0] = 0x00; >>> + packet->data[1] = 0x00; >>> + packet->data[2] = 0x08; >>> + packet->data[3] = 0xC0; >>> + packet->data[4] = 0x00; >>> + packet->data[5] = 0x00; >>> + packet->data[6] = 0x00; >>> + packet->data[7] = 0x00; >>> + packet->data[8] = 0x00; >>> + packet->data[9] = 0x00; >>> + packet->data[10] = 0x00; >>> + packet->data[11] = 0x00; >>> + packet->len = 12; >>> + packet->pending = true; >> >> I wonder of we don't want to convert commands to something like that: >> >> static const u8 power_off_cmd[] = { >> 0x00, 0x00, 0x08, 0xc0, 0x00, 0x00, 0x00, 0x00, >> 0x00, 0x00, 0x00, 0x00, >> }; >> >> ... >> >> memcpy(packet->data, power_off_cmd, sizeof(power_off_cmd)); >> // if we need to change something >> // packet->data[3] += command; >> // packet->data[6] = id; >> packet->len = sizeof(power_off_cmd); >> ... >> >> This should be a separate patch though. I was actually planning to submit a series to clean up all of those unnecessary XTYPE_UNKNOWN checks scattered all over and add some WARN_ONs in pad-specific code to make sure we never get there with the wrong pad type. I can add a patch to convert the commands over to the nicer format, and another to loosen the permissions on those module params to allow root writing. I would also like to track down this URB hang I'm sometimes seeing on resume. I'll probably do that before starting the cleanup work. >> >> Thanks. >> >> -- >> Dmitry > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-27 23:24 ` Cameron Gutman @ 2016-07-28 0:04 ` Dmitry Torokhov 2016-07-28 0:10 ` Cameron Gutman 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-28 0:04 UTC (permalink / raw) To: Cameron Gutman; +Cc: rojtberg, linux-input 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. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-28 0:04 ` Dmitry Torokhov @ 2016-07-28 0:10 ` Cameron Gutman 2016-07-28 0:29 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Cameron Gutman @ 2016-07-28 0:10 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: rojtberg, linux-input 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-28 0:10 ` Cameron Gutman @ 2016-07-28 0:29 ` Dmitry Torokhov 2016-07-28 1:23 ` Cameron Gutman 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-28 0:29 UTC (permalink / raw) To: Cameron Gutman; +Cc: rojtberg, linux-input 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. -- Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-28 0:29 ` Dmitry Torokhov @ 2016-07-28 1:23 ` Cameron Gutman 2016-07-28 1:25 ` Dmitry Torokhov 0 siblings, 1 reply; 9+ messages in thread From: Cameron Gutman @ 2016-07-28 1:23 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: rojtberg, linux-input 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. --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend 2016-07-28 1:23 ` Cameron Gutman @ 2016-07-28 1:25 ` Dmitry Torokhov 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Torokhov @ 2016-07-28 1:25 UTC (permalink / raw) To: Cameron Gutman; +Cc: rojtberg, linux-input 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-28 1:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-26 5:35 [PATCH] Input: xpad - power off wireless 360 controllers on suspend Cameron Gutman 2016-07-27 21:32 ` Dmitry Torokhov 2016-07-27 21:39 ` Dmitry Torokhov 2016-07-27 23:24 ` Cameron Gutman 2016-07-28 0:04 ` Dmitry Torokhov 2016-07-28 0:10 ` Cameron Gutman 2016-07-28 0:29 ` Dmitry Torokhov 2016-07-28 1:23 ` Cameron Gutman 2016-07-28 1:25 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).