On Wed, Jan 18, 2023 at 12:16 AM Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> On Tue, 2023-01-10 at 17:23 -0800, Vicki Pfau wrote:
> > The Steam Deck includes a new report that allows for emulating XInput-style
> > rumble motors with the Deck's actuators. This adds support for passing these
> > values directly to the Deck.
> >
> > Signed-off-by: Vicki Pfau <vi@xxxxxxxxxxx>
> > ---
> > drivers/hid/Kconfig | 8 ++++++
> > drivers/hid/hid-steam.c | 55 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index e2a5d30c8895..e9de0a2d3cd3 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -1025,6 +1025,14 @@ config HID_STEAM
> > without running the Steam Client. It supports both the wired and
> > the wireless adaptor.
> >
> > +config STEAM_FF
> > + bool "Steam Deck force feedback support"
> > + depends on HID_STEAM
> > + select INPUT_FF_MEMLESS
> > + help
> > + Say Y here if you want to enable force feedback support for the Steam
> > + Deck.
> > +
> > config HID_STEELSERIES
> > tristate "Steelseries SRW-S1 steering wheel support"
> > help
> > diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> > index efd192d6c51a..788b5baaf145 100644
> > --- a/drivers/hid/hid-steam.c
> > +++ b/drivers/hid/hid-steam.c
> > @@ -91,6 +91,7 @@ static LIST_HEAD(steam_devices);
> > #define STEAM_CMD_FORCEFEEDBAK 0x8f
> > #define STEAM_CMD_REQUEST_COMM_STATUS 0xb4
> > #define STEAM_CMD_GET_SERIAL 0xae
> > +#define STEAM_CMD_HAPTIC_RUMBLE 0xeb
> >
> > /* Some useful register ids */
> > #define STEAM_REG_LPAD_MODE 0x07
> > @@ -134,6 +135,9 @@ struct steam_device {
> > u8 battery_charge;
> > u16 voltage;
> > struct delayed_work heartbeat;
> > + struct work_struct rumble_work;
> > + u16 rumble_left;
> > + u16 rumble_right;
> > };
> >
> > static int steam_recv_report(struct steam_device *steam,
> > @@ -290,6 +294,45 @@ static inline int steam_request_conn_status(struct steam_device *steam)
> > return steam_send_report_byte(steam, STEAM_CMD_REQUEST_COMM_STATUS);
> > }
> >
> > +static inline int steam_haptic_rumble(struct steam_device *steam,
> > + u16 intensity, u16 left_speed, u16 right_speed,
> > + u8 left_gain, u8 right_gain)
> > +{
> > + u8 report[11] = {STEAM_CMD_HAPTIC_RUMBLE, 9};
> > +
> > + report[3] = intensity & 0xFF;
> > + report[4] = intensity >> 8;
> > + report[5] = left_speed & 0xFF;
> > + report[6] = left_speed >> 8;
> > + report[7] = right_speed & 0xFF;
> > + report[8] = right_speed >> 8;
> > + report[9] = left_gain;
> > + report[10] = right_gain;
> > +
> > + return steam_send_report(steam, report, sizeof(report));
> > +}
> > +
> > +static void steam_haptic_rumble_cb(struct work_struct *work)
> > +{
> > + struct steam_device *steam = container_of(work, struct steam_device,
> > + rumble_work);
> > + steam_haptic_rumble(steam, 0, steam->rumble_left,
> > + steam->rumble_right, 2, 0);
> > +}
> > +
> > +#ifdef CONFIG_STEAM_FF
> > +static int steam_play_effect(struct input_dev *dev, void *data,
> > + struct ff_effect *effect)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > +
> > + steam->rumble_left = effect->u.rumble.strong_magnitude;
> > + steam->rumble_right = effect->u.rumble.weak_magnitude;
> > +
> > + return schedule_work(&steam->rumble_work);
> > +}
> > +#endif
> > +
> > static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
> > {
> > if (enable) {
> > @@ -541,6 +584,15 @@ static int steam_input_register(struct steam_device *steam)
> > input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
> > input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
> >
> > +#ifdef CONFIG_STEAM_FF
> > + if (steam->quirks & STEAM_QUIRK_DECK) {
> > + input_set_capability(input, EV_FF, FF_RUMBLE);
> > + ret = input_ff_create_memless(input, NULL, steam_play_effect);
> > + if (ret)
> > + goto init_ff_fail;
> > + }
> > +#endif
> > +
> > ret = input_register_device(input);
> > if (ret)
> > goto input_register_fail;
> > @@ -549,6 +601,7 @@ static int steam_input_register(struct steam_device *steam)
> > return 0;
> >
> > input_register_fail:
> > +init_ff_fail:
>
> JFYI, this actually causes a compilation warning with CONFIG_STEAM_FF
> disabled:
>
> drivers/hid/hid-steam.c: In function ‘steam_input_register’:
> drivers/hid/hid-steam.c:604:1: warning: label ‘init_ff_fail’ defined but not
> used [-Wunused-label]
> 604 | init_ff_fail:
> | ^~~~~~~~~~~~
>
> TBH I think we should be fine just reusing the input_register_fail: jump label
> for this instead of adding another label.
>
> FWIW as well if you want: you could just drop the Kconfig option for this
> entirely, which bentiss may or may not want. It would at least leave a little
> less chance for compilation warnings like this, since the more Kconfig options
> you have for a module the higher the chance you'll leave a warning by mistake
> in some random kernel config.
I agree with Lyude here. However the whole HID tree is crippled with
those "if input_ff" and that would mean a bigger policy change.
So I would suggest keeping the Kconfig option for now, and if you
want, Vicki (or anybody else) maybe it's time to get rid of those
input_ff Kconfigs and make them slightly more evident for users.
Whether the first driver to use it selects input_ff or they all depend
on input_ff without the ability to disable it is entirely the
responsibility of the submitter :)
Cheers,
Benjamin
>
> If you end up deciding to leave the Kconfig in I'd at least update the commit
> message to mention explicitly you added it so people notice it even if they
> don't look at the diff (e.g. maintainers just merging reviewed patches).
>
> I have no hard opinion either way as long as we fix the compilation warning
> :). With the issues mentioned here addressed, this patch is:
>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
>
> > input_free_device(input);
> > return ret;
> > }
> > @@ -842,6 +895,7 @@ static int steam_probe(struct hid_device *hdev,
> > INIT_WORK(&steam->work_connect, steam_work_connect_cb);
> > INIT_LIST_HEAD(&steam->list);
> > INIT_DEFERRABLE_WORK(&steam->heartbeat, steam_lizard_mode_heartbeat);
> > + INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
> >
> > steam->client_hdev = steam_create_client_hid(hdev);
> > if (IS_ERR(steam->client_hdev)) {
> > @@ -898,6 +952,7 @@ static int steam_probe(struct hid_device *hdev,
> > client_hdev_fail:
> > cancel_work_sync(&steam->work_connect);
> > cancel_delayed_work_sync(&steam->heartbeat);
> > + cancel_work_sync(&steam->rumble_work);
> > steam_alloc_fail:
> > hid_err(hdev, "%s: failed with error %d\n",
> > __func__, ret);
>
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
>
