lkml.org 
[lkml]   [2024]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver
    Date
    Hi Dmitry,

    Thank you for your excellent review. Just a few questions.

    > On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.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



    \
     
     \ /
      Last update: 2024-01-09 23:05    [W:3.295 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site