From: James Ogletree <James.Ogletree@cirrus.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: James Ogletree <jogletre@opensource.cirrus.com>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>, Lee Jones <lee@kernel.org>,
Mark Brown <broonie@kernel.org>, Jeff LaBundy <jeff@labundy.com>,
"open list:CIRRUS LOGIC HAPTIC DRIVERS"
<patches@opensource.cirrus.com>,
"open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..."
<linux-sound@vger.kernel.org>,
"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK,
TOUCHSCREEN)..." <linux-input@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH RESEND v10 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver
Date: Fri, 3 May 2024 15:25:45 +0000 [thread overview]
Message-ID: <BC84DC9F-65FB-4553-A0B9-52151DD549DB@cirrus.com> (raw)
In-Reply-To: <A0925B5F-F8D0-4270-B510-385FE4A38F84@cirrus.com>
Hi Dmitry,
Trimming down my last reply to just the points that need your attention/ack.
I made some small edits for the sake of clarity.
> On Apr 16, 2024, at 6:28 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Apr 08, 2024 at 03:32:13PM +0000, James Ogletree wrote:
>>
>> +/* Describes an area in DSP memory populated by effects */
>> +struct cs40l50_bank {
>> + enum cs40l50_bank_type type;
>> + u32 base_index;
>> + u32 max_index;
>
> This looks like it is written to the device, so I think this needs
> proper endianness annotation. Is there any concern about padding between
> the type and base_index?
The structure itself is never written, so there is no concern about padding.
Only base_index is written, and the endianness conversion is handled by regmap.
>> +static void cs40l50_start_worker(struct work_struct *work)
>> +{
>> + struct cs40l50_work *work_data = container_of(work, struct cs40l50_work, work);
>> + struct cs40l50_vibra *vibra = work_data->vibra;
>> + struct cs40l50_effect *start_effect;
>> +
>> + if (pm_runtime_resume_and_get(vibra->dev) < 0)
>> + goto err_free;
>> +
>> + mutex_lock(&vibra->lock);
>> +
>> + start_effect = cs40l50_find_effect(work_data->effect->id, &vibra->effect_head);
>> + if (start_effect) {
>> + while (--work_data->count >= 0) {
>> + vibra->dsp.write(vibra->dev, vibra->regmap, start_effect->index);
>> + usleep_range(work_data->effect->replay.length,
>> + work_data->effect->replay.length + 100);
>
> If (I am reading this right you are spinning here playing the effect. It
> would be much better if you tracked outstanding number of replays for an
> effect and had a work re-scheduled that would play an instance.
> Similarly to what code in ff-memless.c is doing.
Yes, you are reading it right.
It appears that ff-memless.c handles repeats atomically, however for
reasons explained below, this driver must queue work for playback
executions.
This results in a possible scenario where if a playback is issued with a
high enough repeat value, an erase operation could arrive in the middle of
the chain of re-scheduling and disrupt the playbacks which have yet to be
queued. But with the current approach, the effect is guaranteed to repeat
any number of times without risk of being erased in the middle.
That’s my concern with adopting the re-scheduling approach for this
driver. Please let me know your thoughts.
>> +static int cs40l50_erase(struct input_dev *dev, int effect_id)
>> +{
>> + struct cs40l50_vibra *vibra = input_get_drvdata(dev);
>> + struct cs40l50_work work_data;
>> +
>> + work_data.vibra = vibra;
>> + work_data.effect = &dev->ff->effects[effect_id];
>> +
>> + INIT_WORK(&work_data.work, cs40l50_erase_worker);
>> +
>> + /* Push to workqueue to serialize with playbacks */
>> + queue_work(vibra->vibe_wq, &work_data.work);
>> + flush_work(&work_data.work);
>
> You already take the lock when you play, upload or erase an effect. Why
> do we need additional single-thread workqueue for this? Why do we need
> additional ordering and synchronization?
The workqueue is necessary is because playback blocks (via
pm_runtime_resume_and_get), and so playback must defer work. Upload
and erase are not called in atomic context, but without queueing those
executions, they could cut in front of playbacks waiting in the
workqueue.
But as the callbacks are already serialized via the workqueue, then I do
think the locks are excessive. That’s my thinking, let me know if it is
sound.
Best,
James
next prev parent reply other threads:[~2024-05-03 15:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 15:32 [PATCH RESEND v10 0/5] Add support for CS40L50 James Ogletree
2024-04-08 15:32 ` [PATCH RESEND v10 1/5] firmware: cs_dsp: Add write sequence interface James Ogletree
2024-04-08 15:32 ` [PATCH RESEND v10 2/5] dt-bindings: input: cirrus,cs40l50: Add initial DT binding James Ogletree
2024-04-08 15:32 ` [PATCH RESEND v10 3/5] mfd: cs40l50: Add support for CS40L50 core driver James Ogletree
2024-04-11 15:35 ` Lee Jones
2024-04-08 15:32 ` [PATCH RESEND v10 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver James Ogletree
2024-04-16 23:28 ` Dmitry Torokhov
2024-04-19 18:26 ` James Ogletree
2024-05-03 15:25 ` James Ogletree [this message]
2024-05-20 15:47 ` James Ogletree
2024-04-08 15:32 ` [PATCH RESEND v10 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50 James Ogletree
2024-04-12 16:40 ` James Ogletree
2024-04-14 7:45 ` Mark Brown
2024-04-16 14:19 ` James Ogletree
2024-04-16 12:35 ` Lee Jones
2024-04-16 14:17 ` James Ogletree
2024-04-17 13:14 ` Lee Jones
2024-04-18 17:59 ` James Ogletree
2024-04-19 13:00 ` Lee Jones
2024-04-19 15:41 ` James Ogletree
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BC84DC9F-65FB-4553-A0B9-52151DD549DB@cirrus.com \
--to=james.ogletree@cirrus.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=jeff@labundy.com \
--cc=jogletre@opensource.cirrus.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-sound@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).