[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <564A2601-4933-4BD7-B4E6-C973A585CA61@cirrus.com>
Date: Tue, 9 Jan 2024 22:03:02 +0000
From: James Ogletree <James.Ogletree@...rus.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: James Ogletree <jogletre@...nsource.cirrus.com>,
Fred Treven
<Fred.Treven@...rus.com>,
Ben Bright <Ben.Bright@...rus.com>, Rob Herring
<robh+dt@...nel.org>,
Krzysztof Kozlowski
<krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Simon Trimmer <simont@...nsource.cirrus.com>,
Charles Keepax
<ckeepax@...nsource.cirrus.com>,
Richard Fitzgerald
<rf@...nsource.cirrus.com>,
Lee Jones <lee@...nel.org>, Liam Girdwood
<lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Jaroslav Kysela
<perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
James Schulman
<James.Schulman@...rus.com>,
David Rhodes <David.Rhodes@...rus.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Peng Fan
<peng.fan@....com>, Jeff LaBundy <jeff@...undy.com>,
Sebastian Reichel
<sebastian.reichel@...labora.com>,
Jacky Bai <ping.bai@....com>, Weidong Wang
<wangweidong.a@...nic.com>,
Arnd Bergmann <arnd@...db.de>,
Herve Codina
<herve.codina@...tlin.com>,
Shuming Fan <shumingf@...ltek.com>,
Shenghao Ding
<13916275206@....com>, Ryan Lee <ryans.lee@...log.com>,
Linus Walleij
<linus.walleij@...aro.org>,
"open list:CIRRUS LOGIC HAPTIC DRIVERS"
<patches@...nsource.cirrus.com>,
"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK,
TOUCHSCREEN)..." <linux-input@...r.kernel.org>,
"open list:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>,
open list
<linux-kernel@...r.kernel.org>,
"open list:SOUND - SOC LAYER / DYNAMIC AUDIO
POWER MANAGEM..." <linux-sound@...r.kernel.org>,
"moderated list:CIRRUS LOGIC
AUDIO CODEC DRIVERS" <alsa-devel@...a-project.org>
Subject: Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50
haptic driver
Hi Dmitry,
Thank you for your excellent review. Just a few questions.
> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
>
> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
>> +
>> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
>> + if (!info->add_effect.u.periodic.custom_data)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
>> + effect->u.periodic.custom_data, sizeof(s16) * len)) {
>> + info->add_error = -EFAULT;
>> + goto out_free;
>> + }
>> +
>> + queue_work(info->vibe_wq, &info->add_work);
>> + flush_work(&info->add_work);
>
> I do not understand the need of scheduling a work here. You are
> obviously in a sleeping context (otherwise you would not be able to
> execute flush_work()) so you should be able to upload the effect right
> here.
Scheduling work here is to ensure its ordering with “playback" worker
items, which themselves are called in atomic context and so need
deferred work. I think this explains why we need a workqueue as well,
but please correct me.
>
>> +
>> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
>> +{
>> + struct vibra_info *info = input_get_drvdata(dev);
>> +
>> + if (val > 0) {
>
> value is supposed to signal how many times an effect should be repeated.
> It looks like you are not handling this at all.
For playbacks, we mandate that the input_event value field is set to either 1
or 0 to command either a start playback or stop playback respectively.
Values other than that should be rejected, so in the next version I will fix this
to explicitly check for 1 or 0.
>
>> + info->start_effect = &dev->ff->effects[effect_id];
>> + queue_work(info->vibe_wq, &info->vibe_start_work);
>
> The API allows playback of several effects at once, the way you have it
> done here if multiple requests come at same time only one will be
> handled.
I think I may need some clarification on this point. Why would concurrent
start/stop playback commands get dropped? It seems they would all be
added to the workqueue and executed eventually.
>
>> + } else {
>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
>
> Which effect are you stopping? All of them? You need to stop a
> particular one.
Our implementation of “stop” stops all effects in flight which is intended.
That is probably unusual so I will add a comment here in the next
version.
Best,
James
Powered by blists - more mailing lists