Date: Wed, 10 Jan 2024 14:36:55 +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


> On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
> 
> On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
>> 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
> 
> I am sorry, who is "we"?

Just a royal “I”. Apologies, no claim to authority intended here. :)

> 
>> 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.
> 
> No, please implement the API properly.

Ack.

> 
>> 
>>> 
>>>> + 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.
> 
> You only have one instance of vibe_start_work, as well as only one
> "slot" to hold the effect you want to start. So if you issue 2 request
> back to back to play effect 1 and 2 you are likely to end with
> info->start_effect == 2 and that is what vibe_start_work handler will
> observe, effectively dropping request to play effect 1 on the floor.

Understood, ack.

> 
>> 
>>> 
>>>> + } 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.
> 
> Again, please implement the driver properly, not define your own
> carveouts for the expected behavior.

Ack, and a clarification question: the device is not actually able to
play multiple effects at once. In that case, does stopping a specific
effect entail just cancelling an effect in the queue?

Best,
James

Powered by blists - more mailing lists