Re: [PATCH v2 1/1] HID: shield: Initial driver implementation with Thunderstrike support

[Rahul Rameshbabu]

Hi Jiri,

Thanks for the feedback.

On Thu, 13 Apr, 2023 17:07:00 +0200 Jiri Kosina <jikos@xxxxxxxxxx> wrote:
> On Mon, 10 Apr 2023, Rahul Rameshbabu wrote:
>
>> Supports the Thunderstrike (SHIELD 2017) controller. Implements support for
>> the Thunderstrike HOSTCMD firmware interface. Adds sysfs attributes about a
>> SHIELD device and introduces haptics support for controllers.
>> 
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@xxxxxxxxxx>
>
> Thanks a lot for submitting the driver. I have one minor question:
>
> [ ... snip ... ]
>> +thunderstrike_update_haptics(struct thunderstrike *ts,
>> +			     struct thunderstrike_hostcmd_haptics *motors)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ts->haptics_update_lock, flags);
>> +	ts->haptics_val = *motors;
>> +	spin_unlock_irqrestore(&ts->haptics_update_lock, flags);
>
> Do we really have to disable interrupts when taking haptics_update_lock? 
> Is it ever being taken from interrupt context?

The reason why I disable interrupts is because I call
thunderstrike_update_haptics from the play_effect callback I pass to
input_ff_create_memless. From there, it is used in ml_effect_timer
drivers/input/ff-memless.c, which is a timer interrupt context so we
should disable interrupts when taking haptics_update_lock.

  static void ml_effect_timer(struct timer_list *t)
  {
    struct ml_device *ml = from_timer(ml, t, timer);
    struct input_dev *dev = ml->dev;
    unsigned long flags;

    pr_debug("timer: updating effects\n");

    spin_lock_irqsave(&dev->event_lock, flags);
    ml_play_effects(ml);
    spin_unlock_irqrestore(&dev->event_lock, flags);
  }

Let me know if this seems off.

>
> Thanks,

-- Rahul Rameshbabu