* Re: Re: [PATCH 0/7] mfd: AXP20x: Add support for AXP202 and AXP209
From: Hans de Goede @ 2014-03-01 19:29 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
emilio-0Z03zUJReD5OxF6Tv1QG9Q, wens-jdAy2FN1RRM,
sameo-VuQAYsv1563Yd54FQh9/CA, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <CAOQ7t2ae35E-eiQW4DFspdTe3H7AWGzRpw4B_1t27p0OSkX_Uw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi,
On 03/01/2014 06:17 PM, Carlo Caione wrote:
> On Sat, Mar 1, 2014 at 5:56 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Carlo,
>>
>> Great work, thanks for all the time you're putting into this!
>
> Hi Hans :)
>
>> I've 2 questions:
>>
>> 1) What dependencies does this patch-set have? Obviously it needs the NMI irq
>> patches for A20, anything else ? I no longer see any use of a special flag
>> for ack on unmask, is that no longer needed ?
>
> Yes, the only dependency is on the NMI controller patch.
> After a discussion with Maxime and Thomas I decided to not push for
> including the special flag for ack on unmask in the irqchip core but
> to use the unmask callback as in the v3 version of the NMI controller
> driver (so no flag needed)
>
>> 2) No poweroff functionality ? That would be really great to have.
>
> Actually in [PATCH 1/7] I support the poweroff using the pm_power_off hook.
Ah I missed that, cool.
I'm doing with sunxi hacking for today, but I'll add these to sunxi-devel
and them give them a test run tomorrow.
Regards,
Hans
^ permalink raw reply
* [PATCH] input: ff-memless: don't schedule already playing effect to play again
From: Felix Rueegg @ 2014-03-02 11:35 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Felix Rueegg
When an effect with zero replay length, zero replay delay
and zero envelope attack length is uploaded, it is played and then scheduled to play
again one timer tick later. This triggers a warning (URB submitted while
active) in combination with the xpad driver.
Skipping the rescheduling of this effect fixes the issue.
Signed-off-by: Felix Rueegg <felix.rueegg@gmail.com>
---
drivers/input/ff-memless.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 74c0d8c..2e06948 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -139,10 +139,13 @@ static void ml_schedule_timer(struct ml_device *ml)
if (!test_bit(FF_EFFECT_STARTED, &state->flags))
continue;
- if (test_bit(FF_EFFECT_PLAYING, &state->flags))
+ if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
next_at = calculate_next_time(state);
- else
+ if (next_at == now)
+ continue;
+ } else {
next_at = state->play_at;
+ }
if (time_before_eq(now, next_at) &&
(++events == 1 || time_before(next_at, earliest)))
--
1.9.0
^ permalink raw reply related
* Re: [PATCH] input: ff-memless: don't schedule already playing effect to play again
From: Elias Vanderstuyft @ 2014-03-02 13:17 UTC (permalink / raw)
To: Michal Malý; +Cc: Felix Rueegg, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <1393760143-5986-1-git-send-email-felix.rueegg@gmail.com>
On Sun, Mar 2, 2014 at 12:35 PM, Felix Rueegg <felix.rueegg@gmail.com> wrote:
> When an effect with zero replay length, zero replay delay
> and zero envelope attack length is uploaded, it is played and then scheduled to play
> again one timer tick later. This triggers a warning (URB submitted while
> active) in combination with the xpad driver.
>
> Skipping the rescheduling of this effect fixes the issue.
>
> Signed-off-by: Felix Rueegg <felix.rueegg@gmail.com>
> ---
> drivers/input/ff-memless.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 74c0d8c..2e06948 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct ml_device *ml)
> if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> continue;
>
> - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> next_at = calculate_next_time(state);
> - else
> + if (next_at == now)
> + continue;
> + } else {
> next_at = state->play_at;
> + }
>
> if (time_before_eq(now, next_at) &&
> (++events == 1 || time_before(next_at, earliest)))
> --
> 1.9.0
>
> --
@Michal: Is ff-memless-next also affected by this problem?
Elias
^ permalink raw reply
* Re: [PATCH] input: ff-memless: don't schedule already playing effect to play again
From: Michal Malý @ 2014-03-02 13:37 UTC (permalink / raw)
To: Felix Rueegg; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <CADbOyBSMG_a=2qPmM0EU1LEd7y1ZxrpLuoz4MHroX6ri4-nJ3g@mail.gmail.com>
On Sunday 02 of March 2014 14:17:58 you wrote:
> On Sun, Mar 2, 2014 at 12:35 PM, Felix Rueegg
<felix.rueegg@gmail.com> wrote:
> > When an effect with zero replay length, zero replay delay
> > and zero envelope attack length is uploaded, it is played and then
> > scheduled to play again one timer tick later. This triggers a
warning
> > (URB submitted while active) in combination with the xpad driver.
> >
> > Skipping the rescheduling of this effect fixes the issue.
> >
> > Signed-off-by: Felix Rueegg <felix.rueegg@gmail.com>
> > ---
> >
> > drivers/input/ff-memless.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > index 74c0d8c..2e06948 100644
> > --- a/drivers/input/ff-memless.c
> > +++ b/drivers/input/ff-memless.c
> > @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct
ml_device *ml)
> >
> > if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> >
> > continue;
> >
> > - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> > + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> >
> > next_at = calculate_next_time(state);
> >
> > - else
> > + if (next_at == now)
> > + continue;
> > + } else {
> >
> > next_at = state->play_at;
> >
> > + }
> >
> > if (time_before_eq(now, next_at) &&
> >
> > (++events == 1 || time_before(next_at, earliest)))
> >
> > --
> > 1.9.0
> >
> > --
>
> @Michal: Is ff-memless-next also affected by this problem?
>
> Elias
I hope it's not, see mlnx_get_envelope_update_time(), this part in
particular:
/* Prevent the effect from being started twice */
if (mlnxeff->begin_at == now && mlnx_is_playing(mlnxeff))
return now - 1;
return mlnxeff->begin_at;
Michal
^ permalink raw reply
* Fwd: [PATCH] input: ff-memless: don't schedule already playing effect to play again
From: Elias Vanderstuyft @ 2014-03-02 13:36 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Felix Rueegg, linux-input, linux-kernel, Michal Malý
In-Reply-To: <1452937.KaXPu2ynQ6@sigyn>
---------- Forwarded message ----------
From: Michal Malý <madcatxster@prifuk.cz>
Date: Sun, Mar 2, 2014 at 2:29 PM
Subject: Re: [PATCH] input: ff-memless: don't schedule already playing
effect to play again
To: Elias Vanderstuyft <elias.vds@gmail.com>
On Sunday 02 of March 2014 14:17:58 you wrote:
> On Sun, Mar 2, 2014 at 12:35 PM, Felix Rueegg
<felix.rueegg@gmail.com> wrote:
> > When an effect with zero replay length, zero replay delay
> > and zero envelope attack length is uploaded, it is played and then
> > scheduled to play again one timer tick later. This triggers a
warning
> > (URB submitted while active) in combination with the xpad driver.
> >
> > Skipping the rescheduling of this effect fixes the issue.
> >
> > Signed-off-by: Felix Rueegg <felix.rueegg@gmail.com>
> > ---
> >
> > drivers/input/ff-memless.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > index 74c0d8c..2e06948 100644
> > --- a/drivers/input/ff-memless.c
> > +++ b/drivers/input/ff-memless.c
> > @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct
ml_device *ml)
> >
> > if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> >
> > continue;
> >
> > - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> > + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> >
> > next_at = calculate_next_time(state);
> >
> > - else
> > + if (next_at == now)
> > + continue;
> > + } else {
> >
> > next_at = state->play_at;
> >
> > + }
> >
> > if (time_before_eq(now, next_at) &&
> >
> > (++events == 1 || time_before(next_at, earliest)))
> >
> > --
> > 1.9.0
> >
> > --
>
> @Michal: Is ff-memless-next also affected by this problem?
>
> Elias
I hope it's not, see mlnx_get_envelope_update_time(), this part in
particular:
/* Prevent the effect from being started twice */
if (mlnxeff->begin_at == now && mlnx_is_playing(mlnxeff))
return now - 1;
return mlnxeff->begin_at;
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/6] HID: sony: More Sony controller fixes and improvements.
From: Frank Praznik @ 2014-03-02 16:26 UTC (permalink / raw)
To: Antonio Ospite, Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <20140301145346.d1b3305ba0b186d452a34beb@ao2.it>
On 3/1/2014 08:53, Antonio Ospite wrote:
> Hi Frank,
>
> On Fri, 28 Feb 2014 22:58:55 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> This set consists of one bugfix, two mostly cosmetic changes and three larger
>> patches for the LED subsystem.
>>
>> Patch #4 adds hardware blink support to the controller LEDs. Values from 0 to
>> 2.5 seconds are supported by the hardware. The Sixaxis can set all of the LEDs
>> individually, but the DualShock 4 only has one global setting for the entire
>> light bar so only the value from the most recently set LED is used.
>>
> Adding this is OK, as it adds access to something supported by the
> hardware.
>
>> Patch #5 adds an LED trigger that reports the controller battery status via the
>> registered LEDs. The LEDs will flash if the controller is charging or if the
>> battery is low, and remain solid otherwise.
>>
> This kind of logic _may_ belong to userspace. More comments in the
> actual patch.
Functionally this trigger is no different from the ones registered by
the power supply system when a battery is registered, aside from the
specific conditions under which the LED blinks. I can understand the
reservations about setting it as the default, but at the same time it's
a trigger which can be easily disabled on the controller LEDs or be used
to control other LED devices if the user desires it.
If this is something best kept out of kernel code though, that's fine.
>
>> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
>> blue on the DualShock 4 so there is some indication that the controller is
>> powered on and connected in the case of Bluetooth. The code can be used to set
>> the LEDs based on the device number, but I'm not sure how to actually retrieve
>> the controller number from the system. I saw the xpad patches posted a few
>> weeks ago where the minor number of the joydev device was used, but I'm under
>> the impression that doing that is not ideal. Any suggestions?
> Setting the controller number is done by the bluez sixaxis plugin[1]
> (in bluez 5.x) following the X in /dev/input/jsX, this covers the
> case of a mixed-joypad scenario, IMHO it makes sense that the
> controller number matches the joystick device number.
> Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
> sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
>
> This has been done in userspace with libudev for 2 reasons:
> 1. the hid drivers should not have knowledge of the joystick layer;
> 2. kernel drivers should be as simple as possible, and try to just
> exposing hardware functionalities but with as less "business logic"
> as possible in them.
>
> The current implementation in the bluez plugin uses hidraw, but support
> for the sysfs led class could be added in order to avoid conflicts with
> the rumble; IIRC, currently, setting rumble values could override the
> LED settings done via hidraw, because the LEDs state is not tracked in
> the latter case.
>
> Ciao,
> Antonio
>
> [1]
> http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
>
This can be done in the driver. See
https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
It seems that the main problem with that patch is that modern systems
shouldn't be relying on joydev for this functionality. I'd like to know
what David Herrmann and Greg Kroah-Hartman came up with regarding the
solution mentioned in the reply as it would be nice to be able to set
the LEDs to the proper default values in the driver without needing to
rely on an external daemon. Setting the defaults in the driver doesn't
interfere with setting custom values after the device is initialized, so
there are no issues if the user wants to use a custom LED daemon.
As far as the behavior of patch #6 (setting the LEDs to the same number
or color on every connected device just to indicate that the controller
is turned on), the xpad and wiimote drivers both initialize the LEDs to
some default value where at least one is on to indicate that the
controller is powered on and connected to the system. The xpad driver
increments an atomic counter for assigning values as controllers are
connected and the wiimote always sets LED #1 to on. Not ideal, but it
serves it's purpose.
Personally I don't like the idea of relying on a BlueZ plugin to set the
controller LED values as it seems to bring a lot of issues with it:
users may not have BlueZ installed or enabled, some distros still use an
old version, the plugin relies on joydev to get the device number which
is why the patch I linked was NAKed, the current plugin implementation
doesn't set them via sysfs so the setting will be lost if force-feedback
is used and the plugin could conflict with other user-installed daemons
that set the LEDs (unless udev guarantees a notification order?). In
the latter scenario, the user could disable the plugin, but then you
lose the Sixaxis pairing functionality that it provides. I also have to
question as to why BlueZ is considered an appropriate place to set
controller LEDs, particularly on controllers that aren't connected via
Bluetooth.
^ permalink raw reply
* Re: [PATCH 0/6] HID: sony: More Sony controller fixes and improvements.
From: David Herrmann @ 2014-03-02 17:21 UTC (permalink / raw)
To: Frank Praznik; +Cc: Antonio Ospite, open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <53135BA6.9090501@oh.rr.com>
Hi
On Sun, Mar 2, 2014 at 5:26 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> On 3/1/2014 08:53, Antonio Ospite wrote:
>>
>> Hi Frank,
>>
>> On Fri, 28 Feb 2014 22:58:55 -0500
>> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>>
>>> This set consists of one bugfix, two mostly cosmetic changes and three
>>> larger
>>> patches for the LED subsystem.
>>>
>>> Patch #4 adds hardware blink support to the controller LEDs. Values from
>>> 0 to
>>> 2.5 seconds are supported by the hardware. The Sixaxis can set all of
>>> the LEDs
>>> individually, but the DualShock 4 only has one global setting for the
>>> entire
>>> light bar so only the value from the most recently set LED is used.
>>>
>> Adding this is OK, as it adds access to something supported by the
>> hardware.
>>
>>> Patch #5 adds an LED trigger that reports the controller battery status
>>> via the
>>> registered LEDs. The LEDs will flash if the controller is charging or if
>>> the
>>> battery is low, and remain solid otherwise.
>>>
>> This kind of logic _may_ belong to userspace. More comments in the
>> actual patch.
>
> Functionally this trigger is no different from the ones registered by the
> power supply system when a battery is registered, aside from the specific
> conditions under which the LED blinks. I can understand the reservations
> about setting it as the default, but at the same time it's a trigger which
> can be easily disabled on the controller LEDs or be used to control other
> LED devices if the user desires it.
>
> If this is something best kept out of kernel code though, that's fine.
>
>>
>>> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis
>>> and
>>> blue on the DualShock 4 so there is some indication that the controller
>>> is
>>> powered on and connected in the case of Bluetooth. The code can be used
>>> to set
>>> the LEDs based on the device number, but I'm not sure how to actually
>>> retrieve
>>> the controller number from the system. I saw the xpad patches posted a
>>> few
>>> weeks ago where the minor number of the joydev device was used, but I'm
>>> under
>>> the impression that doing that is not ideal. Any suggestions?
>>
>> Setting the controller number is done by the bluez sixaxis plugin[1]
>> (in bluez 5.x) following the X in /dev/input/jsX, this covers the
>> case of a mixed-joypad scenario, IMHO it makes sense that the
>> controller number matches the joystick device number.
>> Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
>> sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
>>
>> This has been done in userspace with libudev for 2 reasons:
>> 1. the hid drivers should not have knowledge of the joystick layer;
>> 2. kernel drivers should be as simple as possible, and try to just
>> exposing hardware functionalities but with as less "business logic"
>> as possible in them.
>>
>> The current implementation in the bluez plugin uses hidraw, but support
>> for the sysfs led class could be added in order to avoid conflicts with
>> the rumble; IIRC, currently, setting rumble values could override the
>> LED settings done via hidraw, because the LEDs state is not tracked in
>> the latter case.
>>
>> Ciao,
>> Antonio
>>
>> [1]
>> http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
>>
> This can be done in the driver. See
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>
> It seems that the main problem with that patch is that modern systems
> shouldn't be relying on joydev for this functionality. I'd like to know
> what David Herrmann and Greg Kroah-Hartman came up with regarding the
> solution mentioned in the reply as it would be nice to be able to set the
> LEDs to the proper default values in the driver without needing to rely on
> an external daemon. Setting the defaults in the driver doesn't interfere
> with setting custom values after the device is initialized, so there are no
> issues if the user wants to use a custom LED daemon.
The application using the controllers should do this. It allows to set
coherent values across different devices and buses. So you can have
one BT device, one USB device and one whatever device, but assign
sequential numbers to them from the application. We cannot do that in
the kernel, as we don't know which policy to follow. However, we can
provide a sane default. joydev is the *wrong* way to do that, though.
Instead, you should just use an ida/idr in your driver and assign
numbers properly. You can also use usb-interface-ids or similar if
they make sense. But please don't try to access input-handlers like
evdev or joydev. They're one layer atop, not below your driver!
Thanks
David
^ permalink raw reply
* Re: [PATCH 4/6] HID: sony: Add blink support to the LEDs
From: Frank Praznik @ 2014-03-02 23:43 UTC (permalink / raw)
To: Antonio Ospite, Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <20140301152031.1bbf4e527d374f1121401d32@ao2.it>
On 3/1/2014 09:20, Antonio Ospite wrote:
> Hi Frank,
>
> On Fri, 28 Feb 2014 22:58:59 -0500
> Frank Praznik <frank.praznik@oh.rr.com> wrote:
>
>> Add support for setting the blink rate of the LEDs. The Sixaxis allows control
>> over each individual LED, but the Dualshock 4 only has one global control for
>> the light bar so changing any individual color changes them all.
>>
>> Setting the brightness cancels the blinking as per the LED class
>> specifications.
>>
>> The Sixaxis and Dualshock 4 controllers accept delays in decisecond increments
>> from 0 to 255 (2550 milliseconds).
>>
>> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
>> ---
>> drivers/hid/hid-sony.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 93 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index dc6e6fa..914a6cc 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -741,6 +741,8 @@ struct sony_sc {
>> __u8 battery_charging;
>> __u8 battery_capacity;
>> __u8 led_state[MAX_LEDS];
>> + __u8 led_blink_on[MAX_LEDS];
>> + __u8 led_blink_off[MAX_LEDS];
> Are those values meant to be for the duty cycle in deciseconds?
> What about using a more explicative name? leds_blink_on makes me think
> to something boolean (it could be just me), maybe leds_delay_on and
> leds_delay_off?
>
> Also grouping spare arrays into a single array of structs may be worth
> considering:
>
> struct sony_sc {
> ...
> struct {
> struct led_classdev *ldev;
> __u8 state;
> __u8 delay_on;
> __u8 delay_off;
> } leds[MAX_LEDS];
> ...
> };
>
> Defining the struct for leds separately if you prefer so.
It would be a little more organized, but it would also add extra padding
to the struct. I'll think about this one.
>
>> __u8 led_count;
>> };
>>
>> @@ -1127,8 +1129,7 @@ static void sony_led_set_brightness(struct led_classdev *led,
>> struct device *dev = led->dev->parent;
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> struct sony_sc *drv_data;
>> -
>> - int n;
>> + int n, blink_index = 0;
>>
>> drv_data = hid_get_drvdata(hdev);
>> if (!drv_data) {
>> @@ -1136,14 +1137,30 @@ static void sony_led_set_brightness(struct led_classdev *led,
>> return;
>> }
>>
>> + /* Get the index of the LED */
>> for (n = 0; n < drv_data->led_count; n++) {
>> - if (led == drv_data->leds[n]) {
>> - if (value != drv_data->led_state[n]) {
>> - drv_data->led_state[n] = value;
>> - sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>> - }
>> + if (led == drv_data->leds[n])
>> break;
>> - }
>> + }
>> +
>> + /* This LED is not registered on this device */
>> + if (n >= drv_data->led_count)
>> + return;
>> +
>> + /* The DualShock 4 has a global LED and always uses index 0 */
>> + if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER))
>> + blink_index = n;
>> +
> If you feel the need for a comment here, what about not initializing
> blink_index to 0 before and add an else block here, this way the code
> itself is more explicit, and more symmetric too.
Good idea, I'll fix it in v2.
>
>> + if ((value != drv_data->led_state[n]) ||
>> + drv_data->led_blink_on[blink_index] ||
>> + drv_data->led_blink_off[blink_index]) {
>> + drv_data->led_state[n] = value;
>> +
>> + /* Setting the brightness stops the blinking */
>> + drv_data->led_blink_on[blink_index] = 0;
>> + drv_data->led_blink_off[blink_index] = 0;
>> +
>> + sony_set_leds(drv_data, drv_data->led_state, drv_data->led_count);
>> }
>> }
>>
>> @@ -1169,6 +1186,56 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
>> return LED_OFF;
>> }
>>
>> +static int sony_blink_set(struct led_classdev *led, unsigned long *delay_on,
>> + unsigned long *delay_off)
>> +{
>> + struct device *dev = led->dev->parent;
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct sony_sc *drv_data = hid_get_drvdata(hdev);
>> + int n = 0;
>> + __u8 new_on, new_off;
>> +
>> + if (!drv_data) {
>> + hid_err(hdev, "No device data\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Max delay is 255 deciseconds or 2550 milliseconds */
>> + if (*delay_on > 2550)
>> + *delay_on = 2550;
>> + if (*delay_off > 2550)
>> + *delay_off = 2550;
>> +
>> + /* Blink at 1 Hz if both values are zero */
>> + if (!*delay_on && !*delay_off)
>> + *delay_on = *delay_off = 1000;
>> +
>> + new_on = *delay_on / 10;
>> + new_off = *delay_off / 10;
>> +
>> + /* The blink controls are global on the DualShock 4 */
>> + if (!(drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
>> + for (n = 0; n < drv_data->led_count; n++) {
>> + if (led == drv_data->leds[n])
>> + break;
>> + }
>> + }
>> +
>> + /* This LED is not registered on this device */
>> + if (n >= drv_data->led_count)
>> + return -EINVAL;
>> +
>> + /* Don't schedule work if the values didn't change */
>> + if (new_on != drv_data->led_blink_on[n] ||
>> + new_off != drv_data->led_blink_off[n]) {
>> + drv_data->led_blink_on[n] = new_on;
>> + drv_data->led_blink_off[n] = new_off;
>> + schedule_work(&drv_data->state_worker);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static void sony_leds_remove(struct sony_sc *sc)
>> {
>> struct led_classdev *led;
>> @@ -1259,6 +1326,9 @@ static int sony_leds_init(struct sony_sc *sc)
>> led->brightness_get = sony_led_get_brightness;
>> led->brightness_set = sony_led_set_brightness;
>>
>> + if (!(sc->quirks & BUZZ_CONTROLLER))
>> + led->blink_set = sony_blink_set;
>> +
>> ret = led_classdev_register(&hdev->dev, led);
>> if (ret) {
>> hid_err(hdev, "Failed to register LED %d\n", n);
>> @@ -1280,14 +1350,15 @@ error_leds:
>> static void sixaxis_state_worker(struct work_struct *work)
>> {
>> struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
>> + int n;
>> unsigned char buf[] = {
>> 0x01,
>> 0x00, 0xff, 0x00, 0xff, 0x00,
>> 0x00, 0x00, 0x00, 0x00, 0x00,
>> - 0xff, 0x27, 0x10, 0x00, 0x32,
>> - 0xff, 0x27, 0x10, 0x00, 0x32,
>> - 0xff, 0x27, 0x10, 0x00, 0x32,
>> - 0xff, 0x27, 0x10, 0x00, 0x32,
>> + 0xff, 0x27, 0x10, 0x00, 0x32, /* LED 4 */
>> + 0xff, 0x27, 0x10, 0x00, 0x32, /* LED 3 */
>> + 0xff, 0x27, 0x10, 0x00, 0x32, /* LED 2 */
>> + 0xff, 0x27, 0x10, 0x00, 0x32, /* LED 1 */
>> 0x00, 0x00, 0x00, 0x00, 0x00
>> };
>>
>> @@ -1301,6 +1372,13 @@ static void sixaxis_state_worker(struct work_struct *work)
>> buf[10] |= sc->led_state[2] << 3;
>> buf[10] |= sc->led_state[3] << 4;
>>
>> + for (n = 0; n < 4; n++) {
>> + if (sc->led_blink_on[n] || sc->led_blink_off[n]) {
>> + buf[29-(n*5)] = sc->led_blink_off[n];
>> + buf[30-(n*5)] = sc->led_blink_on[n];
> ^^^^^^^^
> Kernel coding style prefers spaces around operators.
> I see that scripts/checkpatch.pl does not warn about this, but it's in
> Documentation/CodingStyle.
>
> However the calculations here made me wonder if it's the case to go
> semantic and represent the output report with a struct instead of an
> array (maybe even using a union), so you can access the individual
> fields in a more meaningful, and less bug prone, way.
>
> For example (untested):
>
> struct sixaxis_led {
> uint8_t time_enabled; /* the total time the led is active (0xff means forever) */
> uint8_t duty_length; /* how long a cycle is in deciseconds (0 means "really fast") */
> uint8_t enabled;
> uint8_t duty_off; /* % of duty_length the led is off (0xff means 100%) */
> uint8_t duty_on; /* % of duty_length the led is on (0xff mean 100%) */
>
> } __attribute__ ((packed));
>
> struct sixaxis_output_report {
> uint8_t report_id;
> uint8_t rumble[5]; /* TODO: add the rumble bits here... */
> uint8_t padding[4];
> uint8_t leds_bitmap; /* bitmap of enabled LEDs: LED_1 = 0x02, LED_2 = 0x04, ... */
> struct sixaxis_led led[4]; /* LEDx at (4 - x), add a macro? */
> struct sixaxis_led _reserved; /* LED5, not actually soldered */
> }; __attribute__ ((packed));
>
> union output_report_01 {
> struct sixaxis_output_report data;
> uint8_t buf[36];
> };
>
> I had the snippet above buried somewhere and I don't remember where
> all the info came from.
Nice, this will be much clearer. I'll tidy it up and add this as a
separate refactor patch in v2. Thanks for the snippet.
>> + }
>> + }
>> +
>> if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>> hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> else
>> @@ -1338,6 +1416,9 @@ static void dualshock4_state_worker(struct work_struct *work)
>> buf[offset++] = sc->led_state[1];
>> buf[offset++] = sc->led_state[2];
>>
>> + buf[offset++] = sc->led_blink_on[0];
>> + buf[offset++] = sc->led_blink_off[0];
>> +
>> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
>> hid_hw_output_report(hdev, buf, 32);
>> else
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 5/6] HID: sony: Add an led trigger to report controller battery status.
From: Frank Praznik @ 2014-03-02 23:48 UTC (permalink / raw)
To: Frank Praznik, linux-input; +Cc: jkosina, dh.herrmann
In-Reply-To: <1393646341-16947-6-git-send-email-frank.praznik@oh.rr.com>
On 2/28/2014 22:59, Frank Praznik wrote:
> Creates an LED trigger that changes LED behavior depending on the state of the
> controller battery.
>
> The trigger function runs on a 500 millisecond timer and only updates the LEDs
> if the controller power state has changed or a new device has been added to the
> trigger.
>
> The trigger sets the LEDs to solid if the controller is in wireless mode and
> the battery is above 20% power or if the controller is plugged in and the
> battery has completed charging. If the controller is not charging and the
> battery drops to or below 20% it blinks the LEDs in 500ms intervals. If the
> controller is plugged in and charging it blinks the LEDs in 1 second intervals.
>
> The order of subsystem initialization had to be changed in sony_probe() so that
> the trigger is created before the LEDs are initialized.
>
> By default the controller LEDs are set to the trigger local to that controller.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
> drivers/hid/hid-sony.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 162 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 914a6cc..d7889ac 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -730,6 +730,17 @@ struct sony_sc {
> struct work_struct state_worker;
> struct power_supply battery;
>
> +#ifdef CONFIG_LEDS_TRIGGERS
> + spinlock_t trigger_lock;
> + struct led_trigger battery_trigger;
> + struct timer_list battery_trigger_timer;
> + atomic_t trigger_device_added;
> + __u8 trigger_initialized;
> + __u8 trigger_timer_initialized;
> + __u8 trigger_capacity;
> + __u8 trigger_charging;
> +#endif
> +
> #ifdef CONFIG_SONY_FF
> __u8 left;
> __u8 right;
> @@ -1329,14 +1340,19 @@ static int sony_leds_init(struct sony_sc *sc)
> if (!(sc->quirks & BUZZ_CONTROLLER))
> led->blink_set = sony_blink_set;
>
> +#ifdef CONFIG_LEDS_TRIGGERS
> + led->default_trigger = sc->battery_trigger.name;
> +#endif
> +
> + sc->leds[n] = led;
> +
> ret = led_classdev_register(&hdev->dev, led);
> if (ret) {
> hid_err(hdev, "Failed to register LED %d\n", n);
> kfree(led);
> + sc->leds[n] = NULL;
> goto error_leds;
> }
> -
> - sc->leds[n] = led;
> }
>
> return ret;
> @@ -1552,6 +1568,137 @@ static void sony_battery_remove(struct sony_sc *sc)
> sc->battery.name = NULL;
> }
>
> +#ifdef CONFIG_LEDS_TRIGGERS
> +static void sony_battery_trigger_callback(unsigned long data)
> +{
> + struct sony_sc *drv_data = (struct sony_sc *)data;
> + struct led_classdev *led;
> + unsigned long flags;
> + unsigned long delay_on, delay_off;
> + int dev_added, ret;
> + __u8 charging, capacity;
> +
> + /* Check if new LEDs were added since the last time */
> + dev_added = atomic_cmpxchg(&drv_data->trigger_device_added, 1, 0);
> +
> + /* Get the battery info */
> + spin_lock_irqsave(&drv_data->lock, flags);
> + charging = drv_data->battery_charging;
> + capacity = drv_data->battery_capacity;
> + spin_unlock_irqrestore(&drv_data->lock, flags);
> +
> + /* Don't set the LEDs if nothing has changed */
> + if (!dev_added && drv_data->trigger_capacity == capacity &&
> + drv_data->trigger_charging == charging)
> + goto reset_timer;
> +
> + if (charging) {
> + /* Charging: blink at 1 sec intervals */
> + delay_on = delay_off = 1000;
> + led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> + &delay_off);
> + } else if (capacity <= 20) {
> + /* Low battery: blink at 500ms intervals */
> + delay_on = delay_off = 500;
> + led_trigger_blink(&drv_data->battery_trigger, &delay_on,
> + &delay_off);
> + } else {
> + /*
> + * Normal: set the brightness to stop blinking
> + *
> + * This just walks the list of LEDs on the trigger and sets the
> + * brightness to the existing value. This leaves the brightness
> + * the same but the blinking is stopped.
> + */
> + read_lock(&drv_data->battery_trigger.leddev_list_lock);
> + list_for_each_entry(led,
> + &drv_data->battery_trigger.led_cdevs, trig_list) {
> + led_set_brightness(led, led->brightness);
> + }
> + read_unlock(&drv_data->battery_trigger.leddev_list_lock);
> + }
> +
> + /* Cache the power state for next time */
> + drv_data->trigger_charging = charging;
> + drv_data->trigger_capacity = capacity;
> +
> +reset_timer:
> + ret = mod_timer(&drv_data->battery_trigger_timer,
> + jiffies + msecs_to_jiffies(500));
> +
> + if (ret < 0)
> + hid_err(drv_data->hdev,
> + "Failed to set battery trigger timer\n");
> +}
> +
> +static void sony_battery_trigger_activate(struct led_classdev *led)
> +{
> + struct sony_sc *sc;
> +
> + sc = container_of(led->trigger, struct sony_sc, battery_trigger);
> +
> + /*
> + * Set the device_added flag to tell the timer function that it
> + * should send an update even if the power state hasn't changed.
> + */
> + atomic_set(&sc->trigger_device_added, 1);
> +}
> +
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> + int ret;
> +
> + sc->battery_trigger.name = kasprintf(GFP_KERNEL,
> + "%s-blink-low-or-charging", sc->battery.name);
> + if (!sc->battery.name)
> + return -ENOMEM;
> +
> + sc->battery_trigger.activate = sony_battery_trigger_activate;
> +
> + ret = led_trigger_register(&sc->battery_trigger);
> + if (ret < 0)
> + goto trigger_failure;
> +
> + setup_timer(&sc->battery_trigger_timer,
> + sony_battery_trigger_callback, (unsigned long)sc);
> +
> + ret = mod_timer(&sc->battery_trigger_timer,
> + jiffies + msecs_to_jiffies(500));
> + if (ret < 0)
> + goto timer_failure;
> +
> + sc->trigger_initialized = 1;
> +
> + return 0;
> +
> +timer_failure:
> + led_trigger_unregister(&sc->battery_trigger);
> +trigger_failure:
> + kfree(sc->battery_trigger.name);
> + return ret;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> + if (sc->trigger_initialized) {
> + del_timer_sync(&sc->battery_trigger_timer);
> + led_trigger_unregister(&sc->battery_trigger);
> + kfree(sc->battery_trigger.name);
> + }
> +}
> +#else
> +static int sony_battery_trigger_init(struct sony_sc *sc)
> +{
> + /* Nothing to do */
> + return 0;
> +}
> +
> +static void sony_battery_trigger_remove(struct sony_sc *sc)
> +{
> + /* Nothing to do */
> +}
> +#endif
> +
> static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
> int w, int h)
> {
> @@ -1786,14 +1933,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (ret < 0)
> goto err_stop;
>
> - if (sc->quirks & SONY_LED_SUPPORT) {
> - ret = sony_leds_init(sc);
> + if (sc->quirks & SONY_BATTERY_SUPPORT) {
> + ret = sony_battery_probe(sc);
> if (ret < 0)
> goto err_stop;
> - }
>
> - if (sc->quirks & SONY_BATTERY_SUPPORT) {
> - ret = sony_battery_probe(sc);
> + ret = sony_battery_trigger_init(sc);
> if (ret < 0)
> goto err_stop;
>
> @@ -1805,6 +1950,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
> }
>
> + if (sc->quirks & SONY_LED_SUPPORT) {
> + ret = sony_leds_init(sc);
> + if (ret < 0)
> + goto err_stop;
> + }
> +
> if (sc->quirks & SONY_FF_SUPPORT) {
> ret = sony_init_ff(sc);
> if (ret < 0)
> @@ -1817,8 +1968,10 @@ err_close:
> err_stop:
> if (sc->quirks & SONY_LED_SUPPORT)
> sony_leds_remove(sc);
> - if (sc->quirks & SONY_BATTERY_SUPPORT)
> + if (sc->quirks & SONY_BATTERY_SUPPORT) {
> + sony_battery_trigger_remove(sc);
> sony_battery_remove(sc);
> + }
> sony_cancel_work_sync(sc);
> sony_remove_dev_list(sc);
> hid_hw_stop(hdev);
> @@ -1834,6 +1987,7 @@ static void sony_remove(struct hid_device *hdev)
>
> if (sc->quirks & SONY_BATTERY_SUPPORT) {
> hid_hw_close(hdev);
> + sony_battery_trigger_remove(sc);
> sony_battery_remove(sc);
> }
>
I'm going to self-NAK this patch as I've found a problem scenario with
it where LEDs might not blink when they should. I would still
appreciate feedback on whether or not this concept is worth persuing
before spending more time on it though.
^ permalink raw reply
* Re: change kmalloc into vmalloc for large memory allocations
From: Wang, Yalin @ 2014-03-03 1:55 UTC (permalink / raw)
To: 'Takashi Iwai'
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'fweisbec@gmail.com', 'dmitry.torokhov@gmail.com',
'coreteam@netfilter.org',
'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'rostedt@goodmis.org',
'netfilter@vger.kernel.org', 'mingo@redhat.com',
'linux-arm-kernel@lists.infradead.org',
'gregkh@linuxfoundation.org',
"'linux-usb@vger.kernel.org'" <linux-us>
In-Reply-To: <s5hlhwvb9q8.wl%tiwai@suse.de>
Hi Takashi,
For this one:
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772
in my kernel log,
the allocation size 62496,
about 62KB,
I think need to change to vmalloc .
How do you think of it ?
Logs:
<4>[ 11.469789] ------------[ cut here ]------------
<4>[ 11.469803] WARNING: at /media/00DE7FE2DE7FCE82/jb-mr2-yukon/kernel/include/linux/slub_def.h:265 __kmalloc+0x38/0x29c()
<4>[ 11.469811] size=62496 flags=80d0
<4>[ 11.469815] Modules linked in:
<4>[ 11.469835] [<c010c534>] (unwind_backtrace+0x0/0x11c) from [<c018ab04>] (warn_slowpath_common+0x4c/0x64)
<4>[ 11.469850] [<c018ab04>] (warn_slowpath_common+0x4c/0x64) from [<c018ab9c>] (warn_slowpath_fmt+0x2c/0x3c)
<4>[ 11.469864] [<c018ab9c>] (warn_slowpath_fmt+0x2c/0x3c) from [<c0249ab4>] (__kmalloc+0x38/0x29c)
<4>[ 11.469879] [<c0249ab4>] (__kmalloc+0x38/0x29c) from [<c06a0124>] (snd_soc_register_card+0x184/0x104c)
<4>[ 11.469896] [<c06a0124>] (snd_soc_register_card+0x184/0x104c) from [<c088183c>] (msm8226_asoc_machine_probe+0x298/0x7f0)
<4>[ 11.469913] [<c088183c>] (msm8226_asoc_machine_probe+0x298/0x7f0) from [<c044b1fc>] (platform_drv_probe+0x14/0x18)
<4>[ 11.469927] [<c044b1fc>] (platform_drv_probe+0x14/0x18) from [<c0449f2c>] (driver_probe_device+0x134/0x334)
<4>[ 11.469940] [<c0449f2c>] (driver_probe_device+0x134/0x334) from [<c044a194>] (__driver_attach+0x68/0x8c)
<4>[ 11.469953] [<c044a194>] (__driver_attach+0x68/0x8c) from [<c044849c>] (bus_for_each_dev+0x48/0x80)
<4>[ 11.469965] [<c044849c>] (bus_for_each_dev+0x48/0x80) from [<c0449528>] (bus_add_driver+0x100/0x26c)
<4>[ 11.469978] [<c0449528>] (bus_add_driver+0x100/0x26c) from [<c044a66c>] (driver_register+0x9c/0x120)
<4>[ 11.469992] [<c044a66c>] (driver_register+0x9c/0x120) from [<c010052c>] (do_one_initcall+0x90/0x160)
<4>[ 11.470007] [<c010052c>] (do_one_initcall+0x90/0x160) from [<c0d00b74>] (kernel_init+0xec/0x1a8)
<4>[ 11.470022] [<c0d00b74>] (kernel_init+0xec/0x1a8) from [<c0106aec>] (kernel_thread_exit+0x0/0x8)
<4>[ 11.470030] ---[ end trace 1b75b31a2719ed4d ]---
BRs/ Yalin
-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de]
Sent: Friday, February 28, 2014 10:20 PM
To: Wang, Yalin
Cc: 'linux-kernel@vger.kernel.org'; 'linux-arm-msm@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-input@vger.kernel.org'; 'balbi@ti.com'; 'gregkh@linuxfoundation.org'; 'lrg@ti.com'; 'broonie@opensource.wolfsonmicro.com'; 'perex@perex.cz'; 'pablo@netfilter.org'; 'kaber@trash.net'; 'davem@davemloft.net'; 'rostedt@goodmis.org'; 'fweisbec@gmail.com'; 'mingo@redhat.com'; 'dmitry.torokhov@gmail.com'; 'rydberg@euromail.se'; 'linux-usb@vger.kernel.org'; 'alsa-devel@alsa-project.org'; 'netfilter-devel@vger.kernel.org'; 'netfilter@vger.kernel.org'; 'coreteam@netfilter.org'; 'netdev@vger.kernel.org'
Subject: Re: change kmalloc into vmalloc for large memory allocations
At Fri, 28 Feb 2014 16:15:23 +0800,
Wang, Yalin wrote:
>
> Hi
>
>
> I find there is some drivers use kmalloc to allocate large Memorys
> during module_init, some can be changed to use vmalloc To save some
> low mem, I add log in kernel to track , And list them here:
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drive
> rs/usb/gadget/f_mass_storage.c?h=master#n2724
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound
> /soc/soc-core.c?h=master#n3772
At least the ASoC runtime case doesn't use the allocated memory as buffer, and they are allocated only once per device, thus it shouldn't be the problem you stated. If it really consumes so much memory, we need to rethink, instead of allocating an array but allocate each object, for example.
thanks,
Takashi
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_ftp.c?h=master#n603
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_h323_main.c?h=master#n1849
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_irc.c?h=master#n247
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_sane.c?h=master#n195
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drive
> rs/input/evdev.c?h=master#n403
>
>
> they allocate large memory from 10k~64K , this will use lots of low
> mem, instead if we use vmalloc for these drivers , it will be good for
> some devices like smart phone, we often encounter some errors like
> kmalloc failed because there is not enough low mem , especially when
> the device has physical memory less than 1GB .
>
>
> could this module change the memory allocation into vmalloc ?
>
> I was thinking that if we can introduce a helper function in kmalloc.h like this :
>
> Kmalloc(size, flags)
> {
> If (size > PAGE_SIZE && flags&CAN_USE_VMALLOC_FLAG)
> return vmalloc(size);
> Else
> return real_kmalloc(size);
> }
>
> Kfree(ptr)
> {
> If (is_vmalloc_addr(ptr))
> Vfree(ptr);
> Else
> Kfree(ptr);
> }
>
>
> But we need add some flags to ensure always use kmalloc for Some
> special use (dma etc..)
>
> How do you think of it ?
>
> Thanks
>
>
>
^ permalink raw reply
* Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
From: Mark Brown @ 2014-03-03 1:56 UTC (permalink / raw)
To: Carlo Caione
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1393692352-10839-7-git-send-email-carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]
On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote:
> index d59c826..0cef101 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -69,3 +69,4 @@ CONFIG_NFS_FS=y
> CONFIG_ROOT_NFS=y
> CONFIG_NLS=y
> CONFIG_PRINTK_TIME=y
> +CONFIG_REGULATOR_AXP20X=y
If you want to update the defconfig do it in a separate patch.
> +static int axp20x_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> +{
> + return regulator_set_voltage_sel_regmap(rdev, 0);
> +}
I'm not entirely clear what this is supposed to do but it's broken - it
both ignores the voltage that is passed in and immediately writes to the
normal voltage select register which will presumably distrupt the system.
> +static int axp20x_io_enable_regmap(struct regulator_dev *rdev)
> +{
> + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> + rdev->desc->enable_mask, AXP20X_IO_ENABLED);
> +}
Please make some helpers for this (or extend the existing ones to cope
with more than a single bit control) - there's several other devices
which have similar multi-bit controls so we should factor this out
rather than open coding.
> + np = of_node_get(pdev->dev.parent->of_node);
> + if (!np)
> + return 0;
> +
> + regulators = of_find_node_by_name(np, "regulators");
> + if (!regulators) {
> + dev_err(&pdev->dev, "regulators node not found\n");
> + return -EINVAL;
> + }
The driver should be able to start up with no configuration provided at
all except for the device being registered - the user won't be able to
change anything but they will be able to read the current state of the
hardware which can be useful for diagnostics.
> +static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 workmode)
> +{
> + unsigned int mask = AXP20X_WORKMODE_DCDC2_MASK;
> +
> + if ((id != AXP20X_DCDC2) && (id != AXP20X_DCDC3))
> + return -EINVAL;
> +
> + if (id == AXP20X_DCDC3)
> + mask = AXP20X_WORKMODE_DCDC3_MASK;
> +
> + return regmap_update_bits(rdev->regmap, AXP20X_DCDC_MODE, mask, workmode);
> +}
What is a workmode?
> + pmic->rdev[i] = regulator_register(&pmic->rdesc[i], &config);
> + if (IS_ERR(pmic->rdev[i])) {
Use devm_regulator_register() and then you don't need to clean up the
regulators either.
> +static int __init axp20x_regulator_init(void)
> +{
> + return platform_driver_register(&axp20x_regulator_driver);
> +}
> +
> +subsys_initcall(axp20x_regulator_init);
module_platform_driver().
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* RE: change kmalloc into vmalloc for large memory allocations
From: Wang, Yalin @ 2014-03-03 2:51 UTC (permalink / raw)
To: 'gregkh@linuxfoundation.org'
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'tiwai@suse.de', 'fweisbec@gmail.com',
'perex@perex.cz', 'mingo@redhat.com',
'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org', 'coreteam@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'rostedt@goodmis.org',
'netfilter@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'netdev@vger.kernel.org',
"'dmitry.torokhov@gmail.com'" <dmit>
In-Reply-To: <20140228163304.GA15614@kroah.com>
Hi greg,
I am sorry,
I make a mistake ,
You are right ,
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/usb/gadget/f_mass_storage.c?h=master#n2724
this one should not changed to use vmalloc,
the buffer will be used by DMA，
others should be safe to change:
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_ftp.c?h=master#n603
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_h323_main.c?h=master#n1849
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_irc.c?h=master#n247
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_sane.c?h=master#n195
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/input/evdev.c?h=master#n403
Thanks
-----Original Message-----
From: 'gregkh@linuxfoundation.org' [mailto:gregkh@linuxfoundation.org]
Sent: Saturday, March 01, 2014 12:33 AM
To: Wang, Yalin
Cc: 'Huang Shijie'; 'linux-kernel@vger.kernel.org'; 'linux-arm-msm@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-input@vger.kernel.org'; 'balbi@ti.com'; 'lrg@ti.com'; 'broonie@opensource.wolfsonmicro.com'; 'perex@perex.cz'; 'tiwai@suse.de'; 'pablo@netfilter.org'; 'kaber@trash.net'; 'davem@davemloft.net'; 'rostedt@goodmis.org'; 'fweisbec@gmail.com'; 'mingo@redhat.com'; 'dmitry.torokhov@gmail.com'; 'rydberg@euromail.se'; 'linux-usb@vger.kernel.org'; 'alsa-devel@alsa-project.org'; 'netfilter-devel@vger.kernel.org'; 'netfilter@vger.kernel.org'; 'coreteam@netfilter.org'; 'netdev@vger.kernel.org'
Subject: Re: change kmalloc into vmalloc for large memory allocations
On Fri, Feb 28, 2014 at 05:20:08PM +0800, Wang, Yalin wrote:
> Hi
>
>
> Yeah,
> Dma buffer must be allocated by kmalloc,
>
> But the modules I list should can all be changed to use vmalloc,
> because the buffer is only used by software, Not by any hardware .
Are you sure about that? The USB gadget driver needs DMA memory from what I can tell, have you tried your change out on a system that does not allow the USB controller to access non-DMA memory?
And I agree with Steve, just fix the individual drivers, don't do a "hidden" change of where the memory is allocated from, that's not a good idea and will cause problems later.
greg k-h
^ permalink raw reply
* Re: change kmalloc into vmalloc for large memory allocations
From: 'gregkh@linuxfoundation.org' @ 2014-03-03 3:08 UTC (permalink / raw)
To: Wang, Yalin
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'tiwai@suse.de', 'fweisbec@gmail.com',
'mingo@redhat.com', 'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org', 'coreteam@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'rostedt@goodmis.org',
'netfilter@vger.kernel.org',
'linux-arm-kernel@lists.infradead.org',
'netdev@vger.kernel.org',
'dmitry.torokhov@gmail.com', 'linux-kern
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F38F@CNBJMBX05.corpusers.net>
On Mon, Mar 03, 2014 at 10:51:23AM +0800, Wang, Yalin wrote:
> Hi greg,
>
> I am sorry,
> I make a mistake ,
> You are right ,
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/usb/gadget/f_mass_storage.c?h=master#n2724
>
> this one should not changed to use vmalloc,
> the buffer will be used by DMA，
Which is why a wrapper function will never work.
> others should be safe to change:
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_ftp.c?h=master#n603
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_h323_main.c?h=master#n1849
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_irc.c?h=master#n247
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_sane.c?h=master#n195
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/input/evdev.c?h=master#n403
Then send individual patches for these and see what happens.
greg k-h
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply
* RE: change kmalloc into vmalloc for large memory allocations
From: Wang, Yalin @ 2014-03-03 8:00 UTC (permalink / raw)
To: 'gregkh@linuxfoundation.org'
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'tiwai@suse.de', 'fweisbec@gmail.com',
'Will Deacon', 'perex@perex.cz',
'mingo@redhat.com', 'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org', 'rusty@rustcorp.com.au',
'coreteam@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'mschmidt@redhat.com', 'rostedt@goodmis.org',
'netfilter@vger.kernel.org', 'linux-arm-kernel
In-Reply-To: <20140303030821.GA16732@kroah.com>
Hi greg,
For
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/input/evdev.c?h=master#n403
there have been a patch for kmalloc failed ,
for
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772
I have not change it , need some more code to change in devm_kzalloc ..
I make a patch for netfilter part :
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_ftp.c?h=master#n603
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_h323_main.c?h=master#n1849
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_irc.c?h=master#n247
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_sane.c?h=master#n195
seems work well ,
these module will allocate 64KB large memory,
is it possible to be merged ?
Thanks
-- >8 --
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index b8a0924..0e92b0d 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -14,7 +14,7 @@
#include <linux/moduleparam.h>
#include <linux/netfilter.h>
#include <linux/ip.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/ipv6.h>
#include <linux/ctype.h>
#include <linux/inet.h>
@@ -593,14 +593,14 @@ static void nf_conntrack_ftp_fini(void)
}
}
- kfree(ftp_buffer);
+ vfree(ftp_buffer);
}
static int __init nf_conntrack_ftp_init(void)
{
int i, j = -1, ret = 0;
- ftp_buffer = kmalloc(65536, GFP_KERNEL);
+ ftp_buffer = vmalloc(65536, GFP_KERNEL);
if (!ftp_buffer)
return -ENOMEM;
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 70866d1..49ae092 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -18,7 +18,7 @@
#include <linux/inet.h>
#include <linux/in.h>
#include <linux/ip.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/udp.h>
#include <linux/tcp.h>
#include <linux/skbuff.h>
@@ -1837,7 +1837,7 @@ static void __exit nf_conntrack_h323_fini(void)
nf_conntrack_helper_unregister(&nf_conntrack_helper_q931[1]);
nf_conntrack_helper_unregister(&nf_conntrack_helper_q931[0]);
nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
- kfree(h323_buffer);
+ vfree(h323_buffer);
pr_debug("nf_ct_h323: fini\n");
}
@@ -1846,7 +1846,7 @@ static int __init nf_conntrack_h323_init(void)
{
int ret;
- h323_buffer = kmalloc(65536, GFP_KERNEL);
+ h323_buffer = vmalloc(65536, GFP_KERNEL);
if (!h323_buffer)
return -ENOMEM;
ret = nf_conntrack_helper_register(&nf_conntrack_helper_h245);
@@ -1876,7 +1876,7 @@ err3:
err2:
nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
err1:
- kfree(h323_buffer);
+ vfree(h323_buffer);
return ret;
}
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 0fd2976..b57df10 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -16,7 +16,7 @@
#include <linux/ip.h>
#include <linux/tcp.h>
#include <linux/netfilter.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <net/netfilter/nf_conntrack.h>
#include <net/netfilter/nf_conntrack_expect.h>
@@ -244,7 +244,7 @@ static int __init nf_conntrack_irc_init(void)
irc_exp_policy.max_expected = max_dcc_channels;
irc_exp_policy.timeout = dcc_timeout;
- irc_buffer = kmalloc(65536, GFP_KERNEL);
+ irc_buffer = vmalloc(65536);
if (!irc_buffer)
return -ENOMEM;
@@ -285,7 +285,7 @@ static void nf_conntrack_irc_fini(void)
for (i = 0; i < ports_c; i++)
nf_conntrack_helper_unregister(&irc[i]);
- kfree(irc_buffer);
+ vfree(irc_buffer);
}
module_init(nf_conntrack_irc_init);
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 4a2134f..a4c8bf3 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -20,7 +20,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/netfilter.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/in.h>
#include <linux/tcp.h>
#include <net/netfilter/nf_conntrack.h>
@@ -185,14 +185,14 @@ static void nf_conntrack_sane_fini(void)
}
}
- kfree(sane_buffer);
+ vfree(sane_buffer);
}
static int __init nf_conntrack_sane_init(void)
{
int i, j = -1, ret = 0;
- sane_buffer = kmalloc(65536, GFP_KERNEL);
+ sane_buffer = vmalloc(65536);
if (!sane_buffer)
return -ENOMEM;
-----Original Message-----
From: 'gregkh@linuxfoundation.org' [mailto:gregkh@linuxfoundation.org]
Sent: Monday, March 03, 2014 11:08 AM
To: Wang, Yalin
Cc: 'Huang Shijie'; 'linux-kernel@vger.kernel.org'; 'linux-arm-msm@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-input@vger.kernel.org'; 'balbi@ti.com'; 'lrg@ti.com'; 'broonie@opensource.wolfsonmicro.com'; 'perex@perex.cz'; 'tiwai@suse.de'; 'pablo@netfilter.org'; 'kaber@trash.net'; 'davem@davemloft.net'; 'rostedt@goodmis.org'; 'fweisbec@gmail.com'; 'mingo@redhat.com'; 'dmitry.torokhov@gmail.com'; 'rydberg@euromail.se'; 'linux-usb@vger.kernel.org'; 'alsa-devel@alsa-project.org'; 'netfilter-devel@vger.kernel.org'; 'netfilter@vger.kernel.org'; 'coreteam@netfilter.org'; 'netdev@vger.kernel.org'
Subject: Re: change kmalloc into vmalloc for large memory allocations
On Mon, Mar 03, 2014 at 10:51:23AM +0800, Wang, Yalin wrote:
> Hi greg,
>
> I am sorry,
> I make a mistake ,
> You are right ,
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drive
> rs/usb/gadget/f_mass_storage.c?h=master#n2724
>
> this one should not changed to use vmalloc, the buffer will be used by
> DMA，
Which is why a wrapper function will never work.
> others should be safe to change:
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound
> /soc/soc-core.c?h=master#n3772
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_ftp.c?h=master#n603
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_h323_main.c?h=master#n1849
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_irc.c?h=master#n247
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/n
> etfilter/nf_conntrack_sane.c?h=master#n195
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drive
> rs/input/evdev.c?h=master#n403
Then send individual patches for these and see what happens.
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: change kmalloc into vmalloc for large memory allocations
From: 'gregkh@linuxfoundation.org' @ 2014-03-03 14:10 UTC (permalink / raw)
To: Wang, Yalin
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'tiwai@suse.de', 'fweisbec@gmail.com',
'Will Deacon', 'perex@perex.cz',
'mingo@redhat.com', 'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org', 'rusty@rustcorp.com.au',
'coreteam@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'mschmidt@redhat.com', 'rostedt@goodmis.org',
'netfilter@vger.kernel.org', 'linux-arm-kernel
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F390@CNBJMBX05.corpusers.net>
On Mon, Mar 03, 2014 at 04:00:59PM +0800, Wang, Yalin wrote:
> Hi greg,
Please read Documentation/SubmittingPatches for how to properly submit
kernel patches in a form that they could be accepted.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/4] HID: sony: do not rely on hid_output_raw_report
From: Benjamin Tissoires @ 2014-03-03 14:28 UTC (permalink / raw)
To: Antonio Ospite
Cc: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <20140301131658.2168c54b91ade7a0a63d9fb6@ao2.it>
On Sat, Mar 1, 2014 at 7:16 AM, Antonio Ospite <ao2@ao2.it> wrote:
> Hi Benjamin,
>
> On Fri, 28 Feb 2014 19:20:36 -0500
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
>> hid_out_raw_report is going to be obsoleted as it is not part of the
>> unified HID low level transport documentation
>> (Documentation/hid/hid-transport.txt)
>>
>> To do so, we need to introduce two new quirks:
>> * HID_QUIRK_NO_OUTPUT_REPORTS: this quirks prevents the transport
>> driver to use the interrupt channel to send output report (and thus
>> force to use HID_REQ_SET_REPORT command)
>
> Maybe a little more informative name? Like:
> HID_QUIRK_NON_STD_OUTPUT_REPORTS
> or
> HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
> or expansions of the above.
>
> The rationale being that, from outside, these are still output reports
> (kind of), it's the channel they are sent over to which changes.
Sorry, putting names on things has always been my weakness. I like
more the second proposition, so I will pick it in the v2 unless
someone else complains.
>
> Another comment about a typo is inlined below.
>
> Thanks,
> Antonio
>
>> * HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
>> include the report ID in the buffer it sends to the device through
>> HID_REQ_SET_REPORT in case of an output report
>>
>> This also fixes a regression introduced in commit 3a75b24949a8
>> (HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
>> The hidraw API was not able to communicate with the PS3 SixAxis
>> controllers in USB mode.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-sony.c | 59 ++++++++++---------------------------------
>> drivers/hid/hidraw.c | 3 ++-
>> drivers/hid/usbhid/hid-core.c | 7 ++++-
>> include/linux/hid.h | 2 ++
>> 4 files changed, 24 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index b5fe65e..08eac71 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -1007,45 +1007,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>> }
>>
>> /*
>> - * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
>> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
>> - * so we need to override that forcing HID Output Reports on the Control EP.
>> - *
>> - * There is also another issue about HID Output Reports via USB, the Sixaxis
>> - * does not want the report_id as part of the data packet, so we have to
>> - * discard buf[0] when sending the actual control message, even for numbered
>> - * reports, humpf!
>> - */
>> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
>> - size_t count, unsigned char report_type)
>> -{
>> - struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>> - struct usb_device *dev = interface_to_usbdev(intf);
>> - struct usb_host_interface *interface = intf->cur_altsetting;
>> - int report_id = buf[0];
>> - int ret;
>> -
>> - if (report_type == HID_OUTPUT_REPORT) {
>> - /* Don't send the Report ID */
>> - buf++;
>> - count--;
>> - }
>> -
>> - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> - HID_REQ_SET_REPORT,
>> - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> - ((report_type + 1) << 8) | report_id,
>> - interface->desc.bInterfaceNumber, buf, count,
>> - USB_CTRL_SET_TIMEOUT);
>> -
>> - /* Count also the Report ID, in case of an Output report. */
>> - if (ret > 0 && report_type == HID_OUTPUT_REPORT)
>> - ret++;
>> -
>> - return ret;
>> -}
>> -
>> -/*
>> * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
>> * to "operational". Without this, the ps3 controller will not report any
>> * events.
>> @@ -1305,11 +1266,8 @@ static void sixaxis_state_worker(struct work_struct *work)
>> buf[10] |= sc->led_state[2] << 3;
>> buf[10] |= sc->led_state[3] << 4;
>>
>> - if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>> - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> - else
>> - hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
>> - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>> + hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
>> + HID_REQ_SET_REPORT);
>> }
>>
>> static void dualshock4_state_worker(struct work_struct *work)
>> @@ -1659,7 +1617,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> }
>>
>> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>> - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>> + /*
>> + * The Sony Sixaxis does not handle HID Output Reports on the
>> + * Interrupt EP like it could, so we need to forcing HID Output
> ^^^^
> typo: "we need to force".
>
k, will fix in v2.
Thanks,
Benjamin
>> + * Reports to use HID_REQ_SET_REPORT on the Control EP.
>> + *
>> + * There is also another issue about HID Output Reports via USB,
>> + * the Sixaxis does not want the report_id as part of the data
>> + * packet, so we have to discard buf[0] when sending the actual
>> + * control message, even for numbered reports, humpf!
>> + */
>> + hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS;
>> + hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
>> ret = sixaxis_set_operational_usb(hdev);
>> sc->worker_initialized = 1;
>> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index 2cc484c..6537e58 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>> goto out_free;
>> }
>>
>> - if (report_type == HID_OUTPUT_REPORT) {
>> + if ((report_type == HID_OUTPUT_REPORT) &&
>> + !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS)) {
>> ret = hid_hw_output_report(dev, buf, count);
>> /*
>> * compatibility with old implementation of USB-HID and I2C-HID:
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 0d1d875..3bc7cad 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
>> int ret, skipped_report_id = 0;
>>
>> /* Byte 0 is the report number. Report data starts at byte 1.*/
>> - buf[0] = reportnum;
>> + if ((rtype == HID_OUTPUT_REPORT) &&
>> + (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
>> + buf[0] = 0;
>> + else
>> + buf[0] = reportnum;
>> +
>> if (buf[0] == 0x0) {
>> /* Don't send the Report ID */
>> buf++;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5eb282e..2baf834 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -287,6 +287,8 @@ struct hid_item {
>> #define HID_QUIRK_NO_EMPTY_INPUT 0x00000100
>> #define HID_QUIRK_NO_INIT_INPUT_REPORTS 0x00000200
>> #define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
>> +#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID 0x00020000
>> +#define HID_QUIRK_NO_OUTPUT_REPORTS 0x00040000
>> #define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
>> #define HID_QUIRK_NO_INIT_REPORTS 0x20000000
>> #define HID_QUIRK_NO_IGNORE 0x40000000
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Antonio Ospite
> http://ao2.it
>
> A: Because it messes up the order in which people normally read text.
> See http://en.wikipedia.org/wiki/Posting_style
> Q: Why is top-posting such a bad thing?
^ permalink raw reply
* [PATCH] netfilter:Change nf_conntrack modules to use vmalloc
From: Wang, Yalin @ 2014-03-04 7:30 UTC (permalink / raw)
To: 'gregkh@linuxfoundation.org'
Cc: 'alsa-devel@alsa-project.org',
'broonie@opensource.wolfsonmicro.com',
'tiwai@suse.de', 'fweisbec@gmail.com',
'Will Deacon', 'perex@perex.cz',
'mingo@redhat.com', 'linux-input@vger.kernel.org',
'rydberg@euromail.se', 'lrg@ti.com',
'pablo@netfilter.org', 'rusty@rustcorp.com.au',
'coreteam@netfilter.org',
'linux-arm-msm@vger.kernel.org',
'mschmidt@redhat.com', 'rostedt@goodmis.org',
'netfilter@vger.kernel.org', 'linux-arm-kernel
In-Reply-To: <20140303141044.GA9139@kroah.com>
This patch change some nf_conntrack modules to
use vmalloc to allocate large memory instead of kmalloc,
to save low memorys.
Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
---
net/netfilter/nf_conntrack_ftp.c | 6 +++---
net/netfilter/nf_conntrack_h323_main.c | 8 ++++----
net/netfilter/nf_conntrack_irc.c | 6 +++---
net/netfilter/nf_conntrack_sane.c | 6 +++---
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index b8a0924..aad7eca 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -14,7 +14,7 @@
#include <linux/moduleparam.h>
#include <linux/netfilter.h>
#include <linux/ip.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/ipv6.h>
#include <linux/ctype.h>
#include <linux/inet.h>
@@ -593,14 +593,14 @@ static void nf_conntrack_ftp_fini(void)
}
}
- kfree(ftp_buffer);
+ vfree(ftp_buffer);
}
static int __init nf_conntrack_ftp_init(void)
{
int i, j = -1, ret = 0;
- ftp_buffer = kmalloc(65536, GFP_KERNEL);
+ ftp_buffer = vmalloc(65536);
if (!ftp_buffer)
return -ENOMEM;
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 70866d1..5baee1a 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -18,7 +18,7 @@
#include <linux/inet.h>
#include <linux/in.h>
#include <linux/ip.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/udp.h>
#include <linux/tcp.h>
#include <linux/skbuff.h>
@@ -1837,7 +1837,7 @@ static void __exit nf_conntrack_h323_fini(void)
nf_conntrack_helper_unregister(&nf_conntrack_helper_q931[1]);
nf_conntrack_helper_unregister(&nf_conntrack_helper_q931[0]);
nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
- kfree(h323_buffer);
+ vfree(h323_buffer);
pr_debug("nf_ct_h323: fini\n");
}
@@ -1846,7 +1846,7 @@ static int __init nf_conntrack_h323_init(void)
{
int ret;
- h323_buffer = kmalloc(65536, GFP_KERNEL);
+ h323_buffer = vmalloc(65536);
if (!h323_buffer)
return -ENOMEM;
ret = nf_conntrack_helper_register(&nf_conntrack_helper_h245);
@@ -1876,7 +1876,7 @@ err3:
err2:
nf_conntrack_helper_unregister(&nf_conntrack_helper_h245);
err1:
- kfree(h323_buffer);
+ vfree(h323_buffer);
return ret;
}
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 0fd2976..b57df10 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -16,7 +16,7 @@
#include <linux/ip.h>
#include <linux/tcp.h>
#include <linux/netfilter.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <net/netfilter/nf_conntrack.h>
#include <net/netfilter/nf_conntrack_expect.h>
@@ -244,7 +244,7 @@ static int __init nf_conntrack_irc_init(void)
irc_exp_policy.max_expected = max_dcc_channels;
irc_exp_policy.timeout = dcc_timeout;
- irc_buffer = kmalloc(65536, GFP_KERNEL);
+ irc_buffer = vmalloc(65536);
if (!irc_buffer)
return -ENOMEM;
@@ -285,7 +285,7 @@ static void nf_conntrack_irc_fini(void)
for (i = 0; i < ports_c; i++)
nf_conntrack_helper_unregister(&irc[i]);
- kfree(irc_buffer);
+ vfree(irc_buffer);
}
module_init(nf_conntrack_irc_init);
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 4a2134f..a4c8bf3 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -20,7 +20,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/netfilter.h>
-#include <linux/slab.h>
+#include <linux/vmalloc.h>
#include <linux/in.h>
#include <linux/tcp.h>
#include <net/netfilter/nf_conntrack.h>
@@ -185,14 +185,14 @@ static void nf_conntrack_sane_fini(void)
}
}
- kfree(sane_buffer);
+ vfree(sane_buffer);
}
static int __init nf_conntrack_sane_init(void)
{
int i, j = -1, ret = 0;
- sane_buffer = kmalloc(65536, GFP_KERNEL);
+ sane_buffer = vmalloc(65536);
if (!sane_buffer)
return -ENOMEM;
--
1.8.2.2
^ permalink raw reply related
* Re: [PATCH v2 0/8] Input: pixcir_i2c_ts: Add Type-B Multi-touch and DT support
From: Roger Quadros @ 2014-03-04 11:24 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Hi Dmitry,
Gentle reminder to comment on this series. Thanks.
cheers,
-roger
On 02/26/2014 05:27 PM, Roger Quadros wrote:
> Hi,
>
> This series does the following
>
> - use devres managed resource allocations
> - convert to Type-B multi touch protocol
> - support upto 5 fingers with hardware supplied tracking IDs
> - device tree support
>
> Changelog:
>
> v2:
> - Addressed review comments and re-arranged patch order
>
> v1:
> - http://article.gmane.org/gmane.linux.kernel/1616417
>
> cheers,
> -roger
>
> ---
> Roger Quadros (8):
> Input: pixcir_i2c_ts: Use devres managed resource allocations
> Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
> Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
> Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
> Input: pixcir_i2c_ts: support upto 5 fingers and hardware provided
> tracking IDs
> Input: pixcir_i2c_ts: Implement wakeup from suspend
> Input: pixcir_i2c_ts: Add device tree support
> ARM: dts: am43x-epos-evm: Correct Touch controller info
>
> .../bindings/input/touchscreen/pixcir_i2c_ts.txt | 26 ++
> .../devicetree/bindings/vendor-prefixes.txt | 1 +
> arch/arm/boot/dts/am43x-epos-evm.dts | 13 +-
> drivers/input/touchscreen/pixcir_i2c_ts.c | 510 ++++++++++++++++++---
> include/linux/input/pixcir_ts.h | 56 ++-
> 5 files changed, 545 insertions(+), 61 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>
^ permalink raw reply
* Re: [PATCH 0/6] HID: sony: More Sony controller fixes and improvements.
From: Antonio Ospite @ 2014-03-04 12:34 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <53135BA6.9090501@oh.rr.com>
On Sun, 02 Mar 2014 11:26:14 -0500
Frank Praznik <frank.praznik@oh.rr.com> wrote:
> On 3/1/2014 08:53, Antonio Ospite wrote:
[...]
> >
> >> Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
> >> blue on the DualShock 4 so there is some indication that the controller is
> >> powered on and connected in the case of Bluetooth. The code can be used to set
> >> the LEDs based on the device number, but I'm not sure how to actually retrieve
> >> the controller number from the system. I saw the xpad patches posted a few
> >> weeks ago where the minor number of the joydev device was used, but I'm under
> >> the impression that doing that is not ideal. Any suggestions?
> > Setting the controller number is done by the bluez sixaxis plugin[1]
> > (in bluez 5.x) following the X in /dev/input/jsX, this covers the
> > case of a mixed-joypad scenario, IMHO it makes sense that the
> > controller number matches the joystick device number.
> > Imagine js0->Sixaxis1, js1->wiimote, js2->Sixaxis2, I think it make
> > sense to have the LEDs on Sixaxis2 say "controller 3", not 2.
> >
> > This has been done in userspace with libudev for 2 reasons:
> > 1. the hid drivers should not have knowledge of the joystick layer;
> > 2. kernel drivers should be as simple as possible, and try to just
> > exposing hardware functionalities but with as less "business logic"
> > as possible in them.
> >
> > The current implementation in the bluez plugin uses hidraw, but support
> > for the sysfs led class could be added in order to avoid conflicts with
> > the rumble; IIRC, currently, setting rumble values could override the
> > LED settings done via hidraw, because the LEDs state is not tracked in
> > the latter case.
> >
> > Ciao,
> > Antonio
> >
> > [1]
> > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/plugins/sixaxis.c
> >
> This can be done in the driver. See
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>
xpad is a joystick driver, while hid-sony is a HID driver.
> It seems that the main problem with that patch is that modern systems
> shouldn't be relying on joydev for this functionality. I'd like to know
> what David Herrmann and Greg Kroah-Hartman came up with regarding the
> solution mentioned in the reply as it would be nice to be able to set
> the LEDs to the proper default values in the driver without needing to
> rely on an external daemon. Setting the defaults in the driver doesn't
> interfere with setting custom values after the device is initialized, so
> there are no issues if the user wants to use a custom LED daemon.
The way I see it, the problem here is not about joydev itself it's
about:
1. the layering relationship between HID and joydev
2. whether or not we consider assigning joypad numbers as a kernel
job.
The NAK came because of 1.; as 2. is more debatable I guess.
> As far as the behavior of patch #6 (setting the LEDs to the same number
> or color on every connected device just to indicate that the controller
> is turned on), the xpad and wiimote drivers both initialize the LEDs to
> some default value where at least one is on to indicate that the
> controller is powered on and connected to the system. The xpad driver
> increments an atomic counter for assigning values as controllers are
> connected and the wiimote always sets LED #1 to on. Not ideal, but it
> serves it's purpose.
>
For the sixaxis the all-blink pattern is used, it's a really dumb
indicator, but it's still an indicator.
> Personally I don't like the idea of relying on a BlueZ plugin to set the
> controller LED values as it seems to bring a lot of issues with it:
> users may not have BlueZ installed or enabled, some distros still use an
> old version, the plugin relies on joydev to get the device number which
> is why the patch I linked was NAKed, the current plugin implementation
> doesn't set them via sysfs so the setting will be lost if force-feedback
> is used and the plugin could conflict with other user-installed daemons
> that set the LEDs (unless udev guarantees a notification order?). In
> the latter scenario, the user could disable the plugin, but then you
> lose the Sixaxis pairing functionality that it provides. I also have to
> question as to why BlueZ is considered an appropriate place to set
> controller LEDs, particularly on controllers that aren't connected via
> Bluetooth.
Let me answer the last question first, the rationale was:
1. the common use case for the sixaxis is considered to be its use
via Bluetooth, can we agree on that?
2. under this assumption BlueZ was chosen as a convenient place to do
the pairing and association.
3. Paring requires to access the device via USB in order to retrieve
its bdaddr and set the master bdaddr.
4. Once we are accessing the device via USB and BT in the BlueZ plugin
anyway, just let's set the LEDs here.
A more general "excuse" is that clutter in userspace is slightly more
acceptable than clutter in kernel code.
And to reply to the other questions, yes, the bluez plugin is not
perfect by any means: it enforces dependencies, it's not using the leds
class yet, but it can be improved and IMHO it's still the most
convenient way to have responsibilities separated: the kernel driver
provides access to the hardware functionality and the userspace
software decides what to do with them.
However, since you are the one currently working on things you are
entitled with a stronger voice here, I am just whispering my opinions :)
Ciao,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call
From: Jiri Kosina @ 2014-03-04 13:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, David Herrmann, David Barksdale, linux-input,
linux-kernel
In-Reply-To: <1393633237-26496-3-git-send-email-benjamin.tissoires@redhat.com>
On Fri, 28 Feb 2014, Benjamin Tissoires wrote:
> I don't have access to the device, so I copied/pasted the code
> from hidraw.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 860db694..c4f87bd 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> + /* Fixme: test which function is actually called for output reports */
I don't completely understand this Fixme (oh, and please spell it as
'FIXME:' so that we are consistent with all the other instances), could
you please elaborate?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call
From: Benjamin Tissoires @ 2014-03-04 14:18 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, David Herrmann, David Barksdale, linux-input,
linux-kernel
In-Reply-To: <alpine.LNX.2.00.1403041445080.30402@pobox.suse.cz>
On Mar 04 2014 or thereabouts, Jiri Kosina wrote:
> On Fri, 28 Feb 2014, Benjamin Tissoires wrote:
>
> > I don't have access to the device, so I copied/pasted the code
> > from hidraw.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> > index 860db694..c4f87bd 100644
> > --- a/drivers/hid/hid-cp2112.c
> > +++ b/drivers/hid/hid-cp2112.c
> > @@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
> > if (!buf)
> > return -ENOMEM;
> >
> > - ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> > + /* Fixme: test which function is actually called for output reports */
>
> I don't completely understand this Fixme (oh, and please spell it as
> 'FIXME:' so that we are consistent with all the other instances), could
> you please elaborate?
Well, sorry:
As I said, this part is a copy/paste of what is in hidraw. However, this
just reflect that we don't know how the device actually behave, which is
not very elegant. I have currently no clues of which function will be
actually called for output reports: hid_hw_output_report() or
hid_hw_raw_request(). Once we got the confirmation of which function is
called, we could make the path more straightforward.
I bought one of these (it may help debugging some Synaptics devices),
and I'll receive it by the end of the week. So by next week, we should
get the actual code path and remove this FIXME.
I need to send a v2 of hid-sony in any cases, so I guess you should not
pull these 4 patches right away. If you prefer having this in linux-next,
the sooner, I can also send the v2 right away, and we will fix this
cp2112 driver next week.
Cheers,
Benjamin
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call
From: Jiri Kosina @ 2014-03-04 14:21 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, David Herrmann, David Barksdale, linux-input,
linux-kernel
In-Reply-To: <20140304141838.GD24154@mail.corp.redhat.com>
On Tue, 4 Mar 2014, Benjamin Tissoires wrote:
> I need to send a v2 of hid-sony in any cases, so I guess you should not
> pull these 4 patches right away. If you prefer having this in linux-next,
> the sooner, I can also send the v2 right away, and we will fix this
> cp2112 driver next week.
That's fine, I'll wait for the whole v2 respin. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/6] HID: sony: More Sony controller fixes and improvements.
From: Frank Praznik @ 2014-03-04 14:54 UTC (permalink / raw)
To: Antonio Ospite, Frank Praznik; +Cc: linux-input, jkosina, dh.herrmann
In-Reply-To: <20140304133426.ea711f53198ec62894a630de@ao2.it>
On 3/4/2014 07:34, Antonio Ospite wrote:
> This can be done in the driver. See
> https://www.mail-archive.com/linux-input@vger.kernel.org/msg08103.html
>
>
> xpad is a joystick driver, while hid-sony is a HID driver.
I tried it and the hack works from HID space with some tweaking, but,
yeah, it's a hack and doesn't belong in *any* driver.
> Let me answer the last question first, the rationale was:
>
> 1. the common use case for the sixaxis is considered to be its use
> via Bluetooth, can we agree on that?
> 2. under this assumption BlueZ was chosen as a convenient place to do
> the pairing and association.
> 3. Paring requires to access the device via USB in order to retrieve
> its bdaddr and set the master bdaddr.
> 4. Once we are accessing the device via USB and BT in the BlueZ plugin
> anyway, just let's set the LEDs here.
>
> A more general "excuse" is that clutter in userspace is slightly more
> acceptable than clutter in kernel code.
>
> And to reply to the other questions, yes, the bluez plugin is not
> perfect by any means: it enforces dependencies, it's not using the leds
> class yet, but it can be improved and IMHO it's still the most
> convenient way to have responsibilities separated: the kernel driver
> provides access to the hardware functionality and the userspace
> software decides what to do with them.
>
> However, since you are the one currently working on things you are
> entitled with a stronger voice here, I am just whispering my opinions :)
>
> Ciao,
> Antonio
>
I understand your rationale, but I still think that the driver should
assign sane defaults for cases where there isn't userspace software to
set the LEDs. Leaving the Sixaxis LEDs blinking doesn't differentiate
between whether or not the controller is connected or trying to connect
and on the Dualshock 4 the default is a bright-white battery draining
light. On the other hand, the current default of 'all-off', which has
been the default since the initial LED patch, doesn't indicate whether
the controller connected successfully or timed-out when using Bluetooth.
I currently have David's suggestion of assigning an id via an IDA
allocator implemented in my working copy. This can be used to both
initialize the LEDs relative to other Sony controllers and provide a
sane unique identifier for the battery identification string instead of
the constantly incrementing atomic integer used now. This is the most
ideal solution IMO since it clearly communicates the current power and
connection status of the controller, lets the user easily differentiate
between controllers 1, 2, etc... and it provides defaults that most
users shouldn't find annoying. Of course, these are just defaults so if
you want to use a BlueZ plugin or something else to set the values via
userspace, there is nothing stopping you.
I'm going wait on v2 of this series until Benjamin Tissoires finishes
his latest patches since I think patch #2 and beyond will conflict with
his work. Patch #1 is just a trivial one-line fix though, so that
should still be good to go.
^ permalink raw reply
* Re: [PATCH] input: ff-memless: don't schedule already playing effect to play again
From: Dmitry Torokhov @ 2014-03-04 17:07 UTC (permalink / raw)
To: Felix Rueegg; +Cc: linux-input, linux-kernel
In-Reply-To: <1393760143-5986-1-git-send-email-felix.rueegg@gmail.com>
Hi Felix,
On Sun, Mar 02, 2014 at 12:35:43PM +0100, Felix Rueegg wrote:
> When an effect with zero replay length, zero replay delay
> and zero envelope attack length is uploaded, it is played and then scheduled to play
> again one timer tick later. This triggers a warning (URB submitted while
> active) in combination with the xpad driver.
>
> Skipping the rescheduling of this effect fixes the issue.
Won't the same issue happen if we happen to schedule a different effect
that would start at "now" time? We should not be "dropping" the new
effect, at least not in core.
It looks to me xpad.c needs [more] love.
>
> Signed-off-by: Felix Rueegg <felix.rueegg@gmail.com>
> ---
> drivers/input/ff-memless.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 74c0d8c..2e06948 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct ml_device *ml)
> if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> continue;
>
> - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> next_at = calculate_next_time(state);
> - else
> + if (next_at == now)
> + continue;
> + } else {
> next_at = state->play_at;
> + }
>
> if (time_before_eq(now, next_at) &&
> (++events == 1 || time_before(next_at, earliest)))
> --
> 1.9.0
>
Thanks.
--
Dmitry
^ permalink raw reply
* Regression in kernel 3.13: mouse freezes after entering in KDE (doesn't happen in 3.12)
From: Dâniel Fraga @ 2014-03-04 17:54 UTC (permalink / raw)
To: linux-input
After I upgraded my kernel from 3.12 to 3.13, the mouse gets
stuck (freezes) after entering in KDE. The interesting is that the
mouse works with gpm at the console and in KDM (before login). Just
after I login into KDE, it freezes (and it freezes when Bitcoin-qt is
starting which is very disk intensive at the start... maybe something
related to high load?).
My hardware:
Asus P8Z68-V Pro/Gen3
Core i7 2700k
HD Seagate 1.5 TB (using ext4)
I tried to bisect between 3.12 and 3.13 but it failed to find
the bad commit. For example:
1) I bisect between 3.12 and 3.13:
37676af15c8d5a9689c9d1220d2a27d510cbe238 is the first bad commit
2) I bisect between 3.12 and 37676af15c8d5a9689c9d1220d2a27d510cbe238
afc4b47372ace24c630c1d79b3a0ed32bf1d19fd is the first bad commit
And this goes on forever. The strangest fact is that bisect
test against commits which it shouldn't test like commits before 3.12
release (3.12-rc7 etc).
So I'm asking for suggestions for what can I try to better find
the bad commit. 3.13-rc1 also fails, so the bad commit is between 3.12
and 3.13-rc1. I tried to bisect between 3.12 and 3.13-rc1 but without
success, since it points to a "bad commit" after the real bad commit (I
know because I always test with the previous commit before the "bad
commit" and it fails too).
Any thins what I can do?
Thanks.
Ps: better yet, could you suggest commits I could test, so I
can report back and we can do a more rational bisect?
--
Linux 3.12.13: One Giant Leap for Frogkind
http://www.youtube.com/DanielFragaBR
http://mcxnow.com?r=danielfragabr
Bitcoin: 12H6661yoLDUZaYPdah6urZS5WiXwTAUgL
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox