Re: [PATCH RESEND v10 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver

[James Ogletree <James.Ogletree@xxxxxxxxxx>]

Hi Dmitry,

Reaching out to get closure on these few comments before I send up the
next version.


> On May 3, 2024, at 10:25 AM, James Ogletree <James.Ogletree@xxxxxxxxxx> wrote:
> 
> 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@xxxxxxxxx> 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